Add missing commits from 0-18-staging
#8170
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit removes the unclear abstraction `shardHandler` that's used in our payment lifecycle. As we'll see in the following commits, `shardHandler` is an unnecessary layer and everything can be cleanly managed inside `paymentLifecycle`.
`handleSwitchErr` is now responsible for failing the given HTLC attempt after deciding to fail the payment or not. This is crucial as previously, we might enter into a state where the payment's HTLC has already been marked as failed, and while we are marking the payment as failed, another HTLC attempt can be made at the same time, leading to potential stuck payments.
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`.
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.
Delete TestSendMPPaymentFailedWithShardsInFlight as it seems to be the same test as TestSendMPPaymentFailed.
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.
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.
```
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
```
|
This was merged into the staging branch as: #7503 Landing here as we deleted the staging branch to help prevent accidental merges like this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These commits were in
0-18-staging. Now that the branch was deleted, these commits should be merged into master(or0-18-merged?