From ae57c0050bfc7ca85eefd191ee1f0500fa622b7a Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Wed, 31 Jul 2024 17:29:39 +0000 Subject: [PATCH] feedback --- svm/src/account_saver.rs | 27 +++++++++++++++------------ svm/src/nonce_info.rs | 14 +++++++------- svm/src/rollback_accounts.rs | 19 ------------------- 3 files changed, 22 insertions(+), 38 deletions(-) diff --git a/svm/src/account_saver.rs b/svm/src/account_saver.rs index e42dc55f012f28..cf70997eefa9d0 100644 --- a/svm/src/account_saver.rs +++ b/svm/src/account_saver.rs @@ -79,17 +79,20 @@ fn collect_accounts_for_successful_tx<'a>( transaction_accounts: &'a [TransactionAccount], ) { let message = transaction.message(); - for (i, (address, account)) in (0..message.account_keys().len()).zip(transaction_accounts) { - if message.is_writable(i) { - // 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 - // invoked account if said account isn't passed to some program. - if !message.is_invoked(i) || message.is_instruction_account(i) { - collected_accounts.push((address, account)); - collected_account_transactions.push(Some(transaction)); + for (_, (address, account)) in (0..message.account_keys().len()) + .zip(transaction_accounts) + .filter(|(i, _)| { + message.is_writable(*i) && { + // 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 + // invoked account if said account isn't passed to some program. + !message.is_invoked(*i) || message.is_instruction_account(*i) } - } + }) + { + collected_accounts.push((address, account)); + collected_account_transactions.push(Some(transaction)); } } @@ -112,7 +115,7 @@ fn collect_accounts_for_failed_tx<'a>( // Since we know we are dealing with a valid nonce account, // unwrap is safe here nonce - .try_advance_account(*durable_nonce, lamports_per_signature) + .try_advance_nonce(*durable_nonce, lamports_per_signature) .unwrap(); collected_accounts.push((nonce.address(), nonce.account())); collected_account_transactions.push(Some(transaction)); @@ -127,7 +130,7 @@ fn collect_accounts_for_failed_tx<'a>( // Since we know we are dealing with a valid nonce account, // unwrap is safe here nonce - .try_advance_account(*durable_nonce, lamports_per_signature) + .try_advance_nonce(*durable_nonce, lamports_per_signature) .unwrap(); collected_accounts.push((nonce.address(), nonce.account())); collected_account_transactions.push(Some(transaction)); diff --git a/svm/src/nonce_info.rs b/svm/src/nonce_info.rs index ffd185bdbe56f6..0c9e43e8bb87aa 100644 --- a/svm/src/nonce_info.rs +++ b/svm/src/nonce_info.rs @@ -39,7 +39,7 @@ impl NoncePartial { // Advance the stored blockhash to prevent fee theft by someone // replaying nonce transactions that have failed with an // `InstructionError`. - pub fn try_advance_account( + pub fn try_advance_nonce( &mut self, durable_nonce: DurableNonce, lamports_per_signature: u64, @@ -114,7 +114,7 @@ mod tests { } #[test] - fn test_try_advance_account_success() { + fn test_try_advance_nonce_success() { let authority = Pubkey::new_unique(); let mut nonce_partial = NoncePartial::new( Pubkey::new_unique(), @@ -127,7 +127,7 @@ mod tests { let new_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); let new_lamports_per_signature = 100; - let result = nonce_partial.try_advance_account(new_nonce, new_lamports_per_signature); + let result = nonce_partial.try_advance_nonce(new_nonce, new_lamports_per_signature); assert_eq!(result, Ok(())); let nonce_versions = StateMut::::state(&nonce_partial.account).unwrap(); @@ -142,26 +142,26 @@ mod tests { } #[test] - fn test_try_advance_account_invalid() { + fn test_try_advance_nonce_invalid() { let mut nonce_partial = NoncePartial::new( Pubkey::new_unique(), AccountSharedData::new(1_000_000, 0, &Pubkey::default()), ); let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); - let result = nonce_partial.try_advance_account(durable_nonce, 5000); + let result = nonce_partial.try_advance_nonce(durable_nonce, 5000); assert_eq!(result, Err(AdvanceNonceError::Invalid)); } #[test] - fn test_try_advance_account_uninitialized() { + fn test_try_advance_nonce_uninitialized() { let mut nonce_partial = NoncePartial::new( Pubkey::new_unique(), create_nonce_account(NonceState::Uninitialized), ); let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); - let result = nonce_partial.try_advance_account(durable_nonce, 5000); + let result = nonce_partial.try_advance_nonce(durable_nonce, 5000); assert_eq!(result, Err(AdvanceNonceError::Uninitialized)); } } diff --git a/svm/src/rollback_accounts.rs b/svm/src/rollback_accounts.rs index 97f245b43e8aca..84f92aec5a225a 100644 --- a/svm/src/rollback_accounts.rs +++ b/svm/src/rollback_accounts.rs @@ -72,25 +72,6 @@ impl RollbackAccounts { } } - 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(), - } - } - /// Number of accounts tracked for rollback pub fn count(&self) -> usize { match self {