-
Notifications
You must be signed in to change notification settings - Fork 421
Avoid force-closing 0-conf channels when funding is reorg'd #4231
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
base: main
Are you sure you want to change the base?
Avoid force-closing 0-conf channels when funding is reorg'd #4231
Conversation
|
I've assigned @wpaulino as a reviewer! |
When we see a funding transaction for one of our chanels reorg'd out, we worry that its possible we've been double-spent and immediately force-close the channel to avoid accepting any more HTLCs on it. This isn't ideal, but is mostly fine as most nodes require 6 confirmations and 6 block reorgs are exceedingly rare. However, this isn't so okay for 0-conf channels - in that case we elected to trust the funder anyway, so reorgs shouldn't worry us. Still, to handle this correctly we needed to track the old SCID and ensure our logic is safe across an SCID change. Luckily, we did that work for splices, and can now take advantage of it here. Fixes lightningdevkit#3836.
83e1ae9 to
b4f7fdf
Compare
| // Reset the original short_channel_id so that we'll generate a closure | ||
| // `channel_update` broadcast event. | ||
| self.funding.short_channel_id = original_scid; | ||
| let err_reason = format!("Funding transaction was un-confirmed. Locked at {} confs, now have {} confs.", |
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.
Nit: no point in logging funding_tx_confirmations when we know it's 0
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
|
✅ Added second reviewer: @tankyleo |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4231 +/- ##
==========================================
+ Coverage 89.33% 89.36% +0.02%
==========================================
Files 180 180
Lines 138353 138616 +263
Branches 138353 138616 +263
==========================================
+ Hits 123598 123870 +272
+ Misses 12132 12123 -9
Partials 2623 2623
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:
|
| } else { | ||
| debug_assert!(false); | ||
| } | ||
| if self.context.minimum_depth(&self.funding).expect("set for a ready channel") > 1 { |
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.
Did we also not want to force close for 1-conf minimum channels ? Zero-conf channels have min depth 0 iiuc
When we see a funding transaction for one of our chanels reorg'd out, we worry that its possible we've been double-spent and immediately force-close the channel to avoid accepting any more HTLCs on it. This isn't ideal, but is mostly fine as most nodes require 6 confirmations and 6 block reorgs are exceedingly rare.
However, this isn't so okay for 0-conf channels - in that case we elected to trust the funder anyway, so reorgs shouldn't worry us. Still, to handle this correctly we needed to track the old SCID and ensure our logic is safe across an SCID change. Luckily, we did that work for splices, and can now take advantage of it here.
Fixes #3836.