test(concurrency): test scenarios of validate_reads with unvalid reads

[OriStarkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.42%. Comparing base (6e06e79) to head (72c30b8).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1955   +/-   ##
=======================================
  Coverage   78.42%   78.42%           
=======================================
  Files          62       62           
  Lines        8913     8913           
  Branches     8913     8913           
=======================================
  Hits         6990     6990           
  Misses       1476     1476           
  Partials      447      447           

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

[meship-starkware]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @barak-b-starkware, @noaov1, @OriStarkware, and @Yoni-Starkware)

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 350 at r1 (raw file):

            declared_contracts: HashMap::from([(class_hash, true)]),
            ..Default::default()
        },

It's the same test as the one below. (7, 8)

[OriStarkware]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @barak-b-starkware, @meship-starkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 350 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

It's the same test as the one below. (7, 8)

Done.

[meship-starkware]

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

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 365 at r2 (raw file):

        TransactionalState::create_transactional(&mut version_state_proxy_5);

    transactional_state_5.get_class_hash_at(contract_address).unwrap();

This line is to get the contract_address in the read set of transactional_state_5?

[OriStarkware]

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

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 365 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

This line is to get the contract_address in the read set of transactional_state_5?

Exactly

[meship-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @barak-b-starkware, @noaov1, and @Yoni-Starkware)

The base branch was changed.

[meship-starkware]

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

[meship-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @barak-b-starkware, @noaov1, and @Yoni-Starkware)

[barak-b-starkware]

Reviewed 1 of 2 files at r1, 1 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1 and @Yoni-Starkware)

[barak-b-starkware]

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

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 331 at r4 (raw file):

    contract_address: ContractAddress,
    safe_versioned_state: ThreadSafeVersionedState<CachedState<DictStateReader>>,
) {

Can you try applying different cases with arg1 for the state maps and arg2 for the contract class mapping and maintain the body of the test something like:

let proxy_for_writing = safe_versioned_State.pin_version(0);
let proxy_for_reading = safe_versioned_State.pin_version(1);
let transactional_state_for_reading = ...;

proxy_for_writing.state().apply_writes(0, &arg1, &arg2);
assert(!proxy_for_reading.validate_reads(&state_maps);

Suggestion:

#[rstest]
#[case::get_storage_at(...)]
#[case::get_nonce(...)]
.
.
.
fn test_false_validate_reads(
    contract_address: ContractAddress,
    safe_versioned_state: ThreadSafeVersionedState<CachedState<DictStateReader>>,
    #[case] arg1,
    #[case] arg2,
) {

[OriStarkware]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @barak-b-starkware, @meship-starkware, @noaov1, and @Yoni-Starkware)

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 331 at r4 (raw file):

Previously, barak-b-starkware wrote…

Can you try applying different cases with arg1 for the state maps and arg2 for the contract class mapping and maintain the body of the test something like:

let proxy_for_writing = safe_versioned_State.pin_version(0);
let proxy_for_reading = safe_versioned_State.pin_version(1);
let transactional_state_for_reading = ...;

proxy_for_writing.state().apply_writes(0, &arg1, &arg2);
assert(!proxy_for_reading.validate_reads(&state_maps);

Done.

[meship-starkware]

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

[barak-b-starkware]

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

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 328 at r5 (raw file):

#[rstest]
#[case::check_declared_contracts(

In all cases:

Suggestion:

#[case::declared_contracts(

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 372 at r5 (raw file):

    }
)]
fn test_false_validate_reads(

What about the "compiled contract classes validation"? Can it be added as another case here? Maybe adding another test for that?

Code quote:

fn test_false_validate_reads(

[OriStarkware]

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

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 328 at r5 (raw file):

Previously, barak-b-starkware wrote…

In all cases:

Done.

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 372 at r5 (raw file):

Previously, barak-b-starkware wrote…

What about the "compiled contract classes validation"? Can it be added as another case here? Maybe adding another test for that?

Done.

[barak-b-starkware]

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

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 381 at r6 (raw file):

        ..Default::default()
    }
)]

I would expect this case to panic for the assertion:

            assert_eq!(
                is_declared,
                self.compiled_contract_classes.read(tx_index + 1, class_hash).is_some()
            );

what do I miss here?

Code quote:

#[case::declared_contracts(
    StateMaps {
        declared_contracts: HashMap::from([(class_hash!(1_u8), false)]),
        ..Default::default()
    },
    StateMaps {
        declared_contracts: HashMap::from([(class_hash!(1_u8), true)]),
        ..Default::default()
    }
)]

[OriStarkware]

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

---

crates/blockifier/src/concurrency/versioned_state_test.rs line 381 at r6 (raw file):

Previously, barak-b-starkware wrote…

I would expect this case to panic for the assertion:

            assert_eq!(
                is_declared,
                self.compiled_contract_classes.read(tx_index + 1, class_hash).is_some()
            );

what do I miss here?

The code will not panic because the state is ok.

[barak-b-starkware]

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

---

-- commits line 5 at r1:
Two commits by mistake?

Code quote:

- 50f7c2a: fix(concurrency): remove compiled_class_hashes check from validate_reads

- c4f43f5: test(concurrency): test scenarios of validate_reads with unvalid reads

[OriStarkware]

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

---

-- commits line 5 at r1:

Previously, barak-b-starkware wrote…

Two commits by mistake?

I don't see 2 commits.

[barak-b-starkware]

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