lightningdevkit / rust-lightning

A highly modular Bitcoin Lightning library written in Rust. It's rust-lightning, not Rusty's Lightning!
Other
1.16k stars 367 forks source link

Don't pay a duplicate BOLT 12 invoice if `ChannelManager` is stale #3313

Closed valentinewallace closed 2 months ago

valentinewallace commented 2 months ago
This fixes the following bug:
- An outbound payment is AwaitingInvoice
- We receive an invoice and lock the HTLCs into the relevant ChannelMonitors
- The monitors are successfully persisted, but the ChannelManager fails to
  persist, so the outbound payment remains AwaitingInvoice
- We restart, causing the channel to close due to a stale ChannelManager
- We receive a duplicate invoice, and attempt to pay it again due to the
  payment still being AwaitingInvoice in the stale ChannelManager

After the fix for this, we will notice that the payment is already locked into
the monitor on startup and transition the incorrectly-AwaitingInvoice payment
to Retryable, which prevents double-paying on duplicate invoice receipt.

Caught by @TheBlueMatt: https://github.com/lightningdevkit/rust-lightning/pull/3140#discussion_r1752506688.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 95.09804% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.62%. Comparing base (6662c5c) to head (fbb3ab2). Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/outbound_payment.rs 78.94% 2 Missing and 2 partials :warning:
lightning/src/ln/offers_tests.rs 98.75% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3313 +/- ## ========================================== - Coverage 89.65% 89.62% -0.03% ========================================== Files 126 126 Lines 102546 102622 +76 Branches 102546 102622 +76 ========================================== + Hits 91935 91975 +40 - Misses 7890 7924 +34 - Partials 2721 2723 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

valentinewallace commented 2 months ago

Thanks, yea, I think this looks right, though it'd be nice to include a test that sent the payment MPP.

Adapted the test to use MPP, as a fixup in case you meant a separate test.

valentinewallace commented 2 months ago

Rebased due to silent conflict.

TheBlueMatt commented 2 months ago

LGTM, feel free to squash IMO.

valentinewallace commented 2 months ago

Squashed.

valentinewallace commented 2 months ago

Kicked CI 🤞