Uh oh!
There was an error while loading. Please reload this page.
Drop 353 resolution and fix block updating in OffersMessageFlow, replacing it all with bitcoin-payment-instructions 353 resolution - #4543
Conversation
In 884158d we dropped built-in BIP 353 resolution logic in favor of the `bitcoin-payment-instructions` crate but forgot to do so in the `OffersMessageFlow`. Here we do so.
It seems we forgot to ensure `OffersMessageHandler::best_block` is consistently updated, leading to us building invalid blinded payment paths for short-lived payment paths after two weeks without restart.
👋 I see @wpaulino was un-assigned. |
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
| let timestamp = &self.highest_seen_timestamp; | ||
| let block_time = header.time as usize; | ||
| *self.best_block.write().unwrap() = BestBlock::new(header.block_hash(), height); |
There was a problem hiding this comment.
Nit: BestBlock::new always wipes previous_blocks history. ChannelManager::best_block_updated uses update_for_new_tip instead, which preserves the block-hash history on single-block advances. While OffersMessageFlow doesn't currently use previous_blocks, using update_for_new_tip would be more consistent and future-proof:
| *self.best_block.write().unwrap() = BestBlock::new(header.block_hash(), height); | |
| self.best_block.write().unwrap().update_for_new_tip(header.block_hash(), height); |
There was a problem hiding this comment.
Yea, I'd avoided it to make it easier to backport and because the history isn't used but you're right I should just do it in a separate commit.
Review SummaryOne new minor issue found beyond prior review comments. Inline comments posted
Prior issues (not re-posted)
|
tnull
left a comment
There was a problem hiding this comment.
Ah, whoops, took the leftovers as somewhat intentional in the first PR.
LGTM, might want to address Claude's nit?
| pub fn best_block_updated(&self, header: &Header, height: u32) { | ||
| let timestamp = &self.highest_seen_timestamp; | ||
| let block_time = header.time as usize; | ||
| self.best_block.write().unwrap().update_for_new_tip(header.block_hash(), height); |
There was a problem hiding this comment.
Bug: ChannelManager::blocks_disconnected (the Listen impl, line ~15765 of channelmanager.rs) updates its own self.best_block to the fork point but does not propagate the height decrease to self.flow. This means after a reorg, OffersMessageFlow.best_block.height remains at the old (higher) value, causing max_cltv_expiry at line 335 to be too permissive.
You'd need either:
- A
blocks_disconnected(or more generalset_best_block) method onOffersMessageFlow, called fromChannelManager::blocks_disconnected, or ChannelManager::blocks_disconnectedcallingself.flow.best_block_updated(...)with the fork-point header/height (though the current signature expects aHeader, not just a hash+height).
Since this commit is flagged for backport, it's worth getting the reorg path right too.
There was a problem hiding this comment.
That also seems like a valid concern? @TheBlueMatt
There was a problem hiding this comment.
Not really - if our best block tip is too low during a reorg that's fine - we'll be reconnecting new blocks immediately to catch up to the new tip anyway, so we actually would rather leave the value high than reset and jump ahead again. It is wrong, however, to use update_for_new_tip here as we'll have bunk history. I reset to not using that and left a comment as to why we aren't in its place.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #4543 +/- ##
==========================================
- Coverage 87.09% 86.99% -0.10%
==========================================
Files 163 163 Lines 108856 108635 -221 Branches 108856 108635 -221 ==========================================
- Hits 94808 94511 -297 - Misses 11563 11647 +84 + Partials 2485 2477 -8
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:
|
7c02097 to
5704e8eCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
OffersMessageFlowOffersMessageFlow, replacing it all with bitcoin-payment-instructions 353 resolutionTheBlueMatt
commented
Jun 10, 2026
The offers message flow didn't exist in 0.1 so there's no need to backport there. |
TheBlueMatt
commented
Jun 11, 2026
Backported to 0.2 in #4683. |
The second commit should be backported.