chore: updated follow executor feedback - #7350
Conversation
|
cyclops audit fast |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
This change separates a follower's certificate-selected execution head from block-driven safe/finalized updates, but a future certificate head can prevent delivered blocks from becoming canonical. One verified high-severity regression and one lower-severity invariant issue require attention.
Reviewer Callouts
- ⚡ Acknowledgement after
SYNCING(actor.rs:287):newPayloadacceptsSYNCINGand the block is still acknowledged. Confirm that marshal redelivery is not required if Reth's bounded block buffer later evicts that payload. - ⚡ Marshal floor dependency (
actor.rs:200-245): verify with a real Reth follower that canonical height, finalized state, persistence, and marshal archive pruning all continue under a multi-block certificate lead. - ⚡ Round provenance (
fcu.rs:27,56): document or test the assumption that a block's embedded consensus round equals the certificate round used to select the head.
| let forkchoice = self | ||
| .latest_fcu | ||
| .update_finalized(&block) | ||
| .then_some(self.latest_fcu); |
There was a problem hiding this comment.
🚨 [SECURITY] Future certificate head stalls follower canonicalisation and finality
This snapshots latest_fcu after advancing finality, but update_finalized preserves a later certificate digest as the head. When certificates lead block delivery, the post-newPayload FCU names a head Reth has not received. Reth returns SYNCING without applying safe/finalized state or advancing its canonical head; under sustained lead, canonicalisation, persistence, pruning, and the marshal floor stall while retained execution state grows without bound. The verifier reproduced this with a two-block certificate lead.
Recommended Fix:
Use the delivered block as the head for its block-driven FCU so Reth can canonicalise it. Keep the later certificate head for standalone/heartbeat FCUs, or otherwise clamp this FCU to an execution-layer-known head.
| } | ||
|
|
||
| pub(super) fn update_head(&mut self, round: Round, digest: Digest) { | ||
| if self.head.0.is_none_or(|current| round > current) { |
There was a problem hiding this comment.
Forkchoice tracks the head's round but not its height, so this accepts any certificate when the current round is absent and cannot ensure the head remains at or above finalized. Current T4 monotonicity makes a malformed state difficult to reach, but the type permits it; Reth rejects such an inconsistent FCU and the follower propagates that rejection as a fatal subsystem exit.
Recommended Fix:
Track head height and reject updates that place it below finalized; assert the invariant before conversion to ForkchoiceState. Recover safely from an execution-layer inconsistent-forkchoice response rather than terminating the follower where possible.
There was a problem hiding this comment.
Non-issue, for finalized heights, round is monotonically increasing
3 participants
No description provided.