feat(consensus): validate BlockInfo

[asmaastarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[asmaastarkware]

• feat(consensus): validate BlockInfo #3979 👈 (View in Graphite)
• feat(consensus): send BlockInfo from proposer #3908
• feat(papyrus_protobuf): add BlockInfo to ProposalPart #3884 : 1 other dependent PR (#3907 )
• feat(papyrus_protobuf): add BlockInfo to proto #3878
• main

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

[guy-starkware]

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

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 847 at r1 (raw file):

        Some(ProposalPart::BlockInfo(_)) => {
            // TODO(Asmaa): Validate the block info.
            HandledProposalPart::Continue

Maybe we should add a panic if we somehow get a block info at this point?

Code quote:

        Some(ProposalPart::BlockInfo(_)) => {
            // TODO(Asmaa): Validate the block info.
            HandledProposalPart::Continue

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 871 at r1 (raw file):

        block_info.height == height
            && (block_info.timestamp >= last_block_timestamp.unwrap_or(0)
                && block_info.timestamp <= block_info.timestamp + block_timestamp_window),

The last comparison against the timestamp window seems to be incorrect (and always true).

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 873 at r1 (raw file):

                && block_info.timestamp <= block_info.timestamp + block_timestamp_window),
        "Invalid block info."
    );

At some point we would probably want to report a failed validation, instead of panic...

Code quote:

        block_info.height == height
            && (block_info.timestamp >= last_block_timestamp.unwrap_or(0)
                && block_info.timestamp <= block_info.timestamp + block_timestamp_window),
        "Invalid block info."
    );

---

crates/starknet_consensus/src/types.rs line 73 at r1 (raw file):

                "block_timestamp_window",
                &self.block_timestamp_window,
                "Maximum allowed deviation (seconds) of a proposed block's timestamp from the \

I'm not sure if this slash (escaping the newline) is what we usually do. I think in most cases we just leave long lines.

[asmaastarkware]

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

---

crates/starknet_consensus/src/types.rs line 73 at r1 (raw file):

Previously, guy-starkware wrote…

I'm not sure if this slash (escaping the newline) is what we usually do. I think in most cases we just leave long lines.

auto formatted
this is a json string; using \ keeps it as one stright line, while using a newline will insert \n.

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 847 at r1 (raw file):

Previously, guy-starkware wrote…

Maybe we should add a panic if we somehow get a block info at this point?

Irrelevant now (we return Failed)

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 871 at r1 (raw file):

Previously, guy-starkware wrote…

The last comparison against the timestamp window seems to be incorrect (and always true).

my bad

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 873 at r1 (raw file):

Previously, guy-starkware wrote…

At some point we would probably want to report a failed validation, instead of panic...

since we return receiver when validation, assertion seems a good catch @matan-starkware WDYT?

[guy-starkware]

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

[matan-starkware]

Reviewed 1 of 5 files at r1, 1 of 1 files at r5.
Reviewable status: 5 of 6 files reviewed, 12 unresolved discussions (waiting on @asmaastarkware and @guy-starkware)

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 873 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

since we return receiver when validation, assertion seems a good catch @matan-starkware WDYT?

I don't think it's a good idea to assert/panic due to an external node sending us an invalid proposal. We do not simply return, we intentionally make assertions affect the whole node, not just the spawned task. Think about what we do in handle_proposal_part, we return Failed.

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 387 at r5 (raw file):

        .times(1)
        .withf(|input| input.proposal_id == ProposalId(1))
        .returning(|_| Ok(()));

Why'd these go away?

Code quote:

    batcher
        .expect_validate_block()
        .times(1)
        .withf(|input| input.proposal_id == ProposalId(0))
        .returning(|_| Ok(()));
    batcher
        .expect_send_proposal_content()
        .withf(|input| {
            input.proposal_id == ProposalId(0) && input.content == SendProposalContent::Abort
        })
        .times(1)
        .returning(move |_| Ok(SendProposalContentResponse { response: ProposalStatus::Aborted }));
    batcher
        .expect_validate_block()
        .times(1)
        .withf(|input| input.proposal_id == ProposalId(1))
        .returning(|_| Ok(()));

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 0 at r4 (raw file):
What about a negative flow. Don't send block info and see that we reject

---

crates/starknet_consensus/src/types.rs line 44 at r4 (raw file):

    pub chain_id: ChainId,
    /// Maximum allowed deviation (seconds) of a proposed block's timestamp from the current time.
    pub block_timestamp_window: u64,

See consensus config sync_retry_interval for durations.

Also in another PR switch startup_delay to also use deserialize_seconds_to_duration.

Code quote:

    /// Maximum allowed deviation (seconds) of a proposed block's timestamp from the current time.
    pub block_timestamp_window: u64,

---

crates/starknet_consensus/src/types.rs line 46 at r4 (raw file):

    pub block_timestamp_window: u64,
    /// The data availability mode, true: Blob, false: Calldata.
    pub l1_da_mode: bool,

I'd like this to be the existing enum. Ideally a user would be able to write something like Blob/Calldata in the command line. You'll probably need to add the parsing logic for this. Make this a follow up PR. Ask around (Yoav Gross for how to do this) and see if there are examples of enums in any of the sequencer/papyrus configs.

Code quote:

    pub l1_da_mode: bool,

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 790 at r5 (raw file):

                    // TODO(Asmaa): Check the correct value for an empty block commitment.
                    if fin_sender
                        .send((BlockHash::default(), ProposalFin { proposal_commitment: id }))

I thought we discussed defining a constant EMPTY_BLOCK_HASH?

Code quote:

                        .send((BlockHash::default(), ProposalFin { proposal_commitment: id }))

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 905 at r5 (raw file):

                .unwrap(),
            l2_gas_price: NonzeroGasPrice::MIN,
        },

Why aren't we just leaving this as default/min?

Code quote:

        eth_gas_prices: GasPriceVector {
            l1_gas_price: NonzeroGasPrice::new(GasPrice(block_info.l1_gas_price_wei)).unwrap(),
            l1_data_gas_price: NonzeroGasPrice::new(GasPrice(block_info.l1_data_gas_price_wei))
                .unwrap(),
            l2_gas_price: NonzeroGasPrice::MIN,
        },

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 931 at r5 (raw file):

            sequencer_address: block_info.builder,
            gas_prices,
            use_kzg_da: block_info.l1_da_mode == L1DataAvailabilityMode::Blob,

Please ask Ayelet to switch to taking the enum, not this bool

Code quote:

            use_kzg_da: block_info.l1_da_mode == L1DataAvailabilityMode::Blob,

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 81 at r4 (raw file):

#[derive(Clone)]
struct ProposalValidation {

What is this used for (add a comment)

Code quote:

#[derive(Clone)]
struct ProposalValidation {

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 164 at r4 (raw file):

    l1_da_mode: L1DataAvailabilityMode,
    block_timestamp_window: u64,
    last_block_timestamp: Option<u64>,

Please double check what the units Starknet uses for timestamp are.

Code quote:

    last_block_timestamp: Option<u64>,

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 451 at r4 (raw file):

        );
        self.last_block_timestamp = Some(
            chrono::Utc::now().timestamp().try_into().expect("Failed to convert timestamp to u64"),

Are you sure this is the correct definition of Starknet timestamp?

Code quote:

            chrono::Utc::now().timestamp().try_into().expect("Failed to convert timestamp to u64"),

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 813 at r4 (raw file):

            }
        }
    };

Please figure out a way to clean up this function. It is very large, we seem to be repeating a pattern and I don't understand why we don't abort here.

Code quote:

    // Validating first proposal part, it should be fin in case this is an empty proposal,
    // or block info.
    tokio::select! {
        _ = cancel_token.cancelled() => {
            warn!("Proposal interrupted");
            return;
        }
        _ = tokio::time::sleep_until(deadline) => {
            warn!("Validation timed out.");
            return;
        }
        proposal_part = content_receiver.next() => {
            match proposal_part {
                None => {
                    warn!("Failed to receive proposal content");
                    return;
                }
                Some(ProposalPart::Fin(ProposalFin { proposal_commitment: id })) => {
                    warn!("Received an empty proposal.");
                    // TODO(Asmaa): Check the correct value for an empty block commitment.
                    if fin_sender
                        .send((BlockHash::default(), ProposalFin { proposal_commitment: id }))
                        .is_err()
                    {
                        // Consensus may exit early (e.g. sync).
                        warn!("Failed to send proposal content ids");
                    }
                    return;
                }
                Some(ProposalPart::BlockInfo(block_info)) => {
                    initiate_validation(
                        proposal_validation.clone(),
                        batcher,
                        block_info,
                        timeout,
                    )
                    .await;
                }
                _ => {
                    panic!("Unexpected proposal part: {proposal_part:?}");
                }
            }
        }
    };

[matan-starkware]

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 387 at r5 (raw file):

Previously, matan-starkware wrote…

Why'd these go away?

Without receiving the block info we never start the block with the batcher. And therefore we also don't need to send the abor.

[matan-starkware]

Reviewed 1 of 5 files at r1.
Reviewable status: 5 of 6 files reviewed, 11 unresolved discussions (waiting on @asmaastarkware and @guy-starkware)

[asmaastarkware]

Reviewable status: 5 of 6 files reviewed, 11 unresolved discussions (waiting on @guy-starkware and @matan-starkware)

---

crates/starknet_consensus/src/types.rs line 44 at r4 (raw file):

Previously, matan-starkware wrote…

See consensus config sync_retry_interval for durations.

Also in another PR switch startup_delay to also use deserialize_seconds_to_duration.

since the timestamp is calculated in seconds, making it in Duration will not add anything

---

crates/starknet_consensus/src/types.rs line 46 at r4 (raw file):

Previously, matan-starkware wrote…

I'd like this to be the existing enum. Ideally a user would be able to write something like Blob/Calldata in the command line. You'll probably need to add the parsing logic for this. Make this a follow up PR. Ask around (Yoav Gross for how to do this) and see if there are examples of enums in any of the sequencer/papyrus configs.

I will do it in the next PRs

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 81 at r4 (raw file):

Previously, matan-starkware wrote…

What is this used for (add a comment)

Done.

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 164 at r4 (raw file):

Previously, matan-starkware wrote…

Please double check what the units Starknet uses for timestamp are.

in seconds since the Unix epoch

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 451 at r4 (raw file):

Previously, matan-starkware wrote…

Are you sure this is the correct definition of Starknet timestamp?

yes

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 813 at r4 (raw file):

Previously, matan-starkware wrote…

Please figure out a way to clean up this function. It is very large, we seem to be repeating a pattern and I don't understand why we don't abort here.

I agree, but it's not that simple..
need to discuss how to refactor it
we don't ask the batcher to start validation, nothing to abort

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 790 at r5 (raw file):

Previously, matan-starkware wrote…

I thought we discussed defining a constant EMPTY_BLOCK_HASH?

Done.

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 905 at r5 (raw file):

Previously, matan-starkware wrote…

Why aren't we just leaving this as default/min?

done

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 931 at r5 (raw file):

Previously, matan-starkware wrote…

Please ask Ayelet to switch to taking the enum, not this bool

👍

[matan-starkware]

Reviewed 2 of 5 files at r1, 1 of 1 files at r4, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @asmaastarkware and @guy-starkware)

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 81 at r4 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

Done.

Isn't this specifically for validating the BlockInfo? This is not all of the fields for validating the proposal.

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 93 at r6 (raw file):

lazy_static! {
    static ref EMPTY_BLOCK_COMMITMENT: BlockHash = BlockHash::default();
}

Please make this a const. Doesn't need to be default.

And then return lazy static to dev dependency

Code quote:

lazy_static! {
    static ref EMPTY_BLOCK_COMMITMENT: BlockHash = BlockHash::default();
}

[asmaastarkware]

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

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 873 at r1 (raw file):

Previously, matan-starkware wrote…

I don't think it's a good idea to assert/panic due to an external node sending us an invalid proposal. We do not simply return, we intentionally make assertions affect the whole node, not just the spawned task. Think about what we do in handle_proposal_part, we return Failed.

Done.

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 81 at r4 (raw file):

Previously, matan-starkware wrote…

Isn't this specifically for validating the BlockInfo? This is not all of the fields for validating the proposal.

fixed

---

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 93 at r6 (raw file):

Previously, matan-starkware wrote…

Please make this a const. Doesn't need to be default.

And then return lazy static to dev dependency

Done.

[matan-starkware]

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