From c39ba527e529b396cc3932e9a9c0d80566451ca2 Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Tue, 24 Oct 2023 19:17:41 -0400 Subject: [PATCH 1/8] Event name changes and other small fixes --- Code/round/src/events.rs | 33 ++++++++++++++++++--------------- Code/round/src/state_machine.rs | 20 ++++++++++---------- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/Code/round/src/events.rs b/Code/round/src/events.rs index aeb9ea8b8..e7376bc8c 100644 --- a/Code/round/src/events.rs +++ b/Code/round/src/events.rs @@ -1,20 +1,23 @@ -use malachite_common::Proposal; +use malachite_common::{Proposal, ValueId}; -use crate::{Value, ValueId}; +use crate::Value; #[derive(Clone, Debug, PartialEq, Eq)] pub enum Event { - NewRound, // Start a new round, not as proposer. - NewRoundProposer(Value), // Start a new round and propose the Value. - Proposal(Proposal), // Receive a proposal with possible polka round. - ProposalInvalid, // Receive an invalid proposal. - PolkaAny, // Receive +2/3 prevotes for anything. - PolkaNil, // Receive +2/3 prevotes for nil. - PolkaValue(ValueId), // Receive +2/3 prevotes for Value. - PrecommitAny, // Receive +2/3 precommits for anything. - PrecommitValue(ValueId), // Receive +2/3 precommits for Value. - RoundSkip, // Receive +1/3 votes from a higher round. - TimeoutPropose, // Timeout waiting for proposal. - TimeoutPrevote, // Timeout waiting for prevotes. - TimeoutPrecommit, // Timeout waiting for precommits. + NewRound, // Start a new round, not as proposer.L20 + NewRoundProposer(Value), // Start a new round and propose the Value.L14 + Proposal(Proposal), // Receive a proposal. L22 + L23 (valid) + ProposalAndPolkaPrevious(Value), // Recieved a proposal and a polka value from a previous round. L28 + L29 (valid) + ProposalInvalid, // Receive an invalid proposal. L26 + L32 (invalid) + PolkaValue(ValueId), // Receive +2/3 prevotes for valueId. L44 + PolkaAny, // Receive +2/3 prevotes for anything. L34 + PolkaNil, // Receive +2/3 prevotes for nil. L44 + ProposalAndPolkaCurrent(Value), // Receive +2/3 prevotes for Value in current round. L36 + PrecommitAny, // Receive +2/3 precommits for anything. L47 + ProposalAndPrecommitValue(Value), // Receive +2/3 precommits for Value. L49 + PrecommitValue(ValueId), // Receive +2/3 precommits for ValueId. L51 + RoundSkip, // Receive +1/3 messages from a higher round. OneCorrectProcessInHigherRound, L55 + TimeoutPropose, // Timeout waiting for proposal. L57 + TimeoutPrevote, // Timeout waiting for prevotes. L61 + TimeoutPrecommit, // Timeout waiting for precommits. L65 } diff --git a/Code/round/src/state_machine.rs b/Code/round/src/state_machine.rs index fc6c7ad91..4bafc6848 100644 --- a/Code/round/src/state_machine.rs +++ b/Code/round/src/state_machine.rs @@ -99,12 +99,12 @@ pub fn apply_event(mut state: State, round: Round, event: Event) -> Transition { // From Prevote. Event must be for current round. (Step::Prevote, Event::PolkaAny) if this_round => schedule_timeout_prevote(state), // L34 (Step::Prevote, Event::PolkaNil) if this_round => precommit_nil(state), // L44 - (Step::Prevote, Event::PolkaValue(value_id)) if this_round => precommit(state, value_id), // L36/L37 - NOTE: only once? + (Step::Prevote, Event::ProposalAndPolkaCurrent(value)) if this_round => precommit(state, value), // L36/L37 - NOTE: only once? (Step::Prevote, Event::TimeoutPrevote) if this_round => precommit_nil(state), // L61 // From Precommit. Event must be for current round. - (Step::Precommit, Event::PolkaValue(value_id)) if this_round => { - set_valid_value(state, value_id) + (Step::Precommit, Event::ProposalAndPolkaCurrent(value)) if this_round => { + set_valid_value(state, value) } // L36/L42 - NOTE: only once? // From Commit. No more state transitions. @@ -114,7 +114,7 @@ pub fn apply_event(mut state: State, round: Round, event: Event) -> Transition { (_, Event::PrecommitAny) if this_round => schedule_timeout_precommit(state), // L47 (_, Event::TimeoutPrecommit) if this_round => round_skip(state, round.increment()), // L65 (_, Event::RoundSkip) if state.round < round => round_skip(state, round), // L55 - (_, Event::PrecommitValue(value_id)) => commit(state, round, value_id), // L49 + (_, Event::ProposalAndPrecommitValue(value)) => commit(state, round, value), // L49 // Invalid transition. _ => Transition::invalid(state), @@ -177,8 +177,8 @@ pub fn prevote_nil(state: State) -> Transition { /// /// NOTE: Only one of this and set_valid_value should be called once in a round /// How do we enforce this? -pub fn precommit(state: State, value_id: ValueId) -> Transition { - let message = Message::precommit(state.round, Some(value_id), ADDRESS); +pub fn precommit(state: State, value: Value) -> Transition { + let message = Message::precommit(state.round, Some(value.id()), ADDRESS); let Some(value) = state .proposal @@ -243,7 +243,7 @@ pub fn schedule_timeout_precommit(state: State) -> Transition { /// Ref: L36/L42 /// /// NOTE: only one of this and precommit should be called once in a round -pub fn set_valid_value(state: State, value_id: ValueId) -> Transition { +pub fn set_valid_value(state: State, value_id: Value) -> Transition { // check that we're locked on this value let Some(locked) = state.locked.as_ref() else { @@ -251,7 +251,7 @@ pub fn set_valid_value(state: State, value_id: ValueId) -> Transition { return Transition::invalid(state); }; - if locked.value.id() != value_id { + if locked.value.id() != value_id.id() { // TODO: Add logging return Transition::invalid(state); } @@ -274,14 +274,14 @@ pub fn round_skip(state: State, round: Round) -> Transition { /// We received +2/3 precommits for a value - commit and decide that value! /// /// Ref: L49 -pub fn commit(state: State, round: Round, value_id: ValueId) -> Transition { +pub fn commit(state: State, round: Round, value: Value) -> Transition { // Check that we're locked on this value let Some(locked) = state.locked.as_ref() else { // TODO: Add logging return Transition::invalid(state); }; - if locked.value.id() != value_id { + if locked.value != value { // TODO: Add logging return Transition::invalid(state); } From 594a6f9f81d8b11f0bd6400a507ccb1f62fd56da Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Wed, 1 Nov 2023 22:16:36 -0400 Subject: [PATCH 2/8] Add event multiplexing in executor and fix tests --- Code/consensus/src/executor.rs | 26 +++++++++++++-- Code/round/src/events.rs | 4 +-- Code/round/src/state_machine.rs | 58 ++++++++++++++++----------------- 3 files changed, 54 insertions(+), 34 deletions(-) diff --git a/Code/consensus/src/executor.rs b/Code/consensus/src/executor.rs index ea9d23c4c..76d51d5dd 100644 --- a/Code/consensus/src/executor.rs +++ b/Code/consensus/src/executor.rs @@ -92,12 +92,13 @@ where match round_msg { RoundMessage::NewRound(round) => { + // TODO: How to loop Event::NewRoundProposer or Event::NewRound back to the executor? // TODO: check if we are the proposer - // XXX: Check if there is an existing state? + assert!(self.round < round); self.round_states .insert(round, RoundState::default().new_round(round)); - + self.round = round; None } @@ -236,8 +237,27 @@ where let data = RoundData::new(round, &self.height, &self.address); + // Multiplex the event with the round state. + let mux_event = match event { + RoundEvent::PolkaValue(value_id) => { + match round_state.proposal { + Some(ref proposal) if proposal.value().id() == value_id => + RoundEvent::ProposalAndPolkaCurrent(proposal.clone()), + _ => RoundEvent::PolkaAny, + } + }, + RoundEvent::PrecommitValue(value_id) => { + match round_state.proposal { + Some(ref proposal) if proposal.value().id() == value_id => + RoundEvent::ProposalAndPrecommitValue(proposal.clone()), + _ => RoundEvent::PrecommitAny, + } + }, + _ => event, + }; + // Apply the event to the round state machine - let transition = round_state.apply_event(&data, event); + let transition = round_state.apply_event(&data, mux_event); // Update state self.round_states.insert(round, transition.next_state); diff --git a/Code/round/src/events.rs b/Code/round/src/events.rs index 5e1349845..5ed0929e0 100644 --- a/Code/round/src/events.rs +++ b/Code/round/src/events.rs @@ -13,9 +13,9 @@ where PolkaValue(ValueId), // Receive +2/3 prevotes for valueId. L44 PolkaAny, // Receive +2/3 prevotes for anything. L34 PolkaNil, // Receive +2/3 prevotes for nil. L44 - ProposalAndPolkaCurrent(Ctx::Value), // Receive +2/3 prevotes for Value in current round. L36 + ProposalAndPolkaCurrent(Ctx::Proposal), // Receive +2/3 prevotes for Value in current round. L36 PrecommitAny, // Receive +2/3 precommits for anything. L47 - ProposalAndPrecommitValue(Ctx::Value), // Receive +2/3 precommits for Value. L49 + ProposalAndPrecommitValue(Ctx::Proposal), // Receive +2/3 precommits for Value. L49 PrecommitValue(ValueId), // Receive +2/3 precommits for ValueId. L51 RoundSkip, // Receive +1/3 messages from a higher round. OneCorrectProcessInHigherRound, L55 TimeoutPropose, // Timeout waiting for proposal. L57 diff --git a/Code/round/src/state_machine.rs b/Code/round/src/state_machine.rs index db0a08939..a809bba37 100644 --- a/Code/round/src/state_machine.rs +++ b/Code/round/src/state_machine.rs @@ -1,4 +1,4 @@ -use malachite_common::{Context, Proposal, Round, TimeoutStep, Value, ValueId}; +use malachite_common::{Context, Proposal, Round, TimeoutStep, Value}; use crate::events::Event; use crate::message::Message; @@ -78,26 +78,22 @@ where .map_or(true, |locked| &locked.value == proposal.value()) { state.proposal = Some(proposal.clone()); - prevote(state, data.address, proposal.round(), proposal.value().id()) + prevote(state, data.address, &proposal) } else { prevote_nil(state, data.address) } } - // TODO - change to ProposalAndPolkaPrevious (Step::Propose, Event::ProposalAndPolkaPrevious(proposal)) if this_round && is_valid_pol_round(&state, proposal.pol_round()) => { // L28 let Some(locked) = state.locked.as_ref() else { - // TODO: Add logging - return Transition::invalid(state); + return prevote_nil(state, data.address); }; - if proposal.value().is_valid() - && (locked.round <= proposal.pol_round() || &locked.value == proposal.value()) - { - prevote(state, data.address, proposal.round(), proposal.value().id()) + if locked.round <= proposal.pol_round() || &locked.value == proposal.value() { + prevote(state, data.address, &proposal) } else { prevote_nil(state, data.address) } @@ -108,14 +104,14 @@ where // From Prevote. Event must be for current round. (Step::Prevote, Event::PolkaAny) if this_round => schedule_timeout_prevote(state), // L34 (Step::Prevote, Event::PolkaNil) if this_round => precommit_nil(state, data.address), // L44 - (Step::Prevote, Event::ProposalAndPolkaCurrent(value)) if this_round => { - precommit(state, data.address, value) // L36/L37 - NOTE: only once? + (Step::Prevote, Event::ProposalAndPolkaCurrent(proposal)) if this_round => { + precommit(state, data.address, proposal) // L36/L37 - NOTE: only once? } (Step::Prevote, Event::TimeoutPrevote) if this_round => precommit_nil(state, data.address), // L61 // From Precommit. Event must be for current round. - (Step::Precommit, Event::ProposalAndPolkaCurrent(value)) if this_round => { - set_valid_value(state, value) // L36/L42 - NOTE: only once? + (Step::Precommit, Event::ProposalAndPolkaCurrent(proposal)) if this_round => { + set_valid_value(state, proposal.value().clone()) // L36/L42 - NOTE: only once? } // From Commit. No more state transitions. @@ -125,7 +121,7 @@ where (_, Event::PrecommitAny) if this_round => schedule_timeout_precommit(state), // L47 (_, Event::TimeoutPrecommit) if this_round => round_skip(state, data.round.increment()), // L65 (_, Event::RoundSkip) if state.round < data.round => round_skip(state, data.round), // L55 - (_, Event::ProposalAndPrecommitValue(value)) => commit(state, data.round, value), // L49 + (_, Event::ProposalAndPrecommitValue(proposal)) => commit(state, data.round, proposal), // L49 // Invalid transition. _ => Transition::invalid(state), @@ -164,12 +160,13 @@ where pub fn prevote( state: State, address: &Ctx::Address, - vr: Round, - proposed: ValueId, + proposal: &Ctx::Proposal, ) -> Transition where Ctx: Context, { + let vr = proposal.round(); + let proposed = proposal.value().id(); let value = match &state.locked { Some(locked) if locked.round <= vr => Some(proposed), // unlock and prevote Some(locked) if locked.value.id() == proposed => Some(proposed), // already locked on value @@ -203,9 +200,9 @@ 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( - state: State, + mut state: State, address: &Ctx::Address, - value: Ctx::Value, + proposal: Ctx::Proposal, ) -> Transition where Ctx: Context, @@ -214,21 +211,24 @@ where return Transition::to(state.clone()); } + let value = proposal.value(); let message = Message::precommit(state.round, Some(value.id()), address.clone()); - let Some(value) = state - .proposal - .as_ref() - .map(|proposal| proposal.value().clone()) - else { - // TODO: Add logging - return Transition::invalid(state.clone()); + let current_value = match state.proposal { + Some(ref proposal) => proposal.value().clone(), + None => { + state.proposal = Some(proposal.clone()); + proposal.value().clone() + } }; + assert_eq!(current_value.id(), value.id()); + let next = state .set_locked(value.clone()) - .set_valid(value) + .set_valid(value.clone()) .with_step(Step::Precommit); + Transition::to(next).with_message(message) } @@ -327,13 +327,13 @@ pub fn round_skip(state: State, round: Round) -> Transition where Ctx: Context, { - Transition::to(state).with_message(Message::NewRound(round)) + Transition::to(state.new_round(round)).with_message(Message::NewRound(round)) } /// We received +2/3 precommits for a value - commit and decide that value! /// /// Ref: L49 -pub fn commit(state: State, round: Round, value: Ctx::Value) -> Transition +pub fn commit(state: State, round: Round, proposal: Ctx::Proposal) -> Transition where Ctx: Context, { @@ -343,7 +343,7 @@ where return Transition::invalid(state); }; - if locked.value != value { + if locked.value.id() != proposal.value().id() { // TODO: Add logging return Transition::invalid(state); } From 139c5b6ee93fd3df74aecbe31b6ec65f8ad61069 Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Wed, 1 Nov 2023 22:44:08 -0400 Subject: [PATCH 3/8] Fix formatting --- Code/consensus/src/executor.rs | 18 ++++++++---------- Code/round/src/events.rs | 10 +++++----- Code/round/src/state_machine.rs | 2 +- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/Code/consensus/src/executor.rs b/Code/consensus/src/executor.rs index 76d51d5dd..7012e03b7 100644 --- a/Code/consensus/src/executor.rs +++ b/Code/consensus/src/executor.rs @@ -239,19 +239,17 @@ where // Multiplex the event with the round state. let mux_event = match event { - RoundEvent::PolkaValue(value_id) => { - match round_state.proposal { - Some(ref proposal) if proposal.value().id() == value_id => - RoundEvent::ProposalAndPolkaCurrent(proposal.clone()), - _ => RoundEvent::PolkaAny, + RoundEvent::PolkaValue(value_id) => match round_state.proposal { + Some(ref proposal) if proposal.value().id() == value_id => { + RoundEvent::ProposalAndPolkaCurrent(proposal.clone()) } + _ => RoundEvent::PolkaAny, }, - RoundEvent::PrecommitValue(value_id) => { - match round_state.proposal { - Some(ref proposal) if proposal.value().id() == value_id => - RoundEvent::ProposalAndPrecommitValue(proposal.clone()), - _ => RoundEvent::PrecommitAny, + RoundEvent::PrecommitValue(value_id) => match round_state.proposal { + Some(ref proposal) if proposal.value().id() == value_id => { + RoundEvent::ProposalAndPrecommitValue(proposal.clone()) } + _ => RoundEvent::PrecommitAny, }, _ => event, }; diff --git a/Code/round/src/events.rs b/Code/round/src/events.rs index 5ed0929e0..8ac2374ec 100644 --- a/Code/round/src/events.rs +++ b/Code/round/src/events.rs @@ -5,18 +5,18 @@ pub enum Event where Ctx: Context, { - NewRound, // Start a new round, not as proposer.L20 - NewRoundProposer(Ctx::Value), // Start a new round and propose the Value.L14 - Proposal(Ctx::Proposal), // Receive a proposal. L22 + L23 (valid) + NewRound, // Start a new round, not as proposer.L20 + NewRoundProposer(Ctx::Value), // Start a new round and propose the Value.L14 + Proposal(Ctx::Proposal), // Receive a proposal. L22 + L23 (valid) ProposalAndPolkaPrevious(Ctx::Proposal), // Recieved a proposal and a polka value from a previous round. L28 + L29 (valid) ProposalInvalid, // Receive an invalid proposal. L26 + L32 (invalid) PolkaValue(ValueId), // Receive +2/3 prevotes for valueId. L44 PolkaAny, // Receive +2/3 prevotes for anything. L34 PolkaNil, // Receive +2/3 prevotes for nil. L44 ProposalAndPolkaCurrent(Ctx::Proposal), // Receive +2/3 prevotes for Value in current round. L36 - PrecommitAny, // Receive +2/3 precommits for anything. L47 + PrecommitAny, // Receive +2/3 precommits for anything. L47 ProposalAndPrecommitValue(Ctx::Proposal), // Receive +2/3 precommits for Value. L49 - PrecommitValue(ValueId), // Receive +2/3 precommits for ValueId. L51 + PrecommitValue(ValueId), // Receive +2/3 precommits for ValueId. L51 RoundSkip, // Receive +1/3 messages from a higher round. OneCorrectProcessInHigherRound, L55 TimeoutPropose, // Timeout waiting for proposal. L57 TimeoutPrevote, // Timeout waiting for prevotes. L61 diff --git a/Code/round/src/state_machine.rs b/Code/round/src/state_machine.rs index a809bba37..dd9350b1c 100644 --- a/Code/round/src/state_machine.rs +++ b/Code/round/src/state_machine.rs @@ -121,7 +121,7 @@ where (_, Event::PrecommitAny) if this_round => schedule_timeout_precommit(state), // L47 (_, Event::TimeoutPrecommit) if this_round => round_skip(state, data.round.increment()), // L65 (_, Event::RoundSkip) if state.round < data.round => round_skip(state, data.round), // L55 - (_, Event::ProposalAndPrecommitValue(proposal)) => commit(state, data.round, proposal), // L49 + (_, Event::ProposalAndPrecommitValue(proposal)) => commit(state, data.round, proposal), // L49 // Invalid transition. _ => Transition::invalid(state), From 5a486c07c523a53e6c82cd3218f68ad9eb7e2843 Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Thu, 2 Nov 2023 19:35:01 -0400 Subject: [PATCH 4/8] Fix executor handling of new round --- Code/common/src/round.rs | 2 +- Code/consensus/src/executor.rs | 18 +++++++++++------- Code/test/tests/consensus_executor.rs | 3 ++- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/Code/common/src/round.rs b/Code/common/src/round.rs index 79fd567a6..651c63463 100644 --- a/Code/common/src/round.rs +++ b/Code/common/src/round.rs @@ -16,7 +16,7 @@ pub enum Round { impl Round { /// The initial, zero round. - pub const INITIAL: Round = Round::new(0); + pub const INITIAL: Round = Round::new(-1); /// Create a new round. /// diff --git a/Code/consensus/src/executor.rs b/Code/consensus/src/executor.rs index 7012e03b7..a6ed3f345 100644 --- a/Code/consensus/src/executor.rs +++ b/Code/consensus/src/executor.rs @@ -50,6 +50,7 @@ where Vote(SignedVote), Decide(Round, Ctx::Value), ScheduleTimeout(Timeout), + NewRound(Round), } impl Executor @@ -92,14 +93,9 @@ where match round_msg { RoundMessage::NewRound(round) => { - // TODO: How to loop Event::NewRoundProposer or Event::NewRound back to the executor? - // TODO: check if we are the proposer // XXX: Check if there is an existing state? assert!(self.round < round); - self.round_states - .insert(round, RoundState::default().new_round(round)); - self.round = round; - None + Some(Message::NewRound(round)) } RoundMessage::Proposal(proposal) => { @@ -233,7 +229,7 @@ where /// Apply the event, update the state. fn apply_event(&mut self, round: Round, event: RoundEvent) -> Option> { // Get the round state, or create a new one - let round_state = self.round_states.remove(&round).unwrap_or_default(); + let mut round_state = self.round_states.remove(&round).unwrap_or_default(); let data = RoundData::new(round, &self.height, &self.address); @@ -251,6 +247,14 @@ where } _ => RoundEvent::PrecommitAny, }, + RoundEvent::NewRound | RoundEvent::NewRoundProposer(_) => { + assert!(self.round < round); + self.round_states + .insert(round, RoundState::default().new_round(round)); + self.round = round; + round_state.round = round; + event + } _ => event, }; diff --git a/Code/test/tests/consensus_executor.rs b/Code/test/tests/consensus_executor.rs index e89afb9b2..217c52804 100644 --- a/Code/test/tests/consensus_executor.rs +++ b/Code/test/tests/consensus_executor.rs @@ -21,6 +21,7 @@ fn to_input_msg(output: Message) -> Option> { Message::Vote(v) => Some(Event::Vote(v)), Message::Decide(_, _) => None, Message::ScheduleTimeout(_) => None, + Message::NewRound(round) => Some(Event::NewRound(round)), } } @@ -532,7 +533,7 @@ fn executor_steps_not_proposer_timeout() { TestStep { desc: "we receive a precommit timeout, start a new round", input_event: Some(Event::TimeoutElapsed(Timeout::precommit(Round::new(0)))), - expected_output: None, + expected_output: Some(Message::NewRound(Round::new(1))), new_state: State { round: Round::new(1), step: Step::NewRound, From 90bd7f313154e34d00845bd4f8a08ecc0f92ea33 Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Fri, 3 Nov 2023 19:28:33 -0400 Subject: [PATCH 5/8] Fix executor initialization --- Code/common/src/round.rs | 3 ++- Code/consensus/src/executor.rs | 4 ++-- Code/test/tests/round.rs | 5 ++++- Code/vote/src/keeper.rs | 4 +++- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/Code/common/src/round.rs b/Code/common/src/round.rs index 651c63463..e3c4d7735 100644 --- a/Code/common/src/round.rs +++ b/Code/common/src/round.rs @@ -16,7 +16,8 @@ pub enum Round { impl Round { /// The initial, zero round. - pub const INITIAL: Round = Round::new(-1); + pub const INITIAL: Round = Round::new(0); + pub const NIL: Round = Round::new(-1); /// Create a new round. /// diff --git a/Code/consensus/src/executor.rs b/Code/consensus/src/executor.rs index a6ed3f345..c8dd81eca 100644 --- a/Code/consensus/src/executor.rs +++ b/Code/consensus/src/executor.rs @@ -65,7 +65,7 @@ where ) -> Self { let votes = VoteKeeper::new( height.clone(), - Round::INITIAL, + Round::NIL, validator_set.total_voting_power(), ); @@ -74,7 +74,7 @@ where private_key: Secret::new(private_key), address, validator_set, - round: Round::INITIAL, + round: Round::NIL, votes, round_states: BTreeMap::new(), } diff --git a/Code/test/tests/round.rs b/Code/test/tests/round.rs index f90c13ea5..4f0dff9e0 100644 --- a/Code/test/tests/round.rs +++ b/Code/test/tests/round.rs @@ -12,9 +12,12 @@ const ADDRESS: Address = Address::new([42; 20]); fn test_propose() { let value = Value::new(42); let height = Height::new(10); + let round = Round::new(0); let mut state: State = State::default(); - let data = RoundData::new(Round::new(0), &height, &ADDRESS); + state.round = round; + + let data = RoundData::new(round, &height, &ADDRESS); let transition = apply_event(state.clone(), &data, Event::NewRoundProposer(value)); diff --git a/Code/vote/src/keeper.rs b/Code/vote/src/keeper.rs index 6149e3927..b346f0632 100644 --- a/Code/vote/src/keeper.rs +++ b/Code/vote/src/keeper.rs @@ -35,7 +35,9 @@ where pub fn new(height: Ctx::Height, round: Round, total_weight: Weight) -> Self { let mut rounds = BTreeMap::new(); - rounds.insert(round, RoundVotes::new(height.clone(), round, total_weight)); + if round != Round::NIL { + rounds.insert(round, RoundVotes::new(height.clone(), round, total_weight)); + } VoteKeeper { height, From dc5a1a55b3f961f8e4be85367d20b873e21fbcae Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Fri, 3 Nov 2023 20:11:13 -0400 Subject: [PATCH 6/8] Add test step for NewRound, fix precommit for nil quorum to emit PrecommitAny --- Code/consensus/src/executor.rs | 2 +- Code/test/tests/consensus_executor.rs | 63 ++++++++++++++++----------- Code/test/tests/vote_keeper.rs | 2 +- Code/vote/src/keeper.rs | 2 +- 4 files changed, 41 insertions(+), 28 deletions(-) diff --git a/Code/consensus/src/executor.rs b/Code/consensus/src/executor.rs index c8dd81eca..a4e337046 100644 --- a/Code/consensus/src/executor.rs +++ b/Code/consensus/src/executor.rs @@ -36,7 +36,7 @@ where private_key: Secret>, address: Ctx::Address, validator_set: Ctx::ValidatorSet, - round: Round, + pub round: Round, votes: VoteKeeper, round_states: BTreeMap>, } diff --git a/Code/test/tests/consensus_executor.rs b/Code/test/tests/consensus_executor.rs index 217c52804..821eb0fb7 100644 --- a/Code/test/tests/consensus_executor.rs +++ b/Code/test/tests/consensus_executor.rs @@ -388,7 +388,7 @@ fn executor_steps_not_proposer() { } #[test] -fn executor_steps_not_proposer_timeout() { +fn executor_steps_not_proposer_timeout_multiple_rounds() { let value = TestContext::DUMMY_VALUE; // TODO: get value from external source let value_id = value.id(); @@ -403,19 +403,19 @@ fn executor_steps_not_proposer_timeout() { let addr3 = Address::from_public_key(&sk3.public_key()); let v1 = Validator::new(sk1.public_key(), 1); - let v2 = Validator::new(sk2.public_key(), 1); - let v3 = Validator::new(sk3.public_key(), 3); + let v2 = Validator::new(sk2.public_key(), 3); + let v3 = Validator::new(sk3.public_key(), 1); - // Proposer is v1, so we are not the proposer - let (my_sk, my_addr) = (sk2, addr2); + // Proposer is v1, so we, v3, are not the proposer + let (my_sk, my_addr) = (sk3, addr3); let vs = ValidatorSet::new(vec![v1.clone(), v2.clone(), v3.clone()]); let mut executor = Executor::new(Height::new(1), vs, my_sk.clone(), my_addr); let steps = vec![ - // Start round 0, we are not the proposer + // Start round 0, we, v3, are not the proposer TestStep { - desc: "Start round 0, we are not the proposer", + desc: "Start round 0, we, v3, are not the proposer", input_event: Some(Event::NewRound(Round::new(0))), expected_output: Some(Message::ScheduleTimeout(Timeout::propose(Round::new(0)))), new_state: State { @@ -426,9 +426,9 @@ fn executor_steps_not_proposer_timeout() { valid: None, }, }, - // Receive a propose timeout, prevote for nil (v1) + // Receive a propose timeout, prevote for nil (from v3) TestStep { - desc: "Receive a propose timeout, prevote for nil (v1)", + desc: "Receive a propose timeout, prevote for nil (v3)", input_event: Some(Event::TimeoutElapsed(Timeout::propose(Round::new(0)))), expected_output: Some(Message::Vote( Vote::new_prevote(Round::new(0), None, my_addr).signed(&my_sk), @@ -441,9 +441,9 @@ fn executor_steps_not_proposer_timeout() { valid: None, }, }, - // Receive our own prevote v1 + // Receive our own prevote v3 TestStep { - desc: "Receive our own prevote v1", + desc: "Receive our own prevote v3", input_event: None, expected_output: None, new_state: State { @@ -454,9 +454,9 @@ fn executor_steps_not_proposer_timeout() { valid: None, }, }, - // v2 prevotes for its own proposal + // v1 prevotes for its own proposal TestStep { - desc: "v2 prevotes for its own proposal", + desc: "v1 prevotes for its own proposal", input_event: Some(Event::Vote( Vote::new_prevote(Round::new(0), Some(value_id), addr1).signed(&sk1), )), @@ -469,11 +469,11 @@ fn executor_steps_not_proposer_timeout() { valid: None, }, }, - // v3 prevotes for nil, it gets +2/3 prevotes, precommit for it (v1) + // v2 prevotes for nil, we get +2/3 nil prevotes and precommit for nil TestStep { - desc: "v3 prevotes for nil, it gets +2/3 prevotes, precommit for it (v1)", + desc: "v2 prevotes for nil, we get +2/3 prevotes, precommit for nil", input_event: Some(Event::Vote( - Vote::new_prevote(Round::new(0), None, addr3).signed(&sk3), + Vote::new_prevote(Round::new(0), None, addr2).signed(&sk2), )), expected_output: Some(Message::Vote( Vote::new_precommit(Round::new(0), None, my_addr).signed(&my_sk), @@ -486,9 +486,9 @@ fn executor_steps_not_proposer_timeout() { valid: None, }, }, - // v1 receives its own precommit + // v3 receives its own precommit TestStep { - desc: "v1 receives its own precommit", + desc: "v3 receives its own precommit", input_event: None, expected_output: None, new_state: State { @@ -499,9 +499,9 @@ fn executor_steps_not_proposer_timeout() { valid: None, }, }, - // v2 precommits its proposal + // v1 precommits its proposal TestStep { - desc: "v2 precommits its proposal", + desc: "v1 precommits its proposal", input_event: Some(Event::Vote( Vote::new_precommit(Round::new(0), Some(value_id), addr1).signed(&sk1), )), @@ -514,13 +514,13 @@ fn executor_steps_not_proposer_timeout() { valid: None, }, }, - // v3 precommits for nil + // v2 precommits for nil TestStep { - desc: "v3 precommits for nil", + desc: "v2 precommits for nil", input_event: Some(Event::Vote( - Vote::new_precommit(Round::new(0), None, addr3).signed(&sk3), + Vote::new_precommit(Round::new(0), None, addr2).signed(&sk2), )), - expected_output: None, + expected_output: Some(Message::ScheduleTimeout(Timeout::precommit(Round::new(0)))), new_state: State { round: Round::new(0), step: Step::Precommit, @@ -542,6 +542,18 @@ fn executor_steps_not_proposer_timeout() { valid: None, }, }, + TestStep { + desc: "Start round 1, we are not the proposer", + input_event: Some(Event::NewRound(Round::new(1))), + expected_output: Some(Message::ScheduleTimeout(Timeout::propose(Round::new(1)))), + new_state: State { + round: Round::new(1), + step: Step::Propose, + proposal: None, + locked: None, + valid: None, + }, + }, ]; let mut previous_message = None; @@ -556,7 +568,8 @@ fn executor_steps_not_proposer_timeout() { let output = executor.execute(execute_message); assert_eq!(output, step.expected_output, "expected output message"); - let new_state = executor.round_state(Round::new(0)).unwrap(); + // TODO - add expected executor round to test and assert before the new_state check below + let new_state = executor.round_state(executor.round).unwrap(); assert_eq!(new_state, &step.new_state, "new state"); previous_message = output.and_then(to_input_msg); diff --git a/Code/test/tests/vote_keeper.rs b/Code/test/tests/vote_keeper.rs index 77eeae569..2eed7ad6a 100644 --- a/Code/test/tests/vote_keeper.rs +++ b/Code/test/tests/vote_keeper.rs @@ -34,7 +34,7 @@ fn precommit_apply_nil() { assert_eq!(msg, None); let msg = keeper.apply_vote(vote, 1); - assert_eq!(msg, None); + assert_eq!(msg, Some(Message::PrecommitAny)); } #[test] diff --git a/Code/vote/src/keeper.rs b/Code/vote/src/keeper.rs index b346f0632..1240dcc70 100644 --- a/Code/vote/src/keeper.rs +++ b/Code/vote/src/keeper.rs @@ -89,7 +89,7 @@ where (VoteType::Prevote, Threshold::Value(v)) => Some(Message::PolkaValue(v)), (VoteType::Precommit, Threshold::Any) => Some(Message::PrecommitAny), - (VoteType::Precommit, Threshold::Nil) => None, + (VoteType::Precommit, Threshold::Nil) => Some(Message::PrecommitAny), (VoteType::Precommit, Threshold::Value(v)) => Some(Message::PrecommitValue(v)), } } From 9c4f56e5bc7eff580b73c759a742e553d9232323 Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Fri, 3 Nov 2023 20:19:42 -0400 Subject: [PATCH 7/8] Review comments --- Code/consensus/src/executor.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/Code/consensus/src/executor.rs b/Code/consensus/src/executor.rs index a4e337046..1e1146ca9 100644 --- a/Code/consensus/src/executor.rs +++ b/Code/consensus/src/executor.rs @@ -138,6 +138,11 @@ where RoundEvent::NewRound }; + assert!(self.round < round); + self.round_states + .insert(round, RoundState::default().new_round(round)); + self.round = round; + self.apply_event(round, event) } @@ -229,7 +234,7 @@ where /// Apply the event, update the state. fn apply_event(&mut self, round: Round, event: RoundEvent) -> Option> { // Get the round state, or create a new one - let mut round_state = self.round_states.remove(&round).unwrap_or_default(); + let round_state = self.round_states.remove(&round).unwrap_or_default(); let data = RoundData::new(round, &self.height, &self.address); @@ -247,14 +252,7 @@ where } _ => RoundEvent::PrecommitAny, }, - RoundEvent::NewRound | RoundEvent::NewRoundProposer(_) => { - assert!(self.round < round); - self.round_states - .insert(round, RoundState::default().new_round(round)); - self.round = round; - round_state.round = round; - event - } + _ => event, }; From 045f0653b24de2b1d8a5f840fac94fdf76ebcfcd Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Fri, 3 Nov 2023 20:39:40 -0400 Subject: [PATCH 8/8] Remove dead code --- Code/round/src/state.rs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/Code/round/src/state.rs b/Code/round/src/state.rs index 4d89770a5..55ef389b4 100644 --- a/Code/round/src/state.rs +++ b/Code/round/src/state.rs @@ -76,29 +76,10 @@ where ..self } } - - pub fn next_step(self) -> Self { - let step = match self.step { - Step::NewRound => Step::Propose, - Step::Propose => Step::Prevote, - Step::Prevote => Step::Precommit, - _ => self.step, - }; - - Self { step, ..self } - } - pub fn with_step(self, step: Step) -> Self { Self { step, ..self } } - pub fn commit_step(self) -> Self { - Self { - step: Step::Commit, - ..self - } - } - pub fn set_locked(self, value: Ctx::Value) -> Self { Self { locked: Some(RoundValue::new(value, self.round)),