Drop the need for fork headers when calling Listen's disconnect #3876
+136
−135
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
👋 I see @joostjager was un-assigned. |
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
jkczyz
reviewed
Jun 19, 2025
|
👋 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
jkczyz
approved these changes
Jun 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.