From 1904dc10cd30f053e961c289e1962ae59d3a47ed Mon Sep 17 00:00:00 2001 From: Ammar Arif Date: Sun, 25 Aug 2024 23:20:46 -0400 Subject: [PATCH] refactor: increment nonce only after passing tx validation --- .../src/blockifier/stateful_validator.rs | 13 ++++++++ .../src/transaction/account_transaction.rs | 31 ++++++++++++++++++- .../src/transaction/transactions_test.rs | 9 ++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/crates/blockifier/src/blockifier/stateful_validator.rs b/crates/blockifier/src/blockifier/stateful_validator.rs index bb89e3d5fc..53c65fba28 100644 --- a/crates/blockifier/src/blockifier/stateful_validator.rs +++ b/crates/blockifier/src/blockifier/stateful_validator.rs @@ -78,6 +78,19 @@ impl StatefulValidator { // Post validations. PostValidationReport::verify(&tx_context, &actual_cost)?; + // See similar comment in `run_revertible` for context. + // + // From what I've seen there is not suitable method that is used by both the validator and + // the normal transaction flow where the nonce increment logic can be placed. So + // this is manually placed here. + // + // TODO: find a better place to put this without needing this duplication. + self.tx_executor + .block_state + .as_mut() + .expect(BLOCK_STATE_ACCESS_ERR) + .increment_nonce(tx_context.tx_info.sender_address())?; + Ok(()) } diff --git a/crates/blockifier/src/transaction/account_transaction.rs b/crates/blockifier/src/transaction/account_transaction.rs index 17bc4303c9..66b65fab5c 100644 --- a/crates/blockifier/src/transaction/account_transaction.rs +++ b/crates/blockifier/src/transaction/account_transaction.rs @@ -230,6 +230,12 @@ impl AccountTransaction { Ok(()) } + /// This method no longer increments the nonce. Refer to individual functions to see + /// in which stage the nonce is incremented. + /// + /// Nonce incremental logics manually placed in + /// [`AccountTransaction::run_revertible`], [`AccountTransaction::run_non_revertible`], + /// [`StatefulValidator::perform_validations`](crate::blockifier::stateful_validator::StatefulValidator::perform_validations) fn handle_nonce( state: &mut dyn State, tx_info: &TransactionInfo, @@ -248,7 +254,8 @@ impl AccountTransaction { account_nonce <= incoming_tx_nonce }; if valid_nonce { - return Ok(state.increment_nonce(address)?); + // return Ok(state.increment_nonce(address)?); + return Ok(()); } Err(TransactionPreValidationError::InvalidNonce { address, @@ -422,7 +429,13 @@ impl AccountTransaction { let mut resources = ExecutionResources::default(); let validate_call_info: Option; let execute_call_info: Option; + if matches!(self, Self::DeployAccount(_)) { + // See similar comment in `run_revertible` for context. + // + // Not sure for this case if incrementing the nonce at this stage is correct. + state.increment_nonce(tx_context.tx_info.sender_address())?; + // Handle `DeployAccount` transactions separately, due to different order of things. // Also, the execution context required form the `DeployAccount` execute phase is // validation context. @@ -449,6 +462,10 @@ impl AccountTransaction { validate, charge_fee, )?; + + // See similar comment in `run_revertible` for context. + state.increment_nonce(tx_context.tx_info.sender_address())?; + execute_call_info = self.run_execute(state, &mut resources, &mut execution_context, remaining_gas)?; } @@ -495,6 +512,18 @@ impl AccountTransaction { charge_fee, )?; + // Increment the sender nonce only after the tx has passed validation. + // + // NOTE: + // + // Before this, the nonce is incremented in `AccountTransaction::handle_nonce` which is + // called at the initial stage of transaction processing (ie in + // ExecutableTransaction::execute_raw of AccountTransaction), which is even before the + // account validation logic is being run. Which is weird because the nonce would get + // incremented even if the transaction failed validation. And this is not what I observed + // from mainnet/testnet. So, I moved the nonce incrementation here. + state.increment_nonce(tx_context.tx_info.sender_address())?; + let n_allotted_execution_steps = execution_context.subtract_validation_and_overhead_steps( &validate_call_info, &self.tx_type(), diff --git a/crates/blockifier/src/transaction/transactions_test.rs b/crates/blockifier/src/transaction/transactions_test.rs index 1a63f1a4ac..2c4b5de366 100644 --- a/crates/blockifier/src/transaction/transactions_test.rs +++ b/crates/blockifier/src/transaction/transactions_test.rs @@ -1015,6 +1015,15 @@ fn test_invalid_nonce( .perform_pre_validation_stage(&mut transactional_state, &valid_tx_context, false, false) .unwrap(); + // See comments on changing where nonce is incremented in `ExecutableTransaction::execute_raw` + // of AccountTransaction. + // + // Basically, before this, the nonce is incremented in `perform_pre_validation_stage` but that + // method is called even before the account validation stage is executed. So, now we have to + // increment the sender's account nonce manually here. + let account = valid_tx_context.tx_info.sender_address(); + transactional_state.increment_nonce(account).expect("failed to increment nonce"); + // Negative flow: account nonce = 1, incoming tx nonce = 0. let invalid_nonce = nonce!(0_u8); let invalid_tx =