From 9b6eb5ef48cbcc592f2d6ebad9b52bfbbe18cbe3 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Thu, 6 Jun 2024 18:08:07 +0000 Subject: [PATCH 1/5] refactor: consolidate fee deduction --- accounts-db/src/accounts.rs | 87 +++++++++----------- runtime/src/bank.rs | 133 ++++++++++--------------------- svm/doc/spec.md | 7 +- svm/src/account_loader.rs | 17 ++-- svm/src/lib.rs | 1 + svm/src/nonce_info.rs | 67 +--------------- svm/src/rollback_accounts.rs | 92 +++++++++++++++++++++ svm/src/transaction_processor.rs | 61 ++++++++------ 8 files changed, 217 insertions(+), 248 deletions(-) create mode 100644 svm/src/rollback_accounts.rs diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index 90a2f7efde8a98..75a68a9af53491 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -29,9 +29,8 @@ use { transaction_context::TransactionAccount, }, solana_svm::{ - account_loader::TransactionLoadResult, - nonce_info::{NonceFull, NonceInfo}, - transaction_results::TransactionExecutionResult, + account_loader::TransactionLoadResult, nonce_info::NonceInfo, + rollback_accounts::RollbackAccounts, transaction_results::TransactionExecutionResult, }, std::{ cmp::Reverse, @@ -723,21 +722,6 @@ impl Accounts { TransactionExecutionResult::NotExecuted(_) => continue, }; - enum AccountCollectionMode<'a> { - Normal, - FailedWithNonce { nonce: &'a NonceFull }, - } - - let collection_mode = match (execution_status, &loaded_transaction.nonce) { - (Ok(_), _) => AccountCollectionMode::Normal, - (Err(_), Some(nonce)) => AccountCollectionMode::FailedWithNonce { nonce }, - (Err(_), None) => { - // Fees for failed transactions which don't use durable nonces are - // deducted in Bank::filter_program_errors_and_collect_fee - continue; - } - }; - // Accounts that are invoked and also not passed as an instruction // account to a program don't need to be stored because it's assumed // to be impossible for a committable transaction to modify an @@ -755,21 +739,24 @@ impl Accounts { }; let message = tx.message(); + let rollback_accounts = &loaded_transaction.rollback_accounts; + let maybe_nonce_address = rollback_accounts.nonce().map(|account| account.address()); + for (i, (address, account)) in (0..message.account_keys().len()) .zip(loaded_transaction.accounts.iter_mut()) .filter(|(i, _)| is_storable_account(message, *i)) { if message.is_writable(i) { - let should_collect_account = match collection_mode { - AccountCollectionMode::Normal => true, - AccountCollectionMode::FailedWithNonce { nonce } => { + let should_collect_account = match execution_status { + Ok(()) => true, + Err(_) => { let is_fee_payer = i == 0; - let is_nonce_account = address == nonce.address(); - post_process_failed_nonce( + let is_nonce_account = Some(&*address) == maybe_nonce_address; + post_process_failed_tx( account, is_fee_payer, is_nonce_account, - nonce, + rollback_accounts, durable_nonce, lamports_per_signature, ); @@ -790,41 +777,41 @@ impl Accounts { } } -fn post_process_failed_nonce( +fn post_process_failed_tx( account: &mut AccountSharedData, is_fee_payer: bool, is_nonce_account: bool, - nonce: &NonceFull, + rollback_accounts: &RollbackAccounts, &durable_nonce: &DurableNonce, lamports_per_signature: u64, ) { if is_nonce_account { - // The transaction failed which would normally drop the account - // processing changes, since this account is now being included - // in the accounts written back to the db, roll it back to - // pre-processing state. - *account = nonce.account().clone(); - - // Advance the stored blockhash to prevent fee theft by someone - // replaying nonce transactions that have failed with an - // `InstructionError`. - // - // Since we know we are dealing with a valid nonce account, - // unwrap is safe here - let nonce_versions = StateMut::::state(account).unwrap(); - if let NonceState::Initialized(ref data) = nonce_versions.state() { - let nonce_state = - NonceState::new_initialized(&data.authority, durable_nonce, lamports_per_signature); - let nonce_versions = NonceVersions::new(nonce_state); - account.set_state(&nonce_versions).unwrap(); + if let Some(nonce) = rollback_accounts.nonce() { + // The transaction failed which would normally drop the account + // processing changes, since this account is now being included + // in the accounts written back to the db, roll it back to + // pre-processing state. + *account = nonce.account().clone(); + + // Advance the stored blockhash to prevent fee theft by someone + // replaying nonce transactions that have failed with an + // `InstructionError`. + // + // Since we know we are dealing with a valid nonce account, + // unwrap is safe here + let nonce_versions = StateMut::::state(account).unwrap(); + if let NonceState::Initialized(ref data) = nonce_versions.state() { + let nonce_state = NonceState::new_initialized( + &data.authority, + durable_nonce, + lamports_per_signature, + ); + let nonce_versions = NonceVersions::new(nonce_state); + account.set_state(&nonce_versions).unwrap(); + } } } else if is_fee_payer { - if let Some(fee_payer_account) = nonce.fee_payer_account() { - // Instruction error and fee-payer for this nonce tx is not - // the nonce account itself, rollback the fee payer to the - // fee-paid original state. - *account = fee_payer_account.clone(); - } + *account = rollback_accounts.fee_payer_account().clone(); } } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index 615f7bf5ae050c..e63ffcd5508caf 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -182,7 +182,6 @@ use { TransactionLoadedAccountsStats, TransactionResults, }, }, - solana_system_program::{get_system_account_kind, SystemAccountKind}, solana_vote::vote_account::{VoteAccount, VoteAccountsHashMap}, solana_vote_program::vote_state::VoteState, std::{ @@ -214,6 +213,7 @@ use { }, solana_program_runtime::{loaded_programs::ProgramCacheForTxBatch, sysvar_cache::SysvarCache}, solana_svm::program_loader::load_program_with_pubkey, + solana_system_program::{get_system_account_kind, SystemAccountKind}, }; /// params to `verify_accounts_hash` @@ -3939,31 +3939,18 @@ impl Bank { fn filter_program_errors_and_collect_fee( &self, - txs: &[SanitizedTransaction], execution_results: &[TransactionExecutionResult], ) -> Vec> { let mut fees = 0; - let results = txs + let results = execution_results .iter() - .zip(execution_results) - .map(|(tx, execution_result)| { - let message = tx.message(); - let details = match &execution_result { - TransactionExecutionResult::Executed { details, .. } => details, - TransactionExecutionResult::NotExecuted(err) => return Err(err.clone()), - }; - - let fee = details.fee_details.total_fee(); - self.check_execution_status_and_charge_fee( - message, - &details.status, - details.is_nonce, - fee, - )?; - - fees += fee; - Ok(()) + .map(|execution_result| match execution_result { + TransactionExecutionResult::Executed { details, .. } => { + fees += details.fee_details.total_fee(); + Ok(()) + } + TransactionExecutionResult::NotExecuted(err) => Err(err.clone()), }) .collect(); @@ -3974,31 +3961,18 @@ impl Bank { // Note: this function is not yet used; next PR will call it behind a feature gate fn filter_program_errors_and_collect_fee_details( &self, - txs: &[SanitizedTransaction], execution_results: &[TransactionExecutionResult], ) -> Vec> { let mut accumulated_fee_details = FeeDetails::default(); - let results = txs + let results = execution_results .iter() - .zip(execution_results) - .map(|(tx, execution_result)| { - let message = tx.message(); - let details = match &execution_result { - TransactionExecutionResult::Executed { details, .. } => details, - TransactionExecutionResult::NotExecuted(err) => return Err(err.clone()), - }; - - self.check_execution_status_and_charge_fee( - message, - &details.status, - details.is_nonce, - details.fee_details.total_fee(), - )?; - - accumulated_fee_details.accumulate(&details.fee_details); - - Ok(()) + .map(|execution_result| match execution_result { + TransactionExecutionResult::Executed { details, .. } => { + accumulated_fee_details.accumulate(&details.fee_details); + Ok(()) + } + TransactionExecutionResult::NotExecuted(err) => Err(err.clone()), }) .collect(); @@ -4009,27 +3983,6 @@ impl Bank { results } - fn check_execution_status_and_charge_fee( - &self, - message: &SanitizedMessage, - execution_status: &transaction::Result<()>, - is_nonce: bool, - fee: u64, - ) -> Result<()> { - // In case of instruction error, even though no accounts - // were stored we still need to charge the payer the - // fee. - // - //...except nonce accounts, which already have their - // post-load, fee deducted, pre-execute account state - // stored - if execution_status.is_err() && !is_nonce { - self.withdraw(message.fee_payer(), fee)?; - } - - Ok(()) - } - /// `committed_transactions_count` is the number of transactions out of `sanitized_txs` /// that was executed. Of those, `committed_transactions_count`, /// `committed_with_failure_result_count` is the number of executed transactions that returned @@ -4148,9 +4101,9 @@ impl Bank { self.update_transaction_statuses(sanitized_txs, &execution_results); let fee_collection_results = if self.feature_set.is_active(&reward_full_priority_fee::id()) { - self.filter_program_errors_and_collect_fee_details(sanitized_txs, &execution_results) + self.filter_program_errors_and_collect_fee_details(&execution_results) } else { - self.filter_program_errors_and_collect_fee(sanitized_txs, &execution_results) + self.filter_program_errors_and_collect_fee(&execution_results) }; update_transaction_statuses_time.stop(); timings.saturating_add_in_place( @@ -5138,32 +5091,6 @@ impl Bank { ); } - fn withdraw(&self, pubkey: &Pubkey, lamports: u64) -> Result<()> { - match self.get_account_with_fixed_root_no_cache(pubkey) { - Some(mut account) => { - let min_balance = match get_system_account_kind(&account) { - Some(SystemAccountKind::Nonce) => self - .rent_collector - .rent - .minimum_balance(nonce::State::size()), - _ => 0, - }; - - lamports - .checked_add(min_balance) - .filter(|required_balance| *required_balance <= account.lamports()) - .ok_or(TransactionError::InsufficientFundsForFee)?; - account - .checked_sub_lamports(lamports) - .map_err(|_| TransactionError::InsufficientFundsForFee)?; - self.store_account(pubkey, &account); - - Ok(()) - } - None => Err(TransactionError::AccountNotFound), - } - } - pub fn accounts(&self) -> Arc { self.rc.accounts.clone() } @@ -7169,6 +7096,32 @@ impl Bank { .get_environments_for_epoch(effective_epoch)?; load_program_with_pubkey(self, &environments, pubkey, self.slot(), reload) } + + pub fn withdraw(&self, pubkey: &Pubkey, lamports: u64) -> Result<()> { + match self.get_account_with_fixed_root(pubkey) { + Some(mut account) => { + let min_balance = match get_system_account_kind(&account) { + Some(SystemAccountKind::Nonce) => self + .rent_collector + .rent + .minimum_balance(nonce::State::size()), + _ => 0, + }; + + lamports + .checked_add(min_balance) + .filter(|required_balance| *required_balance <= account.lamports()) + .ok_or(TransactionError::InsufficientFundsForFee)?; + account + .checked_sub_lamports(lamports) + .map_err(|_| TransactionError::InsufficientFundsForFee)?; + self.store_account(pubkey, &account); + + Ok(()) + } + None => Err(TransactionError::AccountNotFound), + } + } } /// Compute how much an account has changed size. This function is useful when the data size delta diff --git a/svm/doc/spec.md b/svm/doc/spec.md index 94b1a6858e02f1..fec636591aaf17 100644 --- a/svm/doc/spec.md +++ b/svm/doc/spec.md @@ -277,11 +277,10 @@ Steps of `load_and_execute_sanitized_transactions` - Validate the fee payer and the loaded accounts - Validate the programs accounts that have been loaded and checks if they are builtin programs. - Return `struct LoadedTransaction` containing the accounts (pubkey and data), - indices to the excutabe accounts in `TransactionContext` (or `InstructionContext`), + indices to the executable accounts in `TransactionContext` (or `InstructionContext`), the transaction rent, and the `struct RentDebit`. - - Generate a `NonceFull` struct (holds fee subtracted nonce info) when possible, `None` otherwise. - - Returns `TransactionLoadedResult`, a tuple containing the `LoadTransaction` we obtained from `loaded_transaction_accounts`, - and a `Option`. + - Generate a `RollbackAccounts` struct which holds fee-subtracted fee payer account and pre-execution nonce state used for rolling back account state on execution failure. + - Returns `TransactionLoadedResult`, containing the `LoadTransaction` we obtained from `loaded_transaction_accounts` 3. Execute each loaded transactions 1. Compute the sum of transaction accounts' balances. This sum is diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 8eb2f9fa3e81f3..a813d4b4ffd788 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -1,8 +1,7 @@ use { crate::{ - account_overrides::AccountOverrides, - account_rent_state::RentState, - nonce_info::{NonceFull, NoncePartial}, + account_overrides::AccountOverrides, account_rent_state::RentState, + nonce_info::NoncePartial, rollback_accounts::RollbackAccounts, transaction_error_metrics::TransactionErrorMetrics, transaction_processing_callback::TransactionProcessingCallback, }, @@ -45,7 +44,7 @@ pub struct CheckedTransactionDetails { #[derive(PartialEq, Eq, Debug, Clone)] #[cfg_attr(feature = "dev-context-only-utils", derive(Default))] pub struct ValidatedTransactionDetails { - pub nonce: Option, + pub rollback_accounts: RollbackAccounts, pub fee_details: FeeDetails, pub fee_payer_account: AccountSharedData, pub fee_payer_rent_debit: u64, @@ -55,19 +54,13 @@ pub struct ValidatedTransactionDetails { pub struct LoadedTransaction { pub accounts: Vec, pub program_indices: TransactionProgramIndices, - pub nonce: Option, pub fee_details: FeeDetails, + pub rollback_accounts: RollbackAccounts, pub rent: TransactionRent, pub rent_debits: RentDebits, pub loaded_accounts_data_size: usize, } -impl LoadedTransaction { - pub fn fee_payer_account(&self) -> Option<&TransactionAccount> { - self.accounts.first() - } -} - /// Collect rent from an account if rent is still enabled and regardless of /// whether rent is enabled, set the rent epoch to u64::MAX if the account is /// rent exempt. @@ -363,8 +356,8 @@ fn load_transaction_accounts( Ok(LoadedTransaction { accounts, program_indices, - nonce: tx_details.nonce, fee_details: tx_details.fee_details, + rollback_accounts: tx_details.rollback_accounts, rent: tx_rent, rent_debits, loaded_accounts_data_size: accumulated_accounts_data_size, diff --git a/svm/src/lib.rs b/svm/src/lib.rs index f15a854354dd10..201bf671a1be49 100644 --- a/svm/src/lib.rs +++ b/svm/src/lib.rs @@ -7,6 +7,7 @@ pub mod account_rent_state; pub mod message_processor; pub mod nonce_info; pub mod program_loader; +pub mod rollback_accounts; pub mod runtime_config; pub mod transaction_account_state_info; pub mod transaction_error_metrics; diff --git a/svm/src/nonce_info.rs b/svm/src/nonce_info.rs index 5088adb5b5965f..b96a8e24619649 100644 --- a/svm/src/nonce_info.rs +++ b/svm/src/nonce_info.rs @@ -1,8 +1,4 @@ -use solana_sdk::{ - account::{AccountSharedData, ReadableAccount, WritableAccount}, - nonce_account, - pubkey::Pubkey, -}; +use solana_sdk::{account::AccountSharedData, nonce_account, pubkey::Pubkey}; pub trait NonceInfo { fn address(&self) -> &Pubkey; @@ -39,67 +35,6 @@ impl NonceInfo for NoncePartial { } } -/// Holds fee subtracted nonce info -#[derive(Clone, Debug, Default, PartialEq, Eq)] -pub struct NonceFull { - address: Pubkey, - account: AccountSharedData, - fee_payer_account: Option, -} - -impl NonceFull { - pub fn new( - address: Pubkey, - account: AccountSharedData, - fee_payer_account: Option, - ) -> Self { - Self { - address, - account, - fee_payer_account, - } - } - - pub fn from_partial( - partial: NoncePartial, - fee_payer_address: &Pubkey, - mut fee_payer_account: AccountSharedData, - fee_payer_rent_debit: u64, - ) -> Self { - fee_payer_account.set_lamports( - fee_payer_account - .lamports() - .saturating_add(fee_payer_rent_debit), - ); - - let NoncePartial { - address: nonce_address, - account: nonce_account, - } = partial; - - if *fee_payer_address == nonce_address { - Self::new(nonce_address, fee_payer_account, None) - } else { - Self::new(nonce_address, nonce_account, Some(fee_payer_account)) - } - } -} - -impl NonceInfo for NonceFull { - fn address(&self) -> &Pubkey { - &self.address - } - fn account(&self) -> &AccountSharedData { - &self.account - } - fn lamports_per_signature(&self) -> Option { - nonce_account::lamports_per_signature_of(&self.account) - } - fn fee_payer_account(&self) -> Option<&AccountSharedData> { - self.fee_payer_account.as_ref() - } -} - #[cfg(test)] mod tests { use { diff --git a/svm/src/rollback_accounts.rs b/svm/src/rollback_accounts.rs new file mode 100644 index 00000000000000..fa6688a7ce8197 --- /dev/null +++ b/svm/src/rollback_accounts.rs @@ -0,0 +1,92 @@ +use { + crate::nonce_info::{NonceInfo, NoncePartial}, + solana_sdk::{ + account::{AccountSharedData, ReadableAccount, WritableAccount}, + pubkey::Pubkey, + }, +}; + +/// Captured account state used to rollback account state for nonce and fee +/// payer accounts after a failed executed transaction. +#[derive(PartialEq, Eq, Debug, Clone)] +pub enum RollbackAccounts { + FeePayerOnly { + fee_payer_account: AccountSharedData, + }, + SameNonceAndFeePayer { + nonce: NoncePartial, + }, + SeparateNonceAndFeePayer { + nonce: NoncePartial, + fee_payer_account: AccountSharedData, + }, +} + +#[cfg(feature = "dev-context-only-utils")] +impl Default for RollbackAccounts { + fn default() -> Self { + Self::FeePayerOnly { + fee_payer_account: AccountSharedData::default(), + } + } +} + +impl RollbackAccounts { + pub fn new( + nonce: Option, + fee_payer_address: Pubkey, + mut fee_payer_account: AccountSharedData, + fee_payer_rent_debit: u64, + fee_payer_loaded_rent_epoch: u64, + ) -> Self { + // When the fee payer account is rolled back due to transaction failure, + // rent should not be charged so credit the previously debited rent + // amount. + fee_payer_account.set_lamports( + fee_payer_account + .lamports() + .saturating_add(fee_payer_rent_debit), + ); + + if let Some(nonce) = nonce { + if &fee_payer_address == nonce.address() { + RollbackAccounts::SameNonceAndFeePayer { + nonce: NoncePartial::new(fee_payer_address, fee_payer_account), + } + } else { + RollbackAccounts::SeparateNonceAndFeePayer { + nonce, + fee_payer_account, + } + } + } else { + // When rolling back failed transactions which don't use nonces, the + // runtime should not update the fee payer's rent epoch so reset the + // rollback fee payer acocunt's rent epoch to its originally loaded + // rent epoch value. In the future, a feature gate could be used to + // alter this behavior such that rent epoch updates are handled the + // same for both nonce and non-nonce failed transactions. + fee_payer_account.set_rent_epoch(fee_payer_loaded_rent_epoch); + RollbackAccounts::FeePayerOnly { fee_payer_account } + } + } + + pub fn nonce(&self) -> Option<&NoncePartial> { + match self { + Self::FeePayerOnly { .. } => None, + Self::SameNonceAndFeePayer { nonce } | Self::SeparateNonceAndFeePayer { nonce, .. } => { + Some(nonce) + } + } + } + + pub fn fee_payer_account(&self) -> &AccountSharedData { + match self { + Self::FeePayerOnly { fee_payer_account } + | Self::SeparateNonceAndFeePayer { + fee_payer_account, .. + } => fee_payer_account, + Self::SameNonceAndFeePayer { nonce } => nonce.account(), + } + } +} diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 10fe96632dafd3..90b1df4f7c48e1 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -9,8 +9,8 @@ use { }, account_overrides::AccountOverrides, message_processor::MessageProcessor, - nonce_info::NonceFull, program_loader::load_program_with_pubkey, + rollback_accounts::RollbackAccounts, transaction_account_state_info::TransactionAccountStateInfo, transaction_error_metrics::TransactionErrorMetrics, transaction_processing_callback::TransactionProcessingCallback, @@ -416,30 +416,33 @@ impl TransactionBatchProcessor { lamports_per_signature, }| { let message = sanitized_tx.borrow().message(); - let (fee_details, fee_payer_account, fee_payer_rent_debit) = self - .validate_transaction_fee_payer( - callbacks, - message, - feature_set, - fee_structure, - lamports_per_signature, - rent_collector, - error_counters, - )?; - - // Update nonce with fee-subtracted accounts - let fee_payer_address = message.fee_payer(); - let nonce = nonce.map(|nonce| { - NonceFull::from_partial( - nonce, - fee_payer_address, - fee_payer_account.clone(), - fee_payer_rent_debit, - ) - }); + let ( + fee_details, + fee_payer_account, + fee_payer_rent_debit, + fee_payer_rent_epoch, + ) = self.validate_transaction_fee_payer( + callbacks, + message, + feature_set, + fee_structure, + lamports_per_signature, + rent_collector, + error_counters, + )?; + + // Capture fee-subtracted fee payer account and original nonce account state + // to rollback to if transaction execution fails. + let rollback_accounts = RollbackAccounts::new( + nonce, + *message.fee_payer(), + fee_payer_account.clone(), + fee_payer_rent_debit, + fee_payer_rent_epoch, + ); Ok(ValidatedTransactionDetails { - nonce, + rollback_accounts, fee_details, fee_payer_account, fee_payer_rent_debit, @@ -462,7 +465,7 @@ impl TransactionBatchProcessor { lamports_per_signature: u64, rent_collector: &RentCollector, error_counters: &mut TransactionErrorMetrics, - ) -> transaction::Result<(FeeDetails, AccountSharedData, u64)> { + ) -> transaction::Result<(FeeDetails, AccountSharedData, u64, u64)> { let fee_payer_address = message.fee_payer(); let Some(mut fee_payer_account) = callbacks.get_account_shared_data(fee_payer_address) else { @@ -470,6 +473,7 @@ impl TransactionBatchProcessor { return Err(TransactionError::AccountNotFound); }; + let fee_payer_rent_epoch = fee_payer_account.rent_epoch(); let fee_payer_rent_debit = collect_rent_from_account( feature_set, rent_collector, @@ -498,7 +502,12 @@ impl TransactionBatchProcessor { fee_details.total_fee(), )?; - Ok((fee_details, fee_payer_account, fee_payer_rent_debit)) + Ok(( + fee_details, + fee_payer_account, + fee_payer_rent_debit, + fee_payer_rent_epoch, + )) } /// Returns a map from executable program accounts (all accounts owned by any loader) @@ -885,7 +894,7 @@ impl TransactionBatchProcessor { log_messages, inner_instructions, fee_details: loaded_transaction.fee_details, - is_nonce: loaded_transaction.nonce.is_some(), + is_nonce: loaded_transaction.rollback_accounts.nonce().is_some(), return_data, executed_units, accounts_data_len_delta, From af0d74fa85113e995a4e9e7014990c3076a5ac85 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Fri, 7 Jun 2024 23:06:06 +0000 Subject: [PATCH 2/5] fix tests --- accounts-db/Cargo.toml | 1 + accounts-db/src/accounts.rs | 135 +++++++++++++----------------- runtime/src/bank/tests.rs | 137 ++++--------------------------- svm/src/account_loader.rs | 136 ++++++++++++++++++++---------- svm/src/nonce_info.rs | 93 ++------------------- svm/src/rollback_accounts.rs | 136 ++++++++++++++++++++++++++++++ svm/src/transaction_processor.rs | 27 ++++-- 7 files changed, 332 insertions(+), 333 deletions(-) diff --git a/accounts-db/Cargo.toml b/accounts-db/Cargo.toml index fd902c71364d79..16bcd03e7fdcc1 100644 --- a/accounts-db/Cargo.toml +++ b/accounts-db/Cargo.toml @@ -67,6 +67,7 @@ serde_bytes = { workspace = true } solana-accounts-db = { path = ".", features = ["dev-context-only-utils"] } solana-logger = { workspace = true } solana-sdk = { workspace = true, features = ["dev-context-only-utils"] } +solana-svm = { workspace = true, features = ["dev-context-only-utils"] } static_assertions = { workspace = true } strum = { workspace = true, features = ["derive"] } strum_macros = { workspace = true } diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index 75a68a9af53491..7e43e80e4ddc29 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -827,14 +827,17 @@ mod tests { hash::Hash, instruction::{CompiledInstruction, InstructionError}, message::{Message, MessageHeader}, - native_loader, nonce, nonce_account, + native_loader, + nonce::state::Data as NonceData, + nonce_account, rent_debits::RentDebits, signature::{keypair_from_seed, signers::Signers, Keypair, Signer}, system_instruction, system_program, transaction::{Transaction, MAX_TX_ACCOUNT_LOCKS}, }, solana_svm::{ - account_loader::LoadedTransaction, transaction_results::TransactionExecutionDetails, + account_loader::LoadedTransaction, nonce_info::NoncePartial, + transaction_results::TransactionExecutionDetails, }, std::{ borrow::Cow, @@ -858,7 +861,7 @@ mod tests { fn new_execution_result( status: Result<()>, - nonce: Option<&NonceFull>, + nonce: Option<&NoncePartial>, ) -> TransactionExecutionResult { TransactionExecutionResult::Executed { details: TransactionExecutionDetails { @@ -1563,8 +1566,8 @@ mod tests { let loaded0 = Ok(LoadedTransaction { accounts: transaction_accounts0, program_indices: vec![], - nonce: None, fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::default(), rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, @@ -1573,8 +1576,8 @@ mod tests { let loaded1 = Ok(LoadedTransaction { accounts: transaction_accounts1, program_indices: vec![], - nonce: None, fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::default(), rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, @@ -1649,34 +1652,26 @@ mod tests { accounts.accounts_db.clean_accounts_for_tests(); } - fn create_accounts_post_process_failed_nonce() -> ( + fn create_accounts_post_process_failed_tx() -> ( Pubkey, AccountSharedData, AccountSharedData, DurableNonce, u64, - Option, ) { - let data = NonceVersions::new(NonceState::Initialized(nonce::state::Data::default())); + let data = NonceVersions::new(NonceState::Initialized(NonceData::default())); let account = AccountSharedData::new_data(42, &data, &system_program::id()).unwrap(); let mut pre_account = account.clone(); pre_account.set_lamports(43); let durable_nonce = DurableNonce::from_blockhash(&Hash::new(&[1u8; 32])); - ( - Pubkey::default(), - pre_account, - account, - durable_nonce, - 1234, - None, - ) + (Pubkey::default(), pre_account, account, durable_nonce, 1234) } - fn run_post_process_failed_nonce_test( + fn run_post_process_failed_tx_test( account: &mut AccountSharedData, is_fee_payer: bool, is_nonce_account: bool, - nonce: &NonceFull, + rollback_accounts: &RollbackAccounts, durable_nonce: &DurableNonce, lamports_per_signature: u64, expect_account: &AccountSharedData, @@ -1684,17 +1679,17 @@ mod tests { // Verify expect_account's relationship if !is_fee_payer { if is_nonce_account { - assert_ne!(expect_account, nonce.account()); + assert_ne!(expect_account, rollback_accounts.nonce().unwrap().account()); } else { assert_eq!(expect_account, account); } } - post_process_failed_nonce( + post_process_failed_tx( account, is_fee_payer, is_nonce_account, - nonce, + rollback_accounts, durable_nonce, lamports_per_signature, ); @@ -1703,33 +1698,25 @@ mod tests { } #[test] - fn test_post_process_failed_nonce_expected() { - let ( - pre_account_address, - pre_account, - mut post_account, - blockhash, - lamports_per_signature, - maybe_fee_payer_account, - ) = create_accounts_post_process_failed_nonce(); - let nonce = NonceFull::new( - pre_account_address, - pre_account.clone(), - maybe_fee_payer_account, - ); + fn test_post_process_failed_tx_expected() { + let (pre_account_address, pre_account, mut post_account, blockhash, lamports_per_signature) = + create_accounts_post_process_failed_tx(); + let rollback_accounts = RollbackAccounts::SameNonceAndFeePayer { + nonce: NoncePartial::new(pre_account_address, pre_account.clone()), + }; let mut expect_account = pre_account; expect_account .set_state(&NonceVersions::new(NonceState::Initialized( - nonce::state::Data::new(Pubkey::default(), blockhash, lamports_per_signature), + NonceData::new(Pubkey::default(), blockhash, lamports_per_signature), ))) .unwrap(); - assert!(run_post_process_failed_nonce_test( + assert!(run_post_process_failed_tx_test( &mut post_account, false, // is_fee_payer true, // is_nonce_account - &nonce, + &rollback_accounts, &blockhash, lamports_per_signature, &expect_account, @@ -1737,24 +1724,20 @@ mod tests { } #[test] - fn test_post_process_failed_nonce_not_nonce_address() { - let ( - pre_account_address, - pre_account, - mut post_account, - blockhash, - lamports_per_signature, - maybe_fee_payer_account, - ) = create_accounts_post_process_failed_nonce(); + fn test_post_process_failed_tx_not_nonce_address() { + let (pre_account_address, pre_account, mut post_account, blockhash, lamports_per_signature) = + create_accounts_post_process_failed_tx(); - let nonce = NonceFull::new(pre_account_address, pre_account, maybe_fee_payer_account); + let rollback_accounts = RollbackAccounts::SameNonceAndFeePayer { + nonce: NoncePartial::new(pre_account_address, pre_account.clone()), + }; let expect_account = post_account.clone(); - assert!(run_post_process_failed_nonce_test( + assert!(run_post_process_failed_tx_test( &mut post_account, false, // is_fee_payer false, // is_nonce_account - &nonce, + &rollback_accounts, &blockhash, lamports_per_signature, &expect_account, @@ -1768,27 +1751,26 @@ mod tests { AccountSharedData::new_data(42, &(), &system_program::id()).unwrap(); let post_fee_payer_account = AccountSharedData::new_data(84, &[1, 2, 3, 4], &system_program::id()).unwrap(); - let nonce = NonceFull::new( - Pubkey::new_unique(), - nonce_account, - Some(pre_fee_payer_account.clone()), - ); + let rollback_accounts = RollbackAccounts::SeparateNonceAndFeePayer { + nonce: NoncePartial::new(Pubkey::new_unique(), nonce_account), + fee_payer_account: pre_fee_payer_account.clone(), + }; - assert!(run_post_process_failed_nonce_test( + assert!(run_post_process_failed_tx_test( &mut post_fee_payer_account.clone(), false, // is_fee_payer false, // is_nonce_account - &nonce, + &rollback_accounts, &DurableNonce::default(), 1, &post_fee_payer_account, )); - assert!(run_post_process_failed_nonce_test( + assert!(run_post_process_failed_tx_test( &mut post_fee_payer_account.clone(), true, // is_fee_payer false, // is_nonce_account - &nonce, + &rollback_accounts, &DurableNonce::default(), 1, &pre_fee_payer_account, @@ -1803,7 +1785,7 @@ mod tests { let from_address = from.pubkey(); let to_address = Pubkey::new_unique(); let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); - let nonce_state = NonceVersions::new(NonceState::Initialized(nonce::state::Data::new( + let nonce_state = NonceVersions::new(NonceState::Initialized(NonceData::new( nonce_authority.pubkey(), durable_nonce, 0, @@ -1831,7 +1813,7 @@ mod tests { let tx = new_sanitized_tx(&[&nonce_authority, &from], message, blockhash); let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); - let nonce_state = NonceVersions::new(NonceState::Initialized(nonce::state::Data::new( + let nonce_state = NonceVersions::new(NonceState::Initialized(NonceData::new( nonce_authority.pubkey(), durable_nonce, 0, @@ -1840,17 +1822,15 @@ mod tests { AccountSharedData::new_data(42, &nonce_state, &system_program::id()).unwrap(); let from_account_pre = AccountSharedData::new(4242, 0, &Pubkey::default()); - let nonce = Some(NonceFull::new( - nonce_address, - nonce_account_pre.clone(), - Some(from_account_pre.clone()), - )); - + let nonce = NoncePartial::new(nonce_address, nonce_account_pre.clone()); let loaded = Ok(LoadedTransaction { accounts: transaction_accounts, program_indices: vec![], - nonce: nonce.clone(), fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::SeparateNonceAndFeePayer { + nonce: nonce.clone(), + fee_payer_account: from_account_pre.clone(), + }, rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, @@ -1867,7 +1847,7 @@ mod tests { 1, InstructionError::InvalidArgument, )), - nonce.as_ref(), + Some(&nonce), )]; let (collected_accounts, _) = accounts.collect_accounts_to_store( &txs, @@ -1910,7 +1890,7 @@ mod tests { let from_address = from.pubkey(); let to_address = Pubkey::new_unique(); let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); - let nonce_state = NonceVersions::new(NonceState::Initialized(nonce::state::Data::new( + let nonce_state = NonceVersions::new(NonceState::Initialized(NonceData::new( nonce_authority.pubkey(), durable_nonce, 0, @@ -1938,7 +1918,7 @@ mod tests { let tx = new_sanitized_tx(&[&nonce_authority, &from], message, blockhash); let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); - let nonce_state = NonceVersions::new(NonceState::Initialized(nonce::state::Data::new( + let nonce_state = NonceVersions::new(NonceState::Initialized(NonceData::new( nonce_authority.pubkey(), durable_nonce, 0, @@ -1946,17 +1926,14 @@ mod tests { let nonce_account_pre = AccountSharedData::new_data(42, &nonce_state, &system_program::id()).unwrap(); - let nonce = Some(NonceFull::new( - nonce_address, - nonce_account_pre.clone(), - None, - )); - + let nonce = NoncePartial::new(nonce_address, nonce_account_pre.clone()); let loaded = Ok(LoadedTransaction { accounts: transaction_accounts, program_indices: vec![], - nonce: nonce.clone(), fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::SameNonceAndFeePayer { + nonce: nonce.clone(), + }, rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, @@ -1973,7 +1950,7 @@ mod tests { 1, InstructionError::InvalidArgument, )), - nonce.as_ref(), + Some(&nonce), )]; let (collected_accounts, _) = accounts.collect_accounts_to_store( &txs, diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 87aa5a84617b7b..59c7950f64c85e 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -104,7 +104,7 @@ use { transaction_context::TransactionAccount, }, solana_stake_program::stake_state::{self, StakeStateV2}, - solana_svm::nonce_info::NonceFull, + solana_svm::nonce_info::NoncePartial, solana_vote_program::{ vote_instruction, vote_state::{ @@ -233,7 +233,7 @@ fn test_race_register_tick_freeze() { fn new_execution_result( status: Result<()>, - nonce: Option<&NonceFull>, + nonce: Option<&NoncePartial>, fee_details: FeeDetails, ) -> TransactionExecutionResult { TransactionExecutionResult::Executed { @@ -2867,29 +2867,12 @@ fn test_bank_blockhash_compute_unit_fee_structure() { #[test] fn test_filter_program_errors_and_collect_fee() { let leader = solana_sdk::pubkey::new_rand(); - let GenesisConfigInfo { - genesis_config, - mint_keypair, - .. - } = create_genesis_config_with_leader(100_000, &leader, 3); + let GenesisConfigInfo { genesis_config, .. } = + create_genesis_config_with_leader(100_000, &leader, 3); let mut bank = Bank::new_for_tests(&genesis_config); // this test is only for when `feature_set::reward_full_priority_fee` inactivated bank.deactivate_feature(&feature_set::reward_full_priority_fee::id()); - let key = solana_sdk::pubkey::new_rand(); - let tx1 = SanitizedTransaction::from_transaction_for_tests(system_transaction::transfer( - &mint_keypair, - &key, - 2, - genesis_config.hash(), - )); - let tx2 = SanitizedTransaction::from_transaction_for_tests(system_transaction::transfer( - &mint_keypair, - &key, - 5, - genesis_config.hash(), - )); - let tx_fee = 42; let fee_details = FeeDetails::new_for_tests(tx_fee, 0, false); let results = vec![ @@ -2905,7 +2888,7 @@ fn test_filter_program_errors_and_collect_fee() { ]; let initial_balance = bank.get_balance(&leader); - let results = bank.filter_program_errors_and_collect_fee(&[tx1, tx2], &results); + let results = bank.filter_program_errors_and_collect_fee(&results); bank.freeze(); assert_eq!( bank.get_balance(&leader), @@ -2918,29 +2901,12 @@ fn test_filter_program_errors_and_collect_fee() { #[test] fn test_filter_program_errors_and_collect_priority_fee() { let leader = solana_sdk::pubkey::new_rand(); - let GenesisConfigInfo { - genesis_config, - mint_keypair, - .. - } = create_genesis_config_with_leader(1000000, &leader, 3); + let GenesisConfigInfo { genesis_config, .. } = + create_genesis_config_with_leader(1000000, &leader, 3); let mut bank = Bank::new_for_tests(&genesis_config); // this test is only for when `feature_set::reward_full_priority_fee` inactivated bank.deactivate_feature(&feature_set::reward_full_priority_fee::id()); - let key = solana_sdk::pubkey::new_rand(); - let tx1 = SanitizedTransaction::from_transaction_for_tests(system_transaction::transfer( - &mint_keypair, - &key, - 2, - genesis_config.hash(), - )); - let tx2 = SanitizedTransaction::from_transaction_for_tests(system_transaction::transfer( - &mint_keypair, - &key, - 5, - genesis_config.hash(), - )); - let priority_fee = 42; let fee_details: FeeDetails = FeeDetails::new_for_tests(0, priority_fee, false); let results = vec![ @@ -2956,7 +2922,7 @@ fn test_filter_program_errors_and_collect_priority_fee() { ]; let initial_balance = bank.get_balance(&leader); - let results = bank.filter_program_errors_and_collect_fee(&[tx1, tx2], &results); + let results = bank.filter_program_errors_and_collect_fee(&results); bank.freeze(); assert_eq!( bank.get_balance(&leader), @@ -12935,18 +12901,15 @@ fn test_failed_simulation_compute_units() { #[test] fn test_filter_program_errors_and_collect_fee_details() { - // TX | EXECUTION RESULT | is nonce | COLLECT | ADDITIONAL | COLLECT - // | | | (TX_FEE, PRIO_FEE) | WITHDRAW FROM PAYER | RESULT - // ------------------------------------------------------------------------------------------------------ - // tx1 | not executed | n/a | (0 , 0) | 0 | Original Err - // tx2 | executed and no error | n/a | (5_000, 1_000) | 0 | Ok - // tx3 | executed has error | true | (5_000, 1_000) | 0 | Ok - // tx4 | executed has error | false | (5_000, 1_000) | 6_000 | Ok - // tx5 | executed error, - // payer insufficient fund | false | (0 , 0) | 0 | InsufficientFundsForFee + // TX | EXECUTION RESULT | is nonce | COLLECT | COLLECT + // | | | (TX_FEE, PRIO_FEE) | RESULT + // --------------------------------------------------------------------------------- + // tx1 | not executed | n/a | (0 , 0) | Original Err + // tx2 | executed and no error | n/a | (5_000, 1_000) | Ok + // tx3 | executed has error | true | (5_000, 1_000) | Ok + // tx4 | executed has error | false | (5_000, 1_000) | Ok // let initial_payer_balance = 7_000; - let additional_payer_withdraw = 6_000; let tx_fee = 5000; let priority_fee = 1000; let tx_fee_details = FeeDetails::new_for_tests(tx_fee, priority_fee, false); @@ -12960,7 +12923,6 @@ fn test_filter_program_errors_and_collect_fee_details() { Ok(()), Ok(()), Ok(()), - Err(TransactionError::InsufficientFundsForFee), ]; let GenesisConfigInfo { @@ -12970,18 +12932,6 @@ fn test_filter_program_errors_and_collect_fee_details() { } = create_genesis_config_with_leader(initial_payer_balance, &Pubkey::new_unique(), 3); let bank = Bank::new_for_tests(&genesis_config); - let tx = SanitizedTransaction::from_transaction_for_tests(Transaction::new_signed_with_payer( - &[system_instruction::transfer( - &mint_keypair.pubkey(), - &Pubkey::new_unique(), - 2, - )], - Some(&mint_keypair.pubkey()), - &[&mint_keypair], - genesis_config.hash(), - )); - let txs = vec![tx.clone(), tx.clone(), tx.clone(), tx.clone(), tx]; - let results = vec![ TransactionExecutionResult::NotExecuted(TransactionError::AccountNotFound), new_execution_result(Ok(()), None, tx_fee_details), @@ -12990,10 +12940,9 @@ fn test_filter_program_errors_and_collect_fee_details() { 0, SystemError::ResultWithNegativeLamports.into(), )), - Some(&NonceFull::new( + Some(&NoncePartial::new( Pubkey::new_unique(), AccountSharedData::default(), - None, )), tx_fee_details, ), @@ -13005,71 +12954,21 @@ fn test_filter_program_errors_and_collect_fee_details() { None, tx_fee_details, ), - new_execution_result(Err(TransactionError::AccountNotFound), None, tx_fee_details), ]; - let results = bank.filter_program_errors_and_collect_fee_details(&txs, &results); + let results = bank.filter_program_errors_and_collect_fee_details(&results); assert_eq!( expected_collected_fee_details, *bank.collector_fee_details.read().unwrap() ); assert_eq!( - initial_payer_balance - additional_payer_withdraw, + initial_payer_balance, bank.get_balance(&mint_keypair.pubkey()) ); assert_eq!(expected_collect_results, results); } -#[test] -fn test_check_execution_status_and_charge_fee() { - let fee = 5000; - let initial_balance = fee - 1000; - let tx_error = - TransactionError::InstructionError(0, InstructionError::MissingRequiredSignature); - let GenesisConfigInfo { - mut genesis_config, - mint_keypair, - .. - } = create_genesis_config_with_leader(initial_balance, &Pubkey::new_unique(), 3); - genesis_config.fee_rate_governor = FeeRateGovernor::new(5000, 0); - let bank = Bank::new_for_tests(&genesis_config); - let message = new_sanitized_message(Message::new( - &[system_instruction::transfer( - &mint_keypair.pubkey(), - &Pubkey::new_unique(), - 1, - )], - Some(&mint_keypair.pubkey()), - )); - - [Ok(()), Err(tx_error)] - .iter() - .flat_map(|result| [true, false].iter().map(move |is_nonce| (result, is_nonce))) - .for_each(|(result, is_nonce)| { - if result.is_err() && !is_nonce { - assert_eq!( - Err(TransactionError::InsufficientFundsForFee), - bank.check_execution_status_and_charge_fee(&message, result, *is_nonce, fee) - ); - assert_eq!(initial_balance, bank.get_balance(&mint_keypair.pubkey())); - - let small_fee = 1; - assert!(bank - .check_execution_status_and_charge_fee(&message, result, *is_nonce, small_fee) - .is_ok()); - assert_eq!( - initial_balance - small_fee, - bank.get_balance(&mint_keypair.pubkey()) - ); - } else { - assert!(bank - .check_execution_status_and_charge_fee(&message, result, *is_nonce, fee) - .is_ok()); - assert_eq!(initial_balance, bank.get_balance(&mint_keypair.pubkey())); - } - }); -} #[test] fn test_deploy_last_epoch_slot() { solana_logger::setup(); diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index a813d4b4ffd788..3fa5d133f7cb34 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -432,7 +432,7 @@ mod tests { use { super::*, crate::{ - nonce_info::NonceFull, transaction_account_state_info::TransactionAccountStateInfo, + transaction_account_state_info::TransactionAccountStateInfo, transaction_processing_callback::TransactionProcessingCallback, }, nonce::state::Versions as NonceVersions, @@ -1107,6 +1107,70 @@ mod tests { assert_eq!(shared_data, expected); } + #[test] + fn test_load_transaction_accounts_fee_payer() { + let fee_payer_address = Pubkey::new_unique(); + let message = Message { + account_keys: vec![fee_payer_address], + header: MessageHeader::default(), + instructions: vec![], + recent_blockhash: Hash::default(), + }; + + let sanitized_message = new_unchecked_sanitized_message(message); + let mut mock_bank = TestCallbacks::default(); + + let fee_payer_balance = 200; + let mut fee_payer_account_data = AccountSharedData::default(); + fee_payer_account_data.set_lamports(fee_payer_balance); + mock_bank + .accounts_map + .insert(fee_payer_address, fee_payer_account_data.clone()); + let fee_payer_rent_debit = 42; + + let mut error_metrics = TransactionErrorMetrics::default(); + let loaded_programs = ProgramCacheForTxBatch::default(); + + let sanitized_transaction = SanitizedTransaction::new_for_tests( + sanitized_message, + vec![Signature::new_unique()], + false, + ); + let result = load_transaction_accounts( + &mock_bank, + sanitized_transaction.message(), + ValidatedTransactionDetails { + rollback_accounts: RollbackAccounts::default(), + fee_details: FeeDetails::default(), + fee_payer_account: fee_payer_account_data.clone(), + fee_payer_rent_debit, + }, + &mut error_metrics, + None, + &FeatureSet::default(), + &RentCollector::default(), + &loaded_programs, + ); + + let expected_rent_debits = { + let mut rent_debits = RentDebits::default(); + rent_debits.insert(&fee_payer_address, fee_payer_rent_debit, fee_payer_balance); + rent_debits + }; + assert_eq!( + result.unwrap(), + LoadedTransaction { + accounts: vec![(fee_payer_address, fee_payer_account_data),], + program_indices: vec![], + fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::default(), + rent: fee_payer_rent_debit, + rent_debits: expected_rent_debits, + loaded_accounts_data_size: 0, + } + ); + } + #[test] fn test_load_transaction_accounts_native_loader() { let key1 = Keypair::new(); @@ -1140,14 +1204,13 @@ mod tests { vec![Signature::new_unique()], false, ); - let fee_details = FeeDetails::new_for_tests(32, 0, false); let result = load_transaction_accounts( &mock_bank, sanitized_transaction.message(), ValidatedTransactionDetails { - nonce: None, - fee_details, - fee_payer_account: fee_payer_account_data, + rollback_accounts: RollbackAccounts::default(), + fee_details: FeeDetails::default(), + fee_payer_account: fee_payer_account_data.clone(), fee_payer_rent_debit: 0, }, &mut error_metrics, @@ -1161,18 +1224,15 @@ mod tests { result.unwrap(), LoadedTransaction { accounts: vec![ - ( - key1.pubkey(), - mock_bank.accounts_map[&key1.pubkey()].clone() - ), + (key1.pubkey(), fee_payer_account_data), ( native_loader::id(), mock_bank.accounts_map[&native_loader::id()].clone() ) ], program_indices: vec![vec![]], - nonce: None, - fee_details, + fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::default(), rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, @@ -1352,14 +1412,13 @@ mod tests { vec![Signature::new_unique()], false, ); - let fee_details = FeeDetails::new_for_tests(32, 0, false); let result = load_transaction_accounts( &mock_bank, sanitized_transaction.message(), ValidatedTransactionDetails { - nonce: None, - fee_details, - fee_payer_account: fee_payer_account_data, + rollback_accounts: RollbackAccounts::default(), + fee_details: FeeDetails::default(), + fee_payer_account: fee_payer_account_data.clone(), fee_payer_rent_debit: 0, }, &mut error_metrics, @@ -1373,17 +1432,14 @@ mod tests { result.unwrap(), LoadedTransaction { accounts: vec![ - ( - key2.pubkey(), - mock_bank.accounts_map[&key2.pubkey()].clone() - ), + (key2.pubkey(), fee_payer_account_data), ( key1.pubkey(), mock_bank.accounts_map[&key1.pubkey()].clone() ), ], - nonce: None, - fee_details, + fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::default(), program_indices: vec![vec![1]], rent: 0, rent_debits: RentDebits::default(), @@ -1538,14 +1594,13 @@ mod tests { vec![Signature::new_unique()], false, ); - let fee_details = FeeDetails::new_for_tests(32, 0, false); let result = load_transaction_accounts( &mock_bank, sanitized_transaction.message(), ValidatedTransactionDetails { - nonce: None, - fee_details, - fee_payer_account: fee_payer_account_data, + rollback_accounts: RollbackAccounts::default(), + fee_details: FeeDetails::default(), + fee_payer_account: fee_payer_account_data.clone(), fee_payer_rent_debit: 0, }, &mut error_metrics, @@ -1559,10 +1614,7 @@ mod tests { result.unwrap(), LoadedTransaction { accounts: vec![ - ( - key2.pubkey(), - mock_bank.accounts_map[&key2.pubkey()].clone() - ), + (key2.pubkey(), fee_payer_account_data), ( key1.pubkey(), mock_bank.accounts_map[&key1.pubkey()].clone() @@ -1573,8 +1625,8 @@ mod tests { ), ], program_indices: vec![vec![2, 1]], - nonce: None, - fee_details, + fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::default(), rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, @@ -1633,14 +1685,13 @@ mod tests { vec![Signature::new_unique()], false, ); - let fee_details = FeeDetails::new_for_tests(32, 0, false); let result = load_transaction_accounts( &mock_bank, sanitized_transaction.message(), ValidatedTransactionDetails { - nonce: None, - fee_details, - fee_payer_account: fee_payer_account_data, + rollback_accounts: RollbackAccounts::default(), + fee_details: FeeDetails::default(), + fee_payer_account: fee_payer_account_data.clone(), fee_payer_rent_debit: 0, }, &mut error_metrics, @@ -1656,10 +1707,7 @@ mod tests { result.unwrap(), LoadedTransaction { accounts: vec![ - ( - key2.pubkey(), - mock_bank.accounts_map[&key2.pubkey()].clone() - ), + (key2.pubkey(), fee_payer_account_data), ( key1.pubkey(), mock_bank.accounts_map[&key1.pubkey()].clone() @@ -1671,8 +1719,8 @@ mod tests { ), ], program_indices: vec![vec![3, 1], vec![3, 1]], - nonce: None, - fee_details, + fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::default(), rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, @@ -1793,7 +1841,7 @@ mod tests { false, ); let validation_result = Ok(ValidatedTransactionDetails { - nonce: None, + rollback_accounts: RollbackAccounts::default(), fee_details: FeeDetails::default(), fee_payer_account: fee_payer_account_data, fee_payer_rent_debit: 0, @@ -1834,8 +1882,8 @@ mod tests { ), ], program_indices: vec![vec![3, 1], vec![3, 1]], - nonce: None, fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::default(), rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, @@ -1868,7 +1916,7 @@ mod tests { ); let validation_result = Ok(ValidatedTransactionDetails { - nonce: Some(NonceFull::default()), + rollback_accounts: RollbackAccounts::default(), fee_details: FeeDetails::default(), fee_payer_account: AccountSharedData::default(), fee_payer_rent_debit: 0, diff --git a/svm/src/nonce_info.rs b/svm/src/nonce_info.rs index b96a8e24619649..062b8fc221f87f 100644 --- a/svm/src/nonce_info.rs +++ b/svm/src/nonce_info.rs @@ -41,40 +41,21 @@ mod tests { super::*, solana_sdk::{ hash::Hash, - instruction::Instruction, - message::{Message, SanitizedMessage}, - nonce::{self, state::DurableNonce}, - reserved_account_keys::ReservedAccountKeys, - signature::{keypair_from_seed, Signer}, - system_instruction, system_program, + nonce::state::{ + Data as NonceData, DurableNonce, State as NonceState, Versions as NonceVersions, + }, + system_program, }, }; - fn new_sanitized_message( - instructions: &[Instruction], - payer: Option<&Pubkey>, - ) -> SanitizedMessage { - SanitizedMessage::try_from_legacy_message( - Message::new(instructions, payer), - &ReservedAccountKeys::empty_key_set(), - ) - .unwrap() - } - #[test] fn test_nonce_info() { - let lamports_per_signature = 42; - - let nonce_authority = keypair_from_seed(&[0; 32]).unwrap(); - let nonce_address = nonce_authority.pubkey(); - let from = keypair_from_seed(&[1; 32]).unwrap(); - let from_address = from.pubkey(); - let to_address = Pubkey::new_unique(); - + let nonce_address = Pubkey::new_unique(); let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); + let lamports_per_signature = 42; let nonce_account = AccountSharedData::new_data( 43, - &nonce::state::Versions::new(nonce::State::Initialized(nonce::state::Data::new( + &NonceVersions::new(NonceState::Initialized(NonceData::new( Pubkey::default(), durable_nonce, lamports_per_signature, @@ -82,71 +63,15 @@ mod tests { &system_program::id(), ) .unwrap(); - let from_account = AccountSharedData::new(44, 0, &Pubkey::default()); - - const TEST_RENT_DEBIT: u64 = 1; - let rent_collected_nonce_account = { - let mut account = nonce_account.clone(); - account.set_lamports(nonce_account.lamports() - TEST_RENT_DEBIT); - account - }; - let rent_collected_from_account = { - let mut account = from_account.clone(); - account.set_lamports(from_account.lamports() - TEST_RENT_DEBIT); - account - }; - - let instructions = vec![ - system_instruction::advance_nonce_account(&nonce_address, &nonce_authority.pubkey()), - system_instruction::transfer(&from_address, &to_address, 42), - ]; // NoncePartial create + NonceInfo impl - let partial = NoncePartial::new(nonce_address, rent_collected_nonce_account.clone()); + let partial = NoncePartial::new(nonce_address, nonce_account.clone()); assert_eq!(*partial.address(), nonce_address); - assert_eq!(*partial.account(), rent_collected_nonce_account); + assert_eq!(*partial.account(), nonce_account); assert_eq!( partial.lamports_per_signature(), Some(lamports_per_signature) ); assert_eq!(partial.fee_payer_account(), None); - - // NonceFull create + NonceInfo impl - { - let message = new_sanitized_message(&instructions, Some(&from_address)); - let fee_payer_address = message.account_keys().get(0).unwrap(); - let fee_payer_account = rent_collected_from_account.clone(); - let full = NonceFull::from_partial( - partial.clone(), - fee_payer_address, - fee_payer_account, - TEST_RENT_DEBIT, - ); - assert_eq!(*full.address(), nonce_address); - assert_eq!(*full.account(), rent_collected_nonce_account); - assert_eq!(full.lamports_per_signature(), Some(lamports_per_signature)); - assert_eq!( - full.fee_payer_account(), - Some(&from_account), - "rent debit should be refunded in captured fee account" - ); - } - - // Nonce account is fee-payer - { - let message = new_sanitized_message(&instructions, Some(&nonce_address)); - let fee_payer_address = message.account_keys().get(0).unwrap(); - let fee_payer_account = rent_collected_nonce_account; - let full = NonceFull::from_partial( - partial, - fee_payer_address, - fee_payer_account, - TEST_RENT_DEBIT, - ); - assert_eq!(*full.address(), nonce_address); - assert_eq!(*full.account(), nonce_account); - assert_eq!(full.lamports_per_signature(), Some(lamports_per_signature)); - assert_eq!(full.fee_payer_account(), None); - } } } diff --git a/svm/src/rollback_accounts.rs b/svm/src/rollback_accounts.rs index fa6688a7ce8197..8d9ba4b1393a58 100644 --- a/svm/src/rollback_accounts.rs +++ b/svm/src/rollback_accounts.rs @@ -90,3 +90,139 @@ impl RollbackAccounts { } } } + +#[cfg(test)] +mod tests { + use { + super::*, + solana_sdk::{ + account::{ReadableAccount, WritableAccount}, + hash::Hash, + nonce::state::{ + Data as NonceData, DurableNonce, State as NonceState, Versions as NonceVersions, + }, + system_program, + }, + }; + + #[test] + fn test_new_fee_payer_only() { + let fee_payer_address = Pubkey::new_unique(); + let fee_payer_account = AccountSharedData::new(100, 0, &Pubkey::default()); + let fee_payer_rent_epoch = fee_payer_account.rent_epoch(); + + const TEST_RENT_DEBIT: u64 = 1; + let rent_collected_fee_payer_account = { + let mut account = fee_payer_account.clone(); + account.set_lamports(fee_payer_account.lamports() - TEST_RENT_DEBIT); + account.set_rent_epoch(fee_payer_rent_epoch + 1); + account + }; + + let rollback_accounts = RollbackAccounts::new( + None, + fee_payer_address, + rent_collected_fee_payer_account, + TEST_RENT_DEBIT, + fee_payer_rent_epoch, + ); + + let expected_fee_payer_account = fee_payer_account; + match rollback_accounts { + RollbackAccounts::FeePayerOnly { fee_payer_account } => { + assert_eq!(expected_fee_payer_account, fee_payer_account); + } + _ => panic!("Expected FeePayerOnly variant"), + } + } + + #[test] + fn test_new_same_nonce_and_fee_payer() { + let nonce_address = Pubkey::new_unique(); + let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); + let lamports_per_signature = 42; + let nonce_account = AccountSharedData::new_data( + 43, + &NonceVersions::new(NonceState::Initialized(NonceData::new( + Pubkey::default(), + durable_nonce, + lamports_per_signature, + ))), + &system_program::id(), + ) + .unwrap(); + + const TEST_RENT_DEBIT: u64 = 1; + let rent_collected_nonce_account = { + let mut account = nonce_account.clone(); + account.set_lamports(nonce_account.lamports() - TEST_RENT_DEBIT); + account + }; + + let nonce = NoncePartial::new(nonce_address, rent_collected_nonce_account.clone()); + let rollback_accounts = RollbackAccounts::new( + Some(nonce), + nonce_address, + rent_collected_nonce_account, + TEST_RENT_DEBIT, + u64::MAX, // ignored + ); + + match rollback_accounts { + RollbackAccounts::SameNonceAndFeePayer { nonce } => { + assert_eq!(nonce.address(), &nonce_address); + assert_eq!(nonce.account(), &nonce_account); + } + _ => panic!("Expected SameNonceAndFeePayer variant"), + } + } + + #[test] + fn test_separate_nonce_and_fee_payer() { + let nonce_address = Pubkey::new_unique(); + let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); + let lamports_per_signature = 42; + let nonce_account = AccountSharedData::new_data( + 43, + &NonceVersions::new(NonceState::Initialized(NonceData::new( + Pubkey::default(), + durable_nonce, + lamports_per_signature, + ))), + &system_program::id(), + ) + .unwrap(); + + let fee_payer_address = Pubkey::new_unique(); + let fee_payer_account = AccountSharedData::new(44, 0, &Pubkey::default()); + + const TEST_RENT_DEBIT: u64 = 1; + let rent_collected_fee_payer_account = { + let mut account = fee_payer_account.clone(); + account.set_lamports(fee_payer_account.lamports() - TEST_RENT_DEBIT); + account + }; + + let nonce = NoncePartial::new(nonce_address, nonce_account.clone()); + let rollback_accounts = RollbackAccounts::new( + Some(nonce), + fee_payer_address, + rent_collected_fee_payer_account.clone(), + TEST_RENT_DEBIT, + u64::MAX, // ignored + ); + + let expected_fee_payer_account = fee_payer_account; + match rollback_accounts { + RollbackAccounts::SeparateNonceAndFeePayer { + nonce, + fee_payer_account, + } => { + assert_eq!(nonce.address(), &nonce_address); + assert_eq!(nonce.account(), &nonce_account); + assert_eq!(expected_fee_payer_account, fee_payer_account); + } + _ => panic!("Expected SeparateNonceAndFeePayer variant"), + } + } +} diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 90b1df4f7c48e1..fef9f6943a94fe 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -999,7 +999,7 @@ impl TransactionBatchProcessor { mod tests { use { super::*, - crate::account_loader::ValidatedTransactionDetails, + crate::{account_loader::ValidatedTransactionDetails, rollback_accounts::RollbackAccounts}, solana_program_runtime::loaded_programs::{BlockRelation, ProgramCacheEntryType}, solana_sdk::{ account::{create_account_shared_data_for_test, WritableAccount}, @@ -1158,8 +1158,8 @@ mod tests { let mut loaded_transaction = LoadedTransaction { accounts: vec![(Pubkey::new_unique(), AccountSharedData::default())], program_indices: vec![vec![0]], - nonce: None, fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::default(), rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 32, @@ -1285,8 +1285,8 @@ mod tests { (key2, AccountSharedData::default()), ], program_indices: vec![vec![0]], - nonce: None, fee_details: FeeDetails::default(), + rollback_accounts: RollbackAccounts::default(), rent: 0, rent_debits: RentDebits::default(), loaded_accounts_data_size: 0, @@ -1966,7 +1966,11 @@ mod tests { &Hash::new_unique(), )); let fee_payer_address = message.fee_payer(); - let rent_collector = RentCollector::default(); + let current_epoch = 42; + let rent_collector = RentCollector { + epoch: current_epoch, + ..RentCollector::default() + }; let min_balance = rent_collector.rent.minimum_balance(nonce::State::size()); let transaction_fee = lamports_per_signature; let priority_fee = 2_000_000u64; @@ -1977,7 +1981,13 @@ mod tests { so ensure that the starting balance is more than the min balance" ); - let fee_payer_account = AccountSharedData::new(starting_balance, 0, &Pubkey::default()); + let fee_payer_rent_epoch = current_epoch; + let fee_payer_account = AccountSharedData::new_rent_epoch( + starting_balance, + 0, + &Pubkey::default(), + fee_payer_rent_epoch, + ); let mut mock_accounts = HashMap::new(); mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); let mock_bank = MockBankCallback { @@ -2008,7 +2018,8 @@ mod tests { Ok(( FeeDetails::new_for_tests(transaction_fee, priority_fee, false), post_validation_fee_payer_account, - 0 // rent due + 0, // rent due + fee_payer_rent_epoch, )) ); } @@ -2068,6 +2079,7 @@ mod tests { FeeDetails::new_for_tests(transaction_fee, 0, false), post_validation_fee_payer_account, rent_due, + 0, // rent epoch )) ); } @@ -2248,7 +2260,8 @@ mod tests { Ok(( FeeDetails::new_for_tests(transaction_fee, priority_fee, false), post_validation_fee_payer_account, - 0 // rent due + 0, // rent due + 0, // rent epoch )) ); } From 40b0c7b5ee3ef8fea9272237d76055b83e5f8009 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Mon, 17 Jun 2024 07:08:15 +0000 Subject: [PATCH 3/5] feedback --- accounts-db/src/accounts.rs | 30 ++++++---------- rpc/src/transaction_status_service.rs | 1 - runtime/src/bank/tests.rs | 49 +++++++-------------------- svm/src/rollback_accounts.rs | 3 +- svm/src/transaction_processor.rs | 3 +- svm/src/transaction_results.rs | 1 - 6 files changed, 25 insertions(+), 62 deletions(-) diff --git a/accounts-db/src/accounts.rs b/accounts-db/src/accounts.rs index 7e43e80e4ddc29..be168b3edefecc 100644 --- a/accounts-db/src/accounts.rs +++ b/accounts-db/src/accounts.rs @@ -785,6 +785,8 @@ fn post_process_failed_tx( &durable_nonce: &DurableNonce, lamports_per_signature: u64, ) { + // For the case of RollbackAccounts::SameNonceAndFeePayer, it's crucial + // for `is_nonce_account` to be checked earlier than `is_fee_payer`. if is_nonce_account { if let Some(nonce) = rollback_accounts.nonce() { // The transaction failed which would normally drop the account @@ -859,17 +861,13 @@ mod tests { )) } - fn new_execution_result( - status: Result<()>, - nonce: Option<&NoncePartial>, - ) -> TransactionExecutionResult { + fn new_execution_result(status: Result<()>) -> TransactionExecutionResult { TransactionExecutionResult::Executed { details: TransactionExecutionDetails { status, log_messages: None, inner_instructions: None, fee_details: FeeDetails::default(), - is_nonce: nonce.is_some(), return_data: None, executed_units: 0, accounts_data_len_delta: 0, @@ -1595,7 +1593,7 @@ mod tests { .insert_new_readonly(&pubkey); } let txs = vec![tx0.clone(), tx1.clone()]; - let execution_results = vec![new_execution_result(Ok(()), None); 2]; + let execution_results = vec![new_execution_result(Ok(())); 2]; let (collected_accounts, transactions) = accounts.collect_accounts_to_store( &txs, &execution_results, @@ -1842,13 +1840,9 @@ mod tests { let accounts_db = AccountsDb::new_single_for_tests(); let accounts = Accounts::new(Arc::new(accounts_db)); let txs = vec![tx]; - let execution_results = vec![new_execution_result( - Err(TransactionError::InstructionError( - 1, - InstructionError::InvalidArgument, - )), - Some(&nonce), - )]; + let execution_results = vec![new_execution_result(Err( + TransactionError::InstructionError(1, InstructionError::InvalidArgument), + ))]; let (collected_accounts, _) = accounts.collect_accounts_to_store( &txs, &execution_results, @@ -1945,13 +1939,9 @@ mod tests { let accounts_db = AccountsDb::new_single_for_tests(); let accounts = Accounts::new(Arc::new(accounts_db)); let txs = vec![tx]; - let execution_results = vec![new_execution_result( - Err(TransactionError::InstructionError( - 1, - InstructionError::InvalidArgument, - )), - Some(&nonce), - )]; + let execution_results = vec![new_execution_result(Err( + TransactionError::InstructionError(1, InstructionError::InvalidArgument), + ))]; let (collected_accounts, _) = accounts.collect_accounts_to_store( &txs, &execution_results, diff --git a/rpc/src/transaction_status_service.rs b/rpc/src/transaction_status_service.rs index aef87f84eb4dea..f8356296e970a6 100644 --- a/rpc/src/transaction_status_service.rs +++ b/rpc/src/transaction_status_service.rs @@ -330,7 +330,6 @@ pub(crate) mod tests { log_messages: None, inner_instructions: None, fee_details: FeeDetails::default(), - is_nonce: true, return_data: None, executed_units: 0, accounts_data_len_delta: 0, diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 59c7950f64c85e..ba9e4d323535c5 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -231,18 +231,13 @@ fn test_race_register_tick_freeze() { } } -fn new_execution_result( - status: Result<()>, - nonce: Option<&NoncePartial>, - fee_details: FeeDetails, -) -> TransactionExecutionResult { +fn new_execution_result(status: Result<()>, fee_details: FeeDetails) -> TransactionExecutionResult { TransactionExecutionResult::Executed { details: TransactionExecutionDetails { status, log_messages: None, inner_instructions: None, fee_details, - is_nonce: nonce.is_some(), return_data: None, executed_units: 0, accounts_data_len_delta: 0, @@ -2876,13 +2871,12 @@ fn test_filter_program_errors_and_collect_fee() { let tx_fee = 42; let fee_details = FeeDetails::new_for_tests(tx_fee, 0, false); let results = vec![ - new_execution_result(Ok(()), None, fee_details), + new_execution_result(Ok(()), fee_details), new_execution_result( Err(TransactionError::InstructionError( 1, SystemError::ResultWithNegativeLamports.into(), )), - None, fee_details, ), ]; @@ -2910,13 +2904,12 @@ fn test_filter_program_errors_and_collect_priority_fee() { let priority_fee = 42; let fee_details: FeeDetails = FeeDetails::new_for_tests(0, priority_fee, false); let results = vec![ - new_execution_result(Ok(()), None, fee_details), + new_execution_result(Ok(()), fee_details), new_execution_result( Err(TransactionError::InstructionError( 1, SystemError::ResultWithNegativeLamports.into(), )), - None, fee_details, ), ]; @@ -12901,29 +12894,23 @@ fn test_failed_simulation_compute_units() { #[test] fn test_filter_program_errors_and_collect_fee_details() { - // TX | EXECUTION RESULT | is nonce | COLLECT | COLLECT - // | | | (TX_FEE, PRIO_FEE) | RESULT + // TX | EXECUTION RESULT | COLLECT | COLLECT + // | | (TX_FEE, PRIO_FEE) | RESULT // --------------------------------------------------------------------------------- - // tx1 | not executed | n/a | (0 , 0) | Original Err - // tx2 | executed and no error | n/a | (5_000, 1_000) | Ok - // tx3 | executed has error | true | (5_000, 1_000) | Ok - // tx4 | executed has error | false | (5_000, 1_000) | Ok + // tx1 | not executed | (0 , 0) | Original Err + // tx2 | executed and no error | (5_000, 1_000) | Ok + // tx3 | executed has error | (5_000, 1_000) | Ok // let initial_payer_balance = 7_000; let tx_fee = 5000; let priority_fee = 1000; let tx_fee_details = FeeDetails::new_for_tests(tx_fee, priority_fee, false); let expected_collected_fee_details = CollectorFeeDetails { - transaction_fee: 3 * tx_fee, - priority_fee: 3 * priority_fee, + transaction_fee: 2 * tx_fee, + priority_fee: 2 * priority_fee, }; - let expected_collect_results = vec![ - Err(TransactionError::AccountNotFound), - Ok(()), - Ok(()), - Ok(()), - ]; + let expected_collect_results = vec![Err(TransactionError::AccountNotFound), Ok(()), Ok(())]; let GenesisConfigInfo { genesis_config, @@ -12934,24 +12921,12 @@ fn test_filter_program_errors_and_collect_fee_details() { let results = vec![ TransactionExecutionResult::NotExecuted(TransactionError::AccountNotFound), - new_execution_result(Ok(()), None, tx_fee_details), - new_execution_result( - Err(TransactionError::InstructionError( - 0, - SystemError::ResultWithNegativeLamports.into(), - )), - Some(&NoncePartial::new( - Pubkey::new_unique(), - AccountSharedData::default(), - )), - tx_fee_details, - ), + new_execution_result(Ok(()), tx_fee_details), new_execution_result( Err(TransactionError::InstructionError( 0, SystemError::ResultWithNegativeLamports.into(), )), - None, tx_fee_details, ), ]; diff --git a/svm/src/rollback_accounts.rs b/svm/src/rollback_accounts.rs index 8d9ba4b1393a58..6fbd3a9c2e91e8 100644 --- a/svm/src/rollback_accounts.rs +++ b/svm/src/rollback_accounts.rs @@ -2,6 +2,7 @@ use { crate::nonce_info::{NonceInfo, NoncePartial}, solana_sdk::{ account::{AccountSharedData, ReadableAccount, WritableAccount}, + clock::Epoch, pubkey::Pubkey, }, }; @@ -37,7 +38,7 @@ impl RollbackAccounts { fee_payer_address: Pubkey, mut fee_payer_account: AccountSharedData, fee_payer_rent_debit: u64, - fee_payer_loaded_rent_epoch: u64, + fee_payer_loaded_rent_epoch: Epoch, ) -> Self { // When the fee payer account is rolled back due to transaction failure, // rent should not be charged so credit the previously debited rent diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index fef9f6943a94fe..32245d49a99bf5 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -465,7 +465,7 @@ impl TransactionBatchProcessor { lamports_per_signature: u64, rent_collector: &RentCollector, error_counters: &mut TransactionErrorMetrics, - ) -> transaction::Result<(FeeDetails, AccountSharedData, u64, u64)> { + ) -> transaction::Result<(FeeDetails, AccountSharedData, u64, Epoch)> { let fee_payer_address = message.fee_payer(); let Some(mut fee_payer_account) = callbacks.get_account_shared_data(fee_payer_address) else { @@ -894,7 +894,6 @@ impl TransactionBatchProcessor { log_messages, inner_instructions, fee_details: loaded_transaction.fee_details, - is_nonce: loaded_transaction.rollback_accounts.nonce().is_some(), return_data, executed_units, accounts_data_len_delta, diff --git a/svm/src/transaction_results.rs b/svm/src/transaction_results.rs index 1fcc9f7ce4b7c5..9f829a675267ed 100644 --- a/svm/src/transaction_results.rs +++ b/svm/src/transaction_results.rs @@ -85,7 +85,6 @@ pub struct TransactionExecutionDetails { pub log_messages: Option>, pub inner_instructions: Option, pub fee_details: FeeDetails, - pub is_nonce: bool, pub return_data: Option, pub executed_units: u64, /// The change in accounts data len for this transaction. From ce1a9fd4e7437e8485bb775591c88b3ab7a8254f Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Tue, 18 Jun 2024 13:53:15 -0700 Subject: [PATCH 4/5] feedback --- svm/src/transaction_processor.rs | 190 ++++++++++++++++++------------- 1 file changed, 114 insertions(+), 76 deletions(-) diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 32245d49a99bf5..7c9c84f51364ae 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -42,7 +42,7 @@ use { include_loaded_accounts_data_size_in_fee_calculation, remove_rounding_in_fee_calculation, FeatureSet, }, - fee::{FeeDetails, FeeStructure}, + fee::FeeStructure, hash::Hash, inner_instruction::{InnerInstruction, InnerInstructionsList}, instruction::{CompiledInstruction, TRANSACTION_LEVEL_STACK_HEIGHT}, @@ -410,45 +410,18 @@ impl TransactionBatchProcessor { .iter() .zip(check_results) .map(|(sanitized_tx, check_result)| { - check_result.and_then( - |CheckedTransactionDetails { - nonce, - lamports_per_signature, - }| { - let message = sanitized_tx.borrow().message(); - let ( - fee_details, - fee_payer_account, - fee_payer_rent_debit, - fee_payer_rent_epoch, - ) = self.validate_transaction_fee_payer( - callbacks, - message, - feature_set, - fee_structure, - lamports_per_signature, - rent_collector, - error_counters, - )?; - - // Capture fee-subtracted fee payer account and original nonce account state - // to rollback to if transaction execution fails. - let rollback_accounts = RollbackAccounts::new( - nonce, - *message.fee_payer(), - fee_payer_account.clone(), - fee_payer_rent_debit, - fee_payer_rent_epoch, - ); - - Ok(ValidatedTransactionDetails { - rollback_accounts, - fee_details, - fee_payer_account, - fee_payer_rent_debit, - }) - }, - ) + check_result.and_then(|checked_details| { + let message = sanitized_tx.borrow().message(); + self.validate_transaction_fee_payer( + callbacks, + message, + checked_details, + feature_set, + fee_structure, + rent_collector, + error_counters, + ) + }) }) .collect() } @@ -460,12 +433,12 @@ impl TransactionBatchProcessor { &self, callbacks: &CB, message: &SanitizedMessage, + checked_details: CheckedTransactionDetails, feature_set: &FeatureSet, fee_structure: &FeeStructure, - lamports_per_signature: u64, rent_collector: &RentCollector, error_counters: &mut TransactionErrorMetrics, - ) -> transaction::Result<(FeeDetails, AccountSharedData, u64, Epoch)> { + ) -> transaction::Result { let fee_payer_address = message.fee_payer(); let Some(mut fee_payer_account) = callbacks.get_account_shared_data(fee_payer_address) else { @@ -473,7 +446,7 @@ impl TransactionBatchProcessor { return Err(TransactionError::AccountNotFound); }; - let fee_payer_rent_epoch = fee_payer_account.rent_epoch(); + let fee_payer_loaded_rent_epoch = fee_payer_account.rent_epoch(); let fee_payer_rent_debit = collect_rent_from_account( feature_set, rent_collector, @@ -482,6 +455,11 @@ impl TransactionBatchProcessor { ) .rent_amount; + let CheckedTransactionDetails { + nonce, + lamports_per_signature, + } = checked_details; + let fee_details = fee_structure.calculate_fee_details( message, lamports_per_signature, @@ -502,12 +480,22 @@ impl TransactionBatchProcessor { fee_details.total_fee(), )?; - Ok(( + // Capture fee-subtracted fee payer account and original nonce account state + // to rollback to if transaction execution fails. + let rollback_accounts = RollbackAccounts::new( + nonce, + *message.fee_payer(), + fee_payer_account.clone(), + fee_payer_rent_debit, + fee_payer_loaded_rent_epoch, + ); + + Ok(ValidatedTransactionDetails { fee_details, fee_payer_account, fee_payer_rent_debit, - fee_payer_rent_epoch, - )) + rollback_accounts, + }) } /// Returns a map from executable program accounts (all accounts owned by any loader) @@ -998,7 +986,10 @@ impl TransactionBatchProcessor { mod tests { use { super::*, - crate::{account_loader::ValidatedTransactionDetails, rollback_accounts::RollbackAccounts}, + crate::{ + account_loader::ValidatedTransactionDetails, nonce_info::NoncePartial, + rollback_accounts::RollbackAccounts, + }, solana_program_runtime::loaded_programs::{BlockRelation, ProgramCacheEntryType}, solana_sdk::{ account::{create_account_shared_data_for_test, WritableAccount}, @@ -1981,6 +1972,7 @@ mod tests { ); let fee_payer_rent_epoch = current_epoch; + let fee_payer_rent_debit = 0; let fee_payer_account = AccountSharedData::new_rent_epoch( starting_balance, 0, @@ -1998,9 +1990,12 @@ mod tests { let result = batch_processor.validate_transaction_fee_payer( &mock_bank, &message, + CheckedTransactionDetails { + nonce: None, + lamports_per_signature, + }, &FeatureSet::default(), &FeeStructure::default(), - lamports_per_signature, &rent_collector, &mut error_counters, ); @@ -2014,12 +2009,18 @@ mod tests { assert_eq!( result, - Ok(( - FeeDetails::new_for_tests(transaction_fee, priority_fee, false), - post_validation_fee_payer_account, - 0, // rent due - fee_payer_rent_epoch, - )) + Ok(ValidatedTransactionDetails { + rollback_accounts: RollbackAccounts::new( + None, // nonce + *fee_payer_address, + post_validation_fee_payer_account.clone(), + fee_payer_rent_debit, + fee_payer_rent_epoch + ), + fee_details: FeeDetails::new_for_tests(transaction_fee, priority_fee, false), + fee_payer_rent_debit, + fee_payer_account: post_validation_fee_payer_account, + }) ); } @@ -2038,14 +2039,14 @@ mod tests { let transaction_fee = lamports_per_signature; let starting_balance = min_balance - 1; let fee_payer_account = AccountSharedData::new(starting_balance, 0, &Pubkey::default()); - let rent_due = rent_collector + let fee_payer_rent_debit = rent_collector .get_rent_due( fee_payer_account.lamports(), fee_payer_account.data().len(), fee_payer_account.rent_epoch(), ) .lamports(); - assert!(rent_due > 0); + assert!(fee_payer_rent_debit > 0); let mut mock_accounts = HashMap::new(); mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); @@ -2058,9 +2059,12 @@ mod tests { let result = batch_processor.validate_transaction_fee_payer( &mock_bank, &message, + CheckedTransactionDetails { + nonce: None, + lamports_per_signature, + }, &FeatureSet::default(), &FeeStructure::default(), - lamports_per_signature, &rent_collector, &mut error_counters, ); @@ -2068,18 +2072,24 @@ mod tests { let post_validation_fee_payer_account = { let mut account = fee_payer_account.clone(); account.set_rent_epoch(1); - account.set_lamports(starting_balance - transaction_fee - rent_due); + account.set_lamports(starting_balance - transaction_fee - fee_payer_rent_debit); account }; assert_eq!( result, - Ok(( - FeeDetails::new_for_tests(transaction_fee, 0, false), - post_validation_fee_payer_account, - rent_due, - 0, // rent epoch - )) + Ok(ValidatedTransactionDetails { + rollback_accounts: RollbackAccounts::new( + None, // nonce + *fee_payer_address, + post_validation_fee_payer_account.clone(), + fee_payer_rent_debit, + 0, // rent epoch + ), + fee_details: FeeDetails::new_for_tests(transaction_fee, 0, false), + fee_payer_rent_debit, + fee_payer_account: post_validation_fee_payer_account, + }) ); } @@ -2095,9 +2105,12 @@ mod tests { let result = batch_processor.validate_transaction_fee_payer( &mock_bank, &message, + CheckedTransactionDetails { + nonce: None, + lamports_per_signature, + }, &FeatureSet::default(), &FeeStructure::default(), - lamports_per_signature, &RentCollector::default(), &mut error_counters, ); @@ -2124,9 +2137,12 @@ mod tests { let result = batch_processor.validate_transaction_fee_payer( &mock_bank, &message, + CheckedTransactionDetails { + nonce: None, + lamports_per_signature, + }, &FeatureSet::default(), &FeeStructure::default(), - lamports_per_signature, &RentCollector::default(), &mut error_counters, ); @@ -2157,9 +2173,12 @@ mod tests { let result = batch_processor.validate_transaction_fee_payer( &mock_bank, &message, + CheckedTransactionDetails { + nonce: None, + lamports_per_signature, + }, &FeatureSet::default(), &FeeStructure::default(), - lamports_per_signature, &rent_collector, &mut error_counters, ); @@ -2188,9 +2207,12 @@ mod tests { let result = batch_processor.validate_transaction_fee_payer( &mock_bank, &message, + CheckedTransactionDetails { + nonce: None, + lamports_per_signature, + }, &FeatureSet::default(), &FeeStructure::default(), - lamports_per_signature, &RentCollector::default(), &mut error_counters, ); @@ -2237,12 +2259,19 @@ mod tests { let mut error_counters = TransactionErrorMetrics::default(); let batch_processor = TransactionBatchProcessor::::default(); + let nonce = Some(NoncePartial::new( + *fee_payer_address, + fee_payer_account.clone(), + )); let result = batch_processor.validate_transaction_fee_payer( &mock_bank, &message, + CheckedTransactionDetails { + nonce: nonce.clone(), + lamports_per_signature, + }, &feature_set, &FeeStructure::default(), - lamports_per_signature, &rent_collector, &mut error_counters, ); @@ -2256,12 +2285,18 @@ mod tests { assert_eq!( result, - Ok(( - FeeDetails::new_for_tests(transaction_fee, priority_fee, false), - post_validation_fee_payer_account, - 0, // rent due - 0, // rent epoch - )) + Ok(ValidatedTransactionDetails { + rollback_accounts: RollbackAccounts::new( + nonce, + *fee_payer_address, + post_validation_fee_payer_account.clone(), + 0, // fee_payer_rent_debit + 0, // fee_payer_rent_epoch + ), + fee_details: FeeDetails::new_for_tests(transaction_fee, priority_fee, false), + fee_payer_rent_debit: 0, // rent due + fee_payer_account: post_validation_fee_payer_account, + }) ); } @@ -2287,9 +2322,12 @@ mod tests { let result = batch_processor.validate_transaction_fee_payer( &mock_bank, &message, + CheckedTransactionDetails { + nonce: None, + lamports_per_signature, + }, &feature_set, &FeeStructure::default(), - lamports_per_signature, &rent_collector, &mut error_counters, ); From b2d420c50d43968faf3d511f74997b6828256125 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Wed, 19 Jun 2024 08:55:27 -0700 Subject: [PATCH 5/5] feedback --- svm/src/transaction_processor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 7c9c84f51364ae..2aee0aecff619c 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -484,7 +484,7 @@ impl TransactionBatchProcessor { // to rollback to if transaction execution fails. let rollback_accounts = RollbackAccounts::new( nonce, - *message.fee_payer(), + *fee_payer_address, fee_payer_account.clone(), fee_payer_rent_debit, fee_payer_loaded_rent_epoch,