test(concurrency): multithreaded flow test for the scheduler

[barak-b-starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.45%. Comparing base (f64dc0e) to head (7fcaa65).

Files	Patch %	Lines
crates/blockifier/src/abi/sierra_types.rs	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1830      +/-   ##
==========================================
- Coverage   78.47%   78.45%   -0.03%     
==========================================
  Files          62       62              
  Lines        8967     8970       +3     
  Branches     8967     8970       +3     
==========================================
  Hits         7037     7037              
- Misses       1483     1486       +3     
  Partials      447      447              

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

[barak-b-starkware]

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

---

crates/blockifier/src/abi/sierra_types.rs line 39 at r3 (raw file):

    fn from_storage(
        state: &mut dyn StateReader,

Done.

---

crates/blockifier/src/concurrency/flow_test.rs line 90 at r2 (raw file):

Previously, avi-starkware wrote…

I just think that when discussing atomic variables, "atomic operations" evokes "operations on atomic variables", and not "operations in a scope where no other thread can access" so it is a bit confusing.
By the way, I think atomic operations actually mean operations that happen in a single CPU step and thus cannot be split.

Done.

---

crates/blockifier/src/concurrency/flow_test.rs line 30 at r3 (raw file):

    // they both read from and write to the same storage entry, this couples the tx_index with the
    // write set of the corresponding tx. However, to make the test more noisy, we determine the
    // write-set dynamically.

Done.

---

crates/blockifier/src/concurrency/flow_test.rs line 54 at r3 (raw file):

                        .unwrap();
                        // Simulate a write operation of a tx.
                        let writes = state_maps_with_single_storage_entry(

Done.

---

crates/blockifier/src/concurrency/flow_test.rs line 73 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

You can be more restrictive - the value must be there in validate phase.

When tx_index == 0 the .get(&(contract_address, storage_key)) returns None. In this case, we need to read this one somehow.
I can change the comment to explain this specific case, WDYT?

---

crates/blockifier/src/concurrency/flow_test.rs line 77 at r3 (raw file):

                        .unwrap();
                        // If tx number tx_index wrote tx_written_value then it must be read
                        // tx_written_value - 1.

Done.

---

crates/blockifier/src/concurrency/flow_test.rs line 130 at r3 (raw file):

fn state_maps_with_single_storage_entry(
    // tx_index: TxIndex,
    contract_address: ContractAddress,

Done.

[avi-starkware]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @barak-b-starkware, @noaov1, and @Yoni-Starkware)

[Yoni-Starkware]

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

---

crates/blockifier/src/concurrency/flow_test.rs line 73 at r3 (raw file):

Previously, barak-b-starkware wrote…

When tx_index == 0 the .get(&(contract_address, storage_key)) returns None. In this case, we need to read this one somehow.
I can change the comment to explain this specific case, WDYT?

Are you sure? It should return 1

[Yoni-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @barak-b-starkware and @noaov1)

---

crates/blockifier/src/concurrency/flow_test.rs line 73 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Are you sure? It should return 1

Unblocking because I'm OOO next week, but I'm pretty sure it should be fine

[avi-starkware]

Reviewed 4 of 5 files at r5, all commit messages.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @barak-b-starkware and @noaov1)

[avi-starkware]

Reviewed 1 of 5 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @barak-b-starkware and @noaov1)

[Yoni-Starkware]

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

[barak-b-starkware]

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

---

crates/blockifier/src/concurrency/flow_test.rs line 94 at r3 (raw file):

Previously, avi-starkware wrote…

We changed the method finish_validation to finish_abort (to be used only if aborted == true)

Done.

[Yoni-Starkware]

Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @avi-starkware, @barak-b-starkware, and @noaov1)

---

crates/blockifier/src/concurrency/flow_test.rs line 64 at r6 (raw file):

                        }
                    }
                }

This is kind of duplicated and not relevant for this test, since nothing special is happening in the commit phase, but I can't think of an elegant halt condition

Code quote:

                if let Some(mut transaction_committer) = scheduler.try_enter_commit_phase() {
                    while let Some(tx_index) = transaction_committer.try_commit() {
                        let mut state_proxy = versioned_state.pin_version(tx_index);
                        let (reads, writes) =
                            get_reads_writes_for(Task::ValidationTask(tx_index), &versioned_state);
                        let reads_valid = state_proxy.validate_reads(&reads);
                        if !reads_valid {
                            state_proxy.delete_writes(&writes, &ContractClassMapping::default());
                            let (_, new_writes) = get_reads_writes_for(
                                Task::ExecutionTask(tx_index),
                                &versioned_state,
                            );
                            state_proxy.apply_writes(
                                &new_writes,
                                &ContractClassMapping::default(),
                                &HashMap::default(),
                            );
                            scheduler.finish_execution_during_commit(tx_index);
                        }
                    }
                }

---

crates/blockifier/src/concurrency/flow_test.rs line 180 at r6 (raw file):

    contract_address: ContractAddress,
    storage_key: StorageKey,
    value: u128,

Make the contract_address and the storage_key constant

Suggestion:

fn state_maps_with_single_value(value: u128)

[barak-b-starkware]

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

---

crates/blockifier/src/concurrency/flow_test.rs line 64 at r6 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

This is kind of duplicated and not relevant for this test, since nothing special is happening in the commit phase, but I can't think of an elegant halt condition

Why is it "kind of duplicated"?
I think it's relevant for this test because it aims to check all pub functions of the scheduler (and the transaction committer) together in one flow. And in this block, you have:

1. scheduler.try_enter_commit_phase
2. transaction_committer.try_commit
3. scheduler.finish_execution_during_commit

BTW, when running the test, you do get "re-executions" from the if !reads_valid block in the above quote.

---

crates/blockifier/src/concurrency/flow_test.rs line 180 at r6 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Make the contract_address and the storage_key constant

Done.
Notice that we can't use the fixture of contrat_address that way (I think it's a good suggestion to use the constant).
Maybe we can share the string from test_utils, instead of:

#[fixture]
pub fn contract_address() -> ContractAddress {
    contract_address!("0x18031991")
}

We could use the following:

const CONTRACT_ADDRESS: &str = "0x18031991";

#[fixture]
pub fn contract_address() -> ContractAddress {
    contract_address!(CONTRACT_ADDRESS)
}

LMK if you prefer this option

[Yoni-Starkware]

Reviewable status: 1 of 6 files reviewed, all discussions resolved (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/concurrency/flow_test.rs line 64 at r6 (raw file):

Previously, barak-b-starkware wrote…

Why is it "kind of duplicated"?
I think it's relevant for this test because it aims to check all pub functions of the scheduler (and the transaction committer) together in one flow. And in this block, you have:

1. scheduler.try_enter_commit_phase
2. transaction_committer.try_commit
3. scheduler.finish_execution_during_commit

BTW, when running the test, you do get "re-executions" from the if !reads_valid block in the above quote.

In the sense that nothing new is happening in the commit phase (e.g., bouncer).
the following halt condition makes the commit logic unnecessary:
"all tasks were executed and validated"

The worker's commit also has annoying code duplication - it repeats validate and re-execute with small changes.

But I don't have a better solution, and as you said, you add coverage to the commit methods.

[barak-b-starkware]

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

---

crates/blockifier/src/concurrency/flow_test.rs line 64 at r6 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

In the sense that nothing new is happening in the commit phase (e.g., bouncer).
the following halt condition makes the commit logic unnecessary:
"all tasks were executed and validated"

The worker's commit also has annoying code duplication - it repeats validate and re-execute with small changes.

But I don't have a better solution, and as you said, you add coverage to the commit methods.

Got it, so you want to merge it as is?

[barak-b-starkware]

crates/blockifier/src/concurrency/flow_test.rs line 64 at r6 (raw file):

Previously, barak-b-starkware wrote…

Got it, so you want to merge it as is?

Alternatives:

1. Try to add the bouncer logic to this test (I think we shouldn't do that because even without the bouncer, you do check the re-execution flow of the commit mechanism)
2. I can think about how to eliminate the code duplication in the worker, and maybe the solution there can also be implemented in the test.

[Yoni-Starkware]

Reviewable status: 1 of 6 files reviewed, all discussions resolved (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/concurrency/flow_test.rs line 64 at r6 (raw file):

Previously, barak-b-starkware wrote…

Alternatives:

1. Try to add the bouncer logic to this test (I think we shouldn't do that because even without the bouncer, you do check the re-execution flow of the commit mechanism)
2. I can think about how to eliminate the code duplication in the worker, and maybe the solution there can also be implemented in the test.

We can merge it as is for now. I didn't approve since I didn't look at the entire code yet (if someone else approves, you can merge)

[Yoni-Starkware]

Reviewed 3 of 5 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @barak-b-starkware and @noaov1)

---

crates/blockifier/src/concurrency/versioned_state.rs line 248 at r7 (raw file):

    pub fn state(&self) -> LockedVersionedState<'_, S> {
        self.0.lock().expect("Failed to acquire state lock.")
    }

Use use it only once.
Remove and use into_inner_state instead (or .lock().unwrap() if something goes wrong)

Code quote:

    #[cfg(test)]
    pub fn state(&self) -> LockedVersionedState<'_, S> {
        self.0.lock().expect("Failed to acquire state lock.")
    }

[Yoni-Starkware]

One last comment

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @barak-b-starkware and @noaov1)

[barak-b-starkware]

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

---

crates/blockifier/src/concurrency/versioned_state.rs line 248 at r7 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Use use it only once.
Remove and use into_inner_state instead (or .lock().unwrap() if something goes wrong)

Done.

[Yoni-Starkware]

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