chore(starknet_batcher): pass block info from consensus to batcher

[ArniStarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[ArniStarkware]

• chore(starknet_batcher): pass block info from consensus to batcher #2238 : 3 dependent PRs (#2267 , #2302 , #2409 ) 👈 (View in Graphite)
• chore(starknet_consensus_manager): set proposer address in propose block input #2346
• chore(starknet_batcher): set use_kzg_da flag in build block input #2345
• chore(starknet_batcher): set temp gas prices in propose block input #2341
• chore(starknet_batcher): set block timestamp in build block input #2342
• main

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

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 13.20%. Comparing base (e3165c4) to head (616d561).
Report is 699 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/starknet_batcher/src/block_builder.rs	0.00%	9 Missing ⚠️
crates/starknet_api/src/block.rs	0.00%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2238       +/-   ##
===========================================
- Coverage   40.10%   13.20%   -26.91%     
===========================================
  Files          26      144      +118     
  Lines        1895    17078    +15183     
  Branches     1895    17078    +15183     
===========================================
+ Hits          760     2255     +1495     
- Misses       1100    14707    +13607     
- Partials       35      116       +81     

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

[yair-starkware]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alonh5, @ArniStarkware, and @dafnamatsry)

---

a discussion (no related file):
Do gas prices and kzg determined at each proposal, or are they determined at the beginning of each height?
If per height, we can move them to the start height input

---

crates/starknet_batcher/src/proposal_manager_test.rs line 37 at r1 (raw file):

static BLOCK_INFO_AT_INITIAL_HEIGHT: LazyLock<BlockInfo> =
    LazyLock::new(|| BlockInfo { block_number: INITIAL_HEIGHT, ..Default::default() });

Same

Code quote:

static BLOCK_INFO_AT_INITIAL_HEIGHT: LazyLock<BlockInfo> =
    LazyLock::new(|| BlockInfo { block_number: INITIAL_HEIGHT, ..Default::default() });

---

crates/starknet_batcher/src/batcher_test.rs line 60 at r1 (raw file):

static BLOCK_INFO_AT_INITIAL_HEIGHT: LazyLock<BlockInfo> =
    LazyLock::new(|| BlockInfo { block_number: INITIAL_HEIGHT, ..Default::default() });

Consider using dummy values so you won't need the lazy lock.
You're supposed to remove the default anyway, right?

Code quote:

static BLOCK_INFO_AT_INITIAL_HEIGHT: LazyLock<BlockInfo> =
    LazyLock::new(|| BlockInfo { block_number: INITIAL_HEIGHT, ..Default::default() });

---

crates/starknet_batcher/src/batcher.rs line 431 at r1 (raw file):

}

// TODO: Move to a common place.

to starknet_batcher_types

Code quote:

common place

---

crates/starknet_batcher/src/block_builder.rs line 316 at r1 (raw file):

            },
            // TODO: Override the use_kzg_da flag.
            use_kzg_da: self.block_builder_config.use_kzg_da,

Why don't you take these from the next_block_info?

Code quote:

            gas_prices: {
                let tmp_val = NonzeroGasPrice::MIN;
                GasPrices::new(tmp_val, tmp_val, tmp_val, tmp_val, tmp_val, tmp_val)
            },
            // TODO: Override the use_kzg_da flag.
            use_kzg_da: self.block_builder_config.use_kzg_da,

[ArniStarkware]

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @alonh5, @dafnamatsry, and @yair-starkware)

---

a discussion (no related file):

Previously, yair-starkware (Yair) wrote…

Do gas prices and kzg determined at each proposal, or are they determined at the beginning of each height?
If per height, we can move them to the start height input

IIUC, gas prices are determined at each proposal.

---

crates/starknet_batcher/src/batcher.rs line 431 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

to starknet_batcher_types

Done.

---

crates/starknet_batcher/src/batcher_test.rs line 60 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Consider using dummy values so you won't need the lazy lock.
You're supposed to remove the default anyway, right?

Initializing static ContractAddress is difficult, so I do not see how we can lose the lazy lock (other than using a function and computing the value for each test).

Now, we are using create_for_testing() so that we will not use default when it is not needed.

---

crates/starknet_batcher/src/block_builder.rs line 316 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Why don't you take these from the next_block_info?

The value of next_block_info originally comes from: ProposeBlockInput, which still needs to be filled. Once it is filled, the values should be propagated here.
https://reviewable.io/reviews/starkware-libs/sequencer/2237#-OCTUZwq3yG6618ECY0s

I can see why it is confusing - and maybe some other approach should be taken.

---

crates/starknet_batcher/src/proposal_manager_test.rs line 37 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

Same

Done.

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.228 ms 34.716 ms 35.292 ms]
change: [+1.4556% +2.8664% +4.5401%] (p = 0.00 < 0.05)
Performance has regressed.
Found 10 outliers among 100 measurements (10.00%)
2 (2.00%) high mild
8 (8.00%) high severe

[ArniStarkware]

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

---

-- commits line 2 at r3:
Update the commit message before merging.

Code quote:

- 34b91f6: chore(batcher): use thin block info to create block info

[ArniStarkware]

Reviewable status: 5 of 11 files reviewed, 1 unresolved discussion (waiting on @alonh5, @dafnamatsry, and @yair-starkware)

---

crates/starknet_batcher/src/block_builder.rs line 221 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

If it's not trivial then you're right it shouldn't be in this PR. But in general, do you think it would be possible? Where is the retrospective block hash received now in the OS?

TL;DR: I do not think it is correct to move retrospective block hash into BlockInfo.

The term retrospective block hash appears only in the context of the batcher.
The point where they diverge is here:

sequencer/crates/blockifier/src/blockifier/transaction_executor.rs

Lines 60 to 74 in 111c6fd

	pub fn pre_process_and_create(
	initial_state_reader: S,
	block_context: BlockContext,
	old_block_number_and_hash: Option<BlockHashAndNumber>,
	config: TransactionExecutorConfig,
	) -> StateResult<Self> {
	let mut block_state = CachedState::new(initial_state_reader);
	pre_process_block(
	&mut block_state,
	old_block_number_and_hash,
	block_context.block_info().block_number,
	&block_context.versioned_constants.os_constants,
	)?;
	Ok(Self::new(block_state, block_context, config))
	}

The retrospective block hash is used in this code to pre-process the storage.
The block info is hidden inside the block context - and is later sent to the OS.

[yair-starkware]

Reviewed all commit messages.
Reviewable status: 5 of 11 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @dafnamatsry)

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.458 ms 36.023 ms 36.666 ms]
change: [+1.6023% +3.3155% +5.2103%] (p = 0.00 < 0.05)
Performance has regressed.
Found 12 outliers among 100 measurements (12.00%)
12 (12.00%) high severe

[ArniStarkware]

Reviewable status: 5 of 11 files reviewed, 3 unresolved discussions (waiting on @alonh5, @dafnamatsry, and @yair-starkware)

---

crates/starknet_batcher/src/block_builder.rs line 316 at r15 (raw file):

            sequencer_address: block_builder_config.sequencer_address,
            ..block_metadata.block_info
        };

Suggestion:

        let mut block_info = block_metadata.block_info;
        // TODO(Arni): Remove this override, and the corresponding config.
        block_info.sequencer_address = block_builder_config.sequencer_address;

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 169 at r15 (raw file):

            }),
            // TODO(Dan, Matan): Fill block info. Override the sequencer address in the block info
            // in the BlockBuilderFactory.

Revert. X2.

Suggestion:

            // TODO(Dan, Matan): Fill block info.

[ArniStarkware]

Reviewable status: 5 of 11 files reviewed, 1 unresolved discussion (waiting on @alonh5, @dafnamatsry, and @yair-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 169 at r15 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Revert. X2.

Done.

---

crates/starknet_batcher/src/block_builder.rs line 316 at r15 (raw file):

            sequencer_address: block_builder_config.sequencer_address,
            ..block_metadata.block_info
        };

Done.

[yair-starkware]

Reviewed 1 of 7 files at r11, 2 of 4 files at r13, 2 of 2 files at r16, all commit messages.
Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @dafnamatsry)

[alonh5]

Reviewed 3 of 7 files at r11, 1 of 1 files at r12, 2 of 4 files at r13, 2 of 2 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)

---

crates/starknet_batcher/src/block_builder.rs line 221 at r5 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

TL;DR: I do not think it is correct to move retrospective block hash into BlockInfo.

The term retrospective block hash appears only in the context of the batcher.
The point where they diverge is here:

sequencer/crates/blockifier/src/blockifier/transaction_executor.rs

Lines 60 to 74 in 111c6fd

	pub fn pre_process_and_create(
	initial_state_reader: S,
	block_context: BlockContext,
	old_block_number_and_hash: Option<BlockHashAndNumber>,
	config: TransactionExecutorConfig,
	) -> StateResult<Self> {
	let mut block_state = CachedState::new(initial_state_reader);
	pre_process_block(
	&mut block_state,
	old_block_number_and_hash,
	block_context.block_info().block_number,
	&block_context.versioned_constants.os_constants,
	)?;
	Ok(Self::new(block_state, block_context, config))
	}

The retrospective block hash is used in this code to pre-process the storage.
The block info is hidden inside the block context - and is later sent to the OS.

Okay, I didn't fully understand, but it's not important for now. I'll add a task to Monday to reconsider it

[ArniStarkware]

crates/starknet_batcher/src/block_builder.rs line 221 at r5 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Okay, I didn't fully understand, but it's not important for now. I'll add a task to Monday to reconsider it

I think it should not even be on Monday.

[alonh5]

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

You agreed on DMs that all discussions are closed.

[ArniStarkware]

Merge activity

• Dec 5, 4:36 AM EST: A user started a stack merge that includes this pull request via Graphite.
• Dec 5, 4:37 AM EST: A user merged this pull request with Graphite.