channeldb+routing: decouple payment lifecycle flow and fix stuck payments #7503
Conversation
yyforyongyu
added
code health
yyforyongyu
force-pushed
the
new-payment-lifecycle
branch
from
01d2f59 to
428c264
Compare
yyforyongyu
force-pushed
the
new-payment-lifecycle
branch
from
428c264 to
544cf09
Compare
yyforyongyu
force-pushed
the
new-payment-lifecycle
branch
3 times, most recently
from
69d9036 to
16399ac
Compare
|
@bhandras: review reminder |
yyforyongyu
force-pushed
the
new-payment-lifecycle
branch
from
16399ac to
13a57a5
Compare
|
Closing due to inactivity |
1 similar comment
|
Closing due to inactivity |
|
!lightninglabs-deploy mute |
| @@ -147,412 +187,303 @@ func (p *paymentLifecycle) resumePayment() ([32]byte, *route.Route, error) { | |||
| log.Infof("Resuming payment shard %v for payment %v", | |||
| a.AttemptID, p.identifier) | |||
|
|
|||
| shardHandler.collectResultAsync(&a.HTLCAttemptInfo) | |||
| p.resultCollector(&a) | |||
There was a problem hiding this comment.
right, after a restart of lnd 👍
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 lifecycle 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.
|
A point I wanted to highlight concerning compatibility issues with the new payment status is that an outdated Also, if endpoint consumers interpret the unknown status code as as a failure then this could lead to several payouts. An example would be if somebody tries to pay out funds from a custodial system (e.g. lit accounts) and operators haven't upgraded their accounting to reflect the status code (and they interpret it as a failure) this may lead to cases where the user is not debited. |
yyforyongyu
force-pushed
the
new-payment-lifecycle
branch
2 times, most recently
from
846887d to
261cc08
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.
This commit makes sure a testing payment is created via `createDummyLightningPayment` to ensure the payment hash is unique to avoid collision of the same payment hash being used in uint tests. Since the tests are running in parallel and accessing db, if two difference tests are using the same payment hash, no clean test state can be guaranteed.
yyforyongyu
force-pushed
the
new-payment-lifecycle
branch
from
261cc08 to
11af820
Compare
```
lnd_max_htlcs_test.go:149:
Error Trace: /home/runner/work/lnd/lnd/itest/lnd_max_htlcs_test.go:149
/home/runner/work/lnd/lnd/itest/lnd_max_htlcs_test.go:40
/home/runner/work/lnd/lnd/lntest/harness.go:286
/home/runner/work/lnd/lnd/itest/lnd_test.go:136
Error: Not equal:
expected: 3
actual : 0
Test: TestLightningNetworkDaemon/tranche01/60-of-134/btcd/max_htlc_pathfind
Messages: expected accepted
```
|
Landing this as is, we'll make an issue to track resolution of rolling back that breaking change. One way to handle it maybe would be: clients for the subscription call pass in a new flag that states they want the new enum value, and we only send it if so. In "compatibility mode", we just send in flight. |
Depends on,
MPPayment#7391Only the last 19 commits are new.
This 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.