chore(starknet_consensus_manager): add proposal init into validate proposal input

[ArniStarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[ArniStarkware]

• chore(consensus): move const to consensus #2812
• chore(starknet_consensus_manager): add proposal init into validate proposal input #2408 👈 (View in Graphite)
• chore(consensus): use non zero validator id in tests #2550
• chore(consensus): use non zero validator id in consensus config #2549
• main

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

[codecov]

Codecov Report

Attention: Patch coverage is 74.07407% with 7 lines in your changes missing coverage. Please review.

Project coverage is 32.26%. Comparing base (e3165c4) to head (79d57f2).
Report is 809 commits behind head on main.

Files with missing lines	Patch %	Lines
...nsus_orchestrator/src/papyrus_consensus_context.rs	53.33%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2408      +/-   ##
==========================================
- Coverage   40.10%   32.26%   -7.85%     
==========================================
  Files          26      119      +93     
  Lines        1895    14085   +12190     
  Branches     1895    14085   +12190     
==========================================
+ Hits          760     4544    +3784     
- Misses       1100     8989    +7889     
- Partials       35      552     +517     

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

[alonh5]

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

---

crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context.rs line 184 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

This is consistent with how this variable is handled in a similar function in this file:build_proposal

I didn't understand your response. I'm saying proposal_init.height is used like 7 times in this function so it's probably defining a shorter variable name for it. Same goes for build_proposal in that case.

[ArniStarkware]

Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @matan-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context.rs line 184 at r3 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I didn't understand your response. I'm saying proposal_init.height is used like 7 times in this function so it's probably defining a shorter variable name for it. Same goes for build_proposal in that case.

Done.

[alonh5]

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matan-starkware)

[alonh5]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matan-starkware)

[alonh5]

Dismissed @matan-starkware from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)

[alonh5]

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

---

a discussion (no related file):
Make sure with @matan-starkware if he wants this or not, and then merge or close accordingly.

[matan-starkware]

Reviewed 2 of 9 files at r3, 1 of 2 files at r4, 4 of 5 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ArniStarkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context_test.rs line 75 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Also, it is based on the change to the validator ID.

Can you explicitly define Default for ProposalInit and that can have DEFAULT_VALIDATOR_ID. I think that will make usage easier.

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 205 at r6 (raw file):

                proposer: ValidatorId::from(DEFAULT_VALIDATOR_ID),
                ..Default::default()
            },

If you don't care about the field's specifics.

Suggestion:

            ProposalInit::default(),

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 348 at r6 (raw file):

            content_receiver,
        )
        .await;

Let's refactor to highlight the round change.

Suggestion:

    let mut init = ProposalInit {
        round: 0,
        ..Default::default()
    };
    let fin_receiver_past_round =
        context.validate_proposal(init.clone(), TIMEOUT, content_receiver).await;

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 367 at r6 (raw file):

            content_receiver,
        )
        .await;

and for round 2.

Suggestion:

    init.round = 1;
    let fin_receiver_curr_round =
        context.validate_proposal(init.clone(), TIMEOUT, content_receiver).await;

[ArniStarkware]

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

---

crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context_test.rs line 75 at r1 (raw file):

Previously, matan-starkware wrote…

Can you explicitly define Default for ProposalInit and that can have DEFAULT_VALIDATOR_ID. I think that will make usage easier.

Done.

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 205 at r6 (raw file):

Previously, matan-starkware wrote…

If you don't care about the field's specifics.

Done. Throughout the code.

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 348 at r6 (raw file):

Previously, matan-starkware wrote…

Let's refactor to highlight the round change.

Done.

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 367 at r6 (raw file):

Previously, matan-starkware wrote…

and for round 2.

Done.

[ArniStarkware]

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

---

crates/papyrus_protobuf/src/consensus.rs line 81 at r7 (raw file):

            round: Default::default(),
            valid_round: Default::default(),
            // TODO(Arni): Use DEFAULT_VALIDATOR_ID instead of 100.

Done. #2812.

Code quote:

// TODO(Arni): Use DEFAULT_VALIDATOR_ID instead of 100.

[matan-starkware]

Reviewed 9 of 9 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @ArniStarkware)

---

crates/papyrus_protobuf/src/consensus.rs line 83 at r7 (raw file):

            // TODO(Arni): Use DEFAULT_VALIDATOR_ID instead of 100.
            proposer: ContractAddress::from(100_u64),
        }

Suggestion:

    fn default() -> Self {
        ProposalInit {
            // TODO(Arni): Use DEFAULT_VALIDATOR_ID instead of 100.
            proposer: ContractAddress::from(100_u64),
            ..Default::default(),
        }

[matan-starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5 and @ArniStarkware)

[ArniStarkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alonh5)

---

crates/papyrus_protobuf/src/consensus.rs line 83 at r7 (raw file):

            // TODO(Arni): Use DEFAULT_VALIDATOR_ID instead of 100.
            proposer: ContractAddress::from(100_u64),
        }

warning: function cannot return without recursing
This is because we are now defining default, so we can not use default to define it.

I suspect your suggestion does not even work.