refactor: add BlockWeights for the future bouncer

[Yael-Starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f045651) 69.31% compared to head (519278e) 72.89%.
Report is 56 commits behind head on main.

Files	Patch %	Lines
crates/blockifier/src/bouncer.rs	75.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1444      +/-   ##
==========================================
+ Coverage   69.31%   72.89%   +3.57%     
==========================================
  Files          59       60       +1     
  Lines        8093     9345    +1252     
  Branches     8093     9345    +1252     
==========================================
+ Hits         5610     6812    +1202     
- Misses       2026     2091      +65     
+ Partials      457      442      -15     

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

[Yael-Starkware]

As part of the parallelism effort, we are moving the bouncer to the blockifier.
This is the first PR to implement the bouncer, it defines the struct Bouncer and an add() function that gets tx_weights and adds them to the batch accumulated weights. If there is no more room in this batch , it returns an erros.
It also gets a tx_timestamp and updates the batch oldest timestamp if needed.

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @elintul and @noaov1)

[noaov1]

Can you please create a new module called bouncer that will contains boucer.rs and error.rs files?

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @elintul)

[noaov1]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @elintul and @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 36 at r4 (raw file):

#[allow(dead_code)]
pub struct Bouncer {
    max_block_weights: HashMap<String, u64>,

Is this a property of the bouncer or a configurable value?
Same question for max_tx_lifespan.

Code quote:

max_block_weights: HashMap<String, u64>,

---

crates/blockifier/src/bouncer.rs line 37 at r4 (raw file):

pub struct Bouncer {
    max_block_weights: HashMap<String, u64>,
    accumulated_block_weights: HashMap<String, u64>,

Consider defining an enum instead of using strings.

Code quote:

    accumulated_block_weights: HashMap<String, u64>,

---

crates/blockifier/src/bouncer.rs line 61 at r4 (raw file):

            if !self.max_block_weights.contains_key(key) {
                continue;
            }

Can you please explain why?
Are there weights with no upper bound?

Code quote:

        for (key, weight) in weights.iter() {
            if !self.max_block_weights.contains_key(key) {
                continue;
            }

[Yael-Starkware]

done.

this change kind of mixed everything here , sorry for that

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @elintul, @giladchase, and @noaov1)

---

crates/blockifier/src/bouncer.rs line 36 at r4 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Is this a property of the bouncer or a configurable value?
Same question for max_tx_lifespan.

added a TODO for now, asking @giladchase if this should be in versioned_constants

---

crates/blockifier/src/bouncer.rs line 37 at r4 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Consider defining an enum instead of using strings.

Done

---

crates/blockifier/src/bouncer.rs line 61 at r4 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can you please explain why?
Are there weights with no upper bound?

following our huddle, I replaced the continue with an assert.

[elintul]

Reviewed 3 of 6 files at r5.
Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @giladchase, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 2 at r5 (raw file):

pub mod counting_bouncer;
pub mod errors;

Do you foresee additional files in this directory?
Maybe we should start with src/bouncer[_test].rs?

Code quote:

pub mod counting_bouncer;
pub mod errors;

[elintul]

Reviewed all commit messages.
Reviewable status: 3 of 6 files reviewed, 7 unresolved discussions (waiting on @giladchase, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 13 at r5 (raw file):

pub type BouncerResult<T> = Result<T, BouncerError>;

#[derive(Eq, PartialEq, Hash, Clone, Display)]

Alphabetize.

Code quote:

Eq, PartialEq, Hash, Clone, Display

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 15 at r5 (raw file):

#[derive(Eq, PartialEq, Hash, Clone, Display)]
pub enum BouncerWeights {
    GasWeight,

Suggestion:

Gas

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 18 at r5 (raw file):

    MessageSegmentLength,
    NSteps,
    NStepsWithKeccak, // TODO: how to handle Keccak?

Let's add a TODO to handle Keccak separately.

Suggestion:

    NumberOfSteps,

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 20 at r5 (raw file):

    NStepsWithKeccak, // TODO: how to handle Keccak?
    StateDiffSize,
    StateDiffSizeWithKzg,

This too.

Code quote:

StateDiffSizeWithKzg,

[Yael-Starkware]

Reviewable status: 1 of 8 files reviewed, 7 unresolved discussions (waiting on @elintul, @giladchase, and @noaov1)

---

crates/blockifier/src/bouncer.rs line 36 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

added a TODO for now, asking @giladchase if this should be in versioned_constants

added to versioned constants
done

---

crates/blockifier/src/bouncer.rs line 2 at r5 (raw file):

Previously, elintul (Elin) wrote…

Do you foresee additional files in this directory?
Maybe we should start with src/bouncer[_test].rs?

That was what I did initially, but @noaov1 suggested to move to a dedicated directory. whatever you decide if fine with me.

[Yael-Starkware]

Reviewable status: 1 of 8 files reviewed, 4 unresolved discussions (waiting on @elintul, @giladchase, and @noaov1)

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 15 at r5 (raw file):

#[derive(Eq, PartialEq, Hash, Clone, Display)]
pub enum BouncerWeights {
    GasWeight,

done

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 18 at r5 (raw file):

Previously, elintul (Elin) wrote…

Let's add a TODO to handle Keccak separately.

done

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 20 at r5 (raw file):

Previously, elintul (Elin) wrote…

This too.

done

[elintul]

Reviewed all commit messages.
Reviewable status: 1 of 8 files reviewed, 4 unresolved discussions (waiting on @giladchase, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 36 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

added to versioned constants
done

They should also be configurable from Python, please make sure they are (

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 30 at r8 (raw file):

    type Output = Self;

    fn add(self, other: Self) -> Self {

Do we need this, or is derive_more::Add is enough?

Code quote:

add

[elintul]

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

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 46 at r8 (raw file):

#[allow(dead_code)]
impl BouncerWeights {
    pub fn exceeds_limit(self, versioned_constants: &VersionedConstants) -> bool {

Let's use CheckedSub trait instead.

Code quote:

exceeds_limit

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 61 at r8 (raw file):

#[allow(dead_code)]
#[derive(Default)]
pub struct Bouncer {

Let's move to another PR and only include BouncerWeights here.

Code quote:

Bouncer

[elintul]

Reviewed 1 of 7 files at r6.
Reviewable status: 2 of 8 files reviewed, 6 unresolved discussions (waiting on @giladchase, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 36 at r4 (raw file):

Previously, elintul (Elin) wrote…

They should also be configurable from Python, please make sure they are (

I think the fact they're pub is enough, but let's make sure). This also should be a part of a different PR.

---

crates/blockifier/src/bouncer.rs line 2 at r5 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

That was what I did initially, but @noaov1 suggested to move to a dedicated directory. whatever you decide if fine with me.

Let's continue this way and decide if to consolidate in the end. Add a TODO, please?

[elintul]

Reviewable status: 2 of 8 files reviewed, 7 unresolved discussions (waiting on @giladchase, @noaov1, and @Yael-Starkware)

---

-- commits line 2 at r8:
Let's try to keep PRs as self-contained as possible.

Suggestion:

add BouncerWeights

[elintul]

Reviewed 3 of 7 files at r6, 1 of 1 files at r7, 2 of 2 files at r8.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @giladchase, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 1 at r8 (raw file):

use std::ops::Add;

bouncer.rs, we'll only have one kind (in Python we previously thought of more than one implementation).

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 22 at r8 (raw file):

    message_segment_length: u64,
    state_diff_size: u64,
    state_diff_size_with_kzg: u64,

Suggestion:

    gas: u64,
    n_steps: u64,
    message_segment_length: u64,
    state_diff_size: u64,

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 23 at r8 (raw file):

    state_diff_size: u64,
    state_diff_size_with_kzg: u64,
    // TODO(yael) add more weights (n_memory_holes, builtin_steps)

No need for memory holes, they are counted as steps.
You can add the builtins now.

Code quote:

// TODO(yael) add more weights (n_memory_holes, builtin_steps)

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 24 at r8 (raw file):

    state_diff_size_with_kzg: u64,
    // TODO(yael) add more weights (n_memory_holes, builtin_steps)
    // TODO(yael) understand n_steps_with_keccak and state_diff_size_with_kzg special handling

This affect the max weights.

Code quote:

// TODO(yael) understand n_steps_with_keccak and state_diff_size_with_kzg special handling

[Yael-Starkware]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @elintul, @giladchase, and @noaov1)

---

-- commits line 2 at r8:

Previously, elintul (Elin) wrote…

Let's try to keep PRs as self-contained as possible.

done in a different PR

---

crates/blockifier/src/bouncer.rs line 36 at r4 (raw file):

Previously, elintul (Elin) wrote…

I think the fact they're pub is enough, but let's make sure). This also should be a part of a different PR.

what should be configurable?

---

crates/blockifier/src/bouncer.rs line 61 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

following our huddle, I replaced the continue with an assert.

@noaov1 please resolve

---

crates/blockifier/src/bouncer.rs line 2 at r5 (raw file):

Previously, elintul (Elin) wrote…

Let's continue this way and decide if to consolidate in the end. Add a TODO, please?

I removed the directory in the new PR because there is only one file.
adding a TODO,
Done

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 13 at r5 (raw file):

Previously, elintul (Elin) wrote…

Alphabetize.

done.

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 1 at r8 (raw file):

Previously, elintul (Elin) wrote…

bouncer.rs, we'll only have one kind (in Python we previously thought of more than one implementation).

Done.

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 22 at r8 (raw file):

    message_segment_length: u64,
    state_diff_size: u64,
    state_diff_size_with_kzg: u64,

Done.

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 23 at r8 (raw file):

Previously, elintul (Elin) wrote…

No need for memory holes, they are counted as steps.
You can add the builtins now.

done

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 24 at r8 (raw file):

Previously, elintul (Elin) wrote…

This affect the max weights.

let talk f2f to see that I get this right.

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 30 at r8 (raw file):

Previously, elintul (Elin) wrote…

Do we need this, or is derive_more::Add is enough?

Nice!
done

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 46 at r8 (raw file):

Previously, elintul (Elin) wrote…

Let's use CheckedSub trait instead.

not sure I understand how to use it. Iiuc the CheckedSub trait is substracting the 2 struct and returns None in case of an overflow, but I don't want to substract them, only to get the overflow check

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 61 at r8 (raw file):

Previously, elintul (Elin) wrote…

Let's move to another PR and only include BouncerWeights here.

Done.

[Yael-Starkware]

Reviewable status: 1 of 8 files reviewed, 10 unresolved discussions (waiting on @elintul, @giladchase, and @noaov1)

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 30 at r8 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Nice!
done

deleted this for checked_sub

[elintul]

Reviewed 5 of 7 files at r9, all commit messages.
Reviewable status: 6 of 8 files reviewed, 9 unresolved discussions (waiting on @giladchase, @noaov1, and @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 36 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

what should be configurable?

The capacities. Let's move this offline.

---

crates/blockifier/src/bouncer.rs line 10 at r9 (raw file):

mod test;

pub trait CheckedSub: Sized + Sub<Self, Output = Self> {

Why redefine, can't we use the Rust one?

Code quote:

CheckedSub

---

crates/blockifier/src/bouncer.rs line 14 at r9 (raw file):

}

// TODO decide if a separate directory is needed for the bouncer.

WDYM?

Code quote:

// TODO decide if a separate directory is needed for the bouncer.

---

crates/blockifier/src/bouncer.rs line 16 at r9 (raw file):

// TODO decide if a separate directory is needed for the bouncer.
#[allow(dead_code)]
#[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Sub)]

Alphabetize, throughout the file.

Code quote:

derive

---

crates/blockifier/src/bouncer.rs line 16 at r9 (raw file):

// TODO decide if a separate directory is needed for the bouncer.
#[allow(dead_code)]
#[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Sub)]

Fully qualified name, so it's clear where the trait is coming from.
BTW, is there a derive_more::CheckedSub?

Suggestion:

derive_more::Sub

---

crates/blockifier/src/bouncer.rs line 18 at r9 (raw file):

#[derive(Clone, Copy, Debug, Default, Deserialize, PartialEq, Sub)]
// TODO how to initialize the max values
pub struct BouncerWeights {

Please add a short docstring.

Code quote:

BouncerWeights

---

crates/blockifier/src/bouncer.rs line 24 at r9 (raw file):

    state_diff_size: u64,
    builtin_count: BuiltinCount,
    // TODO(yael) understand n_steps_with_keccak and state_diff_size_with_kzg special handling

This TODO belongs in the area we decide on the capacity of the bouncer.

Code quote:

// TODO(yael) understand n_steps_with_keccak and state_diff_size_with_kzg special handling

---

crates/blockifier/src/bouncer/counting_bouncer.rs line 30 at r8 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

deleted this for checked_sub

:)

[Yael-Starkware]

Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @elintul, @giladchase, and @noaov1)

---

crates/blockifier/src/bouncer.rs line 36 at r4 (raw file):

Previously, elintul (Elin) wrote…

The capacities. Let's move this offline.

this too moves to my notes.

---

crates/blockifier/src/bouncer.rs line 10 at r9 (raw file):

Previously, elintul (Elin) wrote…

Why redefine, can't we use the Rust one?

it is not implemented for this type, only for native types.
maybe I should define it just as a regular impl and not a trait?

---

crates/blockifier/src/bouncer.rs line 14 at r9 (raw file):

Previously, elintul (Elin) wrote…

WDYM?

just a TODO that came from one of our previous discussions here, regarding the location of the files.
deleting the todo from here and moving to my notes

---

crates/blockifier/src/bouncer.rs line 16 at r9 (raw file):

Previously, elintul (Elin) wrote…

Alphabetize, throughout the file.

Am I really bad in English? it feels alphabetised to me

---

crates/blockifier/src/bouncer.rs line 16 at r9 (raw file):

Previously, elintul (Elin) wrote…

Fully qualified name, so it's clear where the trait is coming from.
BTW, is there a derive_more::CheckedSub?

No sir :(

done.

---

crates/blockifier/src/bouncer.rs line 18 at r9 (raw file):

Previously, elintul (Elin) wrote…

Please add a short docstring.

done

---

crates/blockifier/src/bouncer.rs line 24 at r9 (raw file):

Previously, elintul (Elin) wrote…

This TODO belongs in the area we decide on the capacity of the bouncer.

I'll move this to my offline notes for now.

Vacation

[noaov1]

Reviewed 3 of 7 files at r6.
Reviewable status: 6 of 8 files reviewed, all discussions resolved (waiting on @giladchase)

[giladchase]

Reviewed 1 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 15 at r12 (raw file):

        })
    };
}

Is it possible to add the whole fn checked_sub(self, v: Self) -> Option<Self> { into the macro as well?
That sig part is also repetative now.
Only if the macro is still relatively simple looking and this doesn't give any trouble.

Code quote:

macro_rules! checked_sub_struct {
    ($self:expr, $other:expr, $( $field:ident ),*) => {
        Some(Self {
            $(
                $field: $self.$field.checked_sub($other.$field)?,
            )*
        })
    };
}

[Yael-Starkware]

Reviewable status: 7 of 8 files reviewed, all discussions resolved (waiting on @giladchase)

---

crates/blockifier/src/bouncer.rs line 15 at r12 (raw file):

Previously, giladchase wrote…

Is it possible to add the whole fn checked_sub(self, v: Self) -> Option<Self> { into the macro as well?
That sig part is also repetative now.
Only if the macro is still relatively simple looking and this doesn't give any trouble.

done

[giladchase]

Reviewed 1 of 1 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

[giladchase]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 21 at r14 (raw file):

}

#[allow(dead_code)]

Is this necessary?

Code quote:

#[allow(dead_code)]

[giladchase]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 21 at r14 (raw file):

Previously, giladchase wrote…

Is this necessary?

ditto for BouncerWeights

[Yael-Starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)

---

crates/blockifier/src/bouncer.rs line 21 at r14 (raw file):

Previously, giladchase wrote…

ditto for BouncerWeights

yes, otherwise there are many warnings

[Yael-Starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)

---

crates/blockifier/src/bouncer.rs line 21 at r14 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

yes, otherwise there are many warnings

will deleted all of them once code is used.

[giladchase]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 21 at r14 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

will deleted all of them once code is used.

Unused code shouldn't be a problem for public interface, only for private functions.
I think the macro is generating a private method instead of a public method, that should fix the issue and allow removing the dead_code.

[Yael-Starkware]

Reviewable status: 7 of 8 files reviewed, 1 unresolved discussion (waiting on @giladchase)

---

crates/blockifier/src/bouncer.rs line 21 at r14 (raw file):

Previously, giladchase wrote…

Unused code shouldn't be a problem for public interface, only for private functions.
I think the macro is generating a private method instead of a public method, that should fix the issue and allow removing the dead_code.

wow nice, it solved it! impressive stuff!
done.

[giladchase]

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

[elintul]

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)

---

crates/blockifier/src/bouncer.rs line 23 at r15 (raw file):

#[derive(Clone, Copy, Debug, Default, derive_more::Sub, Deserialize, PartialEq)]
/// Counted weights of the execution of transactions in a block.
/// These weights are limited by the block's maximum weights.

Move before the derive. A shorter suggestion: "Represents the execution resources counted throughout block creation.`.

Code quote:

/// Counted weights of the execution of transactions in a block.
/// These weights are limited by the block's maximum weights.

[elintul]

Reviewed 1 of 1 files at r12, 1 of 1 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)