chore(concurrency): complete commit_tx - post re-execution, delete writes and invoke revalidate

[avi-starkware]

This change is 

[noaov1]

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

---

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

    fn commit_tx(&self, tx_index: TxIndex) -> StateResult<bool> {
        let execution_output = lock_mutex_in_array(&self.execution_outputs, tx_index);
        let execution_output_ref = execution_output.as_ref().expect(EXECUTION_OUTPUTS_UNWRAP_ERROR);

Suggestion:

        let execution_output = execution_output.as_ref().expect(EXECUTION_OUTPUTS_UNWRAP_ERROR);

---

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

            let execution_output = lock_mutex_in_array(&self.execution_outputs, tx_index);
            let read_set = &execution_output.as_ref().expect(EXECUTION_OUTPUTS_UNWRAP_ERROR).reads;
            self.scheduler.finish_execution_during_commit(tx_index);

Suggestion:

            self.execute_tx(tx_index);
            self.scheduler.finish_execution_during_commit(tx_index);
            let execution_output = lock_mutex_in_array(&self.execution_outputs, tx_index);
            let read_set = &execution_output.as_ref().expect(EXECUTION_OUTPUTS_UNWRAP_ERROR).reads;

---

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

        } else {
            drop(execution_output);
        }

Is it working?
Dropping an already dropped variable.

Suggestion:

            drop(execution_output);

---

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

            if tx_context.tx_info.sender_address()
                != self.block_context.block_info.sequencer_address
            {

Suggestion:

        if let Ok(tx_info) = result_tx_info.as_mut() {
            // TODO(Meshi, 01/06/2024): ask the bouncer if there is enough room for the transaction.
            // Update the sequencer balance.
            // There is no need to update the balance when the sequencer transfers fee to itself, since we
            // use the sequential fee transfer in this case.
            if tx_context.tx_info.sender_address()
                != self.block_context.block_info.sequencer_address
            {

---

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

                    sequencer_balance_value_low,
                    sequencer_balance_value_high,
                );

Suggestion:

                    sequencer_balance_value_high,
                );
                // Changing the sequencer balance storage cell does not trigger (re-)validation of
                // the next transactions.

[avi-starkware]

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

---

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

    fn commit_tx(&self, tx_index: TxIndex) -> StateResult<bool> {
        let execution_output = lock_mutex_in_array(&self.execution_outputs, tx_index);
        let execution_output_ref = execution_output.as_ref().expect(EXECUTION_OUTPUTS_UNWRAP_ERROR);

The drop doesn't work if I do that:

calls to `std::mem::drop` with a reference instead of an owned value does nothing
use `let _ = ...` to ignore the expression or result
`-D dropping-references` implied by `-D warnings`
to override `-D warnings` add `#[allow(dropping_references)]`rustcClick for full compiler diagnostic
worker_logic.rs(178, 18): argument has type `&concurrency::worker_logic::ExecutionTaskOutput`

---

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

Previously, noaov1 (Noa Oved) wrote…

Is it working?
Dropping an already dropped variable.

It doesn't work:

use of moved value: `execution_output`
value used here after moverustcClick for full compiler diagnostic
worker_logic.rs(180, 18): value moved here
worker_logic.rs(164, 13): move occurs because `execution_output` has type `std::sync::MutexGuard<'_, std::option::Option<concurrency::worker_logic::ExecutionTaskOutput>>`, which does not implement the `Copy` trait

---

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

            if tx_context.tx_info.sender_address()
                != self.block_context.block_info.sequencer_address
            {

The comment was already fixed in Meshi's open PR (#1898).
Changing it here will create a conflict.

I reworded the TODO.

---

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

                    sequencer_balance_value_low,
                    sequencer_balance_value_high,
                );

Done.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.04%. Comparing base (966013b) to head (fc7d538).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1916   +/-   ##
=======================================
  Coverage   78.04%   78.04%           
=======================================
  Files          61       61           
  Lines        8759     8759           
  Branches     8759     8759           
=======================================
  Hits         6836     6836           
  Misses       1485     1485           
  Partials      438      438           

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

[noaov1]

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

---

-- commits line 2 at r2:

Suggestion:

- 022616f: chore(concurrency): complete commit_tx: post re-execution: delete writes and invoke re-validate.

---

crates/blockifier/src/concurrency/worker_logic.rs line 180 at r2 (raw file):

                &execution_output_ref.contract_classes,
            );
            drop(execution_output);

Suggestion:

            );
            // Release the execution output lock as it is acquired in execution (avoid dead-lock)
            drop(execution_output);

[meship-starkware]

Reviewed 1 of 2 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware)

---

crates/blockifier/src/concurrency/worker_logic.rs line 171 at r2 (raw file):

        let reads = &execution_output_ref.reads;
        let reads_valid = tx_versioned_state.validate_reads(reads);

If we are dropping execution_output inside the condition there is no need for reads_valid we can just write
if !tx_versioned_state.validate_reads(reads):

---

crates/blockifier/src/concurrency/worker_logic.rs line 231 at r2 (raw file):

                );
                // Changing the sequencer balance storage cell does not trigger (re-)validation of
                // the next transactions.

Is this working because any transaction that updates the sequencer balance (outside fee transfer) will fail in the commit re-validation?

[meship-starkware]

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware)

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic.rs line 231 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Is this working because any transaction that updates the sequencer balance (outside fee transfer) will fail in the commit re-validation?

I think validation does not include this storage cell, so there is no way for it to trigger invalidation.

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic.rs line 171 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

If we are dropping execution_output inside the condition there is no need for reads_valid we can just write
if !tx_versioned_state.validate_reads(reads):

I think it's cleaner with the reads_valid variable.

[avi-starkware]

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

---

-- commits line 2 at r2:
Done.

---

crates/blockifier/src/concurrency/worker_logic.rs line 180 at r2 (raw file):

                &execution_output_ref.contract_classes,
            );
            drop(execution_output);

Done.

[meship-starkware]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @noaov1)

[meship-starkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noaov1)

[noaov1]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @meship-starkware)

[avi-starkware]

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