diff --git a/code/crates/core-driver/src/driver.rs b/code/crates/core-driver/src/driver.rs index 81790ee46..1e6237542 100644 --- a/code/crates/core-driver/src/driver.rs +++ b/code/crates/core-driver/src/driver.rs @@ -315,6 +315,22 @@ where .get_proposal_and_validity_for_round_and_value(round, value_id) } + /// Returns a valid proposal for the given round and value_id, if any. + pub fn valid_proposal_for_round_and_value( + &self, + round: Round, + value_id: ValueId, + ) -> Option<&SignedProposal> { + if let Some((proposal, validity)) = + self.proposal_and_validity_for_round_and_value(round, value_id) + { + if validity.is_valid() { + return Some(proposal); + } + } + None + } + /// Returns the proposals and their validities for the given round, if any. pub fn proposals_and_validities_for_round( &self, diff --git a/code/crates/core-driver/src/mux.rs b/code/crates/core-driver/src/mux.rs index acfd57113..d511b3c87 100644 --- a/code/crates/core-driver/src/mux.rs +++ b/code/crates/core-driver/src/mux.rs @@ -298,17 +298,14 @@ where VKOutput::PrecommitAny => (threshold_round, RoundInput::PrecommitAny), VKOutput::SkipRound(r) => (threshold_round, RoundInput::SkipRound(r)), VKOutput::PrecommitValue(v) => { - if let Some((proposal, validity)) = - self.proposal_and_validity_for_round_and_value(threshold_round, v) + if let Some(proposal) = self.valid_proposal_for_round_and_value(threshold_round, v) { - if validity.is_valid() { - ( - threshold_round, - RoundInput::ProposalAndPrecommitValue(proposal.message.clone()), - ) - } else { - (threshold_round, RoundInput::PrecommitAny) - } + ( + threshold_round, + RoundInput::ProposalAndPrecommitValue(proposal.message.clone()), + ) + } else if threshold_round > self.round() { + (threshold_round, RoundInput::SkipRound(threshold_round)) } else { (threshold_round, RoundInput::PrecommitAny) } diff --git a/code/crates/core-driver/tests/it/extra.rs b/code/crates/core-driver/tests/it/extra.rs index 3cd1ae5f6..8d3d61f0f 100644 --- a/code/crates/core-driver/tests/it/extra.rs +++ b/code/crates/core-driver/tests/it/extra.rs @@ -2124,78 +2124,6 @@ fn driver_step_change_mux_with_proposal_and_polka() { run_steps(&mut driver, steps); } -#[test] -fn driver_step_change_mux_with_proposal_and_commit_quorum() { - let value: 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 height = Height::new(1); - let ctx = TestContext::new(); - let vs = ValidatorSet::new(vec![v1.clone(), v2.clone(), v3.clone()]); - - let proposal = Proposal::new( - Height::new(1), - Round::new(1), - value.clone(), - Round::Nil, - v1.address, - ); - - let mut driver = Driver::new(ctx, height, vs, my_addr, Default::default()); - - let steps = vec![ - TestStep { - desc: "Start round 0, we, v3, are not the proposer, start timeout propose", - input: new_round_input(Round::new(0), v1.address), - expected_outputs: vec![start_propose_timer_output(Round::new(0))], - expected_round: Round::new(0), - new_state: propose_state(Round::new(0)), - }, - TestStep { - desc: "Receive proposal for round 1, store", - input: proposal_input( - Round::new(1), - value.clone(), - Round::Nil, - Validity::Valid, - v1.address, - ), - expected_outputs: vec![], - expected_round: Round::new(0), - new_state: propose_state(Round::new(0)), - }, - TestStep { - desc: "v1 precommits value for round 1", - input: precommit_input_at(Round::new(1), value.clone(), &v1.address), - expected_outputs: vec![], - expected_round: Round::new(0), - new_state: propose_state(Round::new(0)), - }, - TestStep { - desc: "v2 precommits value for round 1, we hit f+1 threshold, move to round 1", - input: precommit_input_at(Round::new(1), value.clone(), &v2.address), - expected_outputs: vec![new_round_output(Round::new(1))], - expected_round: Round::new(1), - new_state: new_round(Round::new(1)), - }, - TestStep { - desc: - "Start round 1, change step to propose, start propose timer, mux proposal, decide", - input: new_round_input(Round::new(1), v2.address), - expected_outputs: vec![ - start_propose_timer_output(Round::new(1)), - decide_output(Round::new(1), proposal), - ], - expected_round: Round::new(1), - new_state: decided_state(Round::new(1), Round::new(1), value), - }, - ]; - - run_steps(&mut driver, steps); -} - #[test] fn proposal_mux_with_polka() { let value: Value = Value::new(9999); @@ -3484,26 +3412,26 @@ fn invalid_proposal_becomes_valid_decision_via_votes() { expected_outputs: vec![], new_state: prevote_state(Round::new(0)), }, + // Assume here that the process crashed, and the validation function was updated. TestStep { - desc: "v1 precommits invalid value v", - input: precommit_input(Round::new(0), value_v.clone(), &v1.address), - expected_outputs: vec![], + desc: "Receive a Proposal for v, which is now deemed Valid", + input: proposal_input_from_proposal(proposal.clone(), Validity::Valid), expected_round: Round::new(0), + expected_outputs: vec![], new_state: prevote_state(Round::new(0)), }, TestStep { - desc: "v2 precommits invalid v, we get +2/3 and start timeout", - input: precommit_input(Round::new(0), value_v.clone(), &v2.address), + desc: "v1 precommits v", + input: precommit_input(Round::new(0), value_v.clone(), &v1.address), + expected_outputs: vec![], expected_round: Round::new(0), - expected_outputs: vec![start_precommit_timer_output(Round::new(0))], new_state: prevote_state(Round::new(0)), }, - // Assume here that the process crashed, and the validation function was updated. TestStep { - desc: "The validity of v is updated to Valid", - input: proposal_input_from_proposal(proposal.clone(), Validity::Valid), + desc: "v2 precommits v, we get +2/3 and decide v", + input: precommit_input(Round::new(0), value_v.clone(), &v2.address), + expected_outputs: vec![decide_output(Round::new(0), proposal)], expected_round: Round::new(0), - expected_outputs: vec![decide_output(Round::new(0), proposal.clone())], new_state: decided_state(Round::new(0), Round::new(0), value_v), }, ]; @@ -3556,23 +3484,31 @@ fn invalid_proposal_becomes_valid_decision_via_certificate() { expected_outputs: vec![], new_state: prevote_state(Round::new(0)), }, + // Assume here that the process crashed, and the validation function was updated. TestStep { - desc: "Commit certificate for invalid value v, start timeout", - input: commit_certificate_input_at( - Round::new(0), - value_v.clone(), - &[v1.address, v2.address], - ), - expected_outputs: vec![start_precommit_timer_output(Round::new(0))], + desc: "Receive a Proposal for v, which is now deemed Valid", + input: proposal_input_from_proposal(proposal.clone(), Validity::Valid), expected_round: Round::new(0), + expected_outputs: vec![], new_state: prevote_state(Round::new(0)), }, - // Assume here that the process crashed, and the validation function was updated. + // This should never happen. We should add an error message here. TestStep { - desc: "The validity of v is updated to Valid", - input: proposal_input_from_proposal(proposal.clone(), Validity::Valid), + desc: "Receive a Proposal for v, back to Invalid, should be ignored", + input: proposal_input_from_proposal(proposal.clone(), Validity::Invalid), expected_round: Round::new(0), + expected_outputs: vec![], + new_state: prevote_state(Round::new(0)), + }, + TestStep { + desc: "we get a certificate for v and decide v", + input: commit_certificate_input_at( + Round::new(0), + value_v.clone(), + &[v1.address, v2.address], + ), expected_outputs: vec![decide_output(Round::new(0), proposal)], + expected_round: Round::new(0), new_state: decided_state(Round::new(0), Round::new(0), value_v), }, ]; @@ -3642,6 +3578,223 @@ fn sync_decision_certificate_then_proposal() { run_steps(&mut driver, steps); } +// Replaces the old driver_step_change_mux_with_proposal_and_commit_quorum. +// When a node has SkipVote(r) and (r, PrecommitValue(v)) possible events at the same time, +// the latter must have precedence: we decide as soon as possible, without starting rounds. +#[test] +fn round_1_decision_during_round_0() { + 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 height = Height::new(1); + let ctx = TestContext::new(); + let vs = ValidatorSet::new(vec![v1.clone(), v2.clone(), v3.clone()]); + + let proposal = Proposal::new( + Height::new(1), + Round::new(1), + value.clone(), + Round::Nil, + v1.address, + ); + + let mut driver = Driver::new(ctx, height, vs, my_addr, Default::default()); + + let steps = vec![ + TestStep { + desc: "Start round 0, we are not the proposer", + input: new_round_input(Round::new(0), v1.address), + expected_outputs: vec![start_propose_timer_output(Round::new(0))], + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + TestStep { + desc: "v1 precommits a proposal in round 1", + input: precommit_input(Round::new(1), value.clone(), &v1.address), + expected_outputs: vec![], + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + TestStep { + desc: "Receive proposal from round 1", + input: proposal_input_from_proposal(proposal.clone(), Validity::Valid), + expected_outputs: vec![], + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + TestStep { + desc: "v2 precommits the same round 1 proposal, we have a decision", + input: precommit_input(Round::new(1), value.clone(), &v2.address), + expected_outputs: vec![decide_output(Round::new(0), proposal)], + expected_round: Round::new(0), + new_state: decided_state(Round::new(0), Round::new(1), value), + }, + ]; + + run_steps(&mut driver, steps); +} + +#[test] +fn round_1_decision_during_round_0_via_certificate() { + 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 height = Height::new(1); + let ctx = TestContext::new(); + let vs = ValidatorSet::new(vec![v1.clone(), v2.clone(), v3.clone()]); + + let proposal = Proposal::new( + Height::new(1), + Round::new(1), + value.clone(), + Round::Nil, + v1.address, + ); + + let mut driver = Driver::new(ctx, height, vs, my_addr, Default::default()); + + let steps = vec![ + TestStep { + desc: "Start round 0, we are not the proposer", + input: new_round_input(Round::new(0), v1.address), + expected_outputs: vec![start_propose_timer_output(Round::new(0))], + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + TestStep { + desc: "we get a commit certificate for v at round 1", + input: commit_certificate_input_at( + Round::new(1), + value.clone(), + &[v1.address, v2.address], + ), + expected_outputs: vec![], + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + TestStep { + desc: "Receive proposal from round 1", + input: proposal_input_from_proposal(proposal.clone(), Validity::Valid), + expected_outputs: vec![decide_output(Round::new(0), proposal)], + expected_round: Round::new(0), + new_state: decided_state(Round::new(0), Round::new(1), value), + }, + ]; + + run_steps(&mut driver, steps); +} + +#[test] +fn round_1_invalid_decision_during_round_0() { + 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 height = Height::new(1); + let ctx = TestContext::new(); + let vs = ValidatorSet::new(vec![v1.clone(), v2.clone(), v3.clone()]); + + let proposal = Proposal::new( + Height::new(1), + Round::new(1), + value.clone(), + Round::Nil, + v1.address, + ); + + let mut driver = Driver::new(ctx, height, vs, my_addr, Default::default()); + + let steps = vec![ + TestStep { + desc: "Start round 0, we are not the proposer", + input: new_round_input(Round::new(0), v1.address), + expected_outputs: vec![start_propose_timer_output(Round::new(0))], + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + TestStep { + desc: "v1 precommits a proposal in round 1", + input: precommit_input(Round::new(1), value.clone(), &v1.address), + expected_outputs: vec![], + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + TestStep { + desc: "Receive proposal from round 1, which is invalid", + input: proposal_input_from_proposal(proposal.clone(), Validity::Invalid), + expected_outputs: vec![], + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + TestStep { + desc: "v2 precommits the same round 1 proposal, but we don't decide", + input: precommit_input(Round::new(1), value.clone(), &v2.address), + expected_outputs: vec![new_round_output(Round::new(1))], + expected_round: Round::new(1), + new_state: new_round(Round::new(1)), + }, + ]; + + run_steps(&mut driver, steps); +} + +#[test] +fn round_1_invalid_decision_during_round_0_via_certificate() { + 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 height = Height::new(1); + let ctx = TestContext::new(); + let vs = ValidatorSet::new(vec![v1.clone(), v2.clone(), v3.clone()]); + + let proposal = Proposal::new( + Height::new(1), + Round::new(1), + value.clone(), + Round::Nil, + v1.address, + ); + + let mut driver = Driver::new(ctx, height, vs, my_addr, Default::default()); + + let steps = vec![ + TestStep { + desc: "Start round 0, we are not the proposer", + input: new_round_input(Round::new(0), v1.address), + expected_outputs: vec![start_propose_timer_output(Round::new(0))], + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + TestStep { + desc: "we get a commit certificate for v at round 1", + input: commit_certificate_input_at( + Round::new(1), + value.clone(), + &[v1.address, v2.address], + ), + expected_outputs: vec![], + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + TestStep { + desc: "Receive proposal from round 1, which is invalid", + input: proposal_input_from_proposal(proposal.clone(), Validity::Invalid), + expected_outputs: vec![], + expected_round: Round::new(0), + new_state: propose_state(Round::new(0)), + }, + ]; + + run_steps(&mut driver, steps); +} + // Commit certificate from driver after re-applied votes (round-certificate simulation). // // We are v2 (local node). Sequence: diff --git a/code/crates/core-votekeeper/src/keeper.rs b/code/crates/core-votekeeper/src/keeper.rs index cd581b17b..b45cf228b 100644 --- a/code/crates/core-votekeeper/src/keeper.rs +++ b/code/crates/core-votekeeper/src/keeper.rs @@ -281,21 +281,6 @@ where } } - if vote.round() > round { - let combined_weight = per_round.addresses_weights.sum(); - - let skip_round = self - .threshold_params - .honest - .is_met(combined_weight, total_weight); - - if skip_round { - let output = Output::SkipRound(vote.round()); - per_round.emitted_outputs.insert(output.clone()); - return Some(output); - } - } - let threshold = compute_threshold( vote.vote_type(), per_round, @@ -304,7 +289,18 @@ where total_weight, ); - let output = threshold_to_output(vote.vote_type(), threshold); + let skip_round = if vote.round() > round + && self + .threshold_params + .honest + .is_met(per_round.addresses_weights.sum(), total_weight) + { + Some(vote.round()) + } else { + None + }; + + let output = threshold_to_output(vote.vote_type(), threshold, skip_round); match output { // Ensure we do not output the same message twice @@ -373,16 +369,31 @@ where } /// Map a vote type and a threshold to a state machine output. -fn threshold_to_output(typ: VoteType, threshold: Threshold) -> Option> { - match (typ, threshold) { - (_, Threshold::Unreached) => None, - - (VoteType::Prevote, Threshold::Any) => Some(Output::PolkaAny), - (VoteType::Prevote, Threshold::Nil) => Some(Output::PolkaNil), - (VoteType::Prevote, Threshold::Value(v)) => Some(Output::PolkaValue(v)), +/// Also considers an optional honest threshold for a future round. +fn threshold_to_output( + typ: VoteType, + threshold: Threshold, + future_round: Option, +) -> Option> { + if future_round.is_none() { + // Thresholds for the current round + match (typ, threshold) { + (_, Threshold::Unreached) => None, + + (VoteType::Prevote, Threshold::Any) => Some(Output::PolkaAny), + (VoteType::Prevote, Threshold::Nil) => Some(Output::PolkaNil), + (VoteType::Prevote, Threshold::Value(v)) => Some(Output::PolkaValue(v)), + + (VoteType::Precommit, Threshold::Any) => Some(Output::PrecommitAny), + (VoteType::Precommit, Threshold::Nil) => Some(Output::PrecommitAny), + (VoteType::Precommit, Threshold::Value(v)) => Some(Output::PrecommitValue(v)), + } + } else { + // Only PrecommitValue(v) has larger priority than SkipRound(r) + match (typ, threshold) { + (VoteType::Precommit, Threshold::Value(v)) => Some(Output::PrecommitValue(v)), - (VoteType::Precommit, Threshold::Any) => Some(Output::PrecommitAny), - (VoteType::Precommit, Threshold::Nil) => Some(Output::PrecommitAny), - (VoteType::Precommit, Threshold::Value(v)) => Some(Output::PrecommitValue(v)), + (_, _) => Some(Output::SkipRound(future_round.unwrap())), + } } } diff --git a/code/crates/core-votekeeper/tests/vote_keeper.rs b/code/crates/core-votekeeper/tests/vote_keeper.rs index 9f78cb940..174790620 100644 --- a/code/crates/core-votekeeper/tests/vote_keeper.rs +++ b/code/crates/core-votekeeper/tests/vote_keeper.rs @@ -270,6 +270,25 @@ fn no_skip_round_full_quorum_with_same_val() { assert_eq!(msg, None); } +#[test] +fn skip_round_and_precommit_value_future_round() { + let ([addr1, addr2, ..], mut keeper) = setup([2, 3, 2]); + + let id = ValueId::new(1); + let val = NilOrVal::Val(id); + let height = Height::new(1); + let cur_round = Round::new(0); + let fut_round = Round::new(1); + + let vote = new_signed_precommit(height, fut_round, val, addr1); + let msg = keeper.apply_vote(vote, cur_round); + assert_eq!(msg, None); + + let vote = new_signed_precommit(height, fut_round, val, addr2); + let msg = keeper.apply_vote(vote, cur_round); + assert_eq!(msg, Some(Output::PrecommitValue(id))); +} + #[test] fn same_votes() { let ([addr1, ..], mut keeper) = setup([1, 1]);