-
Notifications
You must be signed in to change notification settings - Fork 418
Fix update_id
gap during force_shutdown
#3858
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
Conversation
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
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.
Thanks! This should work, but do you mind including a test?
There are other scenarios where we also increment by 1, but those should not result in a panic as they are queued in blocked_monitor_updates
. The force close update is the only one we let fly through regardless of blocked_monitor_updates
.
446155b
to
5e6e74c
Compare
Got it! Will add a test later. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3858 +/- ##
==========================================
- Coverage 89.65% 89.62% -0.04%
==========================================
Files 164 164
Lines 134658 134661 +3
Branches 134658 134661 +3
==========================================
- Hits 120734 120688 -46
- Misses 11246 11292 +46
- Partials 2678 2681 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thanks. I think there's a few more changes I want to make to this pipeline but this change by itself looks good.
@whfuyn do you mind rebasing this? We're going to merge this as is to include it in a release and follow up with a test later. |
5e6e74c
to
0866405
Compare
When a channel is force-closed, there might be blocked monitor updates not yet applied. But `latest_monitor_update_id` has been incremented and assigned to these updates. This results in a panic when trying to apply the `ChannelForceClosed` update. Use the unblocked update id instead. Resolves: lightningdevkit#3857
0866405
to
ceb5a55
Compare
@wpaulino Sorry for the late response. I had some trouble constructing a proper test without using MPP or multi-hop payments. My original test that triggered this problem only involved sending payments between two nodes connected by a single direct channel. However, when I searched the code related to I've rebased this PR. |
A |
Backported in #3932 |
v0.1.5 - Jul 16, 2025 - "Async Path Reduction" Performance Improvements ======================== * `NetworkGraph`'s expensive internal consistency checks have now been disabled in debug builds in addition to release builds (lightningdevkit#3687). Bug Fixes ========= * Pathfinding which results in a multi-path payment is now substantially smarter, using fewer paths and better optimizing fees and successes (lightningdevkit#3890). * A counterparty delaying claiming multiple HTLCs with different expiries can no longer cause our `ChannelMonitor` to continuously rebroadcast invalid transactions or RBF bump attempts (lightningdevkit#3923). * Reorgs can no longer cause us to fail to claim HTLCs after a counterparty delayed claiming multiple HTLCs with different expiries (lightningdevkit#3923). * Force-closing a channel while it is blocked on another channel's async `ChannelMonitorUpdate` can no longer lead to a panic (lightningdevkit#3858). * `ChannelMonitorUpdate`s can no longer be released to storage too early when doing async updates or on restart. This only impacts async `ChannelMonitorUpdate` persistence and can lead to loss of funds only in rare cases with `ChannelMonitorUpdate` persistence order inversions (lightningdevkit#3907). Security ======== 0.1.5 fixes a vulnerability which could allow a peer to overdraw their reserve value, potentially cutting into commitment transaction fees on channels with a low reserve. * Due to a bug in checking whether an HTLC is dust during acceptance, near-dust HTLCs were not counted towards the commitment transaction fee, but did eventually contribute to it when we built a commitment transaction. This can be used by a counterparty to overdraw their reserve value, or, for channels with a low reserve value, cut into the commitment transaction fee (lightningdevkit#3933).
When a channel is force-closed, there might be blocked monitor updates not yet applied. But
latest_monitor_update_id
has been incremented and assigned to these updates. This results in a panic when trying to apply theChannelForceClosed
update. Use the unblocked update id instead.Resolves: #3857