From 57b055dd4bd072c6cd0904bbc14d6d9e595f7081 Mon Sep 17 00:00:00 2001 From: SherlockShemol Date: Wed, 3 Dec 2025 12:06:49 +0800 Subject: [PATCH] fix(consensus): prevent double-commit of equivocating blocks in Linearizer The Linearizer could double-commit equivocating blocks at the same (Round, Author) slot because is_committed() checks full BlockRef including Digest. Two blocks with same slot but different digests were treated as separate blocks. Fix: Add is_any_block_at_slot_committed() check to filter ancestors where any block at that slot has already been committed. Attack scenario: 1. Byzantine validator creates Block A (Round 1), commits via main chain 2. Same validator creates Block B (Round 1, different digest) 3. Block B propagates through side chain 4. Later leader commits Block B even though slot was already committed The fix ensures only one block per (Round, Author) slot can be committed. Fixes #24498 Signed-off-by: SherlockShemol --- consensus/core/src/dag_state.rs | 15 + consensus/core/src/lib.rs | 5 + consensus/core/src/linearizer.rs | 18 +- consensus/core/src/test_dag_builder.rs | 12 + .../src/tests/equivocation_commit_test.rs | 349 ++++++++++++++++++ 5 files changed, 397 insertions(+), 2 deletions(-) create mode 100644 consensus/core/src/tests/equivocation_commit_test.rs diff --git a/consensus/core/src/dag_state.rs b/consensus/core/src/dag_state.rs index 7aad2fb5e73b7..9010a437c89f8 100644 --- a/consensus/core/src/dag_state.rs +++ b/consensus/core/src/dag_state.rs @@ -790,6 +790,21 @@ impl DagState { .committed } + /// Returns true if any block at the given slot has been committed. + /// This is used to prevent double-commit of equivocating blocks where + /// two blocks have the same (Round, Author) but different digests. + pub(crate) fn is_any_block_at_slot_committed(&self, slot: Slot) -> bool { + for (_block_ref, block_info) in self.recent_blocks.range(( + Included(BlockRef::new(slot.round, slot.authority, BlockDigest::MIN)), + Included(BlockRef::new(slot.round, slot.authority, BlockDigest::MAX)), + )) { + if block_info.committed { + return true; + } + } + false + } + /// Recursively sets blocks in the causal history of the root block as hard linked, including the root block itself. /// Returns the list of blocks that are newly linked. /// The returned blocks are guaranteed to be above the GC round. diff --git a/consensus/core/src/lib.rs b/consensus/core/src/lib.rs index 91fd68548a8f7..bb75d971d3d2d 100644 --- a/consensus/core/src/lib.rs +++ b/consensus/core/src/lib.rs @@ -50,6 +50,11 @@ mod test_dag_parser; #[path = "tests/randomized_tests.rs"] mod randomized_tests; +/// Equivocation double-commit prevention tests. +#[cfg(test)] +#[path = "tests/equivocation_commit_test.rs"] +mod equivocation_commit_test; + /// Exported Consensus API. pub use authority_node::{ConsensusAuthority, NetworkType}; pub use block::{BlockAPI, CertifiedBlock, CertifiedBlocksOutput}; diff --git a/consensus/core/src/linearizer.rs b/consensus/core/src/linearizer.rs index 005807149130b..f1105b4a12164 100644 --- a/consensus/core/src/linearizer.rs +++ b/consensus/core/src/linearizer.rs @@ -9,7 +9,7 @@ use itertools::Itertools; use parking_lot::RwLock; use crate::{ - block::{BlockAPI, VerifiedBlock}, + block::{BlockAPI, Slot, VerifiedBlock}, commit::{Commit, CommittedSubDag, TrustedCommit, sort_sub_dag_blocks}, context::Context, dag_state::DagState, @@ -25,6 +25,10 @@ pub(crate) trait BlockStoreAPI { fn set_committed(&mut self, block_ref: &BlockRef) -> bool; fn is_committed(&self, block_ref: &BlockRef) -> bool; + + /// Returns true if any block at the given slot has been committed. + /// This is used to prevent double-commit of equivocating blocks. + fn is_any_block_at_slot_committed(&self, slot: Slot) -> bool; } impl BlockStoreAPI @@ -45,6 +49,10 @@ impl BlockStoreAPI fn is_committed(&self, block_ref: &BlockRef) -> bool { DagState::is_committed(self, block_ref) } + + fn is_any_block_at_slot_committed(&self, slot: Slot) -> bool { + DagState::is_any_block_at_slot_committed(self, slot) + } } /// Expand a committed sequence of leader into a sequence of sub-dags. @@ -183,7 +191,13 @@ impl Linearizer { .iter() .copied() .filter(|ancestor| { - ancestor.round > gc_round && !dag_state.is_committed(ancestor) + ancestor.round > gc_round + && !dag_state.is_committed(ancestor) + // Also check if any block at this slot has been committed. + // This prevents double-commit of equivocating blocks where + // Block A and Block B have the same (Round, Author) but + // different digests. + && !dag_state.is_any_block_at_slot_committed((*ancestor).into()) }) .collect::>(), ) diff --git a/consensus/core/src/test_dag_builder.rs b/consensus/core/src/test_dag_builder.rs index e631b7ff9c1f9..3f7aa5744e495 100644 --- a/consensus/core/src/test_dag_builder.rs +++ b/consensus/core/src/test_dag_builder.rs @@ -185,6 +185,18 @@ impl DagBuilder { .map(|(_, committed)| *committed) .expect("Block should be found in store") } + + fn is_any_block_at_slot_committed(&self, slot: Slot) -> bool { + for (block_ref, (_block, committed)) in self.blocks.range(( + Included(BlockRef::new(slot.round, slot.authority, BlockDigest::MIN)), + Included(BlockRef::new(slot.round, slot.authority, BlockDigest::MAX)), + )) { + if *committed { + return true; + } + } + false + } } let mut storage = BlockStorage { diff --git a/consensus/core/src/tests/equivocation_commit_test.rs b/consensus/core/src/tests/equivocation_commit_test.rs new file mode 100644 index 0000000000000..491bdb82dfcd2 --- /dev/null +++ b/consensus/core/src/tests/equivocation_commit_test.rs @@ -0,0 +1,349 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +//! Tests for equivocation handling in the Linearizer. +//! +//! ## The Bug +//! +//! Before the fix, `linearize_sub_dag()` used `is_committed(BlockRef)` which checks +//! the full BlockRef including Digest. Two equivocating blocks with the same +//! (Round, Author) but different Digests were treated as different blocks. +//! +//! This could lead to double-commit: if Block A is committed, Block B (same slot, +//! different digest) could still be committed later via a different DAG path. +//! +//! ## The Fix +//! +//! Added `is_any_block_at_slot_committed(Slot)` check to prevent committing any +//! block if another block at the same slot has already been committed. + +use std::sync::Arc; + +use consensus_config::AuthorityIndex; +use consensus_types::block::BlockRef; +use parking_lot::RwLock; + +use crate::{ + block::{BlockAPI, Slot, TestBlock, Transaction, VerifiedBlock}, + context::Context, + dag_state::DagState, + leader_schedule::{LeaderSchedule, LeaderSwapTable}, + linearizer::Linearizer, + storage::mem_store::MemStore, + universal_committer::universal_committer_builder::UniversalCommitterBuilder, +}; + +/// Test that equivocating blocks at the same slot cannot be double-committed. +/// +/// Scenario: +/// 1. Create two equivocating blocks A and B (same Round, Author; different Digest) +/// 2. Build main chain referencing Block A and commit it +/// 3. Insert Block B into DAG +/// 4. Build side chain referencing Block B +/// 5. Commit side chain leader +/// 6. Assert: Block B should NOT be in the committed blocks +#[tokio::test] +async fn test_equivocation_double_commit_prevented() { + // Use production gc_depth = 60 (from sui-protocol-config) + const PRODUCTION_GC_DEPTH: u32 = 60; + + let (mut context, _key_pairs) = Context::new_for_test(4); + context + .protocol_config + .set_consensus_gc_depth_for_testing(PRODUCTION_GC_DEPTH); + let context = Arc::new(context); + let store = Arc::new(MemStore::new()); + let dag_state = Arc::new(RwLock::new(DagState::new(context.clone(), store.clone()))); + + let leader_schedule = Arc::new(LeaderSchedule::new( + context.clone(), + LeaderSwapTable::default(), + )); + + let mut linearizer = Linearizer::new(context.clone(), dag_state.clone()); + let committer = UniversalCommitterBuilder::new( + context.clone(), + leader_schedule.clone(), + dag_state.clone(), + ) + .with_number_of_leaders(1) + .build(); + + // Equivocator is Authority 1 (not own_index=0) + let equivocator: u32 = 1; + let equivocation_round: u32 = 1; + + // Get genesis refs + let genesis_refs: Vec = dag_state + .read() + .get_last_cached_block_per_authority(1) + .iter() + .map(|(b, _)| b.reference()) + .collect(); + + // Create equivocating Block A + let block_a = VerifiedBlock::new_for_test( + TestBlock::new(equivocation_round, equivocator) + .set_ancestors(genesis_refs.clone()) + .set_transactions(vec![Transaction::new(b"Block A".to_vec())]) + .set_timestamp_ms(1000) + .build(), + ); + let ref_a = block_a.reference(); + dag_state.write().accept_block(block_a.clone()); + + // Create other Round 1 blocks + for author in 0..4u32 { + if author == equivocator { + continue; + } + let mut auth_ancestors = genesis_refs.clone(); + if let Some(pos) = auth_ancestors + .iter() + .position(|a| a.author.value() == author as usize) + { + auth_ancestors.swap(0, pos); + } + let block = VerifiedBlock::new_for_test( + TestBlock::new(1, author) + .set_ancestors(auth_ancestors) + .set_transactions(vec![Transaction::new(vec![author as u8])]) + .set_timestamp_ms(1000 + author as u64) + .build(), + ); + dag_state.write().accept_block(block); + } + + // Build main chain Rounds 2-6 + for round in 2..=6u32 { + let prev_round_refs: Vec = { + let dag = dag_state.read(); + let mut refs = vec![]; + for author in 0..4u32 { + let slot = Slot::new(round - 1, AuthorityIndex::new_for_test(author)); + let blocks = dag.get_uncommitted_blocks_at_slot(slot); + if let Some(b) = blocks.first() { + refs.push(b.reference()); + } + } + refs + }; + + for author in 0..4u32 { + let mut auth_ancestors = prev_round_refs.clone(); + if let Some(pos) = auth_ancestors + .iter() + .position(|a| a.author.value() == author as usize) + { + auth_ancestors.swap(0, pos); + } + let block = VerifiedBlock::new_for_test( + TestBlock::new(round, author) + .set_ancestors(auth_ancestors) + .set_transactions(vec![Transaction::new(vec![round as u8, author as u8])]) + .set_timestamp_ms(round as u64 * 1000 + author as u64) + .build(), + ); + dag_state.write().accept_block(block); + } + } + + // Commit main chain (includes Block A) + let last_decided = Slot::new(0, AuthorityIndex::new_for_test(0)); + let decided_leaders = committer.try_decide(last_decided); + + let mut first_leader_slot = Slot::new(0, AuthorityIndex::new_for_test(0)); + let mut block_a_committed = false; + + if let Some(decided) = decided_leaders.first() { + if let Some(leader_block) = decided.clone().into_committed_block() { + first_leader_slot = Slot::new(leader_block.round(), leader_block.author()); + let subdags = linearizer.handle_commit(vec![leader_block.clone()]); + + if let Some(subdag) = subdags.first() { + block_a_committed = subdag.blocks.iter().any(|b| b.reference() == ref_a); + } + } + } + + assert!(block_a_committed, "Block A should be committed in main chain"); + + // Create equivocating Block B + let block_b = VerifiedBlock::new_for_test( + TestBlock::new(equivocation_round, equivocator) + .set_ancestors(genesis_refs.clone()) + .set_transactions(vec![Transaction::new(b"Block B".to_vec())]) + .set_timestamp_ms(1001) + .build(), + ); + let ref_b = block_b.reference(); + dag_state.write().accept_block(block_b.clone()); + + // Verify Block A and B are at the same slot with different digests + assert_eq!(ref_a.round, ref_b.round); + assert_eq!(ref_a.author, ref_b.author); + assert_ne!(ref_a.digest, ref_b.digest); + + // Get Round 6 refs for side chain + let round_6_refs: Vec = { + let dag = dag_state.read(); + let mut refs = vec![]; + for author in 0..4u32 { + let slot = Slot::new(6, AuthorityIndex::new_for_test(author)); + let blocks = dag.get_uncommitted_blocks_at_slot(slot); + if let Some(b) = blocks.first() { + refs.push(b.reference()); + } + } + refs + }; + + // Build side chain Round 7 with one block referencing Block B + for author in 0..4u32 { + let mut auth_ancestors = round_6_refs.clone(); + if author == 2 { + // Authority 2 references Block B + auth_ancestors.push(ref_b); + } + if let Some(pos) = auth_ancestors + .iter() + .position(|a| a.author.value() == author as usize) + { + auth_ancestors.swap(0, pos); + } + let block = VerifiedBlock::new_for_test( + TestBlock::new(7, author) + .set_ancestors(auth_ancestors) + .set_transactions(vec![Transaction::new(vec![7u8, author as u8])]) + .set_timestamp_ms(7000 + author as u64) + .build(), + ); + dag_state.write().accept_block(block); + } + + // Build Rounds 8-12 + for round in 8..=12u32 { + let prev_round_refs: Vec = { + let dag = dag_state.read(); + let mut refs = vec![]; + for author in 0..4u32 { + let slot = Slot::new(round - 1, AuthorityIndex::new_for_test(author)); + let blocks = dag.get_uncommitted_blocks_at_slot(slot); + if let Some(b) = blocks.first() { + refs.push(b.reference()); + } + } + refs + }; + + for author in 0..4u32 { + let mut auth_ancestors = prev_round_refs.clone(); + if let Some(pos) = auth_ancestors + .iter() + .position(|a| a.author.value() == author as usize) + { + auth_ancestors.swap(0, pos); + } + let block = VerifiedBlock::new_for_test( + TestBlock::new(round, author) + .set_ancestors(auth_ancestors) + .set_transactions(vec![Transaction::new(vec![round as u8, author as u8])]) + .set_timestamp_ms(round as u64 * 1000 + author as u64) + .build(), + ); + dag_state.write().accept_block(block); + } + } + + // Commit side chain + let decided_leaders_2 = committer.try_decide(first_leader_slot); + + let mut block_b_committed = false; + for decided in &decided_leaders_2 { + if let Some(leader_block) = decided.clone().into_committed_block() { + let subdags = linearizer.handle_commit(vec![leader_block.clone()]); + for subdag in &subdags { + if subdag.blocks.iter().any(|b| b.reference() == ref_b) { + block_b_committed = true; + } + } + } + } + + // THE FIX: Block B should NOT be committed because Block A (same slot) was already committed + assert!( + !block_b_committed, + "Block B should NOT be committed - same slot as Block A which was already committed" + ); +} + +/// Test that is_any_block_at_slot_committed correctly detects committed blocks at a slot. +#[tokio::test] +async fn test_is_any_block_at_slot_committed() { + let (context, _) = Context::new_for_test(4); + let context = Arc::new(context); + let store = Arc::new(MemStore::new()); + let dag_state = Arc::new(RwLock::new(DagState::new(context.clone(), store.clone()))); + + // Use Authority 1 (not own_index=0) to avoid own-equivocation panic + let equivocator: u32 = 1; + + let genesis_refs: Vec = dag_state + .read() + .get_last_cached_block_per_authority(1) + .iter() + .map(|(b, _)| b.reference()) + .collect(); + + // Create two equivocating blocks + let block_a = VerifiedBlock::new_for_test( + TestBlock::new(1, equivocator) + .set_ancestors(genesis_refs.clone()) + .set_transactions(vec![Transaction::new(vec![0xAA])]) + .set_timestamp_ms(1000) + .build(), + ); + + let block_b = VerifiedBlock::new_for_test( + TestBlock::new(1, equivocator) + .set_ancestors(genesis_refs.clone()) + .set_transactions(vec![Transaction::new(vec![0xBB])]) + .set_timestamp_ms(1001) + .build(), + ); + + let ref_a = block_a.reference(); + let ref_b = block_b.reference(); + let slot = Slot::new(1, AuthorityIndex::new_for_test(equivocator)); + + dag_state.write().accept_block(block_a); + dag_state.write().accept_block(block_b); + + // Initially, no block at slot is committed + assert!( + !dag_state.read().is_any_block_at_slot_committed(slot), + "No block should be committed yet" + ); + + // Commit Block A + dag_state.write().set_committed(&ref_a); + + // Now the slot should have a committed block + assert!( + dag_state.read().is_any_block_at_slot_committed(slot), + "Slot should have committed block after committing Block A" + ); + + // Block B's is_committed should still be false (different digest) + assert!( + !dag_state.read().is_committed(&ref_b), + "Block B should not be marked as committed (different digest)" + ); + + // But is_any_block_at_slot_committed should return true + assert!( + dag_state.read().is_any_block_at_slot_committed(slot), + "Slot check should detect Block A is committed" + ); +} +