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.14% +0.37%
==========================================
Files 164 165 +1
Lines 133048 133580 +532
Branches 133048 133580 +532
==========================================
+ Hits 119434 120413 +979
+ Misses 10937 10700 -237
+ Partials 2677 2467 -210 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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. |
1 similar comment
| /// | ||
| /// Each block must be connected in chain order with one call to either | ||
| /// [`Listen::block_connected`] or [`Listen::filtered_block_connected`]. If a call to the | ||
| /// [`Filter`] interface was made during block processing and further transaction(s) from the same |
There was a problem hiding this comment.
Hmm, is this a new requirement? I imagine it's very hard to reliably implement this: keep track of all registered Txids/outputs, then match them on a block, apply the block, then check again if something new was registered, filter the block again, then potentially call once more. Also, why do we require it only here, but not on Confirm?
There was a problem hiding this comment.
This has always been the case, just poorly documented. It is, indeed, not the case on Confirm (which doesn't require "blocks" to be connected in-order at all, though it does require all transactions to be topologically sorted).
| } | ||
| self.chain_listener.block_disconnected(&header.header, header.height); | ||
| } | ||
| if let Some(block) = disconnected_blocks.last() { |
There was a problem hiding this comment.
If we're now happy to use the fork point, why are we keeping a Vec of all disconnected blocks around at all? Shouldn't we now be able to convert this to the new behavior entirely? Seems ChainDifference::common_ancestor would already provide what we need, no?
There was a problem hiding this comment.
Its needed by the cache, which I didn't want to change in this PR - #3876 (comment)
| fn disconnect_blocks(&mut self, mut disconnected_blocks: Vec<ValidatedBlockHeader>) { | ||
| for header in disconnected_blocks.drain(..) { | ||
| fn disconnect_blocks(&mut self, disconnected_blocks: Vec<ValidatedBlockHeader>) { | ||
| for header in disconnected_blocks.iter() { | ||
| if let Some(cached_header) = self.header_cache.block_disconnected(&header.block_hash) { |
There was a problem hiding this comment.
Should we change the Cache API accordingly?
There was a problem hiding this comment.
We could, but it seems somewhat disjoint from this PR, given there's further work to do in lightning-block-sync anyway, so I kinda wanted to keep it as small as can be.
There was a problem hiding this comment.
Mhh, might be good to document somewhere what steps you deem left before you'd consider the transition to this new approach complete. I guess all of them should land before the next release then, to not end up with a half-migrated codebase in the release?
There was a problem hiding this comment.
I mean the API/stuff in lightning in in a perfectly fine state after this PR, and lightning-block-sync just deserves a bit of efficiency in dropping a vec and improving the cache afterwards. This PR obviously doesn't accomplish the goals that #3600 set out to, but is just a step towards it (the "complicated" step that requires making sure all the lightning-internal stuff isn't broken by changing the API).
AFAIU, to get to the point that bitcoind (and all) syncs support swapping to a new chain during a reorg and don't fetch a pile of redundant blocks we need to:
- make
BestBlockcontain a list of 6(ish?) block hashes, not one - use that list in
lightning-block-sync(and eventuallylightning-transaction-sync) to do the sync without leaning on the cache - expand the runtime caching to cache partial chains across clients in the init sync logic
- (eventually) ensure
lightning-block-syncdoesn't have any spare vecs or whatever lying around.
There was a problem hiding this comment.
I still think it would make sense to open an issue for this larger refactoring you're planning, as there are connected aspects to discuss that don't really belong in this PR.
make
BestBlockcontain a list of 6(ish?) block hashes, not one
For example, how do we imagine this to work exactly, especially for the Confirm/lightning-transaction-sync case?
For Esplora, we currently retrieve the tip hash, retrieve the header and the block status (including the hight), before we call best_block_updated, which is already 3 costly RTTs. If we now would require to track the 6 most-recent hashes, we'd need to make at least 5 more subsequent calls (bringing this it at least 8 RTTs per sync round) to retrieve the hashes at [(height-5)..(height-1)]. But, given that these are individual calls, there is no way to tell if any reorg happened during some of these calls, so they are inherently race-y.
For Electrum, the results would be very similar for the polling client in its current form. Note we eventually want to switch the client to a streaming version making use of Electum's subscription model though, and requiring 'last 6 blocks' would probably require us to resort to costly polling again.
If we'd otherwise extend the client to start tracking more of the chain state (i.e., actually tracking a local chain 6-suffix) across rounds this would complicate the client logic quite a bit and make it even more susceptible to race conditions.
TLDR: I still maintain all of this would be/is resulting in a major refactor across our chain syncing crates and logic, and it would be great to discuss what we imagine to be involved and the corresponding trade-offs before we just jump into it heads first.
`OnchainTxHandler` is an internal struct and doesn't implement `Listen`, but its still nice to have its API mirror the `Listen` API so that internal code all looks similar.
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.
`Listen` is somewhat quiet on high-level use and even requirements, which we document further here.
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
TheBlueMatt
force-pushed
the
2025-06-disconnect-less-info
branch
from
ef73b0a to
a6985eb
Compare
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.