diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index f0b8ddda7324f9..77319f31835486 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -47,10 +47,10 @@ use { hash::Hash, inner_instruction::{InnerInstruction, InnerInstructionsList}, instruction::{CompiledInstruction, TRANSACTION_LEVEL_STACK_HEIGHT}, - nonce::state::{State as NonceState, Versions as NonceVersions}, + nonce::state::{DurableNonce, State as NonceState, Versions as NonceVersions}, pubkey::Pubkey, rent_collector::RentCollector, - saturating_add_assign, + saturating_add_assign, system_program, transaction::{self, TransactionError}, transaction_context::{ExecutionRecord, TransactionContext}, }, @@ -290,6 +290,7 @@ impl TransactionBatchProcessor { let (mut validate_fees_us, mut load_transactions_us, mut execution_us): (u64, u64, u64) = (0, 0, 0); + let durable_nonce = DurableNonce::from_blockhash(&environment.blockhash); for (tx, check_result) in sanitized_txs.iter().zip(check_results) { let (validate_result, single_validate_fees_us) = measure_us!(check_result.and_then(|tx_details| { @@ -297,6 +298,7 @@ impl TransactionBatchProcessor { &loaded_accounts_map, tx, tx_details, + &durable_nonce, &environment.feature_set, environment .fee_structure @@ -413,6 +415,7 @@ impl TransactionBatchProcessor { loaded_accounts_map: &LoadedAccountsMap, message: &impl SVMMessage, checked_details: CheckedTransactionDetails, + durable_nonce: &DurableNonce, feature_set: &FeatureSet, fee_structure: &FeeStructure, rent_collector: &dyn SVMRentCollector, @@ -443,7 +446,7 @@ impl TransactionBatchProcessor { .rent_amount; let CheckedTransactionDetails { - nonce, + nonce: advanced_nonce, lamports_per_signature, } = checked_details; @@ -468,31 +471,24 @@ impl TransactionBatchProcessor { // If the nonce has been used in this batch already, we must drop the transaction // This is the same as if it was used is different batches in the same slot - // If the nonce account was closed in the batch, we behave as if the blockhash didn't validate - if let Some(ref nonce_info) = nonce { + // If the nonce account was closed in the batch, we error as if the blockhash didn't validate + // We must vaidate the account in case it was reopened, either as a normal system account, or a fake nonce account + // XXX this logic is *exceedingly* tricky, but i havent thought of a better way + if let Some(ref advanced_nonce_info) = advanced_nonce { let nonces_are_equal = loaded_accounts_map - .get_account(nonce_info.address()) - .and_then(|nonce_account| { - // NOTE we cannot directly compare nonce account data because rent epochs may differ - // XXX TODO FIXME this is fundamentally evil on a number of levels: - // * we dont have a State impl so we have to use StateMut - // but we shouldnt add one because... - // * we have to parse both nonce accounts. we could compare current to DurableNonce(last_blockhash)... - // but we would still need to parse one acccount, and i dont think i want to parse either - // i believe the best way would be to add some bytemuck thing to NonceVersions - // which returns Option<&Hash> or Option<&DurableNonce> (ie, None if we arent NonceState::Initialized) - // but i would like feeback on this idea before i implement it - let current_nonce = StateMut::::state(nonce_account).ok()?; - let future_nonce = - StateMut::::state(nonce_info.account()).ok()?; - match (current_nonce.state(), future_nonce.state()) { - ( - NonceState::Initialized(ref current_data), - NonceState::Initialized(ref future_data), - ) => Some(current_data.blockhash() == future_data.blockhash()), + .get_account(advanced_nonce_info.address()) + .and_then(|current_nonce_account| { + system_program::check_id(current_nonce_account.owner()).then_some(())?; + StateMut::::state(current_nonce_account).ok() + }) + .and_then( + |current_nonce_versions| match current_nonce_versions.state() { + NonceState::Initialized(ref current_nonce_data) => { + Some(¤t_nonce_data.durable_nonce == durable_nonce) + } _ => None, - } - }); + }, + ); match nonces_are_equal { Some(false) => (), @@ -510,7 +506,7 @@ impl TransactionBatchProcessor { // Capture fee-subtracted fee payer account and next nonce account state // to commit if transaction execution fails. let rollback_accounts = RollbackAccounts::new( - nonce, + advanced_nonce, *fee_payer_address, fee_payer_account.clone(), fee_payer_rent_debit, @@ -1924,6 +1920,7 @@ mod tests { nonce: None, lamports_per_signature, }, + &DurableNonce::default(), &FeatureSet::default(), &FeeStructure::default(), &rent_collector, @@ -1999,6 +1996,7 @@ mod tests { nonce: None, lamports_per_signature, }, + &DurableNonce::default(), &FeatureSet::default(), &FeeStructure::default(), &rent_collector, @@ -2051,6 +2049,7 @@ mod tests { nonce: None, lamports_per_signature, }, + &DurableNonce::default(), &FeatureSet::default(), &FeeStructure::default(), &RentCollector::default(), @@ -2080,6 +2079,7 @@ mod tests { nonce: None, lamports_per_signature, }, + &DurableNonce::default(), &FeatureSet::default(), &FeeStructure::default(), &RentCollector::default(), @@ -2113,6 +2113,7 @@ mod tests { nonce: None, lamports_per_signature, }, + &DurableNonce::default(), &FeatureSet::default(), &FeeStructure::default(), &rent_collector, @@ -2144,6 +2145,7 @@ mod tests { nonce: None, lamports_per_signature, }, + &DurableNonce::default(), &FeatureSet::default(), &FeeStructure::default(), &RentCollector::default(), @@ -2175,6 +2177,7 @@ mod tests { nonce: None, lamports_per_signature, }, + &DurableNonce::default(), &FeatureSet::default(), &FeeStructure::default(), &RentCollector::default(), @@ -2225,10 +2228,9 @@ mod tests { let mut error_counters = TransactionErrorMetrics::default(); let batch_processor = TransactionBatchProcessor::::default(); + let durable_nonce = DurableNonce::from_blockhash(&Hash::new_unique()); let mut future_nonce = NonceInfo::new(*fee_payer_address, fee_payer_account.clone()); - future_nonce - .try_advance_nonce(DurableNonce::from_blockhash(&Hash::new_unique()), 0) - .unwrap(); + future_nonce.try_advance_nonce(durable_nonce, 0).unwrap(); let result = batch_processor.validate_transaction_fee_payer( &mock_accounts, @@ -2237,6 +2239,7 @@ mod tests { nonce: Some(future_nonce.clone()), lamports_per_signature, }, + &durable_nonce, &feature_set, &FeeStructure::default(), &rent_collector, @@ -2296,6 +2299,7 @@ mod tests { nonce: None, lamports_per_signature, }, + &DurableNonce::default(), &feature_set, &FeeStructure::default(), &rent_collector, diff --git a/svm/tests/integration_test.rs b/svm/tests/integration_test.rs index 96bd6bbcab57e8..a7c3f3bd0fc5f6 100644 --- a/svm/tests/integration_test.rs +++ b/svm/tests/integration_test.rs @@ -1172,13 +1172,15 @@ fn nonce_reuse(enable_fee_only_transactions: bool, fee_paying_nonce: bool) -> Ve let program_id = program_address(program_name); let fee_payer_keypair = Keypair::new(); + let non_fee_nonce_keypair = Keypair::new(); let fee_payer = fee_payer_keypair.pubkey(); let nonce_pubkey = if fee_paying_nonce { fee_payer } else { - Pubkey::new_unique() + non_fee_nonce_keypair.pubkey() }; + let nonce_size = nonce::State::size(); let initial_durable = DurableNonce::from_blockhash(&Hash::new_unique()); let initial_nonce_data = nonce::state::Data::new(fee_payer, initial_durable, LAMPORTS_PER_SIGNATURE); @@ -1197,6 +1199,13 @@ fn nonce_reuse(enable_fee_only_transactions: bool, fee_paying_nonce: bool) -> Ve .unwrap(); let advance_instruction = system_instruction::advance_nonce_account(&nonce_pubkey, &fee_payer); + let withdraw_instruction = system_instruction::withdraw_nonce_account( + &nonce_pubkey, + &fee_payer, + &fee_payer, + LAMPORTS_PER_SOL, + ); + let successful_noop_instruction = Instruction::new_with_bytes(program_id, &[], vec![]); let failing_noop_instruction = Instruction::new_with_bytes(system_program::id(), &[], vec![]); let fee_only_noop_instruction = Instruction::new_with_bytes(Pubkey::new_unique(), &[], vec![]); @@ -1380,18 +1389,220 @@ fn nonce_reuse(enable_fee_only_transactions: bool, fee_paying_nonce: bool) -> Ve } } + // batch 5: + // * a successful blockhash transaction that closes the nonce + // * a nonce transaction that uses the nonce; this transaction must be dropped + // * a successful blockhash noop transaction that touches the nonce, convenience to see state update + if !fee_paying_nonce { + let mut test_entry = common_test_entry.clone(); + + let first_transaction = Transaction::new_signed_with_payer( + &[withdraw_instruction.clone()], + Some(&fee_payer), + &[&fee_payer_keypair], + Hash::default(), + ); + + test_entry.push_transaction(first_transaction); + test_entry.push_nonce_transaction_with_status( + second_transaction.clone(), + advanced_nonce_info.clone(), + ExecutionStatus::Discarded, + ); + test_entry.push_transaction(Transaction::new_signed_with_payer( + &[Instruction::new_with_bytes( + program_id, + &[], + vec![AccountMeta::new_readonly(nonce_pubkey, false)], + )], + Some(&fee_payer), + &[&fee_payer_keypair], + Hash::default(), + )); + + test_entry + .increase_expected_lamports(&fee_payer, LAMPORTS_PER_SOL - LAMPORTS_PER_SIGNATURE); + + test_entry.update_expected_account_data(nonce_pubkey, &AccountSharedData::default()); + + test_entries.push(test_entry); + } + + // batch 6: + // * a successful blockhash transaction that closes the nonce + // * a successful blockhash transaction that funds the closed account + // * a nonce transaction that uses the account; this transaction must be dropped + // XXX AAAAAAAAA i need to test these and the below as fee-only + if !fee_paying_nonce { + let mut test_entry = common_test_entry.clone(); + + let first_transaction = Transaction::new_signed_with_payer( + &[withdraw_instruction.clone()], + Some(&fee_payer), + &[&fee_payer_keypair], + Hash::default(), + ); + + let middle_transaction = system_transaction::transfer( + &fee_payer_keypair, + &nonce_pubkey, + LAMPORTS_PER_SOL, + Hash::default(), + ); + + test_entry.push_transaction(first_transaction); + test_entry.push_transaction(middle_transaction); + test_entry.push_nonce_transaction_with_status( + second_transaction.clone(), + advanced_nonce_info.clone(), + ExecutionStatus::Discarded, + ); + + test_entry.decrease_expected_lamports(&fee_payer, LAMPORTS_PER_SIGNATURE); + + let mut new_nonce_state = AccountSharedData::default(); + new_nonce_state.set_lamports(LAMPORTS_PER_SOL); + + test_entry.update_expected_account_data(nonce_pubkey, &new_nonce_state); + + test_entries.push(test_entry); + } + + // batch 7: + // * a successful blockhash transaction that closes the nonce + // * a successful blockhash transaction that reopens the account with proper nonce size + // * a nonce transaction that uses the account; this transaction must be dropped + if !fee_paying_nonce { + let mut test_entry = common_test_entry.clone(); + + let first_transaction = Transaction::new_signed_with_payer( + &[withdraw_instruction.clone()], + Some(&fee_payer), + &[&fee_payer_keypair], + Hash::default(), + ); + + let middle_transaction = system_transaction::create_account( + &fee_payer_keypair, + &non_fee_nonce_keypair, + Hash::default(), + LAMPORTS_PER_SOL, + nonce_size as u64, + &system_program::id(), + ); + + test_entry.push_transaction(first_transaction); + test_entry.push_transaction(middle_transaction); + test_entry.push_nonce_transaction_with_status( + second_transaction.clone(), + advanced_nonce_info.clone(), + ExecutionStatus::Discarded, + ); + + test_entry.decrease_expected_lamports(&fee_payer, LAMPORTS_PER_SIGNATURE * 2); + + let new_nonce_state = AccountSharedData::create( + LAMPORTS_PER_SOL, + vec![0; nonce_size], + system_program::id(), + false, + u64::MAX, + ); + + test_entry.update_expected_account_data(nonce_pubkey, &new_nonce_state); + + test_entries.push(test_entry); + } + + // batch 8: + // * a successful blockhash transaction that closes the nonce + // * a successful blockhash transaction that reopens the nonce + // * a nonce transaction that uses the nonce; this transaction must be dropped + if !fee_paying_nonce { + let mut test_entry = common_test_entry.clone(); + + let first_transaction = Transaction::new_signed_with_payer( + &[withdraw_instruction.clone()], + Some(&fee_payer), + &[&fee_payer_keypair], + Hash::default(), + ); + + let create_instructions = system_instruction::create_nonce_account( + &fee_payer, + &nonce_pubkey, + &fee_payer, + LAMPORTS_PER_SOL, + ); + + let middle_transaction = Transaction::new_signed_with_payer( + &create_instructions, + Some(&fee_payer), + &[&fee_payer_keypair, &non_fee_nonce_keypair], + Hash::default(), + ); + + test_entry.push_transaction(first_transaction); + test_entry.push_transaction(middle_transaction); + test_entry.push_nonce_transaction_with_status( + second_transaction.clone(), + advanced_nonce_info.clone(), + ExecutionStatus::Discarded, + ); + + test_entry.decrease_expected_lamports(&fee_payer, LAMPORTS_PER_SIGNATURE * 2); + + test_entries.push(test_entry); + } + + // batch 9: + // * a successful blockhash noop transaction + // * a nonce transaction that uses a spoofed nonce account; this transaction must be dropped + // check_age would never let such a transaction through validation + // this simulates the case where someone closes a nonce account, then reuses the address in the same batch + // but as a non-system account that parses as an initialized nonce account + if !fee_paying_nonce { + let mut test_entry = common_test_entry.clone(); + test_entry.initial_accounts.remove(&nonce_pubkey); + test_entry.final_accounts.remove(&nonce_pubkey); + + let mut fake_nonce_account = initial_nonce_account.clone(); + fake_nonce_account.set_rent_epoch(u64::MAX); + fake_nonce_account.set_owner(Pubkey::new_unique()); + test_entry.add_initial_account(nonce_pubkey, &fake_nonce_account); + + let first_transaction = Transaction::new_signed_with_payer( + &[successful_noop_instruction.clone()], + Some(&fee_payer), + &[&fee_payer_keypair], + Hash::default(), + ); + + test_entry.push_transaction(first_transaction); + test_entry.push_nonce_transaction_with_status( + second_transaction.clone(), + advanced_nonce_info.clone(), + ExecutionStatus::Discarded, + ); + + test_entries.push(test_entry); + } + + for test_entry in &mut test_entries { + test_entry + .initial_programs + .push((program_name.to_string(), DEPLOYMENT_SLOT)); + + if enable_fee_only_transactions { + test_entry + .enabled_features + .push(feature_set::enable_transaction_loading_failure_fees::id()); + } + } + test_entries } -// XXX TODO FIXME more bizarre nonce cases we might want to test: -// * withdraw from nonce, then use (discard) -// * withdraw from nonce, fund, then use (discard) -// * withdraw from nonce, create account of nonce size, then use (discard) -// * as above but use it as the fee-payer of a fee-only transaction (discard) -// reading the code i believe these are all safe, validate_transaction_fee_payer should ? an error -// * withdraw from nonce, create new nonce, then use (discard) -// this one is safe also, because new nonces are initialized with the current durable nonce - // XXX TODO FIXME i decided i dont need to test program deployment intrabatch // by (correctly) enforcing programs are executable during initial loading, we drop anything that could take advantage of it // hm what happens if tx1 deploys the program and tx2 tries to write it tho. i assume the loader program would execute-fail