feat(concurrency): implement execute_chunk

[barak-b-starkware]

This change is 

[barak-b-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 199 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Let's be consistent - either `commit_index` (non-inclusive) or `chunk_size`/`n_committed_txs` everywhere (non-blocking for this PR) I vote for `commit_index`

Done. here

[barak-b-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 57 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Is this necessary? (same re stateful validator) If so, please move `execute_chunk` to a separate impl block

It is necessary, If you want another impl block we need to also move execute_txs that calls to execute_chunk, so we would get one block with execute_txs and execute_chunk and another block with the rest, WDYT?

[avi-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 200 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Why not? Sounds like you should use `replace` if you care about the old value. In our case, it must always be None.

It's not exactly the same thing. replace (see docs) puts the value inside the existing Some wrapper without dropping it. Your suggestion will drop the old Some and create a new wrapper. I'm not sure it is significant, but the reciprocal operation of take is replace, so I think what barak did is the correct way to do it.

[avi-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 199 at r3 (raw file):

                )
            })
            .commit_chunk_and_recover_block_state(commit_index);

This is incorrect and confusing. See my suggestion for the correct code.

Note:

• The array tx_execution_results should be of length n_committed_txs. The current code makes it of length n_committed_txs - 1.
• I think we should use a variable named something like last_committed_tx_index to make it clear what the argument provided to commit_chunk_and_recover_block_state needs to be. Just putting - 1 in the input is unclear.

Suggestion:

        let n_committed_txs = worker_executor_factory.scheduler.get_n_committed_txs();
        let tx_execution_results = worker_executor_factory
            .execution_outputs
            .iter()
            .fold_while(Vec::new(), |mut results, execution_output| {
                if results.len() > n_committed_txs {
                    Done(results)
                } else {
                    let locked_execution_output = execution_output
                        .lock()
                        .expect("Failed to lock execution output.")
                        .take()
                        .expect("Output must be ready.");
                    results.push(
                        locked_execution_output.result.map_err(TransactionExecutorError::from),
                    );
                    Continue(results)
                }
            })
            .into_inner();

        let last_committed_tx_index = n_committed_txs - 1
        let block_state_after_commit = Arc::try_unwrap(worker_executor_factory)
            .unwrap_or_else(|_| {
                panic!(
                    "To consume the block state, you must have only one strong reference to the \
                     worker executor factory. Consider dropping objects that hold a reference to \
                     it."
                )
            })
            .commit_chunk_and_recover_block_state(last_committed_tx_index);

[avi-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 262 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

No?

You are correct Noa

[avi-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 283 at r5 (raw file):

            .into_inner();

        let last_committed_tx_index = n_committed_txs - 1;

Maybe put it inside commit_chunk_and_recover_block_state (condition and variable)?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 33.33333% with 16 lines in your changes missing coverage. Please review.

Project coverage is 78.47%. Comparing base (441c27f) to head (bfc00d7).

Files	Patch %	Lines
.../blockifier/src/blockifier/transaction_executor.rs	33.33%	16 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1953 +/- ## ======================================= Coverage 78.47% 78.47% ======================================= Files 62 62 Lines 8845 8845 Branches 8845 8845 ======================================= Hits 6941 6941 Misses 1467 1467 Partials 437 437 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[avi-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 262 at r10 (raw file):

            concurrent_block_context,
            Mutex::new(&mut self.bouncer),
        ));

As I said offline, I don't think "factory" fits here. You have a single WorkerExecutor instance and each thread gets a pointer to it. The word factory implies that the object generates other objects and it does not.

Nonblocking

Suggestion:

        let worker_executor = Arc::new(WorkerExecutor::initialize(
            block_state,
            chunk,
            concurrent_block_context,
            Mutex::new(&mut self.bouncer),
        ));

[avi-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 306 at r10 (raw file):

                     it."
                )
            })

Suggestion:

            .except(
                "To consume the block state, you must have only one strong reference to the \
                worker executor factory. Consider dropping objects that hold a reference to \
                it."
            )

[avi-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 306 at r10 (raw file):

Previously, barak-b-starkware wrote…

The `except` method requires deriving `Debug` for the `WorkerExecutor`, the `ThreadSafeVersionedState`, the `PapyrusReader`, and the `StorageReader`. The last one is from the `papyrus_storage` crate and doesn't derive `Debug` there. It also requires `+ std::fmt::Debug` in the second `impl` block of the `TransactionExecutor`. What would you like to do?

Keep it as it was. I thought it is a small change.

[barak-b-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 253 at r10 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Where there any tests that did not do it? I think that yes. If concurrency mode is not enabled, then we will not want to execute the transactions concurrently, no?

I'm not sure. The "clone and field changing" that I've added here is not the best solution for what we want, but it's the shorter one. I thought you might want to merge it today, so I chose the shorter one with a TODO for the "block_context passing" issue. I'll open a new PR with a nicer solution, and I'll rebase this one on top of it (or the opposite). I'll ping you once it's ready.

[barak-b-starkware]

crates/blockifier/src/blockifier/transaction_executor.rs line 253 at r10 (raw file):

Previously, barak-b-starkware wrote…

I'm not sure. The "clone and field changing" that I've added here is not the best solution for what we want, but it's the shorter one. I thought you might want to merge it today, so I chose the shorter one with a TODO for the "block_context passing" issue. I'll open a new PR with a nicer solution, and I'll rebase this one on top of it (or the opposite). I'll ping you once it's ready.

New PR to eliminate the comment and the clone.