feat(consensus): send proposal content

[dan-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[dan-starkware]

• test: add consensus to e2e flow test #1811
• feat(consensus): send proposal content #1810 👈
• feat(consensus): run network manager in consensus #1809
• chore(consensus): add network config #1808
• chore: rename proposal init and add log #1807
• chore: add target in fmt layer #1806
• fix(network): fix secret key length in test util #1774
• main

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

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

[codecov]

Codecov Report

Attention: Patch coverage is 66.03774% with 18 lines in your changes missing coverage. Please review.

Project coverage is 26.59%. Comparing base (e3165c4) to head (70ca816).
Report is 239 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/consensus_manager/src/consensus_manager.rs	0.00%	8 Missing ⚠️
...us_orchestrator/src/sequencer_consensus_context.rs	85.29%	2 Missing and 3 partials ⚠️
crates/starknet_api/src/transaction.rs	54.54%	5 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1810       +/-   ##
===========================================
- Coverage   40.10%   26.59%   -13.51%     
===========================================
  Files          26      116       +90     
  Lines        1895    13610    +11715     
  Branches     1895    13610    +11715     
===========================================
+ Hits          760     3620     +2860     
- Misses       1100     9641     +8541     
- Partials       35      349      +314     

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

[matan-starkware]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @asmaastarkware, @dan-starkware, and @guy-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 308 at r1 (raw file):

                    transaction_hashes.push(tx.tx_hash());
                    transactions.push(tx.tx());
                });

I prefer just using a for loop here. This is because iterators are usually lazy/don't have side effects, not doing anything unless there is a return value. This causes me to feel that for_each is surprising.

Documentation seems to agree "...It’s generally more idiomatic to use a for loop..."
https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.for_each

Code quote:

                txs.into_iter().for_each(|tx| {
                    transaction_hashes.push(tx.tx_hash());
                    transactions.push(tx.tx());
                });

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 85 at r1 (raw file):

        mock_register_broadcast_topic().expect("Failed to create mock network");
    let BroadcastTopicChannels { broadcasted_messages_receiver: _, broadcast_topic_client } =
        subscriber_channels;

Suggestion:

    let BroadcastTopicChannels { broadcast_topic_client, .. } = subscriber_channels;

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 88 at r1 (raw file):

    let BroadcastNetworkMock {
        broadcasted_messages_sender: _mock_broadcasted_messages_sender, ..
    } = mock_network;

Why destructure if we never use this?

I would have thought you'd do this to get the receiver out so that we can inspect the actual proposal content send out by the context.

Code quote:

    let BroadcastNetworkMock {
        broadcasted_messages_sender: _mock_broadcasted_messages_sender, ..
    } = mock_network;

---

crates/starknet_api/src/executable_transaction.rs line 62 at r1 (raw file):

            Transaction::Invoke(tx_data) => crate::transaction::Transaction::Invoke(tx_data.tx),
        }
    }

Should this be the Into trait? Or better yet "implement From instead" in the non-executable TX's location?

Code quote:

    pub fn tx(self) -> crate::transaction::Transaction {
        match self {
            Transaction::Declare(tx_data) => crate::transaction::Transaction::Declare(tx_data.tx),
            Transaction::DeployAccount(tx_data) => {
                crate::transaction::Transaction::DeployAccount(tx_data.tx)
            }
            Transaction::Invoke(tx_data) => crate::transaction::Transaction::Invoke(tx_data.tx),
        }
    }

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [33.909 ms 34.415 ms 35.008 ms]
change: [+1.0502% +2.5242% +4.1761%] (p = 0.00 < 0.05)
Performance has regressed.
Found 11 outliers among 100 measurements (11.00%)
2 (2.00%) high mild
9 (9.00%) high severe

[guy-starkware]

This will be very useful for what I'm working on!

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @asmaastarkware, @dan-starkware, and @matan-starkware)

[matan-starkware]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @asmaastarkware)

[github-actions]

Benchmark movements:
full_committer_flow performance improved 😺
full_committer_flow time: [29.313 ms 29.358 ms 29.414 ms]
change: [-2.4817% -1.9841% -1.5781%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe

[matan-starkware]

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @asmaastarkware)

[matan-starkware]

Reviewed 1 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @asmaastarkware)