From f8a24eb538d93b610eb5e6839418244c8ab7ad80 Mon Sep 17 00:00:00 2001 From: Anton Lisanin Date: Thu, 14 Nov 2024 12:51:52 +0300 Subject: [PATCH] Fix transaction finalization after cancel --- evm_loader/program/src/account/state.rs | 8 ++++- .../scheduled_transaction_create.rs | 2 +- .../scheduled_transaction_finish.rs | 1 + .../scheduled_transaction_start.rs | 4 ++- .../src/instruction/transaction_execute.rs | 4 +-- .../src/instruction/transaction_step.rs | 2 +- evm_loader/program/src/types/transaction.rs | 35 +++++++++---------- 7 files changed, 32 insertions(+), 24 deletions(-) diff --git a/evm_loader/program/src/account/state.rs b/evm_loader/program/src/account/state.rs index a4c41a38e..8f3cd8930 100644 --- a/evm_loader/program/src/account/state.rs +++ b/evm_loader/program/src/account/state.rs @@ -330,7 +330,13 @@ impl<'a> StateAccount<'a> { } pub fn finish_scheduled_tx(self, program_id: &Pubkey) -> Result<()> { - super::validate_tag(program_id, &self.account, TAG_SCHEDULED_STATE_FINALIZED)?; + let tag = super::tag(program_id, &self.account)?; + let is_finalized = tag == TAG_SCHEDULED_STATE_FINALIZED; + let is_canceled = tag == TAG_SCHEDULED_STATE_CANCELLED; + if !(is_finalized || is_canceled) { + return Err(Error::StorageAccountInvalidTag(*self.account.key, tag)); + } + debug_print!( "Finalize State {} for scheduled transaction", self.account.key diff --git a/evm_loader/program/src/instruction/scheduled_transaction_create.rs b/evm_loader/program/src/instruction/scheduled_transaction_create.rs index d02c2d146..ae0e37af6 100644 --- a/evm_loader/program/src/instruction/scheduled_transaction_create.rs +++ b/evm_loader/program/src/instruction/scheduled_transaction_create.rs @@ -152,7 +152,7 @@ pub fn process<'a>( let system = System::from_account(&accounts[5])?; // Validate Transaction - let tx = ScheduledTxShell::from_rlp(messsage).map_err(|_| Error::TreeAccountTxInvalidType)?; + let tx = ScheduledTxShell::from_rlp(messsage)?; let tx_hash = tx.hash; let payer = Address::from_solana_address(signer.key); diff --git a/evm_loader/program/src/instruction/scheduled_transaction_finish.rs b/evm_loader/program/src/instruction/scheduled_transaction_finish.rs index 9c89d34cd..1e2eed716 100644 --- a/evm_loader/program/src/instruction/scheduled_transaction_finish.rs +++ b/evm_loader/program/src/instruction/scheduled_transaction_finish.rs @@ -69,6 +69,7 @@ fn validate<'a>( let trx_tree_account = state .tree_account() .expect("Unreachable code path: validation in the State Account contains a bug."); + let actual_tree_pubkey = *tree.info().key; if trx_tree_account != actual_tree_pubkey { return Err(Error::ScheduledTxInvalidTreeAccount( diff --git a/evm_loader/program/src/instruction/scheduled_transaction_start.rs b/evm_loader/program/src/instruction/scheduled_transaction_start.rs index 4fff720ed..a8ad2d80e 100644 --- a/evm_loader/program/src/instruction/scheduled_transaction_start.rs +++ b/evm_loader/program/src/instruction/scheduled_transaction_start.rs @@ -17,7 +17,9 @@ pub fn do_scheduled_start<'a>( let origin = storage.trx_origin(); - storage.trx().validate(origin, &account_storage)?; + storage + .trx() + .validate(origin, &account_storage, Some(&transaction_tree))?; // Increment origin's nonce only once for the whole execution tree. let mut origin_account = account_storage.origin(origin, storage.trx())?; diff --git a/evm_loader/program/src/instruction/transaction_execute.rs b/evm_loader/program/src/instruction/transaction_execute.rs index a6ed54c50..cfd8264ef 100644 --- a/evm_loader/program/src/instruction/transaction_execute.rs +++ b/evm_loader/program/src/instruction/transaction_execute.rs @@ -19,7 +19,7 @@ pub fn execute( let mut account_storage = ProgramAccountStorage::new(accounts)?; let mut backend_data = ExecutorStateData::new(&account_storage); - trx.validate(origin, &account_storage)?; + trx.validate(origin, &account_storage, None)?; account_storage.origin(origin, &trx)?.increment_nonce()?; @@ -62,7 +62,7 @@ pub fn execute_with_solana_call( ) -> Result<()> { let mut account_storage = ProgramAccountStorage::new(accounts)?; - trx.validate(origin, &account_storage)?; + trx.validate(origin, &account_storage, None)?; account_storage.origin(origin, &trx)?.increment_nonce()?; diff --git a/evm_loader/program/src/instruction/transaction_step.rs b/evm_loader/program/src/instruction/transaction_step.rs index a0ed6a72c..98ccc53cd 100644 --- a/evm_loader/program/src/instruction/transaction_step.rs +++ b/evm_loader/program/src/instruction/transaction_step.rs @@ -20,7 +20,7 @@ pub fn do_begin<'a>( let origin = storage.trx_origin(); - storage.trx().validate(origin, &account_storage)?; + storage.trx().validate(origin, &account_storage, None)?; // Increment origin nonce in the first iteration // This allows us to run multiple iterative transactions from the same sender in parallel diff --git a/evm_loader/program/src/types/transaction.rs b/evm_loader/program/src/types/transaction.rs index b30e9d8aa..ecd2f5bb5 100644 --- a/evm_loader/program/src/types/transaction.rs +++ b/evm_loader/program/src/types/transaction.rs @@ -5,6 +5,7 @@ use serde::{Deserialize, Serialize}; use solana_program::instruction::{get_stack_height, TRANSACTION_LEVEL_STACK_HEIGHT}; use std::convert::TryInto; +use crate::account::TransactionTree; use crate::types::vector::VectorVecExt; use crate::{ account_storage::AccountStorage, config::GAS_LIMIT_MULTIPLIER_NO_CHAINID, error::Error, vector, @@ -1074,6 +1075,7 @@ impl Transaction { &self, origin: Address, backend: &impl AccountStorage, + tree: Option<&TransactionTree<'_>>, ) -> Result<(), crate::error::Error> { let chain_id = self .chain_id() @@ -1083,6 +1085,10 @@ impl Transaction { return Err(Error::InvalidChainId(chain_id)); } + if tree.is_some() != self.is_scheduled_tx() { + return Err(Error::TreeAccountTxInvalidType); + } + // Nonce validation is slightly different for classic and scheduled transactions. // // Classic transactions: @@ -1090,29 +1096,22 @@ impl Transaction { // the first iteration and then incremented. // // Scheduled transactions: - // payer's nonce (origin) can be greater than what's stored in the transaction because payer - // is the same for the whole scheduled execution tree. However, payer's nonce is also - // incremented only once - at the start of the first scheduled txn. + // payer's nonce (origin) validated only for the first transaction in the tree let origin_nonce = backend.nonce(origin, chain_id).await; - #[allow(clippy::collapsible_else_if)] - if self.is_scheduled_tx() { - if origin_nonce < self.nonce() { - let error = Error::InvalidTransactionNonce(origin, origin_nonce, self.nonce()); - return Err(error); - } - } else { - if origin_nonce != self.nonce() { - let error = Error::InvalidTransactionNonce(origin, origin_nonce, self.nonce()); - return Err(error); - } + + let validate_nonce = tree.map_or(true, TransactionTree::is_not_started); + if validate_nonce && (origin_nonce != self.nonce()) { + let error = Error::InvalidTransactionNonce(origin, origin_nonce, self.nonce()); + return Err(error); } // The reason to forbid the calls for DynamicFee transactions - priority fee calculation // uses get_processed_sibling_instruction syscall which doesn't work well for CPI. - if self.tx_type() == 2 && get_stack_height() != TRANSACTION_LEVEL_STACK_HEIGHT { - return Err(Error::Custom( - "CPI calls of Neon EVM are forbidden for DynamicFee transaction type.".to_owned(), - )); + let is_root_transaction = get_stack_height() == TRANSACTION_LEVEL_STACK_HEIGHT; + if matches!(self.tx_type(), 2 | 0x80) && !is_root_transaction { + return Err( + "CPI calls of Neon EVM are forbidden for DynamicFee transaction type.".into(), + ); } Ok(())