From 5ee3079e1f27d92cac4f9f843ff87fb0a51a3ac7 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Wed, 6 Dec 2023 12:34:57 +0100 Subject: [PATCH 1/3] feat: Store all received proposals (#91) * wip: Store proposals within driver * Less cloning * Store a list of proposal for value * Less cloning * Cleanup * Drop proposal with polka previous if we are not in Propose step * Fall through instead of dropping the proposal * Rename variable in votekeeper for less ambiguity * Add new test, add comments and fix driver_steps_polka_previous_invalid_proposal_with_locked * Add test summary and cleanup test names * Formatting * Remove commented out code --------- Co-authored-by: Anca Zamfir --- Code/driver/src/driver.rs | 53 +++++--- Code/driver/src/lib.rs | 1 + Code/driver/src/proposals.rs | 48 +++++++ Code/round/src/state.rs | 5 - Code/round/src/state_machine.rs | 20 +-- Code/test/src/utils.rs | 67 +++++----- Code/test/tests/driver.rs | 63 ++------- Code/test/tests/driver_extra.rs | 229 +++++++++++++++++++++++++------- Code/vote/src/keeper.rs | 26 ++-- 9 files changed, 330 insertions(+), 182 deletions(-) create mode 100644 Code/driver/src/proposals.rs diff --git a/Code/driver/src/driver.rs b/Code/driver/src/driver.rs index 110780383..137bd2bb7 100644 --- a/Code/driver/src/driver.rs +++ b/Code/driver/src/driver.rs @@ -16,6 +16,7 @@ use malachite_vote::ThresholdParams; use crate::input::Input; use crate::output::Output; +use crate::proposals::Proposals; use crate::Error; use crate::ProposerSelector; use crate::Validity; @@ -33,6 +34,7 @@ where pub votes: VoteKeeper, pub round_state: RoundState, + pub proposals: Proposals, } impl Driver @@ -57,6 +59,7 @@ where validator_set, votes, round_state: RoundState::default(), + proposals: Proposals::new(), } } @@ -162,11 +165,14 @@ where return Ok(None); } + self.proposals.insert(proposal.clone()); + let polka_for_pol = self.votes.is_threshold_met( &proposal.pol_round(), VoteType::Prevote, Threshold::Value(proposal.value().id()), ); + let polka_previous = proposal.pol_round().is_defined() && polka_for_pol && proposal.pol_round() < self.round_state.round; @@ -181,7 +187,7 @@ where // L32 return self.apply_input( proposal.round(), - RoundInput::InvalidProposalAndPolkaPrevious(proposal.clone()), + RoundInput::InvalidProposalAndPolkaPrevious(proposal), ); } else { return Ok(None); @@ -201,13 +207,12 @@ where ) { return self.apply_input( proposal.round(), - RoundInput::ProposalAndPrecommitValue(proposal.clone()), + RoundInput::ProposalAndPrecommitValue(proposal), ); } - // If the proposal is for a different round drop the proposal - // TODO - this check is also done in the round state machine, decide where to do it - if self.round_state.round != proposal.round() { + // If the proposal is for a different round, drop the proposal + if self.round() != proposal.round() { return Ok(None); } @@ -216,26 +221,28 @@ where VoteType::Prevote, Threshold::Value(proposal.value().id()), ); + let polka_current = polka_for_current && self.round_state.step >= Step::Prevote; // L36 if polka_current { return self.apply_input( proposal.round(), - RoundInput::ProposalAndPolkaCurrent(proposal.clone()), + RoundInput::ProposalAndPolkaCurrent(proposal), ); } // L28 - if polka_previous { + if self.round_state.step == Step::Propose && polka_previous { + // TODO: Check proposal vr is equal to threshold vr return self.apply_input( proposal.round(), - RoundInput::ProposalAndPolkaPrevious(proposal.clone()), + RoundInput::ProposalAndPolkaPrevious(proposal), ); } // TODO - Caller needs to store the proposal (valid or not) as the quorum (polka or commits) may be met later - self.apply_input(proposal.round(), RoundInput::Proposal(proposal.clone())) + self.apply_input(proposal.round(), RoundInput::Proposal(proposal)) } fn apply_vote( @@ -300,20 +307,29 @@ where let data = Info::new(input_round, &self.address, proposer.address()); - // Multiplex the input with the round state. + // Multiplex the event with the round state. let mux_input = match input { - RoundInput::PolkaValue(value_id) => match round_state.proposal { - Some(ref proposal) if proposal.value().id() == value_id => { + RoundInput::PolkaValue(value_id) => { + let proposal = self.proposals.find(&value_id, |p| p.round() == input_round); + + if let Some(proposal) = proposal { + assert_eq!(proposal.value().id(), value_id); RoundInput::ProposalAndPolkaCurrent(proposal.clone()) + } else { + RoundInput::PolkaAny } - _ => RoundInput::PolkaAny, - }, - RoundInput::PrecommitValue(value_id) => match round_state.proposal { - Some(ref proposal) if proposal.value().id() == value_id => { + } + + RoundInput::PrecommitValue(value_id) => { + let proposal = self.proposals.find(&value_id, |p| p.round() == input_round); + + if let Some(proposal) = proposal { + assert_eq!(proposal.value().id(), value_id); RoundInput::ProposalAndPrecommitValue(proposal.clone()) + } else { + RoundInput::PrecommitAny } - _ => RoundInput::PrecommitAny, - }, + } _ => input, }; @@ -339,6 +355,7 @@ where .field("address", &self.address) .field("validator_set", &self.validator_set) .field("votes", &self.votes) + .field("proposals", &self.proposals.proposals) .field("round_state", &self.round_state) .finish() } diff --git a/Code/driver/src/lib.rs b/Code/driver/src/lib.rs index 95c0f560f..fd77260a5 100644 --- a/Code/driver/src/lib.rs +++ b/Code/driver/src/lib.rs @@ -18,6 +18,7 @@ mod driver; mod error; mod input; mod output; +mod proposals; mod proposer; mod util; diff --git a/Code/driver/src/proposals.rs b/Code/driver/src/proposals.rs new file mode 100644 index 000000000..cf0173efe --- /dev/null +++ b/Code/driver/src/proposals.rs @@ -0,0 +1,48 @@ +use alloc::collections::BTreeMap; +use alloc::vec::Vec; + +use malachite_common::ValueId; +use malachite_common::{Context, Proposal, Value}; + +/// Stores proposals at each round, indexed by their value id. +pub struct Proposals +where + Ctx: Context, +{ + pub(crate) proposals: BTreeMap, Vec>, +} + +impl Proposals +where + Ctx: Context, +{ + pub fn new() -> Self { + Self { + proposals: BTreeMap::new(), + } + } + + pub fn insert(&mut self, proposal: Ctx::Proposal) { + let value_id = proposal.value().id(); + self.proposals.entry(value_id).or_default().push(proposal); + } + + pub fn find( + &self, + value_id: &ValueId, + p: impl Fn(&Ctx::Proposal) -> bool, + ) -> Option<&Ctx::Proposal> { + self.proposals + .get(value_id) + .and_then(|proposals| proposals.iter().find(|proposal| p(proposal))) + } +} + +impl Default for Proposals +where + Ctx: Context, +{ + fn default() -> Self { + Self::new() + } +} diff --git a/Code/round/src/state.rs b/Code/round/src/state.rs index 09697a8d8..c15d1e091 100644 --- a/Code/round/src/state.rs +++ b/Code/round/src/state.rs @@ -38,7 +38,6 @@ where pub round: Round, pub step: Step, - pub proposal: Option, pub locked: Option>, pub valid: Option>, } @@ -52,7 +51,6 @@ where height, round, step: Step::NewRound, - proposal: None, locked: None, valid: None, } @@ -108,7 +106,6 @@ where height: self.height.clone(), round: self.round, step: self.step, - proposal: self.proposal.clone(), locked: self.locked.clone(), valid: self.valid.clone(), } @@ -125,7 +122,6 @@ where .field("height", &self.round) .field("round", &self.round) .field("step", &self.step) - .field("proposal", &self.proposal) .field("locked", &self.locked) .field("valid", &self.valid) .finish() @@ -141,7 +137,6 @@ where self.height == other.height && self.round == other.round && self.step == other.step - && self.proposal == other.proposal && self.locked == other.locked && self.valid == other.valid } diff --git a/Code/round/src/state_machine.rs b/Code/round/src/state_machine.rs index 093feb57b..82fbd1ded 100644 --- a/Code/round/src/state_machine.rs +++ b/Code/round/src/state_machine.rs @@ -249,7 +249,7 @@ where /// /// Ref: L22/L28 pub fn prevote( - mut state: State, + state: State, address: &Ctx::Address, proposal: &Ctx::Proposal, ) -> Transition @@ -266,7 +266,6 @@ where }; let output = Output::prevote(state.height.clone(), state.round, value, address.clone()); - state.proposal = Some(proposal.clone()); Transition::to(state.with_step(Step::Prevote)).with_output(output) } @@ -292,7 +291,7 @@ where /// NOTE: Only one of this and set_valid_value should be called once in a round /// How do we enforce this? pub fn precommit( - mut state: State, + state: State, address: &Ctx::Address, proposal: Ctx::Proposal, ) -> Transition @@ -311,16 +310,6 @@ where address.clone(), ); - let current_value = match state.proposal { - Some(ref proposal) => proposal.value().clone(), - None => { - state.proposal = Some(proposal.clone()); - proposal.value().clone() - } - }; - - debug_assert_eq!(current_value.id(), value.id()); - let next = state .set_locked(value.clone()) .set_valid(value.clone()) @@ -394,12 +383,11 @@ where /// Ref: L36/L42 /// /// NOTE: only one of this and precommit should be called once in a round -pub fn set_valid_value(mut state: State, proposal: &Ctx::Proposal) -> Transition +pub fn set_valid_value(state: State, proposal: &Ctx::Proposal) -> Transition where Ctx: Context, { - state.proposal = Some(proposal.clone()); - Transition::to(state.clone().set_valid(proposal.value().clone())) + Transition::to(state.set_valid(proposal.value().clone())) } //--------------------------------------------------------------------- diff --git a/Code/test/src/utils.rs b/Code/test/src/utils.rs index d4450f206..4dfbe5092 100644 --- a/Code/test/src/utils.rs +++ b/Code/test/src/utils.rs @@ -174,7 +174,6 @@ pub fn propose_state(round: Round) -> State { height: Height::new(1), round, step: Step::Propose, - proposal: None, locked: None, valid: None, } @@ -194,9 +193,8 @@ pub fn propose_state_with_proposal_and_valid( height: Height::new(1), round: state_round, step: Step::Propose, - proposal: Some(proposal.clone()), valid: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: valid_round, }), locked: None, @@ -211,13 +209,12 @@ pub fn propose_state_with_proposal_and_locked_and_valid( height: Height::new(1), round, step: Step::Propose, - proposal: Some(proposal.clone()), valid: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: Round::new(0), }), locked: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: Round::new(0), }), } @@ -228,23 +225,11 @@ pub fn prevote_state(round: Round) -> State { height: Height::new(1), round, step: Step::Prevote, - proposal: None, locked: None, valid: None, } } -pub fn prevote_state_with_proposal(round: Round, proposal: Proposal) -> State { - State { - height: Height::new(1), - round, - step: Step::Prevote, - proposal: Some(proposal.clone()), - valid: None, - locked: None, - } -} - pub fn prevote_state_with_proposal_and_valid( state_round: Round, valid_round: Round, @@ -254,9 +239,8 @@ pub fn prevote_state_with_proposal_and_valid( height: Height::new(1), round: state_round, step: Step::Prevote, - proposal: Some(proposal.clone()), valid: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: valid_round, }), locked: None, @@ -271,13 +255,12 @@ pub fn prevote_state_with_proposal_and_locked_and_valid( height: Height::new(1), round, step: Step::Prevote, - proposal: Some(proposal.clone()), valid: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: Round::new(0), }), locked: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: Round::new(0), }), } @@ -291,13 +274,12 @@ pub fn precommit_state_with_proposal_and_locked_and_valid( height: Height::new(1), round, step: Step::Precommit, - proposal: Some(proposal.clone()), valid: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: Round::new(0), }), locked: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: Round::new(0), }), } @@ -308,7 +290,6 @@ pub fn precommit_state(round: Round) -> State { height: Height::new(1), round, step: Step::Precommit, - proposal: None, locked: None, valid: None, } @@ -323,9 +304,8 @@ pub fn precommit_state_with_proposal_and_valid( height: Height::new(1), round: state_round, step: Step::Precommit, - proposal: Some(proposal.clone()), valid: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: valid_round, }), locked: None, @@ -337,7 +317,6 @@ pub fn new_round(round: Round) -> State { height: Height::new(1), round, step: Step::NewRound, - proposal: None, valid: None, locked: None, } @@ -348,9 +327,8 @@ pub fn new_round_with_proposal_and_valid(round: Round, proposal: Proposal) -> St height: Height::new(1), round, step: Step::NewRound, - proposal: Some(proposal.clone()), valid: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: Round::new(0), }), locked: None, @@ -365,13 +343,12 @@ pub fn new_round_with_proposal_and_locked_and_valid( height: Height::new(1), round, step: Step::NewRound, - proposal: Some(proposal.clone()), valid: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: Round::new(0), }), locked: Some(RoundValue { - value: proposal.clone().value, + value: proposal.value, round: Round::new(0), }), } @@ -383,8 +360,26 @@ pub fn decided_state(round: Round, _value: Value) -> State { height: Height::new(1), round, step: Step::Commit, - proposal: None, valid: None, locked: None, } } + +pub fn decided_state_with_proposal_and_locked_and_valid( + round: Round, + proposal: Proposal, +) -> State { + State { + height: Height::new(1), + round, + step: Step::Commit, + valid: Some(RoundValue { + value: proposal.value, + round: Round::new(0), + }), + locked: Some(RoundValue { + value: proposal.value, + round: Round::new(0), + }), + } +} diff --git a/Code/test/tests/driver.rs b/Code/test/tests/driver.rs index 62d4ff454..beec2c586 100644 --- a/Code/test/tests/driver.rs +++ b/Code/test/tests/driver.rs @@ -54,7 +54,6 @@ fn driver_steps_proposer() { height: Height::new(1), round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -68,7 +67,6 @@ fn driver_steps_proposer() { height: Height::new(1), round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -85,7 +83,7 @@ fn driver_steps_proposer() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: None, valid: None, }, @@ -99,7 +97,7 @@ fn driver_steps_proposer() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: None, valid: None, }, @@ -116,7 +114,7 @@ fn driver_steps_proposer() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: None, valid: None, }, @@ -136,7 +134,7 @@ fn driver_steps_proposer() { height: Height::new(1), round: Round::new(0), step: Step::Precommit, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: Some(RoundValue { value, round: Round::new(0), @@ -156,7 +154,7 @@ fn driver_steps_proposer() { height: Height::new(1), round: Round::new(0), step: Step::Precommit, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: Some(RoundValue { value, round: Round::new(0), @@ -179,7 +177,7 @@ fn driver_steps_proposer() { height: Height::new(1), round: Round::new(0), step: Step::Precommit, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: Some(RoundValue { value, round: Round::new(0), @@ -202,7 +200,7 @@ fn driver_steps_proposer() { height: Height::new(1), round: Round::new(0), step: Step::Commit, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: Some(RoundValue { value, round: Round::new(0), @@ -262,7 +260,6 @@ fn driver_steps_proposer_timeout_get_value() { height: Height::new(1), round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -278,7 +275,6 @@ fn driver_steps_proposer_timeout_get_value() { round: Round::new(0), height: Height::new(1), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -335,7 +331,6 @@ fn driver_steps_not_proposer_valid() { height: Height::new(1), round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -352,7 +347,7 @@ fn driver_steps_not_proposer_valid() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: None, valid: None, }, @@ -366,7 +361,7 @@ fn driver_steps_not_proposer_valid() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: None, valid: None, }, @@ -383,7 +378,7 @@ fn driver_steps_not_proposer_valid() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: None, valid: None, }, @@ -403,7 +398,7 @@ fn driver_steps_not_proposer_valid() { height: Height::new(1), round: Round::new(0), step: Step::Precommit, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: Some(RoundValue { value, round: Round::new(0), @@ -423,7 +418,7 @@ fn driver_steps_not_proposer_valid() { height: Height::new(1), round: Round::new(0), step: Step::Precommit, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: Some(RoundValue { value, round: Round::new(0), @@ -446,7 +441,7 @@ fn driver_steps_not_proposer_valid() { height: Height::new(1), round: Round::new(0), step: Step::Precommit, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: Some(RoundValue { value, round: Round::new(0), @@ -469,7 +464,7 @@ fn driver_steps_not_proposer_valid() { height: Height::new(1), round: Round::new(0), step: Step::Commit, - proposal: Some(proposal.clone()), + // proposal: Some(proposal.clone()), locked: Some(RoundValue { value, round: Round::new(0), @@ -532,7 +527,6 @@ fn driver_steps_not_proposer_invalid() { height: Height::new(1), round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -548,7 +542,6 @@ fn driver_steps_not_proposer_invalid() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -562,7 +555,6 @@ fn driver_steps_not_proposer_invalid() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -578,7 +570,6 @@ fn driver_steps_not_proposer_invalid() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -594,7 +585,6 @@ fn driver_steps_not_proposer_invalid() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -610,7 +600,6 @@ fn driver_steps_not_proposer_invalid() { height: Height::new(1), round: Round::new(0), step: Step::Precommit, - proposal: None, locked: None, valid: None, }, @@ -668,7 +657,6 @@ fn driver_steps_not_proposer_other_height() { height: Height::new(1), round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -682,7 +670,6 @@ fn driver_steps_not_proposer_other_height() { height: Height::new(1), round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -740,7 +727,6 @@ fn driver_steps_not_proposer_other_round() { height: Height::new(1), round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -754,7 +740,6 @@ fn driver_steps_not_proposer_other_round() { height: Height::new(1), round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -810,7 +795,6 @@ fn driver_steps_not_proposer_timeout_multiple_rounds() { height: Height::new(1), round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -827,7 +811,6 @@ fn driver_steps_not_proposer_timeout_multiple_rounds() { round: Round::new(0), height: Height::new(1), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -842,7 +825,6 @@ fn driver_steps_not_proposer_timeout_multiple_rounds() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -860,7 +842,6 @@ fn driver_steps_not_proposer_timeout_multiple_rounds() { height: Height::new(1), round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -879,7 +860,6 @@ fn driver_steps_not_proposer_timeout_multiple_rounds() { height: Height::new(1), round: Round::new(0), step: Step::Precommit, - proposal: None, locked: None, valid: None, }, @@ -894,7 +874,6 @@ fn driver_steps_not_proposer_timeout_multiple_rounds() { height: Height::new(1), round: Round::new(0), step: Step::Precommit, - proposal: None, locked: None, valid: None, }, @@ -912,7 +891,6 @@ fn driver_steps_not_proposer_timeout_multiple_rounds() { height: Height::new(1), round: Round::new(0), step: Step::Precommit, - proposal: None, locked: None, valid: None, }, @@ -929,7 +907,6 @@ fn driver_steps_not_proposer_timeout_multiple_rounds() { height: Height::new(1), round: Round::new(0), step: Step::Precommit, - proposal: None, locked: None, valid: None, }, @@ -944,7 +921,6 @@ fn driver_steps_not_proposer_timeout_multiple_rounds() { height: Height::new(1), round: Round::new(1), step: Step::NewRound, - proposal: None, locked: None, valid: None, }, @@ -958,7 +934,6 @@ fn driver_steps_not_proposer_timeout_multiple_rounds() { height: Height::new(1), round: Round::new(1), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -1109,7 +1084,6 @@ fn driver_steps_skip_round_skip_threshold() { height, round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -1126,7 +1100,6 @@ fn driver_steps_skip_round_skip_threshold() { height, round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -1141,7 +1114,6 @@ fn driver_steps_skip_round_skip_threshold() { height, round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -1158,7 +1130,6 @@ fn driver_steps_skip_round_skip_threshold() { height, round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -1175,7 +1146,6 @@ fn driver_steps_skip_round_skip_threshold() { height, round: Round::new(1), step: Step::NewRound, - proposal: None, locked: None, valid: None, }, @@ -1229,7 +1199,6 @@ fn driver_steps_skip_round_quorum_threshold() { height, round: Round::new(0), step: Step::Propose, - proposal: None, locked: None, valid: None, }, @@ -1246,7 +1215,6 @@ fn driver_steps_skip_round_quorum_threshold() { height, round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -1261,7 +1229,6 @@ fn driver_steps_skip_round_quorum_threshold() { height, round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -1278,7 +1245,6 @@ fn driver_steps_skip_round_quorum_threshold() { height, round: Round::new(0), step: Step::Prevote, - proposal: None, locked: None, valid: None, }, @@ -1295,7 +1261,6 @@ fn driver_steps_skip_round_quorum_threshold() { height, round: Round::new(1), step: Step::NewRound, - proposal: None, locked: None, valid: None, }, diff --git a/Code/test/tests/driver_extra.rs b/Code/test/tests/driver_extra.rs index ff9a3b9a7..7397c6569 100644 --- a/Code/test/tests/driver_extra.rs +++ b/Code/test/tests/driver_extra.rs @@ -7,7 +7,25 @@ use malachite_round::state::State; use malachite_test::{Height, Proposal, TestContext, ValidatorSet, Value}; use malachite_test::utils::*; - +// The following tests are performed: +// - L49 with commits from current rounds, no locked value, no valid value: +// `driver_steps_decide_current_with_no_locked_no_valid()` +// +// - L49 with commits from previous rounds, no locked value, no valid value: +// `driver_steps_decide_previous_with_no_locked_no_valid()` +// +// - L49 with commits from previous round, with locked and valid values +// `driver_steps_decide_previous_with_locked_and_valid()` +// +// - L28 in round 1, via L36 in round 0, with locked invalid value v. +// `driver_steps_polka_previous_invalid_proposal()`' +// +// - L28 in round 1 with no locked value, via L36 in round 0 with step precommit. +// `driver_steps_polka_previous_with_no_locked()` +// +// - L28 in round 1 with locked value, via L36 in round 0 with step prevote. +// `driver_steps_polka_previous_with_locked() +// // TODO - move all below to utils? struct TestStep { desc: &'static str, @@ -192,6 +210,124 @@ fn driver_steps_decide_previous_with_no_locked_no_valid() { run_steps(&mut driver, steps); } +// Arrive at L49 with commits from previous round, with locked and valid values +// +// Ev: NewRound(0) Timeout(propose) Proposal +// State: NewRound ------------> Propose ----------------> Prevote ------------> Prevote ---------------> Precommit --> +// Msg: propose_timer Prevote(nil) prevote_timer Precommit(value) +// Alg: L21 L57 L34 L40 +// +// Ev: NewRound(1) +// State: --> Precommit ---------------------------> NewRound -----------> Propose --------------------> Commit +// Msg: new_round(1) propose_timer decide(v, round=1) +// Alg: L56 L21 L49-L54 +// +// v1=2, v2=3, v3=2, we are v3 +// L21 - start round 0, we, v3, are not the proposer, start timeout propose (step propose) +// L57 - timeout propose, prevote for nil (v2) (step prevote) +// L34 - v1 and v2 prevote for same proposal, we get +2/3 prevotes, start prevote timer (step prevote) +// L37-L43 - v3 receives the proposal, sets locked and value (step precommit) +// L55 - v2 precommits for value in round 1, i.e. vr receives f+1 vote for round 1 from v2 (step new_round) +// L11 - v3 starts new round, not proposer, start timeout propose (step propose) +// L49 - v3 gets +2/3 precommits (from v1 and v2) for round 0, it has the proposal, decide +// +#[test] +fn driver_steps_decide_previous_with_locked_and_valid() { + let value = Value::new(9999); + + let [(v1, sk1), (v2, sk2), (v3, sk3)] = make_validators([2, 3, 2]); + let (my_sk, my_addr) = (sk3.clone(), v3.address); + + let ctx = TestContext::new(my_sk.clone()); + let sel = RotateProposer; + let vs = ValidatorSet::new(vec![v1.clone(), v2.clone(), v3.clone()]); + + let mut driver = Driver::new(ctx, sel, vs, my_addr); + + let steps = vec![ + TestStep { + desc: "Start round 0, we, v3, are not the proposer, start timeout propose", + input: new_round_input(Round::new(0)), + expected_output: start_propose_timer_output(Round::new(0)), + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + TestStep { + desc: "Timeout propopse, prevote for nil (v3)", + input: timeout_propose_input(Round::new(0)), + expected_output: prevote_nil_output(Round::new(0), &my_addr, &my_sk), + expected_round: Round::new(0), + new_state: prevote_state(Round::new(0)), + }, + TestStep { + desc: "v1 prevotes a proposal", + input: prevote_input(&v1.address, &sk1), + expected_output: None, + expected_round: Round::new(0), + new_state: prevote_state(Round::new(0)), + }, + TestStep { + desc: "v2 prevotes for same proposal, we get +2/3 prevotes, start prevote timer", + input: prevote_input(&v2.address, &sk2), + expected_output: start_prevote_timer_output(Round::new(0)), + expected_round: Round::new(0), + new_state: prevote_state(Round::new(0)), + }, + TestStep { + desc: "Receive proposal, L37-L43", + input: proposal_input(Round::new(0), value, Round::Nil, Validity::Valid), + expected_output: precommit_output(Round::new(0), Value::new(9999), &v3.address, &sk3), + expected_round: Round::new(0), + new_state: precommit_state_with_proposal_and_locked_and_valid( + Round::new(0), + Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), + ), + }, + TestStep { + desc: "v2 precommits for value in round 1, i.e. f+1 vote for round 1 from v2", + input: precommit_input(Round::new(1), Value::new(9999), &v2.address, &sk2), + expected_output: new_round_output(Round::new(1)), + expected_round: Round::new(1), + new_state: new_round_with_proposal_and_locked_and_valid( + Round::new(1), + Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), + ), + }, + TestStep { + desc: "Start round 1, we, v3, are not the proposer, start timeout propose", + input: new_round_input(Round::new(1)), + expected_output: start_propose_timer_output(Round::new(1)), + expected_round: Round::new(1), + new_state: propose_state_with_proposal_and_locked_and_valid( + Round::new(1), + Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), + ), + }, + TestStep { + desc: "v1 precommits for round 0 and same proposal", + input: precommit_input(Round::new(0), value, &v1.address, &sk1), + expected_output: None, + expected_round: Round::new(1), + new_state: propose_state_with_proposal_and_locked_and_valid( + Round::new(1), + Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), + ), + }, + TestStep { + desc: "v2 precommits for round 0 and same proposal, we get +2/3 precommit, decide", + input: precommit_input(Round::new(0), value, &v2.address, &sk2), + expected_output: decide_output(Round::new(0), value), + expected_round: Round::new(1), + new_state: decided_state_with_proposal_and_locked_and_valid( + Round::new(1), + Proposal::new(Height::new(1), Round::new(1), value, Round::Nil), + ), + }, + ]; + + run_steps(&mut driver, steps); +} + // Arrive at L36 in round 0, with step prevote and then L28 in round 1, with locked value v. // // Ev: NewRound(0) Proposal @@ -241,9 +377,9 @@ fn driver_steps_polka_previous_with_locked() { input: proposal_input(Round::new(0), value, Round::Nil, Validity::Valid), expected_output: prevote_output(Round::new(0), &my_addr, &my_sk), expected_round: Round::new(0), - new_state: prevote_state_with_proposal( + new_state: prevote_state( Round::new(0), - Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), + // Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), ), }, TestStep { @@ -251,9 +387,9 @@ fn driver_steps_polka_previous_with_locked() { input: prevote_input(&v3.address, &sk3), expected_output: None, expected_round: Round::new(0), - new_state: prevote_state_with_proposal( + new_state: prevote_state( Round::new(0), - Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), + // Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), ), }, TestStep { @@ -301,12 +437,33 @@ fn driver_steps_polka_previous_with_locked() { run_steps(&mut driver, steps) } +// Arrive at L36 in round 0, with step precommit and then L28 in round 1 with invalid value. +// +// Ev: NewRound(0) Timeout(propose) +// State: NewRound ------------> Propose --------------> Prevote -------------> Prevote ---------------------------> NewRound --> +// Msg: propose_timer prevote(nil) prevote_timer new_round(1) +// Alg: L21 L59 L35 L56 +// +// Ev: NewRound(1) InvalidProposal(round=0) +// State: --> NewRound ---------------> Propose -------------------------> Prevote +// Msg: propose_timer prevote(nil,round=1) +// Alg: L21 L28-L32 +// +// v1=2, v2=3, v3=2, we are v3 +// L21 - v3 is not proposer starts timeout propose (step propose) +// L57 - propose timeout, prevote nil (step prevote) +// L37 - v1 and v2 prevote for v, v3 gets +2/3 prevotes, start timeout prevote (step prevote) +// L55 - Receive f+1 vote for round 1 from v2, start new round (step new_round) +// L21 - v3 is not proposer starts timeout propose (step propose) +// L28 - v2 receives invalid proposal and has 2f+1 prevotes from round 0 and: +// L29 - with invalid proposal +// L32 - v2 sends prevote(nil, round=1) (step prevote) #[test] -fn driver_steps_polka_previous_invalid_proposal_with_locked() { +fn driver_steps_polka_previous_invalid_proposal() { let value = Value::new(9999); - let [(v1, sk1), (v2, sk2), (v3, sk3)] = make_validators([2, 2, 3]); - let (my_sk, my_addr) = (sk2, v2.address); + let [(v1, sk1), (v2, sk2), (v3, sk3)] = make_validators([2, 3, 2]); + let (my_sk, my_addr) = (sk3, v3.address); let ctx = TestContext::new(my_sk.clone()); let sel = RotateProposer; @@ -316,71 +473,53 @@ fn driver_steps_polka_previous_invalid_proposal_with_locked() { let steps = vec![ TestStep { - desc: "Start round 0, we, v2, are not the proposer, start timeout propose", + desc: "Start round 0, we, v3, are not the proposer, start timeout propose", input: new_round_input(Round::new(0)), expected_output: start_propose_timer_output(Round::new(0)), expected_round: Round::new(0), new_state: propose_state(Round::new(0)), }, TestStep { - desc: "receive a proposal from v1 - L22 send prevote", - input: proposal_input(Round::new(0), value, Round::Nil, Validity::Valid), - expected_output: prevote_output(Round::new(0), &my_addr, &my_sk), + desc: "timeout propopse, prevote for nil (v3)", + input: timeout_propose_input(Round::new(0)), + expected_output: prevote_nil_output(Round::new(0), &my_addr, &my_sk), expected_round: Round::new(0), - new_state: prevote_state_with_proposal( - Round::new(0), - Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), - ), + new_state: prevote_state(Round::new(0)), }, TestStep { - desc: "v3 prevotes the proposal", - input: prevote_input(&v3.address, &sk3), + desc: "v1 prevotes a proposal", + input: prevote_input(&v1.address, &sk1), expected_output: None, expected_round: Round::new(0), - new_state: prevote_state_with_proposal( - Round::new(0), - Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), - ), + new_state: prevote_state(Round::new(0)), }, TestStep { - desc: "v1 prevotes for same proposal, we get +2/3 prevotes, precommit", - input: prevote_input(&v1.address, &sk1), - expected_output: precommit_output(Round::new(0), value, &my_addr, &my_sk), + desc: "v2 prevotes for same proposal, we get +2/3 prevotes, start prevote timer", + input: prevote_input(&v2.address, &sk2), + expected_output: start_prevote_timer_output(Round::new(0)), expected_round: Round::new(0), - new_state: precommit_state_with_proposal_and_locked_and_valid( - Round::new(0), - Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), - ), + new_state: prevote_state(Round::new(0)), }, TestStep { desc: "Receive f+1 vote for round 1 from v3", - input: precommit_input(Round::new(1), Value::new(8888), &v3.address, &sk3), + input: prevote_input_at(Round::new(1), &v2.address, &sk2), expected_output: new_round_output(Round::new(1)), expected_round: Round::new(1), - new_state: new_round_with_proposal_and_locked_and_valid( - Round::new(1), - Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), - ), + new_state: new_round(Round::new(1)), }, TestStep { - desc: "start round 1, we are proposer with a valid value, propose it", + desc: "start round 1, we, v3, are not the proposer, start timeout propose", input: new_round_input(Round::new(1)), - expected_output: proposal_output(Round::new(1), value, Round::new(0)), + expected_output: start_propose_timer_output(Round::new(1)), expected_round: Round::new(1), - new_state: propose_state_with_proposal_and_locked_and_valid( - Round::new(1), - Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), - ), + new_state: propose_state(Round::new(1)), }, TestStep { - desc: "Receive our own proposal", + desc: "receive an invalid proposal for round 0", input: proposal_input(Round::new(1), value, Round::new(0), Validity::Invalid), expected_output: prevote_nil_output(Round::new(1), &my_addr, &my_sk), expected_round: Round::new(1), - new_state: prevote_state_with_proposal_and_locked_and_valid( - Round::new(1), - Proposal::new(Height::new(1), Round::new(0), value, Round::Nil), - ), + new_state: prevote_state(Round::new(1)), }, ]; diff --git a/Code/vote/src/keeper.rs b/Code/vote/src/keeper.rs index e81001f6d..a0e500712 100644 --- a/Code/vote/src/keeper.rs +++ b/Code/vote/src/keeper.rs @@ -118,24 +118,24 @@ where weight: Weight, current_round: Round, ) -> Option>> { - let round = self + let per_round = self .per_round .entry(vote.round()) .or_insert_with(PerRound::new); - round.votes.add_vote( + per_round.votes.add_vote( vote.vote_type(), vote.validator_address().clone(), vote.value().clone(), weight, ); - round + per_round .addresses_weights .set_once(vote.validator_address().clone(), weight); if vote.round() > current_round { - let combined_weight = round.addresses_weights.sum(); + let combined_weight = per_round.addresses_weights.sum(); let skip_round = self .threshold_params @@ -143,26 +143,26 @@ where .is_met(combined_weight, self.total_weight); if skip_round { - let msg = Output::SkipRound(vote.round()); - round.emitted_outputs.insert(msg.clone()); - return Some(msg); + let output = Output::SkipRound(vote.round()); + per_round.emitted_outputs.insert(output.clone()); + return Some(output); } } let threshold = compute_threshold( vote.vote_type(), - round, + per_round, vote.value(), self.threshold_params.quorum, self.total_weight, ); - let msg = threshold_to_output(vote.vote_type(), vote.round(), threshold); + let output = threshold_to_output(vote.vote_type(), vote.round(), threshold); - match msg { - Some(msg) if !round.emitted_outputs.contains(&msg) => { - round.emitted_outputs.insert(msg.clone()); - Some(msg) + match output { + Some(output) if !per_round.emitted_outputs.contains(&output) => { + per_round.emitted_outputs.insert(output.clone()); + Some(output) } _ => None, } From 20b4ef3509fc0dce86c4e6ee0b70d47e82bf38eb Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Wed, 6 Dec 2023 15:23:49 +0100 Subject: [PATCH 2/3] Manually derive impls on round `Input` to avoid inferred bounds on `Ctx` --- Code/round/src/input.rs | 117 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 116 insertions(+), 1 deletion(-) diff --git a/Code/round/src/input.rs b/Code/round/src/input.rs index c095f4dcd..33b0df1c6 100644 --- a/Code/round/src/input.rs +++ b/Code/round/src/input.rs @@ -1,6 +1,7 @@ +use core::fmt; + use malachite_common::{Context, Round, ValueId}; -#[derive(Clone, Debug, PartialEq, Eq)] pub enum Input where Ctx: Context, @@ -73,3 +74,117 @@ where /// L65 TimeoutPrecommit, } + +impl Clone for Input { + fn clone(&self) -> Self { + match self { + Input::NewRound => Input::NewRound, + Input::ProposeValue(value) => Input::ProposeValue(value.clone()), + Input::Proposal(proposal) => Input::Proposal(proposal.clone()), + Input::InvalidProposal => Input::InvalidProposal, + Input::ProposalAndPolkaPrevious(proposal) => { + Input::ProposalAndPolkaPrevious(proposal.clone()) + } + Input::InvalidProposalAndPolkaPrevious(proposal) => { + Input::InvalidProposalAndPolkaPrevious(proposal.clone()) + } + Input::PolkaValue(value_id) => Input::PolkaValue(value_id.clone()), + Input::PolkaAny => Input::PolkaAny, + Input::PolkaNil => Input::PolkaNil, + Input::ProposalAndPolkaCurrent(proposal) => { + Input::ProposalAndPolkaCurrent(proposal.clone()) + } + Input::PrecommitAny => Input::PrecommitAny, + Input::ProposalAndPrecommitValue(proposal) => { + Input::ProposalAndPrecommitValue(proposal.clone()) + } + Input::PrecommitValue(value_id) => Input::PrecommitValue(value_id.clone()), + Input::SkipRound(round) => Input::SkipRound(*round), + Input::TimeoutPropose => Input::TimeoutPropose, + Input::TimeoutPrevote => Input::TimeoutPrevote, + Input::TimeoutPrecommit => Input::TimeoutPrecommit, + } + } +} + +impl PartialEq for Input { + fn eq(&self, other: &Self) -> bool { + match (self, other) { + (Input::NewRound, Input::NewRound) => true, + (Input::ProposeValue(value), Input::ProposeValue(other_value)) => value == other_value, + (Input::Proposal(proposal), Input::Proposal(other_proposal)) => { + proposal == other_proposal + } + (Input::InvalidProposal, Input::InvalidProposal) => true, + ( + Input::ProposalAndPolkaPrevious(proposal), + Input::ProposalAndPolkaPrevious(other_proposal), + ) => proposal == other_proposal, + ( + Input::InvalidProposalAndPolkaPrevious(proposal), + Input::InvalidProposalAndPolkaPrevious(other_proposal), + ) => proposal == other_proposal, + (Input::PolkaValue(value_id), Input::PolkaValue(other_value_id)) => { + value_id == other_value_id + } + (Input::PolkaAny, Input::PolkaAny) => true, + (Input::PolkaNil, Input::PolkaNil) => true, + ( + Input::ProposalAndPolkaCurrent(proposal), + Input::ProposalAndPolkaCurrent(other_proposal), + ) => proposal == other_proposal, + (Input::PrecommitAny, Input::PrecommitAny) => true, + ( + Input::ProposalAndPrecommitValue(proposal), + Input::ProposalAndPrecommitValue(other_proposal), + ) => proposal == other_proposal, + (Input::PrecommitValue(value_id), Input::PrecommitValue(other_value_id)) => { + value_id == other_value_id + } + (Input::SkipRound(round), Input::SkipRound(other_round)) => round == other_round, + (Input::TimeoutPropose, Input::TimeoutPropose) => true, + (Input::TimeoutPrevote, Input::TimeoutPrevote) => true, + (Input::TimeoutPrecommit, Input::TimeoutPrecommit) => true, + _ => false, + } + } +} + +impl Eq for Input {} + +impl fmt::Debug for Input +where + Ctx: Context, + Ctx::Value: fmt::Debug, + Ctx::Proposal: fmt::Debug, +{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Input::NewRound => write!(f, "NewRound"), + Input::ProposeValue(value) => write!(f, "ProposeValue({:?})", value), + Input::Proposal(proposal) => write!(f, "Proposal({:?})", proposal), + Input::InvalidProposal => write!(f, "InvalidProposal"), + Input::ProposalAndPolkaPrevious(proposal) => { + write!(f, "ProposalAndPolkaPrevious({:?})", proposal) + } + Input::InvalidProposalAndPolkaPrevious(proposal) => { + write!(f, "InvalidProposalAndPolkaPrevious({:?})", proposal) + } + Input::PolkaValue(value_id) => write!(f, "PolkaValue({:?})", value_id), + Input::PolkaAny => write!(f, "PolkaAny"), + Input::PolkaNil => write!(f, "PolkaNil"), + Input::ProposalAndPolkaCurrent(proposal) => { + write!(f, "ProposalAndPolkaCurrent({:?})", proposal) + } + Input::PrecommitAny => write!(f, "PrecommitAny"), + Input::ProposalAndPrecommitValue(proposal) => { + write!(f, "ProposalAndPrecommitValue({:?})", proposal) + } + Input::PrecommitValue(value_id) => write!(f, "PrecommitValue({:?})", value_id), + Input::SkipRound(round) => write!(f, "SkipRound({:?})", round), + Input::TimeoutPropose => write!(f, "TimeoutPropose"), + Input::TimeoutPrevote => write!(f, "TimeoutPrevote"), + Input::TimeoutPrecommit => write!(f, "TimeoutPrecommit"), + } + } +} From b8e0f9c5876629062676fd98386f949f5c49634b Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Wed, 6 Dec 2023 17:19:59 +0100 Subject: [PATCH 3/3] Exclude manual impls from coverage --- Code/round/src/input.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Code/round/src/input.rs b/Code/round/src/input.rs index 33b0df1c6..116d33abf 100644 --- a/Code/round/src/input.rs +++ b/Code/round/src/input.rs @@ -76,6 +76,7 @@ where } impl Clone for Input { + #[cfg_attr(coverage_nightly, coverage(off))] fn clone(&self) -> Self { match self { Input::NewRound => Input::NewRound, @@ -108,6 +109,7 @@ impl Clone for Input { } impl PartialEq for Input { + #[cfg_attr(coverage_nightly, coverage(off))] fn eq(&self, other: &Self) -> bool { match (self, other) { (Input::NewRound, Input::NewRound) => true, @@ -158,6 +160,7 @@ where Ctx::Value: fmt::Debug, Ctx::Proposal: fmt::Debug, { + #[cfg_attr(coverage_nightly, coverage(off))] fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Input::NewRound => write!(f, "NewRound"),