lightningdevkit / rust-lightning

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

Block monitor updates to ensure preimages are in each MPP part #3120

Open TheBlueMatt opened 2 weeks ago

TheBlueMatt commented 2 weeks ago

If we claim an MPP payment and only persist some of the ChannelMonitorUpdates which include the preimage prior to shutting down, we may be in a state where some of our ChannelMonitors have the preimage for a payment while others do not.

This, it turns out, is actually mostly safe - on startup ChanelManager will detect there's a payment it has as unclaimed but there's a corresponding payment preimage in a ChannelMonitor and go claim the other MPP parts. This works so long as the ChannelManager has been persisted after the payment has been received but prior to the PaymentClaimable event being processed (and the claim itself occurring). This is not always true and certainly not required on our API, but our lightning-background-processor today does persist prior to event handling so is generally true subject to some race conditions.

In order to address this we need to use copy payment preimages across channels irrespective of the ChannelManager's payment state, but this introduces another wrinkle - if one channel makes substantial progress while other channel(s) are still waiting to get the payment preimage in ChannelMonitor(s) while the ChannelManager hasn't been persisted after the payment was received, we may end up without the preimage on disk.

Here, we address this issue by using the new RAAMonitorUpdateBlockingAction variant for this specific case. We block persistence of an RAA ChannelMonitorUpdate which may remove the preimage from disk until all channels have had the preimage added to their ChannelMonitor.

We do this only in-memory (and not on disk) as we can recreate this blocker during the startup re-claim logic.

This will enable us to claim MPP parts without using the ChannelManager's payment state in later work.

codecov-commenter commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 87.30650% with 41 lines in your changes missing coverage. Please review.

Project coverage is 90.06%. Comparing base (88e1b56) to head (bd1a822). Report is 7 commits behind head on main.

Files Patch % Lines
lightning/src/ln/channelmanager.rs 82.51% 29 Missing and 10 partials :warning:
lightning/src/ln/chanmon_update_fail_tests.rs 97.95% 2 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3120 +/- ## ========================================== + Coverage 89.83% 90.06% +0.22% ========================================== Files 121 121 Lines 98900 101336 +2436 Branches 98900 101336 +2436 ========================================== + Hits 88847 91267 +2420 - Misses 7457 7517 +60 + Partials 2596 2552 -44 ```

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

valentinewallace commented 2 weeks ago

Needs rebase + not building for me locally

TheBlueMatt commented 2 weeks ago

Rebased and fixed the issue.

TheBlueMatt commented 1 week ago

Rebased and addressed comments.

valentinewallace commented 5 days ago

LGTM after outstanding feedback and Jeff takes another look. Would be good with a squash.