refactor(consensus): create manager which encapsulates messages cached between heights

[matan-starkware]

This change is 

[matan-starkware]

• refactor(consensus): minor refactor of the manager #208
• refactor(consensus): create manager which encapsulates messages cached between heights #200 👈
• refactor(consensus): consensus takes a generic network receiver #249
• main

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

Join @matan-starkware and the rest of your teammates on Graphite

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.91%. Comparing base (fe8efb4) to head (0321499).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #200      +/-   ##
==========================================
+ Coverage   76.88%   76.91%   +0.03%     
==========================================
  Files         312      313       +1     
  Lines       34445    34448       +3     
  Branches    34445    34448       +3     
==========================================
+ Hits        26482    26496      +14     
+ Misses       5681     5665      -16     
- Partials     2282     2287       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[asmaastarkware]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)

[ShahakShama]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)

---

crates/sequencing/papyrus_consensus/src/lib.rs line 57 at r3 (raw file):

    loop {
        let decision = manager
            .run_height(&mut context, current_height, validator_id, &mut network_receiver)

Add TODO to support failed rounds (unless the plan is that run_height will run several rounds until it reaches a decision)

---

crates/sequencing/papyrus_consensus/src/manager.rs line 26 at r3 (raw file):

use crate::ProposalWrapper;

/// Used run Tendermint. Handles issues which are not explicitly part of the single height consensus

WDYM "Used run Tendermint"? Try to improve this sentence

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 27 at r3 (raw file):

#[derive(Debug, PartialEq, Clone)]
pub struct TestBlock {
    pub content: Vec<Transaction>,

Why not use something simpler than Transaction, like u64?

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 117 at r3 (raw file):

#[tokio::test]
async fn run_multiple_heights() {

rename to run_multiple_heights_unordered

[matan-starkware]

Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @asmaastarkware and @ShahakShama)

---

crates/sequencing/papyrus_consensus/src/lib.rs line 57 at r3 (raw file):

Previously, ShahakShama wrote…

Add TODO to support failed rounds (unless the plan is that run_height will run several rounds until it reaches a decision)

A height contains multiple rounds. This is something the manager doesn't need to consider.

---

crates/sequencing/papyrus_consensus/src/manager.rs line 26 at r3 (raw file):

Previously, ShahakShama wrote…

WDYM "Used run Tendermint"? Try to improve this sentence

Done.

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 27 at r3 (raw file):

Previously, ShahakShama wrote…

Why not use something simpler than Transaction, like u64?

Because we don't support streaming yet. We have the Proposal ConsensusMessage which includes the field for Transaction. When we support streaming I will instead make us generic on:

ConsensusStreamMessage: From<ProposalInit> + From<ProposalFin>

I tried faking this genericness now and it lead to a lot of ugly code, so I'd prefer to have the complexity in my test until we have streaming.

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 117 at r3 (raw file):

Previously, ShahakShama wrote…

rename to run_multiple_heights_unordered

Done.

[ShahakShama]

Reviewed 1 of 2 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)

---

crates/sequencing/papyrus_consensus/src/manager.rs line 28 at r5 (raw file):

/// Runs Tendermint repeatedly across different heights. Handles issues which are not explicitly
/// part of the single height consensus algorithm (e.g. messages from future heights).
pub struct Manager {

Consider renaming to HeightManager or MultiHeightManager

---

crates/sequencing/papyrus_consensus/src/manager.rs line 29 at r5 (raw file):

/// part of the single height consensus algorithm (e.g. messages from future heights).
pub struct Manager {
    cached_messages: Vec<ConsensusMessage>,

Change this to HashMap<Height, Vec>. Then cleaning it up will be much more efficient

---

crates/sequencing/papyrus_consensus/src/manager.rs line 70 at r5 (raw file):

        let mut current_height_messages = Vec::new();
        for msg in std::mem::take(&mut self.cached_messages) {
            match height.0.cmp(&msg.height()) {

Change the ordering of cmp here so that it will be clearer what's going on in the match arms. You should have in the left the thing that's always changing and in the right the constant thing

[ShahakShama]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @matan-starkware)

[matan-starkware]

Reviewable status: 1 of 4 files reviewed, all discussions resolved (waiting on @asmaastarkware and @ShahakShama)

---

crates/sequencing/papyrus_consensus/src/manager.rs line 29 at r5 (raw file):

Previously, ShahakShama wrote…

Change this to HashMap<Height, Vec>. Then cleaning it up will be much more efficient

Sure, but I consider this to be part of the refactor (next PR). I'd like to keep this one just moving the existing code.

---

crates/sequencing/papyrus_consensus/src/manager.rs line 70 at r5 (raw file):

Previously, ShahakShama wrote…

Change the ordering of cmp here so that it will be clearer what's going on in the match arms. You should have in the left the thing that's always changing and in the right the constant thing

You put this comment in 208 earlier and I already did it, so to avoid conflicts I'll just leave it there.

[asmaastarkware]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)

[matan-starkware]

Merge activity

• Aug 4, 7:35 AM EDT: @matan-starkware started a stack merge that includes this pull request via Graphite.
• Aug 4, 7:35 AM EDT: @matan-starkware merged this pull request with Graphite.