apollo_consensus: SM tracks last self votes not the SHC

[asmaastarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[asmaastarkware]

• apollo_consensus: move caching decision logic to ConsensusCache #10620
• apollo_consensus: rename proposal cache and stream fields in manager #10619
• apollo_consensus: extract run_height_inner into focused helper funcs #10563
• apollo_consensus: use Round type alias instead of u32 #10541
• apollo_consensus: use BlockNumber for Vote.height #10507
• apollo_consensus: unify timeout scheduling requests into single enum variant #10482
• apollo_consensus: centralize future vote/proposal caching via ConsensusCache #10375
• apollo_consensus: refactor shc::handle_event into smaller helpers #10347
• apollo_consensus: make SHC synchronous; execute SMRequests in manager #10341
• apollo_consensus: add observer_does_not_record_self_votes test #10340
• apollo_consensus: SM now consumes events and emits requests; update SHC and tests #10339
• apollo_consensus: SM tracks last self votes not the SHC #10338 👈 (View in Graphite)
• apollo_consensus: move proposal storage to SM; dedupe in SHC #10337
• apollo_consensus: move vote storage to SM; dedupe in SHC #10336
• apollo_consensus: use Vote in Prevote/Precommit SMEvent #10335
• apollo_consensus: move height from SHC to SM #10356
• apollo_consensus: use SM.id in SHC; remove local id #10334
• main-v0.14.1

This stack of pull requests is managed by Graphite. Learn more about stacking.

[matanl-starkware]

@matanl-starkware reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @dafnamatsry)

---

crates/apollo_consensus/src/state_machine.rs line 199 at r1 (raw file):

                None => self.last_self_precommit = Some(vote.clone()),
                Some(last) if round > last.round => self.last_self_precommit = Some(vote.clone()),
                _ => {}

Code duplication.
Consider refactor.

Code quote (i):

                None => self.last_self_precommit = Some(vote.clone()),
                Some(last) if round > last.round => self.last_self_precommit = Some(vote.clone()),
                _ => {}

Code snippet (ii):

let slot = match vote_type {
    VoteType::Prevote => &mut self.last_self_prevote,
    VoteType::Precommit => &mut self.last_self_precommit,
};

if match slot {
    None => true,
    Some(last) => round > last.round,
} {
    *slot = Some(vote.clone());
}

[matanl-starkware]

@matanl-starkware reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)

---

crates/apollo_consensus/src/state_machine.rs line 182 at r2 (raw file):

        round: Round,
        proposal_commitment: Option<ProposalCommitment>,
    ) -> Vote {

Consider returning a reference to last_self_vote and remove the clone on line 200

Code quote:

 Vote

[dafnamatsry]

@dafnamatsry reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @asmaastarkware)

---

crates/apollo_consensus/src/state_machine.rs line 189 at r4 (raw file):

        &mut self,
        vote_type: VoteType,
        round: Round,

If the state machine should only vote for its current round (according to Tendermint), maybe remove this argument and use self.round.

Code quote:

round: Round,

---

crates/apollo_consensus/src/state_machine.rs line 192 at r4 (raw file):

        proposal_commitment: Option<ProposalCommitment>,
    ) -> Vote {
        let vt = vote_type.clone();

You can add #[derive(Copy)] above VoteType and remove the clone().
This makes the value copy instead of move, which is fine since the enum is small and simple.

Code quote:

let vt = vote_type.clone();

---

crates/apollo_consensus/src/state_machine.rs line 208 at r4 (raw file):

            None => true,
            Some(last) => round > last.round,
        } {

Use is_none_or instead:
if last_self_vote.is_none_or(|r| r < round) {..}

Code quote:

        if match last_self_vote {
            None => true,
            Some(last) => round > last.round,
        } {

---

crates/apollo_consensus/src/single_height_consensus.rs line 535 at r4 (raw file):

                     {vote:?}",
                )));
            }

Don't we want to move this assertion to the state machine?

Code quote:

            Some(_) => {
                // According to the Tendermint paper, the state machine should only vote for its
                // current round. It should monotonically increase its round. It should only vote
                // once per step.
                return Err(ConsensusError::InternalInconsistency(format!(
                    "State machine must progress in time: last_vote: {last_vote:?} new_vote: \
                     {vote:?}",
                )));
            }

[matanl-starkware]

@matanl-starkware reviewed all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @asmaastarkware)

[dafnamatsry]

@dafnamatsry reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @asmaastarkware)