feat(sequencing): cache proposals from bigger heights

[guy-starkware]

If a proposal is received by Consensus that has height bigger than the current height, cache it into a hash map that saves it for later.

When starting a new height, will first look to see if any cached proposals exist before listening to messages from the network.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-11-28T09:25:44Z
• Started at 2024-12-05T08:25:39Z
• Started at 2024-12-05T09:01:27Z

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 18.16%. Comparing base (e3165c4) to head (14c5f8e).
Report is 701 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/sequencing/papyrus_consensus/src/manager.rs	80.00%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2325       +/-   ##
===========================================
- Coverage   40.10%   18.16%   -21.94%     
===========================================
  Files          26      119       +93     
  Lines        1895    14129    +12234     
  Branches     1895    14129    +12234     
===========================================
+ Hits          760     2567     +1807     
- Misses       1100    11291    +10191     
- Partials       35      271      +236     

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

[matan-starkware]

Reviewed 3 of 17 files at r1, 1 of 11 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 2 of 19 files reviewed, 3 unresolved discussions (waiting on @guy-starkware)

---

crates/sequencing/papyrus_consensus/src/manager.rs line 155 at r4 (raw file):

                    shc_events.push(task.run());
                }
            }

If you don't think a decision can happen let's explicitly panic or just log. I don't want to risk missing it.

Suggestion:

            match shc_return {
                ShcReturn::Decision(decision) => return Ok(decision),
                ShcReturn::Tasks(tasks) => {
                    for task in tasks {
                        shc_events.push(task.run());
                    }
                }
            }

---

crates/sequencing/papyrus_consensus/src/manager.rs line 163 at r4 (raw file):

                message = next_message(&mut current_height_messages, broadcast_channels) => {
                    self.handle_message(context, height, &mut shc, message?).await?
                },

Please add a TODO to remove the continue_propagation/report_peer from this. They make this function non cancel safe, since it's possible for broadcasted_messages_receiver to return a message, but when we call to one of those functions a different branch completes causing us to drop it without returning.

This is very important so please make sure I handle this as soon as your code is submitted

Code quote:

                message = next_message(&mut current_height_messages, broadcast_channels) => {
                    self.handle_message(context, height, &mut shc, message?).await?
                },

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 84 at r4 (raw file):

}

#[ignore] // TODO(guyn): return this once caching proposals is implemented.

remove? (and below)

Code quote:

#[ignore] // TODO(guyn): return this once caching proposals is implemented.

[guy-starkware]

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

---

crates/sequencing/papyrus_consensus/src/manager.rs line 155 at r4 (raw file):

Previously, matan-starkware wrote…

If you don't think a decision can happen let's explicitly panic or just log. I don't want to risk missing it.

Done.

---

crates/sequencing/papyrus_consensus/src/manager.rs line 163 at r4 (raw file):

Previously, matan-starkware wrote…

Please add a TODO to remove the continue_propagation/report_peer from this. They make this function non cancel safe, since it's possible for broadcasted_messages_receiver to return a message, but when we call to one of those functions a different branch completes causing us to drop it without returning.

This is very important so please make sure I handle this as soon as your code is submitted

Done.

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 84 at r4 (raw file):

Previously, matan-starkware wrote…

remove? (and below)

Yes!

[matan-starkware]

Reviewed 2 of 2 files at r6, 3 of 19 files at r7, 1 of 1 files at r8, all commit messages.
Dismissed @guy-starkware from 3 discussions.
Reviewable status: 3 of 19 files reviewed, 2 unresolved discussions (waiting on @guy-starkware)

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 47 at r7 (raw file):

    #[async_trait]
    impl ConsensusContext for TestContext {

Now that we have removed ProposalWrapper I think we should be able to remove this whole mock scope and use the one from test_util. Can you please look into that in another PR?

Code quote:

    #[async_trait]
    impl ConsensusContext for TestContext {

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 151 at r7 (raw file):

                .unwrap();
            block_receiver
        })

I feel like this pattern repeats a lot. WDYT of adding this helper fn?

This can be done in another PR, and also check other tests in this crate (if so put the fn def in test_util.rs)

I actually see that we don't have any examples here with us being the builder.

Suggestion:

fn validate_result(
    built_id: Felt,
    received_id: Felt,
) -> oneshot::Receiver<(ProposalContentId, ProposalFin)> {
    let (block_sender, block_receiver) = oneshot::channel();
    block_sender
        .send((
            ProposalContentId(built_id),
            ProposalFin { proposal_content_id: BlockHash(received_id) },
        ))
        .unwrap();
    block_receiver
}
       
        .return_once(move |_, _, _, _| {
            validate_result(Felt::TWO, Felt::TWO)
        })

[guy-starkware]

Reviewable status: 2 of 19 files reviewed, 2 unresolved discussions (waiting on @matan-starkware)

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 47 at r7 (raw file):

Previously, matan-starkware wrote…

Now that we have removed ProposalWrapper I think we should be able to remove this whole mock scope and use the one from test_util. Can you please look into that in another PR?

We can remove it because we can put the real SequencerConsensusContext instead?

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 151 at r7 (raw file):

Previously, matan-starkware wrote…

I feel like this pattern repeats a lot. WDYT of adding this helper fn?

This can be done in another PR, and also check other tests in this crate (if so put the fn def in test_util.rs)

I actually see that we don't have any examples here with us being the builder.

Done.

[matan-starkware]

Reviewed 14 of 19 files at r7, 3 of 3 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware)

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 47 at r7 (raw file):

Previously, guy-starkware wrote…

We can remove it because we can put the real SequencerConsensusContext instead?

No, a given implementation of the Context trait cannot have any impact here since we are generic. I believe that we can redefine the test_util struct to have ProposalPart = TestProposalPart with:

enum TestProposalPart { ProposalInit, ProposalFin }.

This is another PR though so we can discuss when you get to it.

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 105 at r9 (raw file):

}

fn context_validates_hashes(context: &mut MockTestContext, block_hash: Felt) {

Suggestion:

fn expect_validate_proposal(context: &mut MockTestContext, block_hash: Felt) {

[guy-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matan-starkware)

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 47 at r7 (raw file):

Previously, matan-starkware wrote…

No, a given implementation of the Context trait cannot have any impact here since we are generic. I believe that we can redefine the test_util struct to have ProposalPart = TestProposalPart with:

enum TestProposalPart { ProposalInit, ProposalFin }.

This is another PR though so we can discuss when you get to it.

Ok, I'm putting it on my list.

---

crates/sequencing/papyrus_consensus/src/manager_test.rs line 105 at r9 (raw file):

}

fn context_validates_hashes(context: &mut MockTestContext, block_hash: Felt) {

Done.

[matan-starkware]

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

[guy-starkware]

Reviewed 14 of 19 files at r7, 12 of 13 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)