feat(consensus): add advance_to_round

[asmaastarkware]

Pull Request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build-related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this introduce a breaking change?

• Yes
• No

Other information

---

This change is 

[matan-starkware]

Reviewed 3 of 3 files at r1.
Reviewable status: 0 of 8 files reviewed, 4 unresolved discussions (waiting on @asmaastarkware)

---

crates/sequencing/papyrus_consensus/src/single_height_consensus.rs line 125 at r1 (raw file):

        self.proposals.insert(ROUND_ZERO, block);
        let leader_fn = |_round: Round| -> ValidatorId {
            context.proposer(&self.validators.clone(), self.height)

Are you sure we need to clone this? I thought we'd be able to pass this by reference. If we don't need to, change in all locations.

Code quote:

self.validators.clone()

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 106 at r1 (raw file):

        event: StateMachineEvent,
        leader_fn: &impl Fn(Round) -> ValidatorId,
    ) -> VecDeque<StateMachineEvent> {

Shahak suggested that if we use the same impl all over the place it's better to use generics (here and below)

Suggestion:

    pub fn handle_event<LeaderFn>(
        &mut self,
        event: StateMachineEvent,
        leader_fn: &LeaderFn,
    ) -> VecDeque<StateMachineEvent> 
    where:
        LeaderFn: Fn(Round) -> ValidatorId,
    {

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 266 at r1 (raw file):

        // Check for an existing quorum in case messages arrived out of order.
        match self.step {
            Step::Propose => unreachable!(),

Suggestion:

unreachable!("Advancing to Propose is done by advancing rounds")

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 292 at r1 (raw file):

        }
        self.send_precommit(*block_hash, round, leader_fn)
    }

We need to change our logic. First of all we only need to check the value if there is a quorum. Secondly, if the quorum is None then we should always send Precommit None. The question is if there is a quorum for Some.

1. If we have no proposal: do nothing
2. If we have an invalid proposal: panic
3. If we have a valid proposal which does not match the quorum: panic
4. If Proposal.hash == Quorum.hash: send Precommit in favor

We want similar logic for the precommits with 2 changes:

1. If the quorum is None advance round
2. If Proposal.hash == Quorum.hash: Decision

Suggestion:

        let Some((block_hash, count)) = leading_vote(&self.prevotes, round) else {
            return VecDeque::new();
        };
        if *count < self.quorum {
            return VecDeque::new();
        }
        let Some(proposed_value) = self.proposals.get(&round) else {
            return VecDeque::new();
        };
        if proposed_value != block_hash {
            error!("Proposal does not match quorum.");
            return VecDeque::new();
        }


        self.send_precommit(*block_hash, round, leader_fn)
    }

[asmaastarkware]

Reviewable status: 0 of 8 files reviewed, 4 unresolved discussions (waiting on @matan-starkware)

---

crates/sequencing/papyrus_consensus/src/single_height_consensus.rs line 125 at r1 (raw file):

Previously, matan-starkware wrote…

Are you sure we need to clone this? I thought we'd be able to pass this by reference. If we don't need to, change in all locations.

Done.

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 106 at r1 (raw file):

Previously, matan-starkware wrote…

Shahak suggested that if we use the same impl all over the place it's better to use generics (here and below)

Done.

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 266 at r1 (raw file):

        // Check for an existing quorum in case messages arrived out of order.
        match self.step {
            Step::Propose => unreachable!(),

Done.

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 292 at r1 (raw file):

Previously, matan-starkware wrote…

We need to change our logic. First of all we only need to check the value if there is a quorum. Secondly, if the quorum is None then we should always send Precommit None. The question is if there is a quorum for Some.

1. If we have no proposal: do nothing
2. If we have an invalid proposal: panic
3. If we have a valid proposal which does not match the quorum: panic
4. If Proposal.hash == Quorum.hash: send Precommit in favor

We want similar logic for the precommits with 2 changes:

1. If the quorum is None advance round
2. If Proposal.hash == Quorum.hash: Decision

Done.

[matan-starkware]

Reviewed 2 of 6 files at r7, 1 of 4 files at r8, all commit messages.
Reviewable status: 3 of 8 files reviewed, 9 unresolved discussions (waiting on @asmaastarkware)

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 346 at r7 (raw file):

            VecDeque::from([StateMachineEvent::Decision(*block_hash, round)])
        } else {
            VecDeque::new()

Suggestion:

            // NIL quorum reached on a different round.
            VecDeque::new()

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 303 at r8 (raw file):

        let Some((block_hash, count)) = leading_vote(&self.prevotes, round) else {
            return VecDeque::new();
        };

Suggestion:

    fn check_prevote_quorum<LeaderFn>(
        &mut self,
        round: u32,
        leader_fn: &LeaderFn,
    ) -> VecDeque<StateMachineEvent>
    where
        LeaderFn: Fn(Round) -> ValidatorId,
    {
        assert_eq(round, self.round, "check_prevote_quorum is only called for the current round");
        let Some((block_hash, count)) = leading_vote(&self.prevotes, round) else {
            return VecDeque::new();
        };

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 315 at r8 (raw file):

        if proposed_value != block_hash {
            panic!("Proposal does not match quorum.");
        }

Also add this in the precommit check

Suggestion:

        if proposed_value != block_hash {
            // TODO(matan): This can be caused by a malicious leader double proposing.
            panic!("Proposal does not match quorum.");
        }

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 155 at r8 (raw file):

    assert_eq!(events.pop_front().unwrap(), StateMachineEvent::Precommit(None, ROUND));
    assert!(events.is_empty(), "{:?}", events);
}

Please combine into a single test and use test_case to have both variants as they are almost identical

Code quote:

#[test]
fn buffer_events_during_get_proposal() {
    let mut state_machine = StateMachine::new(*PROPOSER_ID, 4);
    let leader_fn = |_: Round| *PROPOSER_ID;
    let mut events = state_machine.start(&leader_fn);
    assert_eq!(events.pop_front().unwrap(), StateMachineEvent::GetProposal(None, 0));
    assert!(events.is_empty(), "{:?}", events);

    events.append(
        &mut state_machine.handle_event(StateMachineEvent::Prevote(BLOCK_HASH, ROUND), &leader_fn),
    );
    events.append(
        &mut state_machine.handle_event(StateMachineEvent::Prevote(BLOCK_HASH, ROUND), &leader_fn),
    );
    events.append(
        &mut state_machine.handle_event(StateMachineEvent::Prevote(BLOCK_HASH, ROUND), &leader_fn),
    );
    assert!(events.is_empty(), "{:?}", events);

    // Node finishes building the proposal.
    events =
        state_machine.handle_event(StateMachineEvent::GetProposal(BLOCK_HASH, ROUND), &leader_fn);
    assert_eq!(events.pop_front().unwrap(), StateMachineEvent::Proposal(BLOCK_HASH, ROUND));
    assert_eq!(events.pop_front().unwrap(), StateMachineEvent::Prevote(BLOCK_HASH, ROUND));
    assert_eq!(events.pop_front().unwrap(), StateMachineEvent::Precommit(BLOCK_HASH, ROUND));
    assert!(events.is_empty(), "{:?}", events);
}

#[test]
fn buffer_nil_votes_during_get_proposal() {
    let mut state_machine = StateMachine::new(*PROPOSER_ID, 4);
    let leader_fn = |_: Round| *PROPOSER_ID;
    let mut events = state_machine.start(&leader_fn);
    assert_eq!(events.pop_front().unwrap(), StateMachineEvent::GetProposal(None, 0));
    assert!(events.is_empty(), "{:?}", events);

    events.append(
        &mut state_machine.handle_event(StateMachineEvent::Prevote(None, ROUND), &leader_fn),
    );
    events.append(
        &mut state_machine.handle_event(StateMachineEvent::Prevote(None, ROUND), &leader_fn),
    );
    events.append(
        &mut state_machine.handle_event(StateMachineEvent::Prevote(None, ROUND), &leader_fn),
    );
    assert!(events.is_empty(), "{:?}", events);

    // Node finishes building the proposal.
    events =
        state_machine.handle_event(StateMachineEvent::GetProposal(BLOCK_HASH, ROUND), &leader_fn);
    assert_eq!(events.pop_front().unwrap(), StateMachineEvent::Proposal(BLOCK_HASH, ROUND));
    assert_eq!(events.pop_front().unwrap(), StateMachineEvent::Prevote(BLOCK_HASH, ROUND));
    assert_eq!(events.pop_front().unwrap(), StateMachineEvent::Precommit(None, ROUND));
    assert!(events.is_empty(), "{:?}", events);
}

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 158 at r8 (raw file):

#[test]
fn no_precommit_without_proposal_even_with_prevote_quorum() {

Suggestion:

fn only_send_precommit_with_prevote_quorum_and_proposal() {

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 181 at r8 (raw file):

            .handle_event(StateMachineEvent::Precommit(BLOCK_HASH, ROUND), &leader_fn),
    );
    assert!(events.is_empty(), "{:?}", events);

I don't see the value in sending this precommit .

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 191 at r8 (raw file):

#[test]
fn no_decision_without_proposal_even_with_precommit_quorum() {

Suggestion:

only_decide_with_prcommit_quorum_and_proposal(

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 233 at r8 (raw file):

    );
    assert!(events.is_empty(), "{:?}", events);
}

Where are the tests for multiple rounds? The main point of this PR is advance_to_round

Code quote:

    assert!(events.is_empty(), "{:?}", events);
}

[matan-starkware]

Reviewed 1 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: 3 of 8 files reviewed, 5 unresolved discussions (waiting on @asmaastarkware)

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 102 at r10 (raw file):

#[test_case(BLOCK_HASH ; "with valid block hash votes")]
#[test_case(None ; "with nil votes")]

Suggestion:

#[test_case(BLOCK_HASH ; "valid_proposal")]
#[test_case(None ; "invalid_proposal")]

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 236 at r10 (raw file):

        events.pop_front().unwrap(),
        StateMachineEvent::Decision(BLOCK_HASH.unwrap(), ROUND + 1)
    );

I don't mind the first check since it's a negative one "we have not progressed rounds". For the second check I would like to see this in the public interface though.

Suggestion:

    assert_eq!(state_machine.round, 0);
    // Send valid Proposal for round 1 and see that no Prevote is sent.
    events.append(
        &mut state_machine.handle_event(StateMachineEvent::Precommit(None, ROUND), &leader_fn),
    );
    // Check that a Prevote for round 1 is now sent since we were able to progress after the NIL quorum.

[asmaastarkware]

Reviewable status: 3 of 8 files reviewed, 5 unresolved discussions (waiting on @matan-starkware)

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 346 at r7 (raw file):

            VecDeque::from([StateMachineEvent::Decision(*block_hash, round)])
        } else {
            VecDeque::new()

Done.

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 303 at r8 (raw file):

        let Some((block_hash, count)) = leading_vote(&self.prevotes, round) else {
            return VecDeque::new();
        };

Done.

---

crates/sequencing/papyrus_consensus/src/state_machine.rs line 315 at r8 (raw file):

Previously, matan-starkware wrote…

Also add this in the precommit check

Done.

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 155 at r8 (raw file):

Previously, matan-starkware wrote…

Please combine into a single test and use test_case to have both variants as they are almost identical

Done.

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 158 at r8 (raw file):

#[test]
fn no_precommit_without_proposal_even_with_prevote_quorum() {

Done.

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 191 at r8 (raw file):

#[test]
fn no_decision_without_proposal_even_with_precommit_quorum() {

Done.

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 102 at r10 (raw file):

#[test_case(BLOCK_HASH ; "with valid block hash votes")]
#[test_case(None ; "with nil votes")]

Done.

---

crates/sequencing/papyrus_consensus/src/state_machine_test.rs line 236 at r10 (raw file):

Previously, matan-starkware wrote…

I don't mind the first check since it's a negative one "we have not progressed rounds". For the second check I would like to see this in the public interface though.

Done.

[matan-starkware]

Reviewed 1 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: 3 of 8 files reviewed, all discussions resolved

[matan-starkware]

Reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: 3 of 8 files reviewed, all discussions resolved