feat(sequencing): broadcast proposal in a stream

[guy-starkware]

This PR adds the ability to broadcast a new proposal from Consensus, using the streamed channel.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-11-26T11:37:49Z
• Started at 2024-11-26T11:45:25Z
• Started at 2024-11-26T13:46:17Z
• Started at 2024-11-27T15:27:33Z
• Started at 2024-11-27T15:42:50Z
• Started at 2024-11-28T09:22:08Z
• Started at 2024-12-02T09:13:21Z

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.60%. Comparing base (e3165c4) to head (63a667d).
Report is 649 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2286      +/-   ##
==========================================
- Coverage   40.10%   39.60%   -0.50%     
==========================================
  Files          26      278     +252     
  Lines        1895    32174   +30279     
  Branches     1895    32174   +30279     
==========================================
+ Hits          760    12744   +11984     
- Misses       1100    18365   +17265     
- Partials       35     1065    +1030     

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

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.848 ms 35.363 ms 35.956 ms]
change: [+1.3620% +2.8474% +4.4233%] (p = 0.00 < 0.05)
Performance has regressed.
Found 17 outliers among 100 measurements (17.00%)
2 (2.00%) high mild
15 (15.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.404 ms 34.625 ms 34.934 ms]
change: [-5.1980% -3.4311% -1.7344%] (p = 0.00 < 0.05)
Performance has improved.
Found 14 outliers among 100 measurements (14.00%)
5 (5.00%) high mild
9 (9.00%) high severe

[matan-starkware]

Reviewed 3 of 17 files at r1, 7 of 7 files at r7, all commit messages.
Reviewable status: 10 of 20 files reviewed, 3 unresolved discussions (waiting on @guy-starkware)

---

crates/starknet_integration_tests/src/utils.rs line 84 at r7 (raw file):

        create_network_config_connected_to_broadcast_channels(
            papyrus_network::gossipsub_impl::Topic::new(
                // TODO(guyn): return this to NETWORK_TOPIC once we have integrated streaming.

Are you sure? I think what we want is to add the Vote channel in addition to this streaming one.

Code quote:

                // TODO(guyn): return this to NETWORK_TOPIC once we have integrated streaming.

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 335 at r7 (raw file):

                );
                debug!("Broadcasting proposal fin: {proposal_content_id:?}");
                println!("Broadcasting proposal fin: {proposal_content_id:?}");

Suggestion:

                debug!("Broadcasting proposal fin: {proposal_content_id:?}");

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 98 at r7 (raw file):

            }
            // Ignore this, in case it comes out of the network before some of the other messages.
            StreamMessageBody::Fin => (),

Why is this OK? don't we want to check that after Content(Fin) is sent that Stream::Fin is sent? So here this is unexpected, and then after the loop we want to assert this is received?

Code quote:

            // Ignore this, in case it comes out of the network before some of the other messages.
            StreamMessageBody::Fin => (),

[matan-starkware]

Reviewable status: 10 of 20 files reviewed, 4 unresolved discussions (waiting on @guy-starkware)

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 79 at r7 (raw file):

        broadcasted_messages_receiver.next().await.unwrap().0.unwrap().message,
        StreamMessageBody::Content(ProposalPart::Init(expected_proposal_init))
    );

I think we should get the stream_id from this and assert all messages have this stream_id

Code quote:

    assert_eq!(
        broadcasted_messages_receiver.next().await.unwrap().0.unwrap().message,
        StreamMessageBody::Content(ProposalPart::Init(expected_proposal_init))
    );

[guy-starkware]

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

---

crates/starknet_integration_tests/src/utils.rs line 84 at r7 (raw file):

Previously, matan-starkware wrote…

Are you sure? I think what we want is to add the Vote channel in addition to this streaming one.

Yes, this is not related to the voting channel. This is only for the channels that go into Context.
Since I've left the "old plumbing" for passing proposals, there are two NETWORK_TOPICs right now, but we will get rid of the old one soon.

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 79 at r7 (raw file):

Previously, matan-starkware wrote…

I think we should get the stream_id from this and assert all messages have this stream_id

Good idea. I'm also checking that the first message has message_id zero.

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 98 at r7 (raw file):

Previously, matan-starkware wrote…

Why is this OK? don't we want to check that after Content(Fin) is sent that Stream::Fin is sent? So here this is unexpected, and then after the loop we want to assert this is received?

Yeah, this is a little confusing.
I can reorganize this to make sure we get both different fins.

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 335 at r7 (raw file):

                );
                debug!("Broadcasting proposal fin: {proposal_content_id:?}");
                println!("Broadcasting proposal fin: {proposal_content_id:?}");

Thanks! Removed.

[matan-starkware]

Reviewed 10 of 17 files at r1, 1 of 7 files at r8, 7 of 7 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware)

---

crates/starknet_integration_tests/src/utils.rs line 84 at r7 (raw file):

Previously, guy-starkware wrote…

Yes, this is not related to the voting channel. This is only for the channels that go into Context.
Since I've left the "old plumbing" for passing proposals, there are two NETWORK_TOPICs right now, but we will get rid of the old one soon.

Add a TODO that we need to add the vote channels also

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 116 at r9 (raw file):

            break;
        }
    }

My biggest issue is that I don't like the variables in the loop. I think that they order expected can just be represented in the tests structure.

• Init
• Loop
• Fin

Suggestion:

    // Must be const since `assert_matches` ignores variables.
    const INCOMING_STREAM_ID: u64 = 0;
    assert_eq!(
        broadcasted_messages_receiver.next().await.unwrap().0.unwrap(),
        StreamMessage {
            stream_id: INCOMING_STREAM_ID,
            message_id: 0, // Init must be the first message.
            message: StreamMessageBody::Content(ProposalPart::Init(expected_proposal_init))
        }
    );

    loop {
        let message = broadcasted_messages_receiver.next().await.unwrap().0.unwrap();
        assert_eq!(message.stream_id, INCOMING_STREAM_ID);
        match message.message {
            StreamMessageBody::Content(ProposalPart::Fin(proposal_fin)) => {
                assert_eq!(proposal_fin, expected_proposal_fin);
                break;
            }
            StreamMessageBody::Content(ProposalPart::Transactions(transactions)) => {
                received_tx_hashes.extend(
                    transactions
                        .transactions
                        .iter()
                        .map(|tx| tx.calculate_transaction_hash(&chain_id).unwrap()),
                );
            }
            _ => panic!("Unexpected message: {:?}", message),
        }
    }
    assert_matches!(
        broadcasted_messages_receiver.next().await.unwrap().0.unwrap(),
        StreamMessage {
            stream_id: INCOMING_STREAM_ID,
            message_id: _,
            message: StreamMessageBody::Fin,
        }
    );

[guy-starkware]

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

---

crates/starknet_integration_tests/src/utils.rs line 84 at r7 (raw file):

Previously, matan-starkware wrote…

Add a TODO that we need to add the vote channels also

If I understand this test correctly, it isn't supposed to include the votes. Those go into Consensus. These channels go straight into the Context. I'm not sure where the votes go into the Consensus but it wasn't through this part.

---

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 116 at r9 (raw file):

Previously, matan-starkware wrote…

My biggest issue is that I don't like the variables in the loop. I think that they order expected can just be represented in the tests structure.

• Init
• Loop
• Fin

This is much nicer, but... I'm not sure the stream ID would be 0 (it can be a lot of things, in general, and it wouldn't even remain a single number for much longer).
Also, I've confirmed that the StreamMessage::Fin comes before the ProposalPart::Fin (I'm not sure if this is random order or just the way it comes from the test all the time). So this is a weird test that waits for things to arrive in a weird order.

Let me try to do something more clear.

[matan-starkware]

Reviewed 3 of 16 files at r13, 5 of 7 files at r14, all commit messages.
Reviewable status: 8 of 20 files reviewed, 1 unresolved discussion (waiting on @guy-starkware)

---

crates/starknet_integration_tests/src/utils.rs line 84 at r7 (raw file):

Previously, guy-starkware wrote…

If I understand this test correctly, it isn't supposed to include the votes. Those go into Consensus. These channels go straight into the Context. I'm not sure where the votes go into the Consensus but it wasn't through this part.

Yes, the test doesn't yet have votes because we don't yet support them... Consensus will need a receiver to receive votes, and the context will need a sender to broadcast votes when we want to support this.

[matan-starkware]

Reviewed 10 of 16 files at r13, 7 of 7 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.966 ms 35.442 ms 36.013 ms]
change: [+1.0456% +2.3088% +4.3154%] (p = 0.00 < 0.05)
Performance has regressed.
Found 13 outliers among 100 measurements (13.00%)
3 (3.00%) high mild
10 (10.00%) high severe

[guy-starkware]

Reviewed 12 of 16 files at r13, 8 of 8 files at r21, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)