feat: Let followers import zone blocks from leader sequencers#668
Conversation
# Conflicts: # Cargo.toml # crates/node/src/cli.rs # crates/p2p/README.md # crates/p2p/src/network.rs # crates/p2p/src/runtime.rs
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This PR adds leader-to-follower block replication, but the current implementation has verified safety and liveness issues around delivery guarantees, oversized block handling, Zone-specific validation, and deterministic replay of L1-derived state. I left the actionable findings inline.
Reviewer Callouts
- ⚡ Follower role vs sequencer config:
crates/node/src/node.rs:629still startsZoneEnginewheneversequencer_configexists, while follower behavior is keyed off the P2P role. A node configured as both follower and sequencer should hard-fail rather than both importing leader blocks and producing local blocks. - ⚡ Block-channel memory/backpressure budget:
MAX_MESSAGE_SIZE = 10 MiB,BLOCK_BACKLOG = 128, and receive-side role filtering after delivery mean authenticated peers can impose non-trivial memory/bandwidth pressure. Review Commonware queueing/backpressure before multi-operator exposure. - ⚡ Broader replay determinism: The policy and portal-storage findings are concrete examples of the same class: follower replay currently depends on node-local L1-derived providers. When fixing them, audit every EVM-visible L1 read, especially “latest” fallback paths, and ensure imported block bytes or follower derivation fully determine execution inputs.
| }) | ||
| .await; | ||
| let sent = match sent { | ||
| Ok(sent) => sent?, |
There was a problem hiding this comment.
🚨 [SECURITY] Oversized canonical blocks can permanently stop leader-to-follower replication
sender.send(...) errors are re-raised here. Since the P2P channel is capped at 10 MiB and the leader broadcasts full RLP blocks without a byte-size cap, a canonical block over the Commonware limit returns MessageTooLarge, terminates run_commands, stops the P2P runtime, and leaves followers permanently behind with no backfill.
Recommended Fix:
Handle MessageTooLarge/send errors non-fatally, enforce a block byte cap below the P2P limit or chunk large blocks, and make unexpected P2P/broadcast termination restart or fail the node.
There was a problem hiding this comment.
At 300m gas, the theorotical max is 7.5Mb (claude estimate) so i bumped it to 20MB by default for now. However, this point is moot because if the block size exceeds the max, then the chain will stall anyway since there won't be enough quorum in the ZonePortal to advance the batch
There was a problem hiding this comment.
For reference, this number is 8MB on tempo L1
| } | ||
|
|
||
| // 3. All txns in the block execute properly | ||
| let payload = ZonePayloadTypes::block_to_payload(block, None); |
There was a problem hiding this comment.
🚨 [SECURITY] Follower import relies on NoopConsensus instead of Zone validation
This wraps authenticated leader bytes directly into an execution payload. The Zone node is wired with NoopConsensus, so Reth's normal structural/header/receipt/gas validation hooks and Zone-specific derivation checks are not enforced; a compromised configured leader can canonicalize executable but non-Zone-valid blocks on followers.
Recommended Fix:
Use a Zone-specific consensus/derivation validator for imported blocks and verify the expected advanceTempo grammar, withdrawal rules, absence of extra privileged system calls, and L1-derived inputs before forkchoice.
There was a problem hiding this comment.
All this is ultimately the provers job. The NoopConsensus is intentional, since we don't want any additional concensus at the zone layer.
| .send(Recipients::Some(followers.clone()), block.clone(), true) | ||
| .await | ||
| .map_err(|err| eyre::eyre!("failed broadcasting zone block: {err}"))?; | ||
| if !sent.is_empty() || followers.is_empty() { |
There was a problem hiding this comment.
Returning once sent is non-empty treats delivery to any one follower as success. A temporarily disconnected configured follower gets no retry for this block, and if it later receives N+1, import_leader_block rejects the gap because backfill is not implemented. That follower remains stale indefinitely.
Recommended Fix:
Track per-follower delivery/ACK high-water marks and add backfill or staged-sync recovery. At minimum, warn/metric partial delivery and do not treat it as sufficient for HA/failover assumptions.
There was a problem hiding this comment.
Backfill is fixed in another PR
…k_import # Conflicts: # crates/node/src/node.rs # crates/node/tests/it/utils.rs
There was a problem hiding this comment.
can we move these functions somewhere else and keep this file to ZoneNode
There was a problem hiding this comment.
moved to its own file
| while let Some(persisted_tip) = persisted.next().await { | ||
| if persisted_tip.number < last_broadcast { |
There was a problem hiding this comment.
I believe there's a race condition here, inverse to what's mentioned above:
// Subscribe before reading the database height so persistence cannot race task startup.
let mut persisted = provider.persisted_block_stream();
let mut last_broadcast = match provider.last_block_number() {
last_block_number can already include the newest message in the stream I believe
There was a problem hiding this comment.
Oh good. Catch. Fixed + added test
| // At 30M gas, calldata is bounded below 7.5 MiB; leave headroom for block overhead. | ||
| pub(crate) const MAX_MESSAGE_SIZE: u32 = 20 * 1024 * 1024; |
There was a problem hiding this comment.
this should be increased, we dont expect malicious messages anyway
There was a problem hiding this comment.
I'll fix this in another PR, but there's a thread on slack about limiting this in the block builder (like we do in Tempo L1) so we don't end up with an unbroadcastalbe block
|
Hi @mattsse — your changes-requested review was detected by Voight-Kampff but no live Voight-Kampff agent connection received it, so Voight-Kampff sent a push fallback. The push was not approved before the approval window closed. Your review did not count toward this PR. You can also try requesting changes directly via CLI with |
# Conflicts: # crates/node/tests/it/e2e.rs # crates/node/tests/it/utils.rs
| if let Err(err) = import_leader_block(&provider, &engine, &block).await { | ||
| tracing::error!(target: "zone::p2p", %err, "Rejected leader block"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This logs an import failure and continues, but the local head has not advanced. Every subsequent leader block will then be rejected as a gap, so the follower remains running and serving a permanently stale chain.
We should either terminate the task or try to backfill when a leader block cant be imported. Silently continuing makes the failurelook recoverable but IIUC it isnt.
Note: