From 98aa138b87b1498b8359260662ccacf3253815bb Mon Sep 17 00:00:00 2001 From: Youngjoon Lee Date: Wed, 28 Jun 2023 23:37:27 +0900 Subject: [PATCH] feat: enforce TimeoutQc to be constructed only by new() (#229) --- consensus-engine/src/lib.rs | 74 ++++++++--------- consensus-engine/src/types.rs | 79 ++++++++++++++++++- consensus-engine/tests/fuzz/ref_state.rs | 26 +++--- consensus-engine/tests/fuzz_test.rs | 2 +- .../consensus/src/leader_selection/mod.rs | 2 +- nomos-services/consensus/src/lib.rs | 12 +-- .../consensus/src/network/adapters/waku.rs | 2 +- nomos-services/consensus/src/tally/unhappy.rs | 2 +- simulations/src/node/carnot/messages.rs | 2 +- simulations/src/node/carnot/mod.rs | 12 +-- 10 files changed, 141 insertions(+), 72 deletions(-) diff --git a/consensus-engine/src/lib.rs b/consensus-engine/src/lib.rs index 246ea521..3f3e94ff 100644 --- a/consensus-engine/src/lib.rs +++ b/consensus-engine/src/lib.rs @@ -96,13 +96,13 @@ impl Carnot { pub fn receive_timeout_qc(&self, timeout_qc: TimeoutQc) -> Self { let mut new_state = self.clone(); - if timeout_qc.view < new_state.current_view { + if timeout_qc.view() < new_state.current_view { return new_state; } - new_state.update_high_qc(Qc::Standard(timeout_qc.high_qc.clone())); + new_state.update_high_qc(Qc::Standard(timeout_qc.high_qc().clone())); new_state.update_timeout_qc(timeout_qc.clone()); - new_state.current_view = timeout_qc.view + 1; + new_state.current_view = timeout_qc.view() + 1; new_state.overlay.rebuild(timeout_qc); new_state @@ -159,13 +159,13 @@ impl Carnot { timeout_qc: TimeoutQc, new_views: HashSet, ) -> (Self, Send) { - let new_view = timeout_qc.view + 1; + let new_view = timeout_qc.view() + 1; assert!( new_view > self .last_view_timeout_qc .as_ref() - .map(|qc| qc.view) + .map(|qc| qc.view()) .unwrap_or(0), "can't vote for a new view not bigger than the last timeout_qc" ); @@ -185,7 +185,7 @@ impl Carnot { let high_qc = new_views .iter() .map(|nv| &nv.high_qc) - .chain(std::iter::once(&timeout_qc.high_qc)) + .chain(std::iter::once(timeout_qc.high_qc())) .max_by_key(|qc| qc.view) .unwrap(); new_state.update_high_qc(Qc::Standard(high_qc.clone())); @@ -267,7 +267,7 @@ impl Carnot { (None, timeout_qc) => { self.last_view_timeout_qc = Some(timeout_qc); } - (Some(current_qc), timeout_qc) if timeout_qc.view > current_qc.view => { + (Some(current_qc), timeout_qc) if timeout_qc.view() > current_qc.view() => { self.last_view_timeout_qc = Some(timeout_qc); } _ => {} @@ -643,16 +643,16 @@ mod test { let (mut engine, _) = engine.local_timeout(); assert_eq!(engine.current_view(), 1); - let timeout_qc = TimeoutQc { - view: 1, - high_qc: StandardQc { + let timeout_qc = TimeoutQc::new( + 1, + StandardQc { view: 0, // genesis id: [0; 32], }, - sender: [0; 32], - }; + [0; 32], + ); engine = engine.receive_timeout_qc(timeout_qc.clone()); - assert_eq!(engine.local_high_qc, timeout_qc.high_qc); + assert_eq!(&engine.local_high_qc, timeout_qc.high_qc()); assert_eq!(engine.last_view_timeout_qc, Some(timeout_qc)); assert_eq!(engine.current_view(), 2); } @@ -667,16 +667,16 @@ mod test { // before local_timeout occurs assert_eq!(engine.current_view(), 1); - let timeout_qc = TimeoutQc { - view: 1, - high_qc: StandardQc { + let timeout_qc = TimeoutQc::new( + 1, + StandardQc { view: 0, // genesis id: [0; 32], }, - sender: [0; 32], - }; + [0; 32], + ); engine = engine.receive_timeout_qc(timeout_qc.clone()); - assert_eq!(engine.local_high_qc, timeout_qc.high_qc); + assert_eq!(&engine.local_high_qc, timeout_qc.high_qc()); assert_eq!(engine.last_view_timeout_qc, Some(timeout_qc)); assert_eq!(engine.current_view(), 2); } @@ -689,22 +689,22 @@ mod test { engine = engine.receive_block(block).unwrap(); // received but not approved yet assert_eq!(engine.current_view(), 1); // still waiting for a QC(view=1) - let timeout_qc = TimeoutQc { - view: 1, - high_qc: StandardQc { + let timeout_qc = TimeoutQc::new( + 1, + StandardQc { view: 0, // genesis id: [0; 32], }, - sender: [0; 32], - }; + [0; 32], + ); engine = engine.receive_timeout_qc(timeout_qc.clone()); - assert_eq!(engine.local_high_qc, timeout_qc.high_qc); + assert_eq!(&engine.local_high_qc, timeout_qc.high_qc()); 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 let (engine, send) = engine.approve_new_view(timeout_qc.clone(), HashSet::new()); - assert_eq!(engine.high_qc(), timeout_qc.high_qc); + 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!( @@ -713,7 +713,7 @@ mod test { view: 2, sender: [0; 32], timeout_qc: timeout_qc.clone(), - high_qc: timeout_qc.high_qc, + high_qc: timeout_qc.high_qc().clone(), }) ); } @@ -726,26 +726,26 @@ mod test { engine = engine.receive_block(block).unwrap(); // received but not approved yet assert_eq!(engine.current_view(), 1); - let timeout_qc1 = TimeoutQc { - view: 1, - high_qc: StandardQc { + let timeout_qc1 = TimeoutQc::new( + 1, + StandardQc { view: 0, // genesis id: [0; 32], }, - sender: [0; 32], - }; + [0; 32], + ); engine = engine.receive_timeout_qc(timeout_qc1.clone()); assert_eq!(engine.last_view_timeout_qc, Some(timeout_qc1.clone())); // receiving a timeout_qc2 before approving new_view(timeout_qc1) - let timeout_qc2 = TimeoutQc { - view: 2, - high_qc: StandardQc { + let timeout_qc2 = TimeoutQc::new( + 2, + StandardQc { view: 0, // genesis id: [0; 32], }, - sender: [0; 32], - }; + [0; 32], + ); engine = engine.receive_timeout_qc(timeout_qc2.clone()); assert_eq!(engine.last_view_timeout_qc, Some(timeout_qc2)); diff --git a/consensus-engine/src/types.rs b/consensus-engine/src/types.rs index a8a7869f..40a18e4f 100644 --- a/consensus-engine/src/types.rs +++ b/consensus-engine/src/types.rs @@ -57,9 +57,38 @@ pub struct NewView { #[derive(Debug, Clone, Eq, PartialEq, Hash)] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub struct TimeoutQc { - pub view: View, - pub high_qc: StandardQc, - pub sender: NodeId, + view: View, + high_qc: StandardQc, + sender: NodeId, +} + +impl TimeoutQc { + pub fn new(view: View, high_qc: StandardQc, sender: NodeId) -> Self { + assert!( + view >= high_qc.view, + "timeout_qc.view:{} shouldn't be lower than timeout_qc.high_qc.view:{}", + view, + high_qc.view, + ); + + Self { + view, + high_qc, + sender, + } + } + + pub fn view(&self) -> View { + self.view + } + + pub fn high_qc(&self) -> &StandardQc { + &self.high_qc + } + + pub fn sender(&self) -> NodeId { + self.sender + } } #[derive(Debug, Clone, Eq, PartialEq, Hash)] @@ -186,4 +215,48 @@ mod test { assert_eq!(qc.block(), [0; 32]); assert_eq!(qc.high_qc(), aggregated_qc.high_qc); } + + #[test] + fn new_timeout_qc() { + let timeout_qc = TimeoutQc::new( + 2, + StandardQc { + view: 1, + id: [0; 32], + }, + [0; 32], + ); + assert_eq!(timeout_qc.view(), 2); + assert_eq!(timeout_qc.high_qc().view, 1); + assert_eq!(timeout_qc.high_qc().id, [0; 32]); + assert_eq!(timeout_qc.sender(), [0; 32]); + + let timeout_qc = TimeoutQc::new( + 2, + StandardQc { + view: 2, + id: [0; 32], + }, + [0; 32], + ); + assert_eq!(timeout_qc.view(), 2); + assert_eq!(timeout_qc.high_qc().view, 2); + assert_eq!(timeout_qc.high_qc().id, [0; 32]); + assert_eq!(timeout_qc.sender(), [0; 32]); + } + + #[test] + #[should_panic( + expected = "timeout_qc.view:1 shouldn't be lower than timeout_qc.high_qc.view:2" + )] + fn new_timeout_qc_panic() { + let _ = TimeoutQc::new( + 1, + StandardQc { + view: 2, + id: [0; 32], + }, + [0; 32], + ); + } } diff --git a/consensus-engine/tests/fuzz/ref_state.rs b/consensus-engine/tests/fuzz/ref_state.rs index 4ce4d325..b10d3120 100644 --- a/consensus-engine/tests/fuzz/ref_state.rs +++ b/consensus-engine/tests/fuzz/ref_state.rs @@ -102,10 +102,10 @@ impl ReferenceStateMachine for RefState { Transition::ApprovePastBlock(block) => state.highest_voted_view >= block.view, Transition::LocalTimeout => true, Transition::ReceiveTimeoutQcForRecentView(timeout_qc) => { - timeout_qc.view == state.current_view() + timeout_qc.view() == state.current_view() } Transition::ReceiveTimeoutQcForOldView(timeout_qc) => { - timeout_qc.view < state.current_view() + timeout_qc.view() < state.current_view() } Transition::ApproveNewViewWithLatestTimeoutQc(timeout_qc, _) => { state.latest_timeout_qcs().contains(timeout_qc) @@ -142,7 +142,7 @@ impl ReferenceStateMachine for RefState { Transition::ReceiveTimeoutQcForRecentView(timeout_qc) => { state .chain - .entry(timeout_qc.view) + .entry(timeout_qc.view()) .or_default() .timeout_qcs .insert(timeout_qc.clone()); @@ -262,14 +262,14 @@ impl RefState { .prop_flat_map(move |block| { (current_view..=current_view + delta) // including future views .prop_map(move |view| { - Transition::ReceiveTimeoutQcForRecentView(TimeoutQc { + Transition::ReceiveTimeoutQcForRecentView(TimeoutQc::new( view, - high_qc: StandardQc { + StandardQc { view: block.view, id: block.id, }, - sender: SENDER, - }) + SENDER, + )) }) }) .boxed() @@ -290,11 +290,11 @@ impl RefState { } else { proptest::sample::select(old_view_entries) .prop_map(move |(view, entry)| { - Transition::ReceiveTimeoutQcForOldView(TimeoutQc { + Transition::ReceiveTimeoutQcForOldView(TimeoutQc::new( view, - high_qc: entry.high_qc().unwrap(), - sender: SENDER, - }) + entry.high_qc().unwrap(), + SENDER, + )) }) .boxed() } @@ -349,7 +349,7 @@ impl RefState { } pub fn new_view_from(timeout_qc: &TimeoutQc) -> View { - timeout_qc.view + 1 + timeout_qc.view() + 1 } pub fn high_qc(&self) -> StandardQc { @@ -415,7 +415,7 @@ impl ViewEntry { let iter2 = self .timeout_qcs .iter() - .map(|timeout_qc| timeout_qc.high_qc.clone()); + .map(|timeout_qc| timeout_qc.high_qc().clone()); iter1.chain(iter2).max_by_key(|qc| qc.view) } } diff --git a/consensus-engine/tests/fuzz_test.rs b/consensus-engine/tests/fuzz_test.rs index d4fc50d0..d615a390 100644 --- a/consensus-engine/tests/fuzz_test.rs +++ b/consensus-engine/tests/fuzz_test.rs @@ -18,5 +18,5 @@ prop_state_machine! { #[test] // run 100 state transitions per test case - fn consensus_engine_test(sequential 1..100 => ConsensusEngineTest); + fn consensus_engine_test(sequential 1..30 => ConsensusEngineTest); } diff --git a/nomos-services/consensus/src/leader_selection/mod.rs b/nomos-services/consensus/src/leader_selection/mod.rs index 0cbb4d03..42bdb955 100644 --- a/nomos-services/consensus/src/leader_selection/mod.rs +++ b/nomos-services/consensus/src/leader_selection/mod.rs @@ -42,6 +42,6 @@ impl UpdateableLeaderSelection for RandomBeaconState { } fn on_timeout_qc_received(&self, qc: TimeoutQc) -> Result { - Ok(Self::generate_sad(qc.view, self)) + Ok(Self::generate_sad(qc.view(), self)) } } diff --git a/nomos-services/consensus/src/lib.rs b/nomos-services/consensus/src/lib.rs index 9db0117a..16a9a38a 100644 --- a/nomos-services/consensus/src/lib.rs +++ b/nomos-services/consensus/src/lib.rs @@ -434,7 +434,7 @@ where participating_nodes: carnot.root_committee(), }; let (new_carnot, out) = carnot.approve_new_view(timeout_qc.clone(), new_views); - let new_view = timeout_qc.view + 1; + let new_view = timeout_qc.view() + 1; if carnot.is_next_leader() { let high_qc = carnot.high_qc(); task_manager.push(new_view, async move { @@ -471,7 +471,7 @@ where participating_nodes: carnot.child_committees().into_iter().flatten().collect(), }; task_manager.push( - timeout_qc.view + 1, + timeout_qc.view() + 1, Self::gather_new_views(adapter, self_committee, timeout_qc.clone(), tally_settings), ); if carnot.current_view() != new_state.current_view() { @@ -498,11 +498,7 @@ where .clone(); let mut output = None; if carnot.is_member_of_root_committee() { - let timeout_qc = TimeoutQc { - view: carnot.current_view(), - high_qc, - sender: carnot.id(), - }; + let timeout_qc = TimeoutQc::new(carnot.current_view(), high_qc, carnot.id()); output = Some(Output::BroadcastTimeoutQc { timeout_qc }); } (carnot, output) @@ -606,7 +602,7 @@ where ) -> Event { let tally = NewViewTally::new(tally); let stream = adapter - .new_view_stream(&committee, timeout_qc.view + 1) + .new_view_stream(&committee, timeout_qc.view() + 1) .await; match tally.tally(timeout_qc.clone(), stream).await { Ok((_qc, new_views)) => Event::NewView { diff --git a/nomos-services/consensus/src/network/adapters/waku.rs b/nomos-services/consensus/src/network/adapters/waku.rs index 79ee45af..6c6493b4 100644 --- a/nomos-services/consensus/src/network/adapters/waku.rs +++ b/nomos-services/consensus/src/network/adapters/waku.rs @@ -247,7 +247,7 @@ impl NetworkAdapter for WakuAdapter { let payload = message.payload(); let qc = TimeoutQcMsg::from_bytes(payload); async move { - if qc.qc.view > view { + if qc.qc.view() > view { Some(qc) } else { None diff --git a/nomos-services/consensus/src/tally/unhappy.rs b/nomos-services/consensus/src/tally/unhappy.rs index 68500489..bdba87d1 100644 --- a/nomos-services/consensus/src/tally/unhappy.rs +++ b/nomos-services/consensus/src/tally/unhappy.rs @@ -48,7 +48,7 @@ impl Tally for NewViewTally { while let Some(vote) = vote_stream.next().await { // check vote view is valid - if !vote.vote.view != timeout_qc.view { + if !vote.vote.view != timeout_qc.view() { continue; } diff --git a/simulations/src/node/carnot/messages.rs b/simulations/src/node/carnot/messages.rs index 807ffe1f..12f10af4 100644 --- a/simulations/src/node/carnot/messages.rs +++ b/simulations/src/node/carnot/messages.rs @@ -17,7 +17,7 @@ impl CarnotMessage { match self { CarnotMessage::Proposal(msg) => msg.view, CarnotMessage::Vote(msg) => msg.vote.view, - CarnotMessage::TimeoutQc(msg) => msg.qc.view, + CarnotMessage::TimeoutQc(msg) => msg.qc.view(), CarnotMessage::Timeout(msg) => msg.vote.view, CarnotMessage::NewView(msg) => msg.vote.view, } diff --git a/simulations/src/node/carnot/mod.rs b/simulations/src/node/carnot/mod.rs index 4a2055ee..78834aa6 100644 --- a/simulations/src/node/carnot/mod.rs +++ b/simulations/src/node/carnot/mod.rs @@ -428,7 +428,7 @@ impl> Node for Car tracing::info!( node = parse_idx(&self.id), current_view = self.engine.current_view(), - timeout_view = timeout_qc.view, + timeout_view = timeout_qc.view(), "receive new view message" ); let (new, out) = self.engine.approve_new_view(timeout_qc.clone(), new_views); @@ -439,7 +439,7 @@ impl> Node for Car tracing::info!( node = parse_idx(&self.id), current_view = self.engine.current_view(), - timeout_view = timeout_qc.view, + timeout_view = timeout_qc.view(), "receive timeout qc message" ); self.engine = self.engine.receive_timeout_qc(timeout_qc.clone()); @@ -457,11 +457,11 @@ impl> Node for Car .max_by_key(|qc| qc.view) .expect("empty root committee") .clone(); - let timeout_qc = TimeoutQc { - view: timeouts.iter().next().unwrap().view, + let timeout_qc = TimeoutQc::new( + timeouts.iter().next().unwrap().view, high_qc, - sender: self.id(), - }; + self.id(), + ); output.push(Output::BroadcastTimeoutQc { timeout_qc }); } }