apollo_consensus: SM now consumes events and emits requests; update SHC and tests

[asmaastarkware]

No description provided.

[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 👈 (View in Graphite)
• apollo_consensus: SM tracks last self votes not the SHC #10338
• 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.

[reviewable-StarkWare]

This change is 

[asmaastarkware]

Reviewable status: 2 of 4 files reviewed, 20 unresolved discussions (waiting on @dafnamatsry and @matanl-starkware)

---

crates/apollo_consensus/src/single_height_consensus.rs line 241 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Please add TODO to reduce the amount of duplicated code between the Prevote and Precommit arms

in this PR I tried not to break the old code (easy to review)
Donr in the upcoming PRs

---

crates/apollo_consensus/src/single_height_consensus.rs line 272 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Please keep the debug log. It's essential to understand what's happening with Tendermint...

Done.

---

crates/apollo_consensus/src/single_height_consensus.rs line 410 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

let timeout = match vote.vote_type {
}
let task = ShcTask::Rebroadcast {timeout, vote.clone()};

in this PR I tried not to break the old code (easy to review)
but its done in #10341

---

crates/apollo_consensus/src/single_height_consensus.rs line 445 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

This TODO was lost...

Done.

---

crates/apollo_consensus/src/single_height_consensus.rs line 320 at r2 (raw file):

Previously, dafnamatsry wrote…

IIUC Prevote and Precommit only represent peer votes and are not like the other events that indicates a completed task.
Consider completely removing these event types, and simply call state machine handle_prevote and handle_precommit directly from handle_vote.

added TODO

---

crates/apollo_consensus/src/single_height_consensus.rs line 474 at r2 (raw file):

Previously, dafnamatsry wrote…

What is this for?

will be used next

---

crates/apollo_consensus/src/single_height_consensus.rs line 477 at r2 (raw file):

Previously, dafnamatsry wrote…

And this?

will be used next

---

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

Previously, matanl-starkware (Matan Lior) wrote…

What does this parameter mean?

valid_round

---

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

Previously, matanl-starkware (Matan Lior) wrote…

Now Events are only to the state machine IIUC

Done.

---

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

Previously, matanl-starkware (Matan Lior) wrote…

We usually don't use struct-style definition for enums.
Why did you decide to do it here?
Anyhow, I prefer not mixing (some struct-style, some tuple-style)

Done.

---

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

Previously, matanl-starkware (Matan Lior) wrote…

How come it's dead code?

It will be used when the SHC asks the manager to start validation—currently, the SHC does this itself

---

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

Previously, matanl-starkware (Matan Lior) wrote…

if self.is_observer {
return output;
}

rest of flow here...

Done.

---

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

Previously, matanl-starkware (Matan Lior) wrote…

Eventually, you'll return a single request from this function.
I think it's OK to add a comment explaining why you chose to do so (instead of simply return Option)

Done.

---

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

Previously, matanl-starkware (Matan Lior) wrote…

What about the if self.is_observer check that was under previous arms?

Done.

---

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

Previously, matanl-starkware (Matan Lior) wrote…

I thought that for case of Repropose you already saved the Init somewhere

We don't save the init, we save the round and the proposal_id
(it's a new round)

---

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

Previously, matanl-starkware (Matan Lior) wrote…

Still sends or only receives now?

Done.

---

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

Previously, dafnamatsry wrote…

These are only for incoming votes no? If so, fix the documentation.

Done.

---

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

Previously, dafnamatsry wrote…

These are now mean the timeout is completed right?
Maybe call them TimeoutExpired or TimeoutCompleted? and also fix the documentaion.

yes.. if it's not clear that the timeout expired, I will rename it in another PR

---

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

Previously, matanl-starkware (Matan Lior) wrote…

log vote_type as well

Done.

---

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

Previously, dafnamatsry wrote…

I think the ProposalInit can be constructed by whom will be performing the repropose (the manager?).
The SMRquest can include the necessary information (proposal_id, round and valid_round)

Currently, the SHC is the one who reproposes..
It needs to get the data (H, R, Id) from the SM, that's why I prefer to prepare it here

[asmaastarkware]

Reviewable status: 1 of 4 files reviewed, 20 unresolved discussions (waiting on @dafnamatsry and @matanl-starkware)

---

crates/apollo_consensus/src/single_height_consensus.rs line 474 at r2 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

will be used next

deleted as discussed in huddle

---

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

Previously, dafnamatsry wrote…

What does this event mean? It sounds more like a task.

In this PR, I left the name as "task" because the SHC is still handling all the logic.
In #10341, once the manager became responsible for broadcasting and the doing the hard work, it was renamed to VoteBroadcasted

[matanl-starkware]

@matanl-starkware reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry)

---

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

Previously, asmaastarkware (asmaa-starkware) wrote…

Done.

Do you need to check under the ScheduleTimeout* that we are not observer?
Previously:
if self.is_observer {
continue;
}

---

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

    BroadcastVote(Vote),
    /// Request to schedule a TimeoutPropose.
    ScheduleTimeoutPropose(Round),

Consider merging all Timeout requests into a single enum:
ScheduleTimeout(Step, Round)

Code quote:

ScheduleTimeoutPropose

[asmaastarkware]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dafnamatsry and @matanl-starkware)

---

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

Previously, matanl-starkware (Matan Lior) wrote…

Do you need to check under the ScheduleTimeout* that we are not observer?
Previously:
if self.is_observer {
continue;
}

was it?
observer should act normally, without voting/proposing

[matanl-starkware]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry)

---

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

Previously, asmaastarkware (asmaa-starkware) wrote…

was it?
observer should act normally, without voting/proposing

IDK
In original code, we had these arms:
StateMachineEvent::Proposal(_, , )
| StateMachineEvent::Prevote()
| StateMachineEvent::Precommit() => {
if self.is_observer {
continue;
}

Is it all handled the same way now?

[asmaastarkware]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @dafnamatsry and @matanl-starkware)

---

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

Previously, matanl-starkware (Matan Lior) wrote…

IDK
In original code, we had these arms:
StateMachineEvent::Proposal(_, , )
| StateMachineEvent::Prevote()
| StateMachineEvent::Precommit() => {
if self.is_observer {
continue;
}

Is it all handled the same way now?

Yes, make_self_vote and advance_to_round prevent voting/proposing when the node is observer

[matanl-starkware]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dafnamatsry)

---

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

    }

    fn advance_to_step(&mut self, step: Step) -> VecDeque<SMRequest> {

Please define as a type (in future PR)

Code quote:

VecDeque<SMRequest>

[dafnamatsry]

@dafnamatsry reviewed 1 of 3 files at r2, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @asmaastarkware)

---

crates/apollo_consensus/src/state_machine_test.rs line 449 at r4 (raw file):

    );
    assert_eq!(wrapper.next_request().unwrap(), SMRequest::ScheduleTimeoutPrevote(ROUND));
    // Nil Prevote quorum, so we broadcast a nil Precommit.

Why didn't we have the vote broadcasting before?

---

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

    /// Precommit message, sent from the SHC to the state machine.
    Precommit(Vote),
    /// TimeoutPropose event, sent from the state machine to the SHC.

And also below

Suggestion:

 from the SHC to the state machine