Conversation
| atomically(|| self.provider.check_proposal(&prop)).await | ||
| let _ = finality; | ||
| // Proposal acceptance should not depend on local parent-view checks. | ||
| // Deterministic selection is derived from consensus vote extensions. | ||
| true |
There was a problem hiding this comment.
This means a malicious proposer could include an arbitrary TopDownExec message and every honest validator would accept it. The vote extensions constrain what honest proposers include, but there's nothing preventing a Byzantine proposer from fabricating the result.
Two viable approaches:
a) Keep the local provider check (the old check_proposal). This is the lightweight option: each validator independently verifies that the proposed parent finality (height, hash) is something it has also observed. The trade-off is potential transient liveness issues if validators' caches diverge, which should eventually resolve itself (see this comment). If we do so, though, we'd need to ensure validators lagging behind the parent chain wait on block execution until their cache catches up (same as we do with F3).
b) Include the vote extension quorum proof in the block. The proposer would bundle the relevant subset of vote extensions alongside the top-down message, and validators in ProcessProposal could verify 2/3+ power signed that candidate. This is more robust but adds message overhead and complexity.
Option (a) seems like the pragmatic minimum. Removing it entirely is a security regression. (And we don't know how long this "temporary" fix will have to remain in our codebase.)
| let top_down_msg = match topdown_override { | ||
| Some(maybe_finality) => maybe_finality.map(|finality| { | ||
| ChainMessage::Ipc(IpcMessage::TopDownExec(ParentFinality { | ||
| height: finality.height as fvm_shared::clock::ChainEpoch, | ||
| block_hash: finality.block_hash, | ||
| })) | ||
| }), | ||
| None => self.top_down_manager.chain_message_for_proposal().await, | ||
| }; |
There was a problem hiding this comment.
When local_last_commit is absent (first block after genesis or right after enabling vote extensions), this falls back to chain_message_for_proposal() which now proposes unilaterally from the local cache — and attest_legacy no longer validates it. Consider returning None here instead, skipping the top-down message for the first block until vote extensions are available. One block of delay at genesis is negligible, and it avoids a window where a single proposer can commit arbitrary parent finality.
| pub async fn vote_extension_candidate(&self) -> Option<IPCParentFinality> { | ||
| atomically(|| self.provider.next_proposal()).await | ||
| } |
There was a problem hiding this comment.
next_proposal() uses max_proposal_range and proposal_delay to pick a candidate potentially many blocks ahead of last_committed. Since these are off-chain per-node config parameters, validators with different cache states or config values may produce different candidates. Combined with the strict equality check in verify_vote_extension, this makes quorum fragile — any divergence means rejected extensions and no progress.
Consider voting just last_committed + 1 (the first non-null block after the last committed finality) instead. This maximizes convergence because all validators agree on last_committed (on-chain state) and only need minimal parent sync to agree on the next block.
2 participants
No description provided.