refactor(execution): bouncer update function

[Yael-Starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 94.28571% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 75.16%. Comparing base (59150ce) to head (b5f195d).

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1644      +/-   ##
==========================================
+ Coverage   74.47%   75.16%   +0.69%     
==========================================
  Files          59       59              
  Lines        8536     8549      +13     
  Branches     8536     8549      +13     
==========================================
+ Hits         6357     6426      +69     
+ Misses       1759     1699      -60     
- Partials      420      424       +4     

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

[Yael-Starkware]

Context:
This PR contains the high level parts of the update flow of the bouncer, and is the most loaded I think and apologize.
The bouncer update function (previously known as add() in python) will proceed in the following steps:

1. update_auxiliary_info
2. update_capacity (BouncerWeights)
   a. get_tx_weights
   b. check if keccak was used and update the capacity accordingly
   c. check if the tx is too big to fit any batch, if so ->error
   d. try to update the bouncer by subtracting the tx_weights
   if succeeds -> commit the new values, OK
   else -> keep the old bouncer values, Error

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @ayeletstarkware, @noaov1, and @Yoni-Starkware)

[Yael-Starkware]

The logic here should be the same as the logic of the python bouncer add() function.
The only logic that remains in the python bouncer is the logic that handles time : The logic that check that each that lifespan hasn't passed since the oldest transaction arrived to the system and the logic that limits the block creation rate.

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @ArniStarkware, @noaov1, and @Yoni-Starkware)

[ArniStarkware]

Reviewed 1 of 4 files at r2.
Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @noaov1, @Yael-Starkware, and @Yoni-Starkware)

---

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

            block_contains_keccak,
        }
    }

WDYT?

Suggestion:

    pub fn new(available_capacity: BouncerWeights, block_contains_keccak: bool) -> Self {
        Bouncer {
            executed_class_hashes: HashSet::new(),
            state_changes_keys: StateChangesKeys::default(),
            visited_storage_entries: HashSet::new(),
            available_capacity: capacity,
            block_contains_keccak,
        }
    }

---

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

    // The transaction bouncer can be modified only through the update method.
    transactional: Bouncer,
}

Transactional vs transactional_bouncer, I don't mind. Leaning towards the former.

Suggestion:

pub struct TransactionalBouncer {
    // The block bouncer can be modified only through the merge method.
    block_bouncer: Bouncer,
    // The transaction bouncer can be modified only through the update method.
    transactional: Bouncer,
}

---

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

    }

    pub fn update<S: StateReader>(

Document, please. In what context is this function called? also, the next function.

Code quote:

    pub fn update<S: StateReader>(

---

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

    }

    fn update_capacity<S: StateReader>(

?

Suggestion:

fn update_available_capacity<S: StateReader>(

---

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

        if !self.transactional.block_contains_keccak && tx_weights.builtin_count.keccak > 0 {
            self.update_available_capacity_with_keccak(bouncer_config)?;
        }

If it is true, please rephrase (and pass through Grammarly etc).

Suggestion:

        if !self.transactional.block_contains_keccak && tx_weights.builtin_count.keccak > 0 {
            // This is update is the first update to contain keccak in this block.
            self.update_available_capacity_with_keccak(bouncer_config)?;
        }

---

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

    ) -> TransactionExecutorResult<()> {
        // First zero the keccak capacity to be able to subtract the max_capacity_with_keccak from
        // max_capacity (that is without keccak).

You explain here that max_capacity is without keccak. This is because of the assumptions of when we call this function, is it?
If so - please document this in the tx's docstring under assumptions.

(Good job writing it out at all; it made it easy to review).

Code quote:

        // First zero the keccak capacity to be able to subtract the max_capacity_with_keccak from
        // max_capacity (that is without keccak).

---

crates/blockifier/src/transaction/errors.rs line 55 at r2 (raw file):

pub enum TransactionExecutionError {
    #[error("Transaction cannot be added to the current block, block capacity reached.")]
    BlockFull,

There should be a test for this scenario.

Code quote:

    #[error("Transaction cannot be added to the current block, block capacity reached.")]
    BlockFull,

---

crates/blockifier/src/transaction/errors.rs line 91 at r2 (raw file):

    TryFromIntError(#[from] std::num::TryFromIntError),
    #[error("Transaction size exceeds the maximum block capacity.")]
    TxTooLarge,

There should be a test for this scenario.

Code quote:

    #[error("Transaction size exceeds the maximum block capacity.")]
    TxTooLarge,

[Yael-Starkware]

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

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

WDYT?

agreed, done.

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

Transactional vs transactional_bouncer, I don't mind. Leaning towards the former.

Actually we already had this naming discussion , with both Yoni and Elin. So I prefer to keep the former. (though I also suggested to call it block :) )

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

Document, please. In what context is this function called? also, the next function.

done.

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

?

done.

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

If it is true, please rephrase (and pass through Grammarly etc).

Done.

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

You explain here that max_capacity is without keccak. This is because of the assumptions of when we call this function, is it?
If so - please document this in the tx's docstring under assumptions.

(Good job writing it out at all; it made it easy to review).

Thanks.
Done.
not sure what you mean by the tx's docstring under assumptions, I hope I did it right.

---

crates/blockifier/src/transaction/errors.rs line 55 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

There should be a test for this scenario.

Added a pretty comprehensive test now. Was indeed needed here.

---

crates/blockifier/src/transaction/errors.rs line 91 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

There should be a test for this scenario.

Same

[Yael-Starkware]

Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @ArniStarkware, @noaov1, and @Yoni-Starkware)

---

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

    fn from(data: ExecutionResources) -> Self {
        BouncerWeights {
            n_steps: data.n_steps + data.n_memory_holes,

Changed this code since the ExecutionResources only contain hash resources.
double checking with @Yoni-Starkware that is makes sense.

[ArniStarkware]

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

---

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

    /// This function is called only if the block does not contain keccak yet.
    /// Assumption: max_capacity without keccak is smaller than max_capacity with keccak and has
    /// keccak capacity zero.

Suggestion:

    /// Assumption: the block does not contain keccak yet.

[Yael-Starkware]

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

---

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

    /// This function is called only if the block does not contain keccak yet.
    /// Assumption: max_capacity without keccak is smaller than max_capacity with keccak and has
    /// keccak capacity zero.

done.

[ArniStarkware]

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

---

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

        self.transactional.block_contains_keccak = true;
        Ok(())
    }

Sorry for not suggesting this from the beginning.
How about something like this, and then call it another way?

Suggestion:

    /// Updates the available capacity in case until this moment the block did not contain keccak, and now it does.
    fn optionally_update_available_capacity_for_keccak(
        &mut self,
        bouncer_config: &BouncerConfig,
        keccak_builtin_counter: usize, //whatever should be here.
    ) -> TransactionExecutorResult<()> {
        if self.transactional.block_contains_keccak || keccak_builtin_counter == 0 {
            return Ok(());
        } // This is almost like an assert!(!self.transactional.block_contains_keccak && ...);
        
        // First zero the keccak capacity to be able to subtract the max_capacity_with_keccak from
        // max_capacity (that is without keccak).
        let mut max_capacity_with_keccak_tmp = bouncer_config.block_max_capacity_with_keccak;
        max_capacity_with_keccak_tmp.builtin_count.keccak = 0;
        // compute the diff between the max_capacity and the max_capacity_with_keccak.
        let max_capacity_with_keccak_diff = bouncer_config
            .block_max_capacity
            .checked_sub(max_capacity_with_keccak_tmp)
            .expect("max_capacity_with_keccak should be smaller than max_capacity");
        // Subtract the diff from the available capacity.
        self.transactional.available_capacity = self
            .transactional
            .available_capacity
            .checked_sub(max_capacity_with_keccak_diff)
            .ok_or(TransactionExecutionError::BlockFull)?;
        // Add back the keccack capacity that was reset at the beggining.
        self.transactional.available_capacity.builtin_count.keccak =
            bouncer_config.block_max_capacity_with_keccak.builtin_count.keccak;
        // Mark this block as contains keccak.
        self.transactional.block_contains_keccak = true;
        Ok(())
    }

[Yael-Starkware]

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

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

Sorry for not suggesting this from the beginning.
How about something like this, and then call it another way?

done.

[ArniStarkware]

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

Previously, Yael-Starkware (YaelD) wrote…

done.

I don't see it.

[Yael-Starkware]

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

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Perhaps the other way around

pub struct Bouncer {
    accumulated_weights: BouncerWeights,
    
    // Additional info; maintained and used to calculate the residual contribution of a transaction to the accumulated weights.
    pub executed_class_hashes: HashSet<ClassHash>,
    pub visited_storage_entries: HashSet<StorageEntry>,
    pub state_changes_keys: StateChangesKeys,
    pub bouncer_config: BouncerConfig,

Gilad taught me that private variables are after public.

[Yael-Starkware]

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

---

crates/blockifier/src/bouncer.rs line 232 at r16 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Waiting for the rebase

Done.

[Yael-Starkware]

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

---

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

Previously, Yael-Starkware (YaelD) wrote…

Gilad taught me that private variables are after public.

other than that , done.

[ArniStarkware]

(no tests)

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

[ArniStarkware]

(on tests)

[Yoni-Starkware]

Reviewed 1 of 4 files at r2, 1 of 3 files at r10, all commit messages.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @noaov1, and @Yael-Starkware)

---

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

    pub bouncer_config: BouncerConfig,

    accumulated_weights: BouncerWeights,

Do wee need all of them to be pub?

Suggestion:

    // Additional info; maintained and used to calculate the residual contribution of a transaction
    // to the accumulated weights.
    pub executed_class_hashes: HashSet<ClassHash>,
    pub visited_storage_entries: HashSet<StorageEntry>,
    pub state_changes_keys: StateChangesKeys,
    
    pub bouncer_config: BouncerConfig,

    accumulated_weights: BouncerWeights,

[Yoni-Starkware]

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

[Yael-Starkware]

Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @giladchase, and @noaov1)

---

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

Previously, Yoni-Starkware (Yoni) wrote…

Do wee need all of them to be pub?

@giladchase said that every field should be public unless there is a real constraint.

[Yoni-Starkware]

Reviewed 1 of 3 files at r17, 1 of 1 files at r20, all commit messages.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @ArniStarkware and @noaov1)

[ArniStarkware]

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

[Yoni-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @Yael-Starkware)

---

crates/blockifier/src/bouncer_test.rs line 22 at r20 (raw file):

#[test]
fn test_block_weights_sub_checked() {

Rename and adjust a little to test has_room?

---

crates/blockifier/src/state/cached_state.rs line 717 at r20 (raw file):

    #[cfg(any(feature = "testing", test))]
    pub fn create_for_testing(nonce_keys: HashSet<ContractAddress>) -> Self {

According to @noaov1, this should be defined in struct_impls.rs

Code quote:

    #[cfg(any(feature = "testing", test))]
    pub fn create_for_testing(nonce_keys: HashSet<ContractAddress>) -> Self {

[Yael-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)

---

crates/blockifier/src/bouncer_test.rs line 22 at r20 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Rename and adjust a little to test has_room?

oopsy, done.

---

crates/blockifier/src/state/cached_state.rs line 717 at r20 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

According to @noaov1, this should be defined in struct_impls.rs

couldn't move it since the fields of StateChangesKeys are private so you can't access them outside of the module the object was defined in.
@noaov1 approved to keep this function here.

[Yoni-Starkware]

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

[giladchase]

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

---

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

Previously, Yael-Starkware (YaelD) wrote…

@giladchase said that every field should be public unless there is a real constraint.

Constraint ==> Invariant
:P

[Yael-Starkware]

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

---

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

Previously, giladchase wrote…

Constraint ==> Invariant
:P

yes exactly :)

[Yoni-Starkware]

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