apollo_protobuf: merge ProposalInit into ConsensusBlockInfo

[asmaastarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[asmaastarkware]

• apollo_protobuf: add concatenated_counts to fin #11752
• apollo_protobuf: add version_constant_commitment to ConsensusBlockInfo #11751
• apollo_protobuf: add parent_proposal_commitment field to ConsensusBlockInfo #11716
• apollo_protobuf: add starknet_version field to ConsensusBlockInfo #11711
• apollo_consensus: use PreviousBlockInfo instead of full ConsensusBlockInfo #11710
• apollo_protobuf: merge ProposalInit into ConsensusBlockInfo #11577 👈 (View in Graphite)
• apollo_protobuf: move executed_transaction_count into ProposalFin #11492
• main-v0.14.1-committer

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

[matanl-starkware]

@matanl-starkware reviewed 24 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry).

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 139 at r1 (raw file):

    is_block_info_valid(
        args.block_info_validation.clone(),
        args.block_info.clone(),

I don't like this clone

Code quote:

.clone()

---

crates/apollo_consensus_orchestrator/src/validate_proposal.rs line 149 at r1 (raw file):

        args.deps.batcher.clone(),
        args.deps.state_sync_client,
        args.block_info.clone(),

Same for this clone

Code quote:

.clone()

---

crates/apollo_protobuf/src/proto/p2p/proto/consensus/consensus.proto line 78 at r1 (raw file):

        BlockInfo block_info          = 1;
        ProposalFin fin               = 2;
        TransactionBatch transactions = 4;

Didn't change to 3 for a reason?

Code quote:

 4;

---

NODE_14_CONSENSUS_FAILURE_REPORT.md line 1 at r1 (raw file):

# Node 14 Consensus Failure - Root Cause Analysis

What is this file?

Code quote:

# Node 14 Consensus Failure - Root Cause Analysis

---

crates/apollo_consensus/src/single_height_consensus_test.rs line 37 at r1 (raw file):

const ROUND_1: Round = 1;

fn get_block_info_for_height(height: BlockNumber, round: Round) -> ConsensusBlockInfo {

Why don't we use block_info from the other module?

Code quote:

get_block_info_for_height

---

crates/apollo_consensus/src/single_height_consensus_test.rs line 131 at r1 (raw file):

    if repeat_proposal {
        // Duplicate block info should be ignored.
        let ret = shc.handle_proposal(&leader_fn, block_info.clone());

Why is cloning necessary now?

Code quote:

.clone()

[asmaastarkware]

@asmaastarkware made 3 comments.
Reviewable status: 18 of 24 files reviewed, 5 unresolved discussions (waiting on @dafnamatsry and @matanl-starkware).

---

crates/apollo_consensus/src/single_height_consensus_test.rs line 37 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Why don't we use block_info from the other module?

Done.

---

crates/apollo_consensus/src/single_height_consensus_test.rs line 131 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Why is cloning necessary now?

ConsensusBlockInfo does not derive Copy, are u ok with deriving it?

---

crates/apollo_protobuf/src/proto/p2p/proto/consensus/consensus.proto line 78 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Didn't change to 3 for a reason?

no, done

[matanl-starkware]

@matanl-starkware reviewed 12 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: 23 of 24 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry).

---

crates/apollo_consensus/src/single_height_consensus_test.rs line 131 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

ConsensusBlockInfo does not derive Copy, are u ok with deriving it?

No
I would like to avoid copying altogether.
Can't we pass a ref?

[asmaastarkware]

@asmaastarkware made 1 comment.
Reviewable status: 23 of 24 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @matanl-starkware).

---

crates/apollo_consensus/src/single_height_consensus_test.rs line 131 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

No
I would like to avoid copying altogether.
Can't we pass a ref?

handle_proposal takes ownership of ConsensusBlockInfo because it moves it into SMRequest::StartValidateProposal, which requires ownership. If we used a reference, we'd need to clone it first, which is unnecessary since the caller already owns it and doesn't need it afterward.

[matanl-starkware]

@matanl-starkware made 1 comment.
Reviewable status: 22 of 24 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @dafnamatsry).

---

crates/apollo_consensus/src/single_height_consensus_test.rs line 131 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

handle_proposal takes ownership of ConsensusBlockInfo because it moves it into SMRequest::StartValidateProposal, which requires ownership. If we used a reference, we'd need to clone it first, which is unnecessary since the caller already owns it and doesn't need it afterward.

IIUC, when you derive Copy, you allow the compiler to silently shallow-copy the entire struct whenever required.
So either we transfer ownership and then the Copy is not required, or we have to copy the data to allow double data existence.

[matanl-starkware]

@matanl-starkware reviewed 5 files and all commit messages, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry).

[dafnamatsry]

@dafnamatsry made 1 comment.
Reviewable status: 22 of 24 files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @matanl-starkware).

---

crates/apollo_protobuf/src/protobuf/protoc_output.rs line 493 at r7 (raw file):

    #[allow(clippy::derive_partial_eq_without_eq)]
    #[derive(Clone, PartialEq, ::prost::Oneof)]
    pub enum Message {

I think keeping Init and removing the BlockInfo variant would be better. Semantically, Init better describes the purpose of this message—marking the beginning of a proposal stream. We can simply embed the required BlockInfo data inside the ProposalInit struct.

So my suggestion is:

pub mod proposal_part {
    pub enum Message {
        Init(super::ProposalInit),
        Fin(super::ProposalFin),
        Transactions(super::TransactionBatch),
    }
}

pub struct ProposalInit {
    pub height: u64,
    pub round: u32,
    pub valid_round: ::core::option::Option<u32>,
    pub proposer: ::core::option::Option<Address>,

    pub block_info: BlockInfo,
}

@matanl-starkware WDYT?