channeldb+routing: refactor payment lifecycle #6683
Conversation
|
|
||
| // PaymentStatus is a memory representation of the current status of the | ||
| // payment. | ||
| type PaymentStatus byte |
There was a problem hiding this comment.
This is an awesome PR. So many smaller and bigger improvements.
It is fairly large though. For review it is probably beneficial to isolate the trivial changes (renames, moves) and minor refactors such as exitWithErr in a preparatory PR? The more involved changes such as the addition of a new channel and state machine can then receive exclusive focus in a part 2. Also potentially saves rebase headaches.
channeldb/payments.go
Outdated
| // | false | false | true | yes | StatusFailed | | ||
| // | false | false | true | no | StatusAttemptsFailed | | ||
| // | false | false | false | yes | StatusFailed | | ||
| // | false | false | false | no | StatusInitiated | |
There was a problem hiding this comment.
Would almost express this table in code and look up the status directly from that rather than implementing it as a switch statement.
channeldb/payments.go
Outdated
| StatusFailed PaymentStatus = 7 | ||
|
|
||
| // StatusPendingError is the status where a payment has conflict states | ||
| // across its HTLCs and has inflight HTLCs. |
There was a problem hiding this comment.
How does it end up with conflict states and is this an internal error?
channeldb/mp_payment.go
Outdated
| func (m *MPPayment) Terminated() bool { | ||
| return m.Status == StatusFailed || m.Status == StatusSucceeded | ||
| return m.Status == StatusFailed || m.Status == StatusSucceeded || | ||
| m.Status == StatusError |
There was a problem hiding this comment.
Commit message says pure move, but this looks like a functional change. Different commit?
| // Once the result is collected, we signal it by writing an | ||
| // empty struct to `attemptResultChan`. We do this by first | ||
| // freeing the channel in a non-blocking read. | ||
| select { |
There was a problem hiding this comment.
Interesting use of channels here. I wonder if is more intuitive if this goroutine is just always sending, and the receiver (next commit) does an extra for { select { ... } } to discard any extra signals if they would be waiting.
|
@bhandras: review reminder |
7ff8dd8 to
cafb615
Compare
81b2199 to
496f767
Compare
139f01c to
ee0be30
Compare
ee0be30 to
9e0618e
Compare
For context, is this a pure refactor PR or are there actual functional changes for users? If that is the case, it might be useful to describe the visible problems that are fixed in the PR description to emphasise the relevance. |
Def. Will update it once it's out of draft mode. |
routing/payment_lifecycle.go
Outdated
| if reason == nil { | ||
| return nil | ||
| // Fail the attempt. | ||
| _, err = p.failAttempt(attempt, sendErr) |
There was a problem hiding this comment.
This commit looks to be the first real fix. Perhaps it is worth splitting off all the previous renaming/moving in a separate pr to get that out of the way.
There was a problem hiding this comment.
yeah was thinking about the same thing. will do!
9e0618e to
6ca71a5
Compare
424572e to
af33248
Compare
This commit removes the `launchOutcome` and `shardResult` and uses `attemptResult` instead. This struct is also used in `failAttempt` so we can future distinguish critical vs non-critical errors when handling HTLC attempts.
This commit adds a new method `registerAttempt` to take care of creating and saving an htlc attempt to disk.
This commit starts handling switch error inside `sendAttempt` when an error is returned from sending the HTLC. To make sure the updated `HTLCAttempt` is always returned to the callsite, `handleSwitchErr` now also returns a `attemptResult`.
bb6da1b to
cd0d456
Compare
This commit removes the method `launchShard` and splits its original functionality into two steps - first create the attempt, second send the attempt. This enables us to have finer control over "which error is returned from which system and how to handle it".
This commit refactors the `resumePayment` method by adding the methods `checkTimeout` and `requestRoute` so it's easier to understand the flow and reason about the error handling.
This commit makes sure we only fail attempt inside `handleSwitchErr` to ensure the orders in failing payment and attempts. It refactors `collectResult` to return `attemptResult`, and expands `handleSwitchErr` to also handle the case where the attemptID is not found.
This commit adds a new struct, `stateStep`, to decide the workflow inside `resumePayment`. It also refactors `collectResultAsync` introducing a new channel `resultCollected`. This channel is used to signal the payment lifecycle that an HTLC attempt result is ready to be processed.
This commit adds a new interface method `TerminalInfo` and changes its implementation to return an `*HTLCAttempt` so it includes the route for a successful payment. Method `GetFailureReason` is now removed as its returned value can be found in the above method.
The old payment lifecycle is removed due to it's not "unit" - maintaining these tests probably takes as much work as the actual methods being tested, if not more so. Moreover, the usage of the old mockers in current payment lifecy test is removed as it re-implements other interfaces and sometimes implements it uniquely just for the tests. This is bad as, not only we need to work on the actual interface implementations and test them , but also re-implement them again in the test without testing them!
This commit adds more mockers to be used in coming unit tests and simplified the mockers to be more straightforward.
Thus adding following unit tests can be a bit easier.
cd0d456 to
860df9a
Compare
This commit adds unit tests for `resumePayment`. In addition, the `resumePayment` has been split into two parts so it's easier to be tested, 1) sending the htlc, and 2) collecting results. As seen in the new tests, this split largely reduces the complexity involved and makes the unit test flow sequential. This commit also makes full use of `mock.Mock` in the unit tests to provide a more clear testing flow.
860df9a to
e4eff05
Compare
|
Closed and replaced it with a new one as the comments here are very outdated. |
Depends on,
MPPayment#7391This PR refactors the payment lifecycle to provide cleaner layers of abstractions so it’s easier to understand and reason about them. Changes included,
PaymentStatusas the single source of truth.shardHandleras its responsibility is too vague.launchShardso requesting route, registering, and sending HTLC are separated since they deal with different subsystems likePaymentSession,ControlTowerandhtlcswitch. Previously the errors returned from different systems were mixed which made it hard to distinguish a terminal error vs a temp error. By separating them we can now easily decide when to fail the lifecycle, fail or retry the payment.TODOs
SendToRoute