fix(concurrency): add the sequencer key when checking if the tx is

[meship-starkware]

too big in concurrency mode

---

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.85%. Comparing base (6e06e79) to head (2bdfae7).
Report is 6 commits behind head on main.

Files	Patch %	Lines
.../blockifier/src/transaction/account_transaction.rs	50.00%	0 Missing and 2 partials ⚠️
...lockifier/src/transaction/transaction_execution.rs	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1980      +/-   ##
==========================================
+ Coverage   78.42%   78.85%   +0.42%     
==========================================
  Files          62       62              
  Lines        8913     9031     +118     
  Branches     8913     9031     +118     
==========================================
+ Hits         6990     7121     +131     
+ Misses       1476     1460      -16     
- Partials      447      450       +3     

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

[meship-starkware]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/state/cached_state.rs line 578 at r1 (raw file):

pub struct StateChangesKeys {
    nonce_keys: HashSet<ContractAddress>,
    class_hash_keys: HashSet<ContractAddress>,

Im not sure changing this filed to pub(crate) is the right solution to add the sequencer balance key to the storage keys.

[meship-starkware]

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

---

crates/blockifier/src/concurrency/worker_logic.rs line 225 at r1 (raw file):

        let mut execution_output = lock_mutex_in_array(&self.execution_outputs, tx_index);
        let writes = &execution_output.as_ref().expect(EXECUTION_OUTPUTS_UNWRAP_ERROR).writes;
        let reads = &execution_output.as_ref().expect(EXECUTION_OUTPUTS_UNWRAP_ERROR).reads;

It might be better to fix the sequencer balance before checking if it has a place in the bouncer. We just need to ensure that if the transaction fails because it has no space, we won't need to revert this change.

[Yoni-Starkware]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @avi-starkware, @meship-starkware, and @noaov1)

---

crates/blockifier/src/state/cached_state.rs line 578 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Im not sure changing this filed to pub(crate) is the right solution to add the sequencer balance key to the storage keys.

Instead, create a StateChangesKeys object and use extend

---

crates/blockifier/src/transaction/transaction_execution.rs line 192 at r2 (raw file):

                .storage_keys
                .insert((tx_context.fee_token_address(), sequencer_balance_low));
        }

Share code with the worker logic.

Code quote:

        if block_context.concurrency_mode
            && block_context.block_info.sequencer_address != tx_context.tx_info.sender_address()
        {
            // Add the deleted sequencer balance key to the storage keys.
            let sequencer_balance_low =
                get_fee_token_var_address(block_context.block_info.sequencer_address);
            tx_state_changes_keys
                .storage_keys
                .insert((tx_context.fee_token_address(), sequencer_balance_low));
        }

[meship-starkware]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/state/cached_state.rs line 578 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Instead, create a StateChangesKeys object and use extend

I tried using Extend by creating a new StateChangesKeys and using StateChangesKeys::default(), but it told me that I couldn't use the default because the fields are private.

[meship-starkware]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @avi-starkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/transaction/transaction_execution.rs line 192 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Share code with the worker logic.

Done.

[avi-starkware]

crates/blockifier/src/state/cached_state.rs line 623 at r3 (raw file):

    pub fn update_sequencer_key_in_storage(&mut self, tx_context: &TransactionContext) {
        let seqencer_address = tx_context.block_context.block_info.sequencer_address;
        let seqencer_is_sender = seqencer_address == tx_context.tx_info.sender_address();

Suggestion:

        let sequencer_address = tx_context.block_context.block_info.sequencer_address;
        let sequencer_is_sender = sequencer_address == tx_context.tx_info.sender_address();

[avi-starkware]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @meship-starkware, @noaov1, and @Yoni-Starkware)

[meship-starkware]

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

---

crates/blockifier/src/state/cached_state.rs line 623 at r3 (raw file):

    pub fn update_sequencer_key_in_storage(&mut self, tx_context: &TransactionContext) {
        let seqencer_address = tx_context.block_context.block_info.sequencer_address;
        let seqencer_is_sender = seqencer_address == tx_context.tx_info.sender_address();

Done.

[avi-starkware]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @noaov1 and @Yoni-Starkware)

[Yoni-Starkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @meship-starkware and @noaov1)

---

crates/blockifier/src/state/cached_state.rs line 621 at r4 (raw file):

    }

    pub fn update_sequencer_key_in_storage(&mut self, tx_context: &TransactionContext) {

Pass the fee, and add the key only if the fee is greater than zero

Code quote:

 pub fn update_sequencer_key_in_storage(&mut self, tx_context: &TransactionContext) 

---

crates/blockifier/src/state/cached_state.rs line 623 at r4 (raw file):

    pub fn update_sequencer_key_in_storage(&mut self, tx_context: &TransactionContext) {
        let sequencer_address = tx_context.block_context.block_info.sequencer_address;
        let sequencer_is_sender = sequencer_address == tx_context.tx_info.sender_address();

Move this check to be a function of tx_context, and use it here and in other places if there are

Code quote:

sequencer_address == tx_context.tx_info.sender_address()

---

crates/blockifier/src/state/cached_state.rs line 625 at r4 (raw file):

        let sequencer_is_sender = sequencer_address == tx_context.tx_info.sender_address();
        // Checks if we run in concurrency mode.
        if tx_context.block_context.concurrency_mode && !sequencer_is_sender {

Comments are written in ציווי. Anyway, the comment does not help here.

Suggestion:

        if tx_context.block_context.concurrency_mode && !sequencer_is_sender {

---

crates/blockifier/src/transaction/transaction_execution.rs line 180 at r4 (raw file):

        let tx_execution_summary = tx_execution_info.summarize();
        let mut tx_state_changes_keys = state.get_actual_state_changes()?.into_keys();
        tx_state_changes_keys.update_sequencer_key_in_storage(&tx_context);

Suggestion:

        // types, since now running Transaction::execute_raw is not identical to
        // AccountTransaction::execute_raw.
        let tx_execution_info = match self {
            Self::AccountTransaction(account_tx) => {
                account_tx.execute_raw(state, block_context, charge_fee, validate)?
            }
            Self::L1HandlerTransaction(tx) => {
                tx.execute_raw(state, block_context, charge_fee, validate)?
            }
        };

        // Check if the transaction is too large to fit any block.
        // TODO(Yoni, 1/8/2024): consider caching these two.
        let tx_execution_summary = tx_execution_info.summarize();
        let mut tx_state_changes_keys = state.get_actual_state_changes()?.into_keys();
        tx_state_changes_keys.update_sequencer_key_in_storage(&block_context.to_tx_context(self));

[Yoni-Starkware]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @meship-starkware and @noaov1)

---

crates/blockifier/src/state/cached_state.rs line 621 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Pass the fee, and add the key only if the fee is greater than zero

Or the execution result, whatever is more convenient

[Yoni-Starkware]

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)

---

crates/blockifier/src/context.rs line 21 at r5 (raw file):

        self.block_context.chain_info.fee_token_address(&self.tx_info.fee_type())
    }
    pub fn sequencer_is_the_sender(&self) -> bool {

Suggestion:

is_sequencer_the_sender

[meship-starkware]

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

---

crates/blockifier/src/state/cached_state.rs line 621 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Or the execution result, whatever is more convenient

Done.

---

crates/blockifier/src/state/cached_state.rs line 623 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Move this check to be a function of tx_context, and use it here and in other places if there are

Done.

---

crates/blockifier/src/state/cached_state.rs line 625 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Comments are written in ציווי. Anyway, the comment does not help here.

Done.

---

crates/blockifier/src/transaction/transaction_execution.rs line 180 at r4 (raw file):

        let tx_execution_summary = tx_execution_info.summarize();
        let mut tx_state_changes_keys = state.get_actual_state_changes()?.into_keys();
        tx_state_changes_keys.update_sequencer_key_in_storage(&tx_context);

Done.

[Yoni-Starkware]

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

[noaov1]

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

---

crates/blockifier/src/concurrency/worker_logic.rs line 252 at r6 (raw file):

            // Update the sequencer balance (in state + call info).
            if tx_context.tx_info.sender_address()
                == self.block_context.block_info.sequencer_address

Suggestion:

            if tx_context.is_sequencer_the_sender()

[noaov1]

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

---

crates/blockifier/src/concurrency/worker_logic.rs line 252 at r6 (raw file):

            // Update the sequencer balance (in state + call info).
            if tx_context.tx_info.sender_address()
                == self.block_context.block_info.sequencer_address

Do we want to add fee == 0 here as well?

[Yoni-Starkware]

Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @meship-starkware)

---

crates/blockifier/src/concurrency/worker_logic.rs line 252 at r6 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Do we want to add fee == 0 here as well?

Better not to. You assume that fee==0 -> no fee transfer

[Yoni-Starkware]

Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @meship-starkware)

---

crates/blockifier/src/concurrency/worker_logic.rs line 252 at r6 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Better not to. You assume that fee==0 -> no fee transfer

Which is true now but we gain nothing from this extra assumption here

[noaov1]

Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @Yoni-Starkware)

---

crates/blockifier/src/concurrency/worker_logic.rs line 252 at r6 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Which is true now but we gain nothing from this extra assumption here

We will add the sequencer balance keys to the call info storage read values

[Yoni-Starkware]

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