Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propose checks, id in prevote and precommit #7

Merged
merged 7 commits into from
Oct 24, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Formatting and clippy
romac committed Oct 23, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit ae59ac2f7d00dfce973ccfc09b3cae8834d05a68
12 changes: 4 additions & 8 deletions Code/consensus/src/executor.rs
Original file line number Diff line number Diff line change
@@ -58,7 +58,7 @@

self.round_states
.insert(round, RoundState::new(self.height).new_round(round));
None

Check warning on line 61 in Code/consensus/src/executor.rs

Codecov / codecov/patch

Code/consensus/src/executor.rs#L61

Added line #L61 was not covered by tests
}
RoundMessage::Proposal(p) => {
// sign the proposal
@@ -72,11 +72,11 @@
}
RoundMessage::Timeout(_) => {
// schedule the timeout
None

Check warning on line 75 in Code/consensus/src/executor.rs

Codecov / codecov/patch

Code/consensus/src/executor.rs#L75

Added line #L75 was not covered by tests
}
RoundMessage::Decision(_) => {
// update the state
None

Check warning on line 79 in Code/consensus/src/executor.rs

Codecov / codecov/patch

Code/consensus/src/executor.rs#L79

Added line #L79 was not covered by tests
}
}
}
@@ -96,7 +96,7 @@
let value = self.get_value();
RoundEvent::NewRoundProposer(value)
} else {
RoundEvent::NewRound

Check warning on line 99 in Code/consensus/src/executor.rs

Codecov / codecov/patch

Code/consensus/src/executor.rs#L99

Added line #L99 was not covered by tests
};
self.apply_event(round, event)
}
@@ -108,41 +108,41 @@

let Some(round_state) = self.round_states.get(&self.round) else {
// TODO: Add logging
return None;

Check warning on line 111 in Code/consensus/src/executor.rs

Codecov / codecov/patch

Code/consensus/src/executor.rs#L111

Added line #L111 was not covered by tests
};

if round_state.proposal.is_some() {
return None;

Check warning on line 115 in Code/consensus/src/executor.rs

Codecov / codecov/patch

Code/consensus/src/executor.rs#L115

Added line #L115 was not covered by tests
}

if round_state.height != proposal.height || proposal.round != self.round {
return None;

Check warning on line 119 in Code/consensus/src/executor.rs

Codecov / codecov/patch

Code/consensus/src/executor.rs#L119

Added line #L119 was not covered by tests
}

if !proposal.pol_round.is_valid()
|| proposal.pol_round.is_defined() && proposal.pol_round >= round_state.round
{
return None;

Check warning on line 125 in Code/consensus/src/executor.rs

Codecov / codecov/patch

Code/consensus/src/executor.rs#L125

Added line #L125 was not covered by tests
}

// TODO verify proposal signature (make some of these checks part of message validation)

match proposal.pol_round {

Check warning on line 130 in Code/consensus/src/executor.rs

Codecov / codecov/patch

Code/consensus/src/executor.rs#L130

Added line #L130 was not covered by tests
Round::None => {
// Is it possible to get +2/3 prevotes before the proposal?
// Do we wait for our own prevote to check the threshold?
self.apply_event(round, event)
}
Round::Some(_)
if self.votes.check_threshold(
&proposal.pol_round,
VoteType::Prevote,
Threshold::Value(Arc::from(proposal.value.id())),
) =>
{
self.apply_event(round, event)

Check warning on line 143 in Code/consensus/src/executor.rs

Codecov / codecov/patch

Code/consensus/src/executor.rs#L137-L143

Added lines #L137 - L143 were not covered by tests
}
_ => None,

Check warning on line 145 in Code/consensus/src/executor.rs

Codecov / codecov/patch

Code/consensus/src/executor.rs#L145

Added line #L145 was not covered by tests
}
}

@@ -194,7 +194,7 @@
#[cfg(test)]
mod tests {
use super::*;
use malachite_common::{Validator, Value, Proposal};
use malachite_common::{Proposal, Validator, Value};
use malachite_round::state::{RoundValue, State, Step};

#[test]
@@ -211,12 +211,7 @@

let mut executor = Executor::new(Height::new(1), vs, key.clone());

let proposal = Proposal::new(
Height::new(1),
Round::new(0),
value.clone(),
Round::new(-1),
);
let proposal = Proposal::new(Height::new(1), Round::new(0), value.clone(), Round::new(-1));
struct TestStep {
input_message: Option<Message>,
expected_output_message: Option<Message>,
@@ -318,7 +313,8 @@
previous_message.clone()
} else {
step.input_message
}.unwrap();
}
.unwrap();
let message = executor.execute(execute_message);
assert_eq!(message, step.expected_output_message);
let new_state = executor.round_states.get(&Round::new(0)).unwrap();
6 changes: 5 additions & 1 deletion Code/round/src/state_machine.rs
Original file line number Diff line number Diff line change
@@ -72,25 +72,25 @@
state.proposal = Some(proposal.clone());
prevote(state, proposal.round, proposal.value.id())
} else {
prevote_nil(state)

Check warning on line 75 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L75

Added line #L75 was not covered by tests
}
}

(Step::Propose, Event::Proposal(proposal))

Check warning on line 79 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L79

Added line #L79 was not covered by tests
if this_round && is_valid_pol_round(&state, proposal.pol_round) =>
{
// L28
let Some(locked) = state.locked.as_ref() else {

Check warning on line 83 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L83

Added line #L83 was not covered by tests
// TODO: Add logging
return Transition::invalid(state);

Check warning on line 85 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L85

Added line #L85 was not covered by tests
};

if proposal.value.valid()
&& (locked.round <= proposal.pol_round || locked.value == proposal.value)

Check warning on line 89 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L88-L89

Added lines #L88 - L89 were not covered by tests
{
prevote(state, proposal.round, proposal.value.id())

Check warning on line 91 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L91

Added line #L91 was not covered by tests
} else {
prevote_nil(state)

Check warning on line 93 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L93

Added line #L93 was not covered by tests
}
}
(Step::Propose, Event::ProposalInvalid) if this_round => prevote_nil(state), // L22/L25, L28/L31
@@ -103,8 +103,8 @@
(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)

Check warning on line 107 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L106-L107

Added lines #L106 - L107 were not covered by tests
} // L36/L42 - NOTE: only once?

// From Commit. No more state transitions.
@@ -112,9 +112,9 @@

// From all (except Commit). Various round guards.
(_, 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

Check warning on line 117 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L115-L117

Added lines #L115 - L117 were not covered by tests

// Invalid transition.
_ => Transition::invalid(state),
@@ -150,7 +150,7 @@
pub fn prevote(state: State, vr: Round, proposed: ValueId) -> Transition {
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

Check warning on line 153 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L153

Added line #L153 was not covered by tests
Some(_) => None, // we're locked on a higher round with a different value, prevote nil
None => Some(proposed), // not locked, prevote the value
};
@@ -180,9 +180,13 @@
pub fn precommit(state: State, value_id: ValueId) -> Transition {
let message = Message::precommit(state.round, Some(value_id), ADDRESS);

let Some(value) = state.proposal.as_ref().map(|proposal| proposal.value.clone()) else {
let Some(value) = state
.proposal
.as_ref()
.map(|proposal| proposal.value.clone())
else {
// TODO: Add logging
return Transition::invalid(state);

Check warning on line 189 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L189

Added line #L189 was not covered by tests
};

let next = state.set_locked(value.clone()).set_valid(value).next_step();
@@ -239,20 +243,20 @@
/// 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 {

Check warning on line 246 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L246

Added line #L246 was not covered by tests
// check that we're locked on this value

let Some(locked) = state.locked.as_ref() else {

Check warning on line 249 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L249

Added line #L249 was not covered by tests
// TODO: Add logging
return Transition::invalid(state);

Check warning on line 251 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L251

Added line #L251 was not covered by tests
};

if locked.value.id() != value_id {

Check warning on line 254 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L254

Added line #L254 was not covered by tests
// TODO: Add logging
return Transition::invalid(state);
}

Transition::to(state.clone().set_valid(locked.value.clone()))

Check warning on line 259 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L256-L259

Added lines #L256 - L259 were not covered by tests
}

//---------------------------------------------------------------------
@@ -270,19 +274,19 @@
/// 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 {

Check warning on line 277 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L277

Added line #L277 was not covered by tests
// Check that we're locked on this value
let Some(locked) = state.locked.as_ref() else {

Check warning on line 279 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L279

Added line #L279 was not covered by tests
// TODO: Add logging
return Transition::invalid(state);

Check warning on line 281 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L281

Added line #L281 was not covered by tests
};

if locked.value.id() != value_id {

Check warning on line 284 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L284

Added line #L284 was not covered by tests
// TODO: Add logging
return Transition::invalid(state);
}

let message = Message::decision(round, locked.value.clone());

Check warning on line 289 in Code/round/src/state_machine.rs

Codecov / codecov/patch

Code/round/src/state_machine.rs#L286-L289

Added lines #L286 - L289 were not covered by tests
Transition::to(state.commit_step()).with_message(message)
}

6 changes: 3 additions & 3 deletions Code/vote/src/count.rs
Original file line number Diff line number Diff line change
@@ -90,14 +90,14 @@
// No quorum
Threshold::Init
}
pub fn check_threshold(&self, threshold: Threshold) -> bool {
match threshold {
Threshold::Init => false,
Threshold::Any => self.values_weights.highest_weighted_value().is_some(),
Threshold::Nil => self.nil > 0,
Threshold::Value(value) => self.values_weights.value_weights.contains_key(&value),

Check warning on line 98 in Code/vote/src/count.rs

Codecov / codecov/patch

Code/vote/src/count.rs#L93-L98

Added lines #L93 - L98 were not covered by tests
}
}

Check warning on line 100 in Code/vote/src/count.rs

Codecov / codecov/patch

Code/vote/src/count.rs#L100

Added line #L100 was not covered by tests
}

//-------------------------------------------------------------------------
@@ -153,7 +153,7 @@
#[test]
fn add_votes_single_value() {
let v = ValueId::new(1);
let val = Some(v.clone());
let val = Some(v);
let total = 4;
let weight = 1;

@@ -182,8 +182,8 @@
fn add_votes_multi_values() {
let v1 = ValueId::new(1);
let v2 = ValueId::new(2);
let val1 = Some(v1.clone());
let val2 = Some(v2.clone());
let val1 = Some(v1);
let val2 = Some(v2);
let total = 15;

let mut round_votes = RoundVotes::new(Height::new(1), Round::new(0), total);
8 changes: 3 additions & 5 deletions Code/vote/src/keeper.rs
Original file line number Diff line number Diff line change
@@ -42,22 +42,22 @@
Self::to_event(vote_type, threshold)
}

pub fn check_threshold(
&self,
round: &Round,
vote_type: VoteType,
threshold: Threshold,
) -> bool {
let round = match self.rounds.get(round) {
Some(round) => round,
None => return false,

Check warning on line 53 in Code/vote/src/keeper.rs

Codecov / codecov/patch

Code/vote/src/keeper.rs#L45-L53

Added lines #L45 - L53 were not covered by tests
};

match vote_type {
VoteType::Prevote => round.prevotes.check_threshold(threshold),
VoteType::Precommit => round.precommits.check_threshold(threshold),

Check warning on line 58 in Code/vote/src/keeper.rs

Codecov / codecov/patch

Code/vote/src/keeper.rs#L56-L58

Added lines #L56 - L58 were not covered by tests
}
}

Check warning on line 60 in Code/vote/src/keeper.rs

Codecov / codecov/patch

Code/vote/src/keeper.rs#L60

Added line #L60 was not covered by tests

/// Map a vote type and a threshold to a state machine event.
fn to_event(typ: VoteType, threshold: Threshold) -> Option<Event> {
@@ -70,9 +70,7 @@

(VoteType::Precommit, Threshold::Any) => Some(Event::PrecommitAny),
(VoteType::Precommit, Threshold::Nil) => None,
(VoteType::Precommit, Threshold::Value(v)) => {
Some(Event::PrecommitValue(*v.as_ref()))
}
(VoteType::Precommit, Threshold::Value(v)) => Some(Event::PrecommitValue(*v.as_ref())),
}
}
}
@@ -120,7 +118,7 @@
let mut keeper = VoteKeeper::new(Height::new(1), Round::INITIAL, 4);

let v = ValueId::new(1);
let val = Some(v.clone());
let val = Some(v);
let vote = Vote::new_prevote(Round::new(0), val, Address::new(1));

let event = keeper.apply_vote(vote.clone(), 1);
@@ -142,7 +140,7 @@
let mut keeper = VoteKeeper::new(Height::new(1), Round::INITIAL, 4);

let v = ValueId::new(1);
let val = Some(v.clone());
let val = Some(v);
let vote = Vote::new_precommit(Round::new(0), val, Address::new(1));

let event = keeper.apply_vote(vote.clone(), 1);

Unchanged files with check annotations Beta

pub fn is_valid(&self) -> bool {
match self {
Round::None => true,
Round::Some(r) => *r >= 0,

Check warning on line 40 in Code/common/src/round.rs

Codecov / codecov/patch

Code/common/src/round.rs#L40

Added line #L40 was not covered by tests
}
}