Correctly handle new ChannelMonitorUpdates to old post-FC chans#4136
Conversation
|
👋 Thanks for assigning @joostjager as a reviewer! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4136 +/- ##
==========================================
+ Coverage 88.62% 88.63% +0.01%
==========================================
Files 180 180
Lines 134895 135243 +348
Branches 134895 135243 +348
==========================================
+ Hits 119546 119877 +331
- Misses 12587 12600 +13
- Partials 2762 2766 +4
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:
|
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
| .get_mut(&channel_id) | ||
| .expect("Channels originating a payment resolution must have a monitor"); | ||
| *update_id += 1; | ||
| *update_id = update_id.saturating_add(1); |
There was a problem hiding this comment.
Given the current and future caveats described in the commit msg, does this need a log on overflow?
There was a problem hiding this comment.
Hmm, not sure if its really needed. Maybe the commit message isn't clear, but I don't see any issues with this with the current code. I do call out that its possible that other code changes in the future which introduces an issue, but I don't think we really need to log a warning on the assumption that future code might break really ancient channel closes.
There was a problem hiding this comment.
No, I think the commit message is clear.
For someone looking at this statement though, it must be hard to understand why this is safe. I thought a warning log would both help debug any future issue with this, and at the same time add a bit more documentation. Or maybe move some of the commit message to a code comment.
There was a problem hiding this comment.
I added comments.
There was a problem hiding this comment.
It's still pretty thin and doesn't explain why it is okay to not increment when at the max already.
|
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
TheBlueMatt
force-pushed
the
2025-09-update_id-overflow
branch
from
0813413 to
ecc0da7
Compare
In 0.1 (1481216) we started setting `ChannelMonitorUpdate::update_id` to a non-`u64::MAX` value for updates generated after a channel has been closed. This is great, but in 71a364c we then started calculating the next `update_id` by incrementing the last `update_id` we saw when we started and were looking at the `ChannelMonitor`. However, the last-applied `update_id` may well be `u64::MAX` for old `ChannelMonitor`s which were closed prior to 0.1. In that case the increment would overflow. Here we fix this naively by simply replacing the increment with a `saturating_add`. While its possible this will result in a `ChannelMonitorUpdate` being tracked as in-flight (only for the `ReleasePaymentComplete` updates added in 71a364c) at the same `update_id` as other updates already in-flight and handling post-`ChannelMonitorUpdate` actions too early, this should only apply to releasing payment complete updates, which have no post-`ChannelMonitorUpdate` action. Its also possible that this leads to a regression in the future, where we have some new post-closure update that does have a post-`ChannelMonitorUpdate` action and we run it too early, but by then presumably its fairly rare to have a `ChannelMonitor` for a channel closed pre-0.1 that still needs multiple updates.
TheBlueMatt
force-pushed
the
2025-09-update_id-overflow
branch
from
ecc0da7 to
dd21fce
Compare
| .get_mut(&channel_id) | ||
| .expect("Channels originating a payment resolution must have a monitor"); | ||
| *update_id += 1; | ||
| *update_id = update_id.saturating_add(1); |
There was a problem hiding this comment.
It's still pretty thin and doesn't explain why it is okay to not increment when at the max already.
4 participants
In 0.1 (1481216) we started setting
ChannelMonitorUpdate::update_idto a non-u64::MAXvalue for updates generated after a channel has been closed.This is great, but in 71a364c we then started calculating the next
update_idby incrementing the lastupdate_idwe saw when we started and were looking at theChannelMonitor. However, the last-appliedupdate_idmay well beu64::MAXfor oldChannelMonitors which were closed prior to 0.1. In that case the increment would overflow.Here we fix this naively by simply replacing the increment with a
saturating_add. While its possible this will result in aChannelMonitorUpdatebeing tracked as in-flight (only for theReleasePaymentCompleteupdates added in 71a364c) at the sameupdate_idas other updates already in-flight and handling post-ChannelMonitorUpdateactions too early, this should only apply to releasing payment complete updates, which have no post-ChannelMonitorUpdateaction.Its also possible that this leads to a regression in the future, where we have some new post-closure update that does have a post-
ChannelMonitorUpdateaction and we run it too early, but by then presumably its fairly rare to have aChannelMonitorfor a channel closed pre-0.1 that still needs multiple updates.