Skip to content
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

Commits on May 19, 2022

  1. Enable removal of OnionPayload::Invoice::_legacy_hop_data later

    In 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`.
    TheBlueMatt committed May 19, 2022
    Configuration menu
    Copy the full SHA
    2a42595 View commit details
    Browse the repository at this point in the history
  2. Store an events::PaymentPurpose with each claimable payment

    In 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.
    TheBlueMatt committed May 19, 2022
    Configuration menu
    Copy the full SHA
    bd1e20d View commit details
    Browse the repository at this point in the history

Commits on May 26, 2022

  1. 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.
    TheBlueMatt committed May 26, 2022
    Configuration menu
    Copy the full SHA
    28c70ac View commit details
    Browse the repository at this point in the history

Commits on May 28, 2022

  1. Add a PaymentClaimed event to indicate a payment was claimed

    This replaces the return value of `claim_funds` with an event. It
    does not yet change behavior in any material way.
    TheBlueMatt committed May 28, 2022
    Configuration menu
    Copy the full SHA
    0a2a40c View commit details
    Browse the repository at this point in the history
  2. Provide a redundant Event::PaymentClaimed on restart if needed

    If 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.
    TheBlueMatt committed May 28, 2022
    Configuration menu
    Copy the full SHA
    0e25421 View commit details
    Browse the repository at this point in the history
  3. 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.
    TheBlueMatt committed May 28, 2022
    Configuration menu
    Copy the full SHA
    11c2f12 View commit details
    Browse the repository at this point in the history
  4. 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.
    TheBlueMatt committed May 28, 2022
    Configuration menu
    Copy the full SHA
    a12d37e View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    8a56702 View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    531d6c8 View commit details
    Browse the repository at this point in the history