refactor: move skip validation check to py_validator

[Yael-Starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 7.50000% with 37 lines in your changes missing coverage. Please review.

Project coverage is 79.89%. Comparing base (73c3821) to head (295492c).
Report is 39 commits behind head on main.

Files	Patch %	Lines
crates/native_blockifier/src/py_validator.rs	0.00%	26 Missing ⚠️
...es/blockifier/src/blockifier/stateful_validator.rs	21.42%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2013      +/-   ##
==========================================
- Coverage   80.17%   79.89%   -0.28%     
==========================================
  Files          69       69              
  Lines        9739     9739              
  Branches     9739     9739              
==========================================
- Hits         7808     7781      -27     
- Misses       1471     1501      +30     
+ Partials      460      457       -3     

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

[Yael-Starkware]

python PR for CI testing:
https://github.com/starkware-industries/starkware/pull/35347

Reviewable status: 0 of 3 files reviewed, all discussions resolved

[Yael-Starkware]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion

---

-- commits line 2 at r1:
blocking until python tests run and pass

[ArniStarkware]

-- commits line 2 at r1:

Previously, Yael-Starkware (YaelD) wrote…

blocking until python tests run and pass

link?

[ArniStarkware]

Reviewed 2 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @Yael-Starkware, and @Yoni-Starkware)

---

crates/blockifier/src/blockifier/stateful_validator_test.rs line 0 at r1 (raw file):
A test that checks the skip_validate functionality is missing (And many more tests TBH).

---

crates/native_blockifier/src/py_validator.rs line 79 at r1 (raw file):

                    deploy_account_tx_hash,
                )?;
        }

Suggestion:

        let skip_validate = if let AccountTransaction::Invoke(tx) = &account_tx {
                self.stateful_validator.skip_validate_due_to_unprocessed_deploy_account(
                    tx.tx.sender_address(),
                    tx.tx.nonce(),
                    deploy_account_tx_hash,
                )?
        } else {false}

[Yoni-Starkware]

Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)

---

crates/native_blockifier/src/py_validator.rs line 76 at r1 (raw file):

                self.stateful_validator.skip_validate_due_to_unprocessed_deploy_account(
                    tx.tx.sender_address(),
                    tx.tx.nonce(),

Can you avoid using the raw python tx?

Suggestion:

                    &account_tx.create_tx_info(),

[Yael-Starkware]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)

---

-- commits line 2 at r1:

Previously, ArniStarkware (Arnon Hod) wrote…

link?

was state at the top :
https://reviewable.io/reviews/starkware-industries/starkware/35347

---

crates/blockifier/src/blockifier/stateful_validator_test.rs line at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

A test that checks the skip_validate functionality is missing (And many more tests TBH).

added a test, done

---

crates/native_blockifier/src/py_validator.rs line 76 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Can you avoid using the raw python tx?

done

---

crates/native_blockifier/src/py_validator.rs line 79 at r1 (raw file):

                    deploy_account_tx_hash,
                )?;
        }

done.

[dafnamatsry]

Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @Yoni-Starkware)

---

crates/blockifier/src/blockifier/stateful_validator.rs line 117 at r2 (raw file):

    // `__validate__` method for subsequent transactions for a better user experience.
    // (they will otherwise fail solely because the deploy account hasn't been processed yet).
    pub fn skip_validate_due_to_unprocessed_deploy_account(

I would move this logic as well to py_validator.

Code quote:

pub fn skip_validate_due_to_unprocessed_deploy_account(

---

crates/blockifier/src/blockifier/stateful_validator.rs line 120 at r2 (raw file):

        &mut self,
        sender_address: ContractAddress,
        tx_nonce: Nonce,

Consider passing AccountTrnasaction, and check that this is Invoke here

Code quote:

        sender_address: ContractAddress,
        tx_nonce: Nonce,

---

crates/native_blockifier/src/py_validator.rs line 71 at r2 (raw file):

        let deploy_account_tx_hash = deploy_account_tx_hash.map(|hash| TransactionHash(hash.0));
        // We check if the transaction should be skipped due to the deploy account not being
        // processed.

Suggestion:

        let deploy_account_tx_hash = deploy_account_tx_hash.map(|hash| TransactionHash(hash.0));
        
        // We check if the transaction should be skipped due to the deploy account not being
        // processed.

[Yael-Starkware]

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yoni-Starkware)

---

crates/blockifier/src/blockifier/stateful_validator.rs line 117 at r2 (raw file):

Previously, dafnamatsry wrote…

I would move this logic as well to py_validator.

I tried but this function is touching private fields of stateful_validator so I can't use it outside of the crate. I think it also makes sense logically to keep it there because it exposes the implementation details of the stateful validator.

---

crates/blockifier/src/blockifier/stateful_validator.rs line 120 at r2 (raw file):

Previously, dafnamatsry wrote…

Consider passing AccountTrnasaction, and check that this is Invoke here

done

---

crates/native_blockifier/src/py_validator.rs line 71 at r2 (raw file):

        let deploy_account_tx_hash = deploy_account_tx_hash.map(|hash| TransactionHash(hash.0));
        // We check if the transaction should be skipped due to the deploy account not being
        // processed.

Done.

[ArniStarkware]

Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @dafnamatsry, @Yael-Starkware, and @Yoni-Starkware)

---

-- commits line 2 at r1:

Previously, Yael-Starkware (YaelD) wrote…

was state at the top :
https://reviewable.io/reviews/starkware-industries/starkware/35347

tnx.

---

crates/blockifier/src/blockifier/stateful_validator_test.rs line 91 at r3 (raw file):

        scenario: INVALID,
        ..transaction_args
    });

Suggestion:

    // Create a transaction that does not pass validations.
    let tx = create_account_tx_for_validate_test_nonce_0(FaultyAccountTxCreatorArgs {
        scenario: INVALID,
        tx_type: TransactionType::InvokeFunction,
        tx_version: TransactionVersion::THREE,
        sender_address: faulty_account.get_instance_address(0),
        class_hash: faulty_account.get_class_hash(),
        max_fee: Fee(BALANCE),
        ..Default::default()
    });

---

crates/blockifier/src/blockifier/stateful_validator_test.rs line 95 at r3 (raw file):

    let mut stateful_validator = StatefulValidator::create(state, block_context, nonce!(0_u32));
    // The transaction validations should be skipped and the function should return Ok.
    let result = stateful_validator.perform_validations(tx, true);

Suggestion:

    // The transaction validations should be skipped and the function should return Ok.
    let skip_validate = true;
    let result = stateful_validator.perform_validations(tx, skip_validate);

---

crates/blockifier/src/blockifier/stateful_validator_test.rs line 96 at r3 (raw file):

    // The transaction validations should be skipped and the function should return Ok.
    let result = stateful_validator.perform_validations(tx, true);
    assert!(result.is_ok());

Better error representation.
(If the test fails, it will print out why it failed).

Suggestion:

assert_matches!(result, Ok());

[Yael-Starkware]

Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @Yoni-Starkware)

---

crates/blockifier/src/blockifier/stateful_validator_test.rs line 91 at r3 (raw file):

        scenario: INVALID,
        ..transaction_args
    });

Done.

---

crates/blockifier/src/blockifier/stateful_validator_test.rs line 95 at r3 (raw file):

    let mut stateful_validator = StatefulValidator::create(state, block_context, nonce!(0_u32));
    // The transaction validations should be skipped and the function should return Ok.
    let result = stateful_validator.perform_validations(tx, true);

Done.

---

crates/blockifier/src/blockifier/stateful_validator_test.rs line 96 at r3 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Better error representation.
(If the test fails, it will print out why it failed).

Done.

[Yoni-Starkware]

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @ArniStarkware and @dafnamatsry)

---

crates/blockifier/src/blockifier/stateful_validator.rs line 117 at r2 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I tried but this function is touching private fields of stateful_validator so I can't use it outside of the crate. I think it also makes sense logically to keep it there because it exposes the implementation details of the stateful validator.

You can move max_nonce_for_validation_skip (also not related to the rust class) and expose justget_nonce

[ArniStarkware]

Reviewed 1 of 1 files at r4.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry)

[ArniStarkware]

Reviewed all commit messages.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry)

[dafnamatsry]

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

---

crates/blockifier/src/blockifier/stateful_validator.rs line 117 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

You can move max_nonce_for_validation_skip (also not related to the rust class) and expose justget_nonce

+1 to Yoni's suggestion.

---

crates/blockifier/src/blockifier/stateful_validator.rs line 122 at r4 (raw file):

        account_tx: &AccountTransaction,
        sender_address: ContractAddress,
        tx_nonce: Nonce,

Can these be extracted from account_tx?

Code quote:

        sender_address: ContractAddress,
        tx_nonce: Nonce,

[Yael-Starkware]

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

---

crates/blockifier/src/blockifier/stateful_validator.rs line 117 at r2 (raw file):

Previously, dafnamatsry wrote…

+1 to Yoni's suggestion.

indeed looks better now.
Done.

---

crates/blockifier/src/blockifier/stateful_validator.rs line 122 at r4 (raw file):

Previously, dafnamatsry wrote…

Can these be extracted from account_tx?

Done.

[dafnamatsry]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)

---

crates/blockifier/src/blockifier/stateful_validator.rs line 151 at r5 (raw file):

            .expect(BLOCK_STATE_ACCESS_ERR)
            .get_nonce_at(account_address)?;
        Ok(nonce)

A bit cleaner. non-blocking...

Suggestion:

        self
            .tx_executor
            .block_state
            .as_ref()
            .expect(BLOCK_STATE_ACCESS_ERR)
            .get_nonce_at(account_address);

---

crates/blockifier/src/blockifier/stateful_validator_test.rs line 95 at r3 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Done.

Not critical at all, but IMO the original was better. VSCode does the job of showing the names of the arguments for us.

---

crates/native_blockifier/src/py_validator.rs line 90 at r5 (raw file):

// (they will otherwise fail solely because the deploy account hasn't been processed yet).
pub fn skip_validate_due_to_unprocessed_deploy_account(
    py_validator: &mut PyValidator,

You can define it as a method of PyValidator, and pass self instead.

Code quote:

py_validator: &mut PyValidator,

[Yael-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dafnamatsry)

---

crates/blockifier/src/blockifier/stateful_validator.rs line 151 at r5 (raw file):

Previously, dafnamatsry wrote…

A bit cleaner. non-blocking...

it doesn't work because the error type is different between the value returned from get_nonce_at and get_nonce. Wrapping it with Ok does the job.
Done.

---

crates/blockifier/src/blockifier/stateful_validator_test.rs line 95 at r3 (raw file):

Previously, dafnamatsry wrote…

Not critical at all, but IMO the original was better. VSCode does the job of showing the names of the arguments for us.

Done.

---

crates/native_blockifier/src/py_validator.rs line 90 at r5 (raw file):

Previously, dafnamatsry wrote…

You can define it as a method of PyValidator, and pass self instead.

originally that was what I did and it didn't compile because of the #[pyclass] requirement.

But here since you insisted I found a suitable solution, put in in a different impl clause.

[dafnamatsry]

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

---

crates/native_blockifier/src/py_validator.rs line 90 at r5 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

originally that was what I did and it didn't compile because of the #[pyclass] requirement.

But here since you insisted I found a suitable solution, put in in a different impl clause.

Nice!

[dafnamatsry]

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

[dafnamatsry]

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