-
Notifications
You must be signed in to change notification settings - Fork 339
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
Immediately unblock channels on duplicate claims #2661
Immediately unblock channels on duplicate claims #2661
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First high-level pass.
1792bef
to
9c487f4
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2661 +/- ##
==========================================
+ Coverage 89.00% 89.63% +0.63%
==========================================
Files 112 112
Lines 87207 91365 +4158
Branches 87207 91365 +4158
==========================================
+ Hits 77619 81897 +4278
+ Misses 7353 7231 -122
- Partials 2235 2237 +2
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still going through a first pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably ready for a 2nd reviewer
lightning/src/ln/channelmanager.rs
Outdated
let fee_earned_msat = if let Some(claimed_htlc_value) = htlc_claim_value_msat { | ||
Some(claimed_htlc_value - forwarded_htlc_value) | ||
} else { None }; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move this to where the event is generated below. Can we also stop gating this whole thing on if let Some(forwarded_htlc_value) ..
or add a comment for why we're doing so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, its fine cause we only have None
when claiming from a(n old) monitor, which we dont have to restore, but I'll move it, good idea.
Feel free to squash. |
6ab8b00
to
80792aa
Compare
Pushed a number of further changes so didn't squash yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No real feedback, LGTM after a 2nd reviewer.
80792aa
to
a6d4676
Compare
Squashed with one additional assertion and a comment fix: $ git diff-tree -U3 80792aab a6d4676c
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 217922ca5..b12fc3c86 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -5617,10 +5617,21 @@ where
// There should be a `BackgroundEvent` pending...
assert!(background_events.iter().any(|ev| {
match ev {
- // to apply a monitor update that blocked channel,
+ // to apply a monitor update that blocked the claiming channel,
BackgroundEvent::MonitorUpdateRegeneratedOnStartup {
- funding_txo, ..
- } => *funding_txo == claiming_chan_funding_outpoint,
+ funding_txo, update, ..
+ } => {
+ if *funding_txo == claiming_chan_funding_outpoint {
+ assert!(update.updates.iter().any(|upd|
+ if let ChannelMonitorUpdateStep::PaymentPreimage {
+ payment_preimage: update_preimage
+ } = upd {
+ payment_preimage == *update_preimage
+ } else { false }
+ ), "{:?}", update);
+ true
+ } else { false }
+ },
// or the channel we'd unblock is already closed,
BackgroundEvent::ClosedMonitorUpdateRegeneratedOnStartup((funding_txo, ..))
=> *funding_txo == next_channel_outpoint,
$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, feel free to squash.
This may help in debugging blocking actions in the future.
While we'd previously avoided this, this is sadly now required in the next commit.
When `MonitorUpdateCompletionAction`s were added, we didn't consider the case of a duplicate claim during normal HTLC processing (as the handling only had an `if let` rather than a `match`, which made the branch easy to miss). This can lead to a channel freezing indefinitely if an HTLC is claimed (without a `commitment_signed`), the peer disconnects, and then the HTLC is claimed again, leading to a never-completing `MonitorUpdateCompletionAction`. The fix is simple - if we get back an `UpdateFulfillCommitFetch::DuplicateClaim` when claiming from the inbound edge, immediately unlock the outbound edge channel with a new `MonitorUpdateCompletionAction::FreeOtherChannelImmediately`. Here we add the new variant, which we start generating in the next commit.
When `MonitorUpdateCompletionAction`s were added, we didn't consider the case of a duplicate claim during normal HTLC processing (as the handling only had an `if let` rather than a `match`, which made the branch easy to miss). This can lead to a channel freezing indefinitely if an HTLC is claimed (without a `commitment_signed`), the peer disconnects, and then the HTLC is claimed again, leading to a never-completing `MonitorUpdateCompletionAction`. The fix is simple - if we get back an `UpdateFulfillCommitFetch::DuplicateClaim` when claiming from the inbound edge, immediately unlock the outbound edge channel with a new `MonitorUpdateCompletionAction::FreeOtherChannelImmediately`. Here we implement this fix by actually generating the new variant when a claim is duplicative.
5b71cd9
to
f47270e
Compare
Squashed with a small wording tweak in the log:
|
When
MonitorUpdateCompletionAction
s were added, we didn'tconsider the case of a duplicate claim during normal HTLC
processing (as the handling only had an
if let
rather than amatch
, which made the branch easy to miss). This can lead to achannel freezing indefinitely if an HTLC is claimed (without a
commitment_signed
), the peer disconnects, and then the HTLC isclaimed again, leading to a never-completing
MonitorUpdateCompletionAction
.The fix is simple - if we get back an
UpdateFulfillCommitFetch::DuplicateClaim
when claiming from theinbound edge, immediately unlock the outbound edge channel with a
new
MonitorUpdateCompletionAction::FreeOtherChannelImmediately
.