chore: Update L2 block stream with new block format and checkpoints #19074
Conversation
Flakey Tests🤖 says: This CI run detected 4 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
There was a problem hiding this comment.
The switch to the new models across all components is great, but I'd want to discuss more the change in the getTips interface.
The design I had in mind was to just add another chain tip, also tied to block numbers. So, instead of having latest, proven, finalized block number/hash, we'd have proposed, checkpointed, proven, finalized (again, all block numbers, not checkpoint numbers).
This'd mean that the block stream would still emit block-added whenever a new block was seen, and we'd get a new chain-checkpointed event (similar to chain-proven or chain-finalized) with the block number (not checkpoint number) that was checkpointed.
Adding a checkpoint number directly to L2Tips feels a bit weird to me, since we're not making it clear whether it's the latest checkpointed checkpoint or the latest proven checkpoint. And IIUC that checkpoint number is only needed in the sentinel, though it could be removed by fetching the latest checkpointed block reported by the chain-checkpointed event, then getting the block's checkpoint number, and then fetching the checkpoint.
That said, perhaps a shape for L2 tips that mixes both designs (as in the one I had in mind and the one in this PR) could be something like this? It should make things easier for any downstream component that depends on either checkpoints or blocks.
BlockId = { blockNumber, blockHash, checkpointNumber }
L2Tips = {
proposed: Omit<BlockId, checkpointNumber>,
checkpointed: BlockId,
proven: BlockId,
finalized: BlockId,
}
WDYT?
| // TODO(pw/mbps): Don't convert to legacy blocks here | ||
| const blocks: L2Block[] = (await Promise.all(newBlocks.map(x => this.getBlock(x.number)))).filter(isDefined); | ||
| //const blocks: L2Block[] = (await Promise.all(newBlocks.map(x => this.getBlock(x.number)))).filter(isDefined); |
| if (event.type !== 'checkpoint-added') { | ||
| return; | ||
| } | ||
| const checkpointNumber = CheckpointNumber(event.checkpoint.number); |
There was a problem hiding this comment.
We should type the CheckpointNumber in the event itself
There was a problem hiding this comment.
It already is. That's an error.
| return; | ||
| } | ||
| const checkpointNumber = CheckpointNumber(event.checkpoint.number); | ||
| const [checkpoint] = await this.archiver.getPublishedCheckpoints(checkpointNumber, 1); |
There was a problem hiding this comment.
Why do we need to go back to the archiver to fetch the checkpoint? Can't we use the event.checkpoint directly? Or is it missing the Published info? If so, can we add it to the event instead, given all checkpoints are Published?
There was a problem hiding this comment.
Currently, the L2BlockStream works in 2 different ways. For 'blocks-added' it emits the full blocks, for other events it just emits L2BlockIds. Currently, with checkpoints, it is just emitting CheckpointIds. But yes, this could change to retrieve the checkpoint and emit the whole thing.
I actually started down this exact route of simply adding
I was concerned how this would hold up under e.g. prune scenarios or historic syncing when the link between block number and checkpoint number can change. I figured tracking the However, I admit I didn't consider the semantics of whether this is 'checkpointed' or 'proven' or what. Also it leads to questions such as, should the If we were to adopt your alternative proposal, should the |
I think we can stick to Alternatively, we could have separate
In addition to, not instead of. IMO it should contain block number and hash, and checkpoint number and hash. Though I can't remember if the checkpoint hash was the same as the hash of its last block, in which case having block and checkpoint hash would be redundant. |
| requestResponseHandlers[ReqRespSubProtocol.TX] = txHandler.bind(this); | ||
| } | ||
|
|
||
| // Define the sub protocol validators - This is done within this start() method to gain a callback to the existing validateTx function |
There was a problem hiding this comment.
I think the change to p2p_client to use the tips store caused timing differences in the tests. Those timing differences highlighted something that I think flakes quite a lot and in this branch fails all the time.
We previously initialised all req/resp sub-protocols after starting libp2p. This means that for a short period the node is 'online' but not responsive to handshakes. Libp2p seems perfectly happy with us setting up these sub-protocols without having started the service, so that's what we do now, setup sub-protocols, then start Libp2p.
This PR focuses on 2 main things.
Firtsly updating the L2Tips definition and plumbing that through to all components that report chain tips and/or depend on components that report chain tips. The definition of L2Tips now looks like:
This impacts components such as Archiver, P2PClient, WorldState, Sequencer and the Sentinel.
Secondly the
L2BlockStreamhas been updated to track and emit events for the checkpointed chain.