Ensure partial MPP claims continue to blocks channels on restart #3928
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
Rebased. |
TheBlueMatt
force-pushed
the
2024-09-future-async-tests
branch
from
490d6e4 to
6c34c56
Compare
|
Needs a rebase |
|
🔔 7th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
Rebased. |
TheBlueMatt
force-pushed
the
2024-09-future-async-tests
branch
from
6c34c56 to
12c37b2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3928 +/- ##
==========================================
+ Coverage 88.94% 89.00% +0.05%
==========================================
Files 174 174
Lines 124201 124299 +98
Branches 124201 124299 +98
==========================================
+ Hits 110472 110632 +160
+ Misses 11251 11187 -64
- Partials 2478 2480 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // If there are monitor updates in flight, we may be in the case | ||
| // described above, replaying a claim on startup which needs an RAA | ||
| // blocker to remain blocked. Thus, in such a case we simply push the | ||
| // post-update action to the blocked list and move on. |
There was a problem hiding this comment.
Based on an existing test, it seems like we can also hit this case when !during_init, but the test failure is test_glacial_peer_cant_hang so probably rare enough to not need to document.
There was a problem hiding this comment.
Hmm, if I put an assert!(during_init) in the Some(raa_blocker) branch I don't see it get hit at all? I think it also makes sense - raa blockers are only used when claiming payments, not forwarded HTLCs, and hitting a DuplicateClaim for a payment at runtime seems weird - we shouldn't be double-claiming something cause we deduplicate on the claimable_payments set.
There was a problem hiding this comment.
I'm talking about adding an assert here:
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index b60b0419a..8b7f89119 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -8302,6 +8302,7 @@ where
// post-update action to the blocked list and move on.
let in_flight_mons = peer_state.in_flight_monitor_updates.get(&chan_id);
if in_flight_mons.map(|(_, mons)| !mons.is_empty()).unwrap_or(false) {
+ assert!(during_init);
peer_state
.monitor_update_blocked_actions
.entry(chan_id)So the case where we have (1) a duplicate claim, (2) in-flight monitor updates present and (3) a post-update action whilst !during_init.
There was a problem hiding this comment.
I went ahead and tweaked the comment, but it shouldn't matter too much
$ git diff-tree -U1 c15c1ecbd6 55eea49cd0
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index b60b0419a2..229d594d06 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -8302,2 +8302,4 @@ where
// post-update action to the blocked list and move on.
+ // In any case, we should air on the side of caution and not process
+ // the post-update action no matter the situation.
let in_flight_mons = peer_state.in_flight_monitor_updates.get(&chan_id);
TheBlueMatt
force-pushed
the
2024-09-future-async-tests
branch
from
12c37b2 to
c15c1ec
Compare
|
Tweaked the commit message. |
TheBlueMatt
force-pushed
the
2024-09-future-async-tests
branch
from
c15c1ec to
55eea49
Compare
In 9cc6e08, we started using the `RAAMonitorUpdateBlockingAction` logic to block RAAs which may remove a preimage from one `ChannelMonitor` if it isn't durably stored in another that is a part of the same MPP claim. Then, in 254b78f, when we claimed a payment, if we saw that the HTLC was already claimed in the channel, we'd simply drop the new RAA blocker. This can happen on reload when replaying MPP claims. However, just because an HTLC is no longer present in `ChannelManager`'s `Channel`, doesn't mean that the `ChannelMonitorUpdate` which stored the preimage actually made it durably into the `ChannelMonitor` on disk. We could begin an MPP payment, have one channel get the preimage durably into its `ChannelMonitor`, then step forward another update with the peer. Then, we could reload, causing the MPP claims to be replayed across all chanels, leading to the RAA blocker(s) being dropped and all channels being unlocked. Finally, if the first channel managed to step forward a further update with its peer before the (now-replayed) `ChannelMonitorUpdate`s for all MPP parts make it to disk we could theoretically lose the preimage. This is, of course, a somewhat comically unlikely scenario, but I had an old note to expand the test and it turned up the issue, so we might as well fix it.
TheBlueMatt
force-pushed
the
2024-09-future-async-tests
branch
from
55eea49 to
583a9a3
Compare
|
I believe its safe to backport this as-is. |
In 9cc6e08, we started using the
RAAMonitorUpdateBlockingActionlogic to block RAAs which may remove a preimage from oneChannelMonitorif it isn't durably stored in another that is a part of the same MPP claim.At the time, when we claimed a payment, if we saw that the HTLC was already claimed in the channel, we'd simply drop the new RAA blocker. This can happen on reload when replaying MPP claims.
However, just because an HTLC is no longer present in
ChannelManager'sChannel, doesn't mean that theChannelMonitorUpdatewhich stored the preimage actually made it durably into theChannelMonitoron disk.We could begin an MPP payment, have one channel get the preimage durably into its
ChannelMonitor, then step forward another update with the peer. Then, we could reload, causing the MPP claims to be replayed across all chanels, leading to the RAA blocker(s) being dropped and all channels being unlocked. Finally, if the first channel managed to step forward a further update with its peer before the (now-replayed)ChannelMonitorUpdates for all MPP parts make it to disk we could theoretically lose the preimage.This is, of course, a somewhat comically unlikely scenario, but I had an old note to expand the test and it turned up the issue, so we might as well fix it.