Drop the need for fork headers when calling Listen's disconnect #3876
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
TheBlueMatt
force-pushed
the
2025-06-disconnect-less-info
branch
from
b045453 to
5942c8b
Compare
The `Listen::block_disconnected` method is nice in that listeners learn about each block disconnected in series. Further, it included the header of the block that is being disconnected to allow the listeners to do some checking that the interface is being used correctly (namely, asserting that the header's block hash matches their current understanding of the best chain). However, this interface has some substantial drawbacks. Namely, the requirement that fork headers be passed in means that restarting with a new node that has no idea about a previous fork leaves us unable to replay the chain at all. Further, while when various listeners were initially written learning about each block disconnected in series seemed useful, but now we no longer rely on that anyway because the `Confirm` interface does not allow for it. Thus, here, we replace `Listen::block_disconnected` with a new `Listen::blocks_disconnected`, taking only information about the fork point/new best chain tip (in the form of its block hash and height) rather than information about previous fork blocks and only requiring a single call to complete multiple block disconnections during a reorg. We also swap to using a single `BestBlock` to describe the new chain tip, in anticipation of future extensions to `BestBlock`. This requires removing some assertions on block disconnection ordering, but because we now provide `lightning-block-sync` and expect users to use it when using the `Listen` interface, these assertions are much less critical.
TheBlueMatt
force-pushed
the
2025-06-disconnect-less-info
branch
from
5942c8b to
ddc0400
Compare
|
👋 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: @joostjager |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3876 +/- ##
==========================================
+ Coverage 89.76% 90.46% +0.70%
==========================================
Files 164 164
Lines 133048 139516 +6468
Branches 133048 139516 +6468
==========================================
+ Hits 119434 126218 +6784
+ Misses 10937 10589 -348
- Partials 2677 2709 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Now that the `Listen` interface allows blocks to be disconnected in batches rather than one at a time, we should test this. Here we add a new `ConnectStyle` for the functional test framework which tests doing so.
When calling `Channel::best_block_updated` we pass it the timestamp of the block we're connecting so that it can track the highest timestamp it has seen. However, in some cases, we don't actually have a timestamp to pass, which `Channel::best_block_updated` will happily ignore as it always takes the `max` of its existing value. Thus, we really should pass a `None` to ensure the API is understandable, which we do here.
TheBlueMatt
force-pushed
the
2025-06-disconnect-less-info
branch
from
b06cbd4 to
a54159c
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
|
||
| for output_info in state_lock.outputs.iter_mut() { | ||
| if output_info.status.confirmation_hash() == Some(block_hash) { | ||
| debug_assert_eq!(output_info.status.confirmation_height(), Some(height)); | ||
| if output_info.status.confirmation_height() > Some(new_best_block.height) { |
There was a problem hiding this comment.
Hmm, not super happy we have to resort to height over hash in a few places like here now, as it would easily would become a footgun if users implemented their own syncing logic.
Can we at least sprinkle in a few more assert/debug_asserts to ascertain that the height difference is exactly 1 everywhere we lean on it?
There was a problem hiding this comment.
Hmm, not super happy we have to resort to height over hash in a few places like here now, as it would easily would become a footgun if users implemented their own syncing logic.
Indeed, it does worry me a bit, though luckily I imagine most folks these days use our lightning-block-sync or lightning-transaction-sync crates, which should be fairly robust at this point.
Can we at least sprinkle in a few more assert/debug_asserts to ascertain that the height difference is exactly 1 everywhere we lean on it?
Sadly we don't lean on this anywhere anymore in the disconnect pipeline. We still rely on it in the connection logic, but those asserts are still there.
There was a problem hiding this comment.
Sadly we don't lean on this anywhere anymore in the disconnect pipeline. We still rely on it in the connection logic, but those asserts are still there.
Hmm, but we could still check that the old best block height is new_best_block.height + 1 in blocks_disconnected above though, and also in a few other places, no?
There was a problem hiding this comment.
Hmm, but we could still check that the old best block height is new_best_block.height + 1 in blocks_disconnected above though, and also in a few other places, no?
No, this is a very specific API change to not require that anymore. ie you can call blocks_disconnected once for many blocks disconnected. This is important for allowing a node to recover from being on a fork as you no longer need to know anything about any blocks that were on the chain you were on, only blocks that are on the new main chain (you just need to be able to figure out a fork point, but its even okay if that fork point is behind the actual fork point).
`Listen` is somewhat quiet on high-level use and even requirements, which we document further here.
The
Listen::block_disconnectedmethod is nice in that listenerslearn about each block disconnected in series. Further, it included
the header of the block that is being disconnected to allow the
listeners to do some checking that the interface is being used
correctly (namely, asserting that the header's block hash matches
their current understanding of the best chain).
However, this interface has some substantial drawbacks. Namely, the
requirement that fork headers be passed in means that restarting
with a new node that has no idea about a previous fork leaves us
unable to replay the chain at all. Further, while when various
listeners were initially written learning about each block
disconnected in series seemed useful, but now we no longer rely on
that anyway because the
Confirminterface does not allow for it.Thus, here, we replace
Listen::block_disconnectedwith a newListen::blocks_disconnected, taking only information about thefork point/new best chain tip (in the form of its block hash and
height) rather than information about previous fork blocks and only
requiring a single call to complete multiple block disconnections
during a reorg.
This requires removing some assertions on block disconnection
ordering, but because we now provide
lightning-block-syncandexpect users to use it when using the
Listeninterface, theseassertions are much less critical.