feat: skip validation of an invoke transaction after deploy account

[Yael-Starkware]

This change is 

[Yael-Starkware]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @dafnamatsry)

---

Cargo.toml line 39 at r1 (raw file):

# TODO(YaelD, 28/5/2024): The special Blockifier version is needed in order to be aligned with the
# starknet-api version. This should be removed once we have a mono-repo.
blockifier = { git = "https://github.com/starkware-libs/blockifier.git", rev = "3511cd37" }

Do not merge until the blockifier commit is pushed to main-mempool!
also revert this change!

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.66%. Comparing base (2c64fcd) to head (2522f8b).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...ates/gateway/src/stateful_transaction_validator.rs	87.50%	0 Missing and 2 partials ⚠️
crates/gateway/src/gateway.rs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #345      +/-   ##
==========================================
- Coverage   83.69%   83.66%   -0.04%     
==========================================
  Files          37       37              
  Lines        1717     1726       +9     
  Branches     1717     1726       +9     
==========================================
+ Hits         1437     1444       +7     
  Misses        203      203              
- Partials       77       79       +2     

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

[Yael-Starkware]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @dafnamatsry)

---

Cargo.toml line 39 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Do not merge until the blockifier commit is pushed to main-mempool!
also revert this change!

The PR that should be merge before this one: https://reviewable.io/reviews/starkware-libs/blockifier/2034#-

[ArniStarkware]

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

---

crates/gateway/src/stateful_transaction_validator.rs line 72 at r1 (raw file):

            self.config.max_nonce_for_validation_skip,
        )?;
        println!("skip_validate: {:?}", skip_validate);

Delete, please.

Code quote:

        println!("skip_validate: {:?}", skip_validate);

---

crates/gateway/src/stateful_transaction_validator_test.rs line 103 at r1 (raw file):

    external_invoke_tx(invoke_tx_args!{nonce: Nonce(StarkFelt::TWO)}),
    false
)]

Suggestion:

#[case::should_not_skip_validation_nonce_over_max_nonce_for_skip(
    external_invoke_tx(invoke_tx_args!{nonce: Nonce(StarkFelt::TWO)}),
    false
)]

---

crates/gateway/src/stateful_transaction_validator_test.rs line 104 at r1 (raw file):

    false
)]
#[case::should_not_skip_validation_deploy_account(deploy_account_tx(), false)]

What do you think?

Suggestion:

#[case::should_not_skip_validation_non_invoke(deploy_account_tx(), false)]

---

crates/gateway/src/stateful_transaction_validator_test.rs line 129 at r1 (raw file):

        assert!(result.is_ok());
    } else {
        assert!(result.is_err());

Do we want to make sure the error is as expected?
Is there an error dedicated to skip validation?

Code quote:

        assert!(result.is_err());

[Yael-Starkware]

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

---

crates/gateway/src/stateful_transaction_validator.rs line 72 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Delete, please.

Done.

---

crates/gateway/src/stateful_transaction_validator_test.rs line 103 at r1 (raw file):

    external_invoke_tx(invoke_tx_args!{nonce: Nonce(StarkFelt::TWO)}),
    false
)]

Done.

---

crates/gateway/src/stateful_transaction_validator_test.rs line 104 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

What do you think?

yes done

---

crates/gateway/src/stateful_transaction_validator_test.rs line 129 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Do we want to make sure the error is as expected?
Is there an error dedicated to skip validation?

skip_validate doesn't return an error, since validations are skipped it returns ok at (almost) all times.

The exact error returned from the blockifier is very complex and I'm not Sure we want to get into the implementation details of the blockifier, but I did add a check for the general type.
wdyt??

[ArniStarkware]

crates/gateway/src/stateful_transaction_validator_test.rs line 129 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

skip_validate doesn't return an error, since validations are skipped it returns ok at (almost) all times.

The exact error returned from the blockifier is very complex and I'm not Sure we want to get into the implementation details of the blockifier, but I did add a check for the general type.
wdyt??

I like it.

[Yael-Starkware]

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

---

Cargo.toml line 39 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

The PR that should be merge before this one: https://reviewable.io/reviews/starkware-libs/blockifier/2034#-

done

[dafnamatsry]

Reviewed 2 of 5 files at r1, 1 of 2 files at r2.
Reviewable status: 2 of 5 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware and @Yael-Starkware)

---

crates/gateway/src/stateful_transaction_validator.rs line 58 at r1 (raw file):

            state,
            block_context,
            self.config.max_nonce_for_validation_skip,

Do we need to keep this config? Is it ever going to be not 1?

Code quote:

self.config.max_nonce_for_validation_skip,

---

crates/gateway/src/stateful_transaction_validator.rs line 85 at r3 (raw file):

    max_nonce_for_validation_skip: Nonce,
) -> StatefulTransactionValidatorResult<bool> {
    if let RPCTransaction::Invoke(RPCInvokeTransaction::V3(tx)) = tx {

Consider using match case here:

match tx {
    Declare | Deploy => false,
    Invoke(V3(tx)) => { ... }
}

This way we make sure future invoke versions won't fall under the false case.

[Yael-Starkware]

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

---

crates/gateway/src/stateful_transaction_validator.rs line 58 at r1 (raw file):

Previously, dafnamatsry wrote…

Do we need to keep this config? Is it ever going to be not 1?

I don't think it will ever be not 1, but still didn't want to close the option , you think we should delete it completely?

---

crates/gateway/src/stateful_transaction_validator.rs line 85 at r3 (raw file):

Previously, dafnamatsry wrote…

Consider using match case here:

match tx {
    Declare | Deploy => false,
    Invoke(V3(tx)) => { ... }
}

This way we make sure future invoke versions won't fall under the false case.

done.
it is a bit longer now.

[Yael-Starkware]

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

---

-- commits line 2 at r4:
branch needs to be rebase the it can't work due to sync issues between blockifier and papyrus, waiting for a fix from Papyrus

[dafnamatsry]

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

---

crates/gateway/src/stateful_transaction_validator.rs line 58 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

I don't think it will ever be not 1, but still didn't want to close the option , you think we should delete it completely?

Ok, let's keep it.

---

crates/gateway/src/stateful_transaction_validator.rs line 39 at r4 (raw file):

        // StatefulTransactionValidator and update it only once a new block is created.
        let latest_block_info = get_latest_block_info(state_reader_factory)?;
        let prev_block_number = latest_block_info.block_number;

I think latest is more accurate.

Code quote:

prev_block_number

---

crates/gateway/src/stateful_transaction_validator.rs line 71 at r4 (raw file):

        let skip_validate = skip_stateful_validations(
            external_tx,
            &state_reader_factory.get_state_reader(prev_block_number),

Just pass the account nonce using validator.get_nonce(tx.sender_address)

Code quote:

&state_reader_factory.get_state_reader(prev_block_number)

---

crates/gateway/src/stateful_transaction_validator.rs line 100 at r4 (raw file):

            } else {
                Ok(false)
            }

Suggestion:

            Ok(tx.nonce >= Nonce(StarkFelt::ONE)
                && tx.nonce <= max_nonce_for_validation_skip
                && account_nonce == Nonce(StarkFelt::ZERO))

---

crates/gateway/src/stateful_transaction_validator.rs line 103 at r4 (raw file):

        }
        RPCTransaction::DeployAccount(RPCDeployAccountTransaction::V3(_))
        | RPCTransaction::Declare(RPCDeclareTransaction::V3(_)) => Ok(false),

Suggestion:

        RPCTransaction::DeployAccount(_)
        | RPCTransaction::Declare(_) => Ok(false),

---

crates/gateway/src/stateful_transaction_validator_test.rs line 96 at r4 (raw file):

}

#[rstest]

Please add a test case for account nonce != 0 (i.e. account nonce == tx nonce == 1).

---

crates/gateway/src/stateful_transaction_validator_test.rs line 110 at r4 (raw file):

    #[case] should_pass_validation: bool,
) {
    let block_context = &BlockContext::create_for_testing();

Please put it in a test fixture, and use it in the other test as well.

Code quote:

let block_context = &BlockContext::create_for_testing();

---

crates/gateway/src/stateful_transaction_validator_test.rs line 125 at r4 (raw file):

            chain_info: block_context.chain_info().clone().into(),
        },
    };

Same. I think it can be a in test fixture.

Code quote:

    let stateful_validator = StatefulTransactionValidator {
        config: StatefulTransactionValidatorConfig {
            max_nonce_for_validation_skip: Nonce(StarkFelt::ONE),
            validate_max_n_steps: block_context.versioned_constants().validate_max_n_steps,
            max_recursion_depth: block_context.versioned_constants().max_recursion_depth,
            chain_info: block_context.chain_info().clone().into(),
        },
    };

---

crates/gateway/src/stateful_transaction_validator_test.rs line 134 at r4 (raw file):

        // executed also when skip_validate is true.
        assert_matches!(result, Err(StatefulTransactionValidatorError::StatefulValidatorError(err)) 
            if !matches!(err, StatefulValidatorError::TransactionPreValidationError(_)));

@yair-starkware is adding a mock blockifier validator to be used in this test. Once it is added you can just check that validate was called or not.

Code quote:

    if should_pass_validation {
        assert_matches!(result, Ok(_));
    } else {
        // To be sure that the validations were actually skipped, we check that the error came from
        // the blockifier stateful validations, and not from the pre validations since those are
        // executed also when skip_validate is true.
        assert_matches!(result, Err(StatefulTransactionValidatorError::StatefulValidatorError(err))
            if !matches!(err, StatefulValidatorError::TransactionPreValidationError(_)));

[Yael-Starkware]

Reviewable status: 3 of 7 files reviewed, 13 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @yair-starkware)

---

crates/gateway/src/stateful_transaction_validator.rs line 39 at r4 (raw file):

Previously, dafnamatsry wrote…

I think latest is more accurate.

Done.

---

crates/gateway/src/stateful_transaction_validator.rs line 71 at r4 (raw file):

Previously, dafnamatsry wrote…

Just pass the account nonce using validator.get_nonce(tx.sender_address)

Done.

---

crates/gateway/src/stateful_transaction_validator.rs line 100 at r4 (raw file):

            } else {
                Ok(false)
            }

Done.

---

crates/gateway/src/stateful_transaction_validator.rs line 103 at r4 (raw file):

        }
        RPCTransaction::DeployAccount(RPCDeployAccountTransaction::V3(_))
        | RPCTransaction::Declare(RPCDeclareTransaction::V3(_)) => Ok(false),

Done.

---

crates/gateway/src/stateful_transaction_validator_test.rs line 96 at r4 (raw file):

Previously, dafnamatsry wrote…

Please add a test case for account nonce != 0 (i.e. account nonce == tx nonce == 1).

Done.

---

crates/gateway/src/stateful_transaction_validator_test.rs line 110 at r4 (raw file):

Previously, dafnamatsry wrote…

Please put it in a test fixture, and use it in the other test as well.

Turns out blockifier has this fixure, I used it in the stateful _validator fixture.

---

crates/gateway/src/stateful_transaction_validator_test.rs line 125 at r4 (raw file):

Previously, dafnamatsry wrote…

Same. I think it can be a in test fixture.

Done.

---

crates/gateway/src/stateful_transaction_validator_test.rs line 134 at r4 (raw file):

Previously, dafnamatsry wrote…

@yair-starkware is adding a mock blockifier validator to be used in this test. Once it is added you can just check that validate was called or not.

validate is always called, but with a skip_validate flag, since there are some un-skipped pre-validation checks. maybe the mock can also say if validation was skipped or not.

[dafnamatsry]

Reviewed 4 of 4 files at r5.
Reviewable status: 6 of 7 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @Yael-Starkware, and @yair-starkware)

---

crates/gateway/src/stateful_transaction_validator_test.rs line 134 at r4 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

validate is always called, but with a skip_validate flag, since there are some un-skipped pre-validation checks. maybe the mock can also say if validation was skipped or not.

I believe that using the mock, it is possible to check the value of skip_validate passed to validate.

---

crates/gateway/src/stateful_transaction_validator_test.rs line 145 at r5 (raw file):

}

fn empty_state_reader_factory() -> TestStateReaderFactory {

what is the difference between this and local_test_state_reader_factory?
I think there are enough use cases to generalise the state reader factory initialisation in tests.

Having said that, with the mock blockifier validator, it would probably not be necessary at all.

Code quote:

empty_state_reader_factory()

[Yael-Starkware]

Reviewable status: 6 of 7 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @yair-starkware)

---

crates/gateway/src/stateful_transaction_validator_test.rs line 134 at r4 (raw file):

Previously, dafnamatsry wrote…

I believe that using the mock, it is possible to check the value of skip_validate passed to validate.

awesome!

---

crates/gateway/src/stateful_transaction_validator_test.rs line 145 at r5 (raw file):

Previously, dafnamatsry wrote…

what is the difference between this and local_test_state_reader_factory?
I think there are enough use cases to generalise the state reader factory initialisation in tests.

Having said that, with the mock blockifier validator, it would probably not be necessary at all.

the empty_state_reader_factory makes the transaction invalid, so it passes only if we skip validations.
with local_test_state_reader_factory , it will be valid so it will pass anyway.

nice about the mock, so lets modify all tests after the mock is done.

[Yael-Starkware]

@dafnamatsry I will remove the max_nonce_for_validaiton_skip in a separate PR

Reviewable status: 0 of 7 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @yair-starkware)

[Yael-Starkware]

Reviewable status: 0 of 7 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware, @dafnamatsry, and @yair-starkware)

---

crates/gateway/src/stateful_transaction_validator.rs line 83 at r7 (raw file):

    tx: &RPCTransaction,
    account_nonce: Nonce,
    max_nonce_for_validation_skip: Nonce,

removed this parameter from here, and will delete the field from the actual config in a separate PR

[ArniStarkware]

Reviewed 1 of 2 files at r2.
Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @dafnamatsry, @Yael-Starkware, and @yair-starkware)

---

crates/gateway/src/stateful_transaction_validator.rs line 83 at r7 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

removed this parameter from here, and will delete the field from the actual config in a separate PR

Do we not want this number to be configurable anymore? I missed this decision.

[Yael-Starkware]

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

---

crates/gateway/src/stateful_transaction_validator.rs line 83 at r7 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Do we not want this number to be configurable anymore? I missed this decision.

yes, the requirement is to have only nonce=1 support, no need to support more than that atm.

[dafnamatsry]

Reviewed 6 of 7 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @yair-starkware)

[dafnamatsry]

Reviewed 8 of 8 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @yair-starkware)

[ArniStarkware]

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