chore(batcher): impl default to ExecutionConfig for addition to Batch…

[ayeletstarkware]

…erConfig

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 2.17391% with 45 lines in your changes missing coverage. Please review.

Project coverage is 55.13%. Comparing base (b0cfe82) to head (4bd5d2e).
Report is 375 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/bouncer.rs	0.00%	25 Missing ⚠️
crates/batcher/src/block_builder.rs	0.00%	12 Missing ⚠️
crates/blockifier/src/versioned_constants.rs	0.00%	7 Missing ⚠️
crates/batcher/src/batcher.rs	0.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (b0cfe82) and HEAD (4bd5d2e). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (b0cfe82)	HEAD (4bd5d2e)
	3	1

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1277       +/-   ##
===========================================
- Coverage   74.18%   55.13%   -19.06%     
===========================================
  Files         359      148      -211     
  Lines       36240    17306    -18934     
  Branches    36240    17306    -18934     
===========================================
- Hits        26886     9541    -17345     
- Misses       7220     7343      +123     
+ Partials     2134      422     -1712     

Flag	Coverage Δ
	?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[noaov1]

Can you please give more context? :)
Are we planning to use the default values in the regular flow?
Can we add a config to allow an easy modification to the values? (eliminates the need to re-publish)

Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: 2 of 5 files reviewed, all discussions resolved (waiting on @Yael-Starkware)

[Yael-Starkware]

Reviewed 3 of 5 files at r1, all commit messages.
Reviewable status: 4 of 5 files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware and @noaov1)

---

crates/blockifier/src/bouncer.rs line 136 at r1 (raw file):

        Self {
            gas: 4950000,
            n_steps: 20000000,

I see 40000000 in some starknet config files.
lets verify all numbers with @noaov1

Code quote:

20000000

---

crates/blockifier/src/bouncer.rs line 137 at r1 (raw file):

            gas: 4950000,
            n_steps: 20000000,
            message_segment_length: 3700,

I see the @Yoni-Starkware recently changed it from 3750 to 3700, did you check with him?

Code quote:

3700

---

crates/blockifier/src/bouncer.rs line 139 at r1 (raw file):

            message_segment_length: 3700,
            n_events: 5000,
            state_diff_size: 20000,

Suggestion:

//TODO: override this value when using kzg_da
state_diff_size: 20000,

---

crates/blockifier/src/bouncer.rs line 140 at r1 (raw file):

            n_events: 5000,
            state_diff_size: 20000,
            builtin_count: BuiltinCount::default(),

please implement this default as well.

Code quote:

builtin_count: BuiltinCount::default()

---

crates/blockifier/src/versioned_constants.rs line 868 at r1 (raw file):

            validate_max_n_steps: 1000000,
            max_recursion_depth: 50,
            invoke_tx_max_n_steps: 4000000,

I think it should be 10000000 according to starknet version 0_13_3
did you ask @noaov1 ?

Suggestion:

10000000

---

crates/blockifier/src/blockifier/config.rs line 32 at r1 (raw file):

impl Default for ConcurrencyConfig {
    fn default() -> Self {
        Self { enabled: true, n_workers: 28, chunk_size: 100 }

this was the number provided by @noaov1 ?

Code quote:

28

[Yoni-Starkware]

Reviewable status: 4 of 5 files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware, @noaov1, @Yael-Starkware, and @Yoni-Starkware)

---

crates/blockifier/src/bouncer.rs line 137 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I see the @Yoni-Starkware recently changed it from 3750 to 3700, did you check with him?

3700 if fine (very high)

---

crates/blockifier/src/bouncer.rs line 139 at r1 (raw file):

            message_segment_length: 3700,
            n_events: 5000,
            state_diff_size: 20000,

Let it be 4000 in any case.

---

crates/blockifier/src/versioned_constants.rs line 868 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I think it should be 10000000 according to starknet version 0_13_3
did you ask @noaov1 ?

Why do you need to put concrete values here?
The override is for overriding the VC values, primarily for appchains or for bugfixes.
I.e., it should be passed as Option

[Yael-Starkware]

yes we plan to use these in the regular flow.
These are the default values and can be overridden by a config file.

Reviewable status: 4 of 5 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @noaov1)

[Yael-Starkware]

Reviewable status: 4 of 5 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @noaov1)

---

crates/blockifier/src/versioned_constants.rs line 868 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Why do you need to put concrete values here?
The override is for overriding the VC values, primarily for appchains or for bugfixes.
I.e., it should be passed as Option

I fully agree, but this is the existing blockifier code.

[Yael-Starkware]

Reviewable status: 4 of 5 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @noaov1)

---

crates/blockifier/src/versioned_constants.rs line 868 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I fully agree, but this is the existing blockifier code.

we can do this as a separate PR.

[Yael-Starkware]

Reviewable status: 4 of 5 files reviewed, 7 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, and @noaov1)

---

crates/batcher/src/block_builder.rs line 65 at r1 (raw file):

            chain_info: ChainInfo::default(),
            execute_config: TransactionExecutorConfig::default(),
            bouncer_config: BouncerConfig::empty(),

why empty?

Code quote:

bouncer_config: BouncerConfig::empty()

[ayeletstarkware]

Reviewable status: 4 of 5 files reviewed, 7 unresolved discussions (waiting on @ArniStarkware, @noaov1, and @Yael-Starkware)

---

crates/batcher/src/block_builder.rs line 65 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

why empty?

A mistake. Done.

---

crates/blockifier/src/bouncer.rs line 136 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I see 40000000 in some starknet config files.
lets verify all numbers with @noaov1

used 20000000

---

crates/blockifier/src/bouncer.rs line 137 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

3700 if fine (very high)

Done.

---

crates/blockifier/src/bouncer.rs line 139 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Let it be 4000 in any case.

Done.

---

crates/blockifier/src/bouncer.rs line 140 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

please implement this default as well.

Done.

---

crates/blockifier/src/versioned_constants.rs line 868 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

we can do this as a separate PR.

changed to 10000000. I will create a separate PR for passing VersionedConstantsOverrides as an option.

---

crates/blockifier/src/blockifier/config.rs line 32 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

this was the number provided by @noaov1 ?

stale

[Yoni-Starkware]

Reviewable status: 0 of 5 files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @noaov1, and @Yael-Starkware)

---

crates/batcher/src/block_builder.rs line 49 at r2 (raw file):

#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
pub struct ExecutionConfig {

I suggest a more specific name. Execution is used too much in our code

Suggestion:

BlockBuilderConfig

[Yoni-Starkware]

Reviewable status: 0 of 5 files reviewed, 10 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 135 at r2 (raw file):

    fn default() -> Self {
        Self {
            gas: 2500000,

Why?

Code quote:

gas: 2500000,

---

crates/blockifier/src/bouncer.rs line 303 at r2 (raw file):

            poseidon: 78125,
            range_check: 156250,
            range_check96: 0,

There should not be zeros here. Where did it come from?

Code quote:

            add_mod: 0,
            bitwise: 39062,
            ecdsa: 1220,
            ec_op: 2441,
            keccak: 0,
            mul_mod: 0,
            pedersen: 78125,
            poseidon: 78125,
            range_check: 156250,
            range_check96: 0,

[ArniStarkware]

Non-blocking. If you have more comments for address in this PR, please fix the creation of execution_config in batcher.rs:

sequencer/crates/batcher/src/batcher.rs

Line 126 in 033ec45

let execution_config = ExecutionConfig {

Should turn into:

let execution_config = ExecutionConfig::default();

[ayeletstarkware]

Reviewable status: 0 of 5 files reviewed, 11 unresolved discussions (waiting on @ArniStarkware, @noaov1, @Yael-Starkware, and @Yoni-Starkware)

---

crates/blockifier/src/bouncer.rs line 135 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Why?

All values, except state_diff_size: 4000 (as you requested), were taken from starkware/starknet/services/batcher/config.yml as Lior suggested. These are placeholder values to get the program running and will be updated later.

---

crates/blockifier/src/bouncer.rs line 303 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

There should not be zeros here. Where did it come from?

The builtins were calculated as n_steps (2500000) / ratio from src/starkware/cairo/lang/instances_external.py. I set unlisted builtins to zero, which I think won’t stop the program from running, right?

[Yoni-Starkware]

Reviewable status: 0 of 5 files reviewed, 10 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 135 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

All values, except state_diff_size: 4000 (as you requested), were taken from starkware/starknet/services/batcher/config.yml as Lior suggested. These are placeholder values to get the program running and will be updated later.

Ok

[Yoni-Starkware]

Reviewable status: 0 of 5 files reviewed, 10 unresolved discussions (waiting on @ArniStarkware, @ayeletstarkware, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 303 at r2 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

The builtins were calculated as n_steps (2500000) / ratio from src/starkware/cairo/lang/instances_external.py. I set unlisted builtins to zero, which I think won’t stop the program from running, right?

It would fail txs the use these builtins (even in tests it's not good)
use starknet_with_keccak_instance ratios and use the ratio of range_check for
"add_mod_builtin", "mul_mod_builtin", "range_check96_builtin"

[ayeletstarkware]

Reviewable status: 0 of 6 files reviewed, 9 unresolved discussions (waiting on @ArniStarkware, @noaov1, @Yael-Starkware, and @Yoni-Starkware)

---

crates/blockifier/src/bouncer.rs line 303 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

It would fail txs the use these builtins (even in tests it's not good)
use starknet_with_keccak_instance ratios and use the ratio of range_check for
"add_mod_builtin", "mul_mod_builtin", "range_check96_builtin"

Done.

[ArniStarkware]

Reviewed 1 of 2 files at r3.
Reviewable status: 1 of 6 files reviewed, 9 unresolved discussions (waiting on @noaov1, @Yael-Starkware, and @Yoni-Starkware)

[Yael-Starkware]

Reviewed 1 of 5 files at r1, 2 of 4 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status: 4 of 6 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/bouncer.rs line 137 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Done.

I still see 3750

[Yael-Starkware]

Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/bouncer.rs line 291 at r3 (raw file):

}

impl Default for BuiltinCount {

please add a todo for each struct that doesn't represent the real production values, so we won't forget to fix it in the future.

[ayeletstarkware]

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @noaov1 and @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 137 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I still see 3750

Done

---

crates/blockifier/src/bouncer.rs line 291 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

please add a todo for each struct that doesn't represent the real production values, so we won't forget to fix it in the future.

Done.

[Yael-Starkware]

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1 and @Yoni-Starkware)

[Yoni-Starkware]

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

[ArniStarkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[ArniStarkware]

-- commits line 2 at r4:
Extremely non-blocking. (you can fix the merge massage)

Suggestion:

impl default to BlockBuilderConfig towards implementation of BatcherConfig