From 5199ee12e911863cae544f345eb0d36e4c877a08 Mon Sep 17 00:00:00 2001 From: Youngjoon Lee Date: Tue, 4 Jul 2023 18:31:33 +0900 Subject: [PATCH] fix: add a guard on the view for LeaderProof validation (#233) --- consensus-engine/src/lib.rs | 222 +++++++++++++++-------- consensus-engine/tests/fuzz/ref_state.rs | 15 +- 2 files changed, 157 insertions(+), 80 deletions(-) diff --git a/consensus-engine/src/lib.rs b/consensus-engine/src/lib.rs index 3f3e94ff..79dc7355 100644 --- a/consensus-engine/src/lib.rs +++ b/consensus-engine/src/lib.rs @@ -63,7 +63,8 @@ impl Carnot { match block.leader_proof { LeaderProof::LeaderId { leader_id } => { // This only accepts blocks from the leader of current_view + 1 - if leader_id != self.overlay.next_leader() { + if leader_id != self.overlay.next_leader() || block.view != self.current_view() + 1 + { return Err(()); } } @@ -392,27 +393,33 @@ impl Carnot { #[cfg(test)] mod test { + use std::convert::Infallible; + use crate::overlay::{FlatOverlay, RoundRobin, Settings}; use super::*; - fn init_from_genesis() -> Carnot> { + fn init(nodes: Vec) -> Carnot> { + assert!(!nodes.is_empty()); + Carnot::from_genesis( - [0; 32], + *nodes.first().unwrap(), Block { view: 0, id: [0; 32], parent_qc: Qc::Standard(StandardQc::genesis()), - leader_proof: LeaderProof::LeaderId { leader_id: [0; 32] }, + leader_proof: LeaderProof::LeaderId { + leader_id: *nodes.first().unwrap(), + }, }, FlatOverlay::new(Settings { - nodes: vec![[0; 32]], + nodes, leader: RoundRobin::default(), }), ) } - fn next_block(block: &Block) -> Block { + fn next_block(engine: &Carnot>, block: &Block) -> Block { let mut next_id = block.id; next_id[0] += 1; @@ -423,14 +430,30 @@ mod test { view: block.view, id: block.id, }), - leader_proof: LeaderProof::LeaderId { leader_id: [0; 32] }, + leader_proof: LeaderProof::LeaderId { + leader_id: engine.overlay().next_leader(), + }, } } + fn update_leader_selection( + engine: &Carnot>, + ) -> Carnot> { + engine + .update_overlay(|overlay| { + overlay.update_leader_selection( + |leader_selection| -> Result { + Ok(leader_selection.advance()) + }, + ) + }) + .unwrap() + } + #[test] // Ensure that all states are initialized correctly with the genesis block. fn from_genesis() { - let engine = init_from_genesis(); + let engine = init(vec![[0; 32]]); assert_eq!(engine.current_view(), 0); assert_eq!(engine.highest_voted_view, -1); @@ -444,8 +467,8 @@ mod test { #[test] // Ensure that all states are updated correctly after a block is received. fn receive_block() { - let mut engine = init_from_genesis(); - let block = next_block(&engine.genesis_block()); + let mut engine = init(vec![[0; 32]]); + let block = next_block(&engine, &engine.genesis_block()); engine = engine.receive_block(block.clone()).unwrap(); assert_eq!(engine.blocks_in_view(block.view), vec![block.clone()]); @@ -456,12 +479,13 @@ mod test { #[test] // Ensure that receive_block() returns early if the same block ID has already been received. fn receive_duplicate_block_id() { - let mut engine = init_from_genesis(); + let mut engine = init(vec![[0; 32]]); - let block1 = next_block(&engine.genesis_block()); + let block1 = next_block(&engine, &engine.genesis_block()); engine = engine.receive_block(block1.clone()).unwrap(); + engine = update_leader_selection(&engine); - let mut block2 = next_block(&block1); + let mut block2 = next_block(&engine, &block1); block2.id = block1.id; engine = engine.receive_block(block2).unwrap(); assert_eq!(engine.blocks_in_view(1), vec![block1]); @@ -471,7 +495,7 @@ mod test { #[should_panic(expected = "out of order view not supported, missing parent block")] // Ensure that receive_block() fails if the parent block has never been received. fn receive_block_with_unknown_parent() { - let engine = init_from_genesis(); + let engine = init(vec![[0; 32]]); let mut parent_block_id = engine.genesis_block().id; parent_block_id[0] += 1; // generate an unknown parent block ID let block = Block { @@ -490,71 +514,51 @@ mod test { #[test] // Ensure that receive_block() returns Err for unsafe blocks. fn receive_unsafe_blocks() { - let mut engine = init_from_genesis(); + let mut engine = init(vec![[0; 32]]); - let block = next_block(&engine.genesis_block()); + let block = next_block(&engine, &engine.genesis_block()); engine = engine.receive_block(block.clone()).unwrap(); + engine = update_leader_selection(&engine); - let block = next_block(&block); + let block = next_block(&engine, &block); engine = engine.receive_block(block.clone()).unwrap(); + engine = update_leader_selection(&engine); - let mut unsafe_block = next_block(&block); + let mut unsafe_block = next_block(&engine, &block); unsafe_block.view = engine.current_view() - 1; // UNSAFE: view < engine.current_view assert!(engine.receive_block(unsafe_block).is_err()); - let mut unsafe_block = next_block(&block); + let mut unsafe_block = next_block(&engine, &block); unsafe_block.view = unsafe_block.parent_qc.view() + 2; // UNSAFE: view != parent_qc.view + 1 assert!(engine.receive_block(unsafe_block).is_err()); } - #[test] - // Ensure that multiple blocks (forks) can be received at the current view. - fn receive_multiple_blocks_at_the_current_view() { - let mut engine = init_from_genesis(); - - let block1 = next_block(&engine.genesis_block()); - engine = engine.receive_block(block1.clone()).unwrap(); - - let block2 = next_block(&block1); - engine = engine.receive_block(block2.clone()).unwrap(); - - #[allow(clippy::redundant_clone)] - let mut block3 = block2.clone(); - block3.id = [3; 32]; // use a new ID, so that this block isn't ignored - engine = engine.receive_block(block3.clone()).unwrap(); - - assert_eq!(engine.current_view(), block3.view); - assert!(engine - .blocks_in_view(engine.current_view()) - .contains(&block2)); - assert!(engine - .blocks_in_view(engine.current_view()) - .contains(&block3)); - } - #[test] // Ensure that the grandparent of the current view can be committed fn receive_block_and_commit() { - let mut engine = init_from_genesis(); + let mut engine = init(vec![[0; 32]]); assert_eq!(engine.latest_committed_block(), engine.genesis_block()); - let block1 = next_block(&engine.genesis_block()); + let block1 = next_block(&engine, &engine.genesis_block()); engine = engine.receive_block(block1.clone()).unwrap(); assert_eq!(engine.latest_committed_block(), engine.genesis_block()); + engine = update_leader_selection(&engine); - let block2 = next_block(&block1); + let block2 = next_block(&engine, &block1); engine = engine.receive_block(block2.clone()).unwrap(); assert_eq!(engine.latest_committed_block(), engine.genesis_block()); + engine = update_leader_selection(&engine); - let block3 = next_block(&block2); + let block3 = next_block(&engine, &block2); engine = engine.receive_block(block3.clone()).unwrap(); assert_eq!(engine.latest_committed_block(), block1); assert_eq!( engine.committed_blocks(), vec![block1.id, engine.genesis_block().id] // without block2 and block3 ); + engine = update_leader_selection(&engine); - let block4 = next_block(&block3); + let block4 = next_block(&engine, &block3); engine = engine.receive_block(block4).unwrap(); assert_eq!(engine.latest_committed_block(), block2); assert_eq!( @@ -563,17 +567,74 @@ mod test { ); } + #[test] + // Ensure that the leader check in receive_block fails + // if the block is proposed by an unexpected leader. + fn receive_block_with_unexpected_leader() { + let mut engine = init(vec![[0; 32], [1; 32]]); + + let block = next_block(&engine, &engine.genesis_block()); + engine = engine.receive_block(block.clone()).unwrap(); + engine = update_leader_selection(&engine); + // now next_leader is [1; 32] + + let mut block = next_block(&engine, &block); + block.leader_proof = LeaderProof::LeaderId { + leader_id: [0; 32], // unexpected leader + }; + assert!(engine.receive_block(block).is_err()); + } + + #[test] + // Ensure that the leader check in receive_block fails + // if block.view is not the expected view. + fn receive_block_with_unexpected_view() { + let mut engine = init(vec![[0; 32], [1; 32]]); + + let block1 = next_block(&engine, &engine.genesis_block()); + engine = engine.receive_block(block1.clone()).unwrap(); + assert_eq!(engine.current_view(), 1); + engine = update_leader_selection(&engine); + + // a future block should be rejected + let future_block = Block { + id: [10; 32], + view: 11, // a future view + parent_qc: Qc::Aggregated(AggregateQc { + view: 10, + high_qc: StandardQc { + // a known parent block + id: block1.id, + view: block1.view, + }, + }), + leader_proof: LeaderProof::LeaderId { + leader_id: engine.overlay().next_leader(), + }, + }; + assert!(engine.receive_block(future_block).is_err()); + + // a past block should be also rejected + let mut past_block = block1; // with the same view as block1 + past_block.id = [10; 32]; + assert!(engine.receive_block(past_block).is_err()); + } + #[test] // Ensure that approve_block updates highest_voted_view and returns a correct Send. fn approve_block() { - let mut engine = init_from_genesis(); + let mut engine = init(vec![[0; 32], [1; 32], [3; 32]]); - let block = next_block(&engine.genesis_block()); + let block = next_block(&engine, &engine.genesis_block()); engine = engine.receive_block(block.clone()).unwrap(); + engine = update_leader_selection(&engine); let (engine, send) = engine.approve_block(block.clone()); assert_eq!(engine.highest_voted_view, block.view); - assert_eq!(send.to, vec![[0; 32]].into_iter().collect()); // next leader + assert_eq!( + send.to, + vec![engine.overlay().next_leader()].into_iter().collect() + ); assert_eq!( send.payload, Payload::Vote(Vote { @@ -587,9 +648,9 @@ mod test { #[should_panic(expected = "not in")] // Ensure that approve_block cannot accept not-received blocks. fn approve_block_not_received() { - let engine = init_from_genesis(); + let engine = init(vec![[0; 32]]); - let block = next_block(&engine.genesis_block()); + let block = next_block(&engine, &engine.genesis_block()); let _ = engine.approve_block(block); } @@ -597,10 +658,12 @@ mod test { #[should_panic(expected = "can't vote for a block in the past")] // Ensure that approve_block cannot vote blocks in the past. fn approve_block_in_the_past() { - let mut engine = init_from_genesis(); + let mut engine = init(vec![[0; 32]]); - let block = next_block(&engine.genesis_block()); + let block = next_block(&engine, &engine.genesis_block()); engine = engine.receive_block(block.clone()).unwrap(); + engine = update_leader_selection(&engine); + let (engine, _) = engine.approve_block(block.clone()); // trying to approve the block again that was already voted. @@ -610,16 +673,17 @@ mod test { #[test] // Ensure that local_timeout() votes on the current view. fn local_timeout() { - let mut engine = init_from_genesis(); - let block = next_block(&engine.genesis_block()); + let mut engine = init(vec![[0; 32], [1; 32]]); + let block = next_block(&engine, &engine.genesis_block()); engine = engine.receive_block(block).unwrap(); // received but not approved yet + engine = update_leader_selection(&engine); let (engine, send) = engine.local_timeout(); assert_eq!(engine.highest_voted_view, 1); // updated from 0 (genesis) to 1 (current_view) assert_eq!( send, Some(Send { - to: vec![[0; 32]].into_iter().collect(), // root_committee + to: engine.overlay().root_committee(), payload: Payload::Timeout(Timeout { view: 1, sender: [0; 32], @@ -636,9 +700,10 @@ mod test { #[test] // Ensure that receive_timeout_qc updates current_view, last_view_timeout_qc and local_high_qc. fn receive_timeout_qc_after_local_timeout() { - let mut engine = init_from_genesis(); - let block = next_block(&engine.genesis_block()); + let mut engine = init(vec![[0; 32]]); + let block = next_block(&engine, &engine.genesis_block()); engine = engine.receive_block(block).unwrap(); // received but not approved yet + engine = update_leader_selection(&engine); let (mut engine, _) = engine.local_timeout(); @@ -660,9 +725,10 @@ mod test { #[test] // Ensure that receive_timeout_qc works even before local_timeout occurs. fn receive_timeout_qc_before_local_timeout() { - let mut engine = init_from_genesis(); - let block = next_block(&engine.genesis_block()); + let mut engine = init(vec![[0; 32]]); + let block = next_block(&engine, &engine.genesis_block()); engine = engine.receive_block(block).unwrap(); // received but not approved yet + engine = update_leader_selection(&engine); // before local_timeout occurs @@ -684,9 +750,10 @@ mod test { #[test] // Ensure that approve_new_view votes on the new view correctly. fn approve_new_view() { - let mut engine = init_from_genesis(); - let block = next_block(&engine.genesis_block()); + let mut engine = init(vec![[0; 32], [1; 32], [2; 32]]); + let block = next_block(&engine, &engine.genesis_block()); engine = engine.receive_block(block).unwrap(); // received but not approved yet + engine = update_leader_selection(&engine); assert_eq!(engine.current_view(), 1); // still waiting for a QC(view=1) let timeout_qc = TimeoutQc::new( @@ -702,28 +769,33 @@ mod test { assert_eq!(engine.last_view_timeout_qc, Some(timeout_qc.clone())); assert_eq!(engine.current_view(), 2); assert_eq!(engine.highest_voted_view, -1); // didn't vote on anything yet + engine = update_leader_selection(&engine); let (engine, send) = engine.approve_new_view(timeout_qc.clone(), HashSet::new()); assert_eq!(&engine.high_qc(), timeout_qc.high_qc()); assert_eq!(engine.current_view(), 2); // not changed assert_eq!(engine.highest_voted_view, 2); assert_eq!( - send.payload, - Payload::NewView(NewView { - view: 2, - sender: [0; 32], - timeout_qc: timeout_qc.clone(), - high_qc: timeout_qc.high_qc().clone(), - }) + send, + Send { + to: vec![engine.overlay().next_leader()].into_iter().collect(), + payload: Payload::NewView(NewView { + view: 2, + sender: [0; 32], + timeout_qc: timeout_qc.clone(), + high_qc: timeout_qc.high_qc().clone(), + }) + } ); } #[test] #[should_panic(expected = "can't vote for a new view not bigger than the last timeout_qc")] fn approve_new_view_not_bigger_than_timeout_qc() { - let mut engine = init_from_genesis(); - let block = next_block(&engine.genesis_block()); + let mut engine = init(vec![[0; 32]]); + let block = next_block(&engine, &engine.genesis_block()); engine = engine.receive_block(block).unwrap(); // received but not approved yet + engine = update_leader_selection(&engine); assert_eq!(engine.current_view(), 1); let timeout_qc1 = TimeoutQc::new( @@ -736,6 +808,7 @@ mod test { ); engine = engine.receive_timeout_qc(timeout_qc1.clone()); assert_eq!(engine.last_view_timeout_qc, Some(timeout_qc1.clone())); + engine = update_leader_selection(&engine); // receiving a timeout_qc2 before approving new_view(timeout_qc1) let timeout_qc2 = TimeoutQc::new( @@ -748,6 +821,7 @@ mod test { ); engine = engine.receive_timeout_qc(timeout_qc2.clone()); assert_eq!(engine.last_view_timeout_qc, Some(timeout_qc2)); + engine = update_leader_selection(&engine); // we expect new_view(timeout_qc2), but... let _ = engine.approve_new_view(timeout_qc1, HashSet::new()); diff --git a/consensus-engine/tests/fuzz/ref_state.rs b/consensus-engine/tests/fuzz/ref_state.rs index b10d3120..2332b2f1 100644 --- a/consensus-engine/tests/fuzz/ref_state.rs +++ b/consensus-engine/tests/fuzz/ref_state.rs @@ -164,17 +164,20 @@ impl ReferenceStateMachine for RefState { impl RefState { // Generate a Transition::ReceiveSafeBlock. fn transition_receive_safe_block(&self) -> BoxedStrategy { - let recent_parents = self + let parents: Vec = self .chain - .range(self.current_view() - 1..) - .flat_map(|(_view, entry)| entry.blocks.iter().cloned()) - .collect::>(); + .get(&self.current_view()) + .cloned() + .unwrap_or_default() + .blocks + .into_iter() + .collect(); - if recent_parents.is_empty() { + if parents.is_empty() { Just(Transition::Nop).boxed() } else { // proptest::sample::select panics if the input is empty - proptest::sample::select(recent_parents) + proptest::sample::select(parents) .prop_map(move |parent| -> Transition { Transition::ReceiveSafeBlock(Self::consecutive_block(&parent)) })