-
Notifications
You must be signed in to change notification settings - Fork 363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Robustness of Inbound MPP Claims Across Restart #1434
Improve Robustness of Inbound MPP Claims Across Restart #1434
Commits on May 19, 2022
-
Enable removal of
OnionPayload::Invoice::_legacy_hop_data
laterIn fc77c57 we stopped using the `FinalOnionHopData` in `OnionPayload::Invoice` directly and renamed it `_legacy_hop_data` with the intent of removing it in a few versions. However, we continue to check that it was included in the serialized data, meaning we would not be able to remove it without breaking ability to serialize full `ChannelManager`s. This fixes that by making the `_legacy_hop_data` an `Option` which we will happily handle just fine if its `None`.
Configuration menu - View commit details
-
Copy full SHA for 2a42595 - Browse repository at this point
Copy the full SHA 2a42595View commit details -
Store an
events::PaymentPurpose
with each claimable paymentIn fc77c57 we stopped using the `FInalOnionHopData` in `OnionPayload::Invoice` directly and intend to remove it eventually. However, in the next few commits we need access to the payment secret when claimaing a payment, as we create a new `PaymentPurpose` during the claim process for a new event. In order to get access to a `PaymentPurpose` without having access to the `FinalOnionHopData` we here change the storage of `claimable_htlcs` to store a single `PaymentPurpose` explicitly with each set of claimable HTLCs.
Configuration menu - View commit details
-
Copy full SHA for bd1e20d - Browse repository at this point
Copy the full SHA bd1e20dView commit details
Commits on May 26, 2022
-
Ensure all HTLCs for a claimed payment are claimed on startup
While the HTLC-claim process happens across all MPP parts under one lock, this doesn't imply that they are claimed fully atomically on disk. Ultimately, an application can crash after persisting one `ChannelMonitorUpdate` out of multiple monitor updates needed for the full claim. Previously, this would leave us in a very bad state - because of the all-channels-available check in `claim_funds` we'd refuse to claim the payment again on restart (even though the `PaymentReceived` event will be passed to the user again), and we'd end up having partially claimed the payment! The fix for the consistency part of this issue is pretty straightforward - just check for this condition on startup and complete the claim across all channels/`ChannelMonitor`s if we detect it. This still leaves us in a confused state from the perspective of the user, however - we've actually claimed a payment but when they call `claim_funds` we return `false` indicating it could not be claimed.
Configuration menu - View commit details
-
Copy full SHA for 28c70ac - Browse repository at this point
Copy the full SHA 28c70acView commit details
Commits on May 28, 2022
-
Add a
PaymentClaimed
event to indicate a payment was claimedThis replaces the return value of `claim_funds` with an event. It does not yet change behavior in any material way.
Configuration menu - View commit details
-
Copy full SHA for 0a2a40c - Browse repository at this point
Copy the full SHA 0a2a40cView commit details -
Provide a redundant
Event::PaymentClaimed
on restart if neededIf we crashed during a payment claim and then detected a partial claim on restart, we should ensure the user is aware that the payment has been claimed. We do so here by using the new partial-claim detection logic to create a `PaymentClaimed` event.
Configuration menu - View commit details
-
Copy full SHA for 0e25421 - Browse repository at this point
Copy the full SHA 0e25421View commit details -
Do additional pre-flight checks before claiming a payment
As additional sanity checks, before claiming a payment, we check that we have the full amount available in `claimable_htlcs` that the payment should be for. Concretely, this prevents one somewhat-absurd edge case where a user may receive an MPP payment, wait many *blocks* before claiming it, allowing us to fail the pending HTLCs and the sender to retry some subset of the payment before we go to claim. More generally, this is just good belt-and-suspenders against any edge cases we may have missed.
Configuration menu - View commit details
-
Copy full SHA for 11c2f12 - Browse repository at this point
Copy the full SHA 11c2f12View commit details -
Drop return value from
fail_htlc_backwards
, clarify docs`ChannelManager::fail_htlc_backwards`' bool return value is quite confusing - just because it returns false doesn't mean the payment wasn't (already) failed. Worse, in some race cases around shutdown where a payment was claimed before an unclean shutdown and then retried on startup, `fail_htlc_backwards` could return true even though (a duplicate copy of the same payment) was claimed, but the claim event has not been seen by the user yet. While its possible to use it correctly, its somewhat confusing to have a return value at all, and definitely lends itself to misuse. Instead, we should push users towards a model where they don't care if `fail_htlc_backwards` succeeds - either they've locally marked the payment as failed (prior to seeing any `PaymentReceived` events) and will fail any attempts to pay it, or they have not and the payment is still receivable until its timeout time is reached. We can revisit this decision based on user feedback, but will need to very carefully document the potential failure modes here if we do.
Configuration menu - View commit details
-
Copy full SHA for a12d37e - Browse repository at this point
Copy the full SHA a12d37eView commit details -
Configuration menu - View commit details
-
Copy full SHA for 8a56702 - Browse repository at this point
Copy the full SHA 8a56702View commit details -
Configuration menu - View commit details
-
Copy full SHA for 531d6c8 - Browse repository at this point
Copy the full SHA 531d6c8View commit details