diff --git a/banking-bench/src/main.rs b/banking-bench/src/main.rs index 4603509b4b..b42fc7ff66 100644 --- a/banking-bench/src/main.rs +++ b/banking-bench/src/main.rs @@ -200,7 +200,8 @@ fn main() { }); bank.clear_signatures(); //sanity check, make sure all the transactions can execute in parallel - let res = bank.process_transactions(&transactions); + + let res = bank.process_transactions(transactions.iter()); for r in res { assert!(r.is_ok(), "sanity parallel execution error: {:?}", r); } diff --git a/banks-server/src/banks_server.rs b/banks-server/src/banks_server.rs index 62b9c473c1..c2af1470b9 100644 --- a/banks-server/src/banks_server.rs +++ b/banks-server/src/banks_server.rs @@ -70,7 +70,7 @@ impl BanksServer { .map(|info| deserialize(&info.wire_transaction).unwrap()) .collect(); let bank = bank_forks.read().unwrap().working_bank(); - let _ = bank.process_transactions(&transactions); + let _ = bank.try_process_transactions(transactions.iter()); } } diff --git a/core/benches/banking_stage.rs b/core/benches/banking_stage.rs index 6eb8126e79..c85af3b6c2 100644 --- a/core/benches/banking_stage.rs +++ b/core/benches/banking_stage.rs @@ -193,7 +193,7 @@ fn bench_banking(bencher: &mut Bencher, tx_type: TransactionType) { }); bank.clear_signatures(); //sanity check, make sure all the transactions can execute in parallel - let res = bank.process_transactions(&transactions); + let res = bank.process_transactions(transactions.iter()); for r in res { assert!(r.is_ok(), "sanity parallel execution"); } diff --git a/core/src/banking_stage.rs b/core/src/banking_stage.rs index b0b2df0004..96771b020a 100644 --- a/core/src/banking_stage.rs +++ b/core/src/banking_stage.rs @@ -27,7 +27,6 @@ use solana_runtime::{ transaction_batch::TransactionBatch, vote_sender_types::ReplayVoteSender, }; -use solana_sdk::hashed_transaction::HashedTransaction; use solana_sdk::{ clock::{ Slot, DEFAULT_TICKS_PER_SLOT, MAX_PROCESSING_AGE, MAX_TRANSACTION_FORWARDING_DELAY, @@ -35,6 +34,7 @@ use solana_sdk::{ }, message::Message, pubkey::Pubkey, + sanitized_transaction::SanitizedTransaction, short_vec::decode_shortu16_len, signature::Signature, timing::{duration_as_ms, timestamp}, @@ -863,11 +863,11 @@ impl BankingStage { record_time.stop(); let mut commit_time = Measure::start("commit_time"); - let hashed_txs = batch.hashed_transactions(); + let sanitized_txs = batch.sanitized_transactions(); let num_to_commit = num_to_commit.unwrap(); if num_to_commit != 0 { let tx_results = bank.commit_transactions( - hashed_txs, + sanitized_txs, &mut loaded_accounts, &results, tx_count, @@ -875,7 +875,7 @@ impl BankingStage { &mut execute_timings, ); - bank_utils::find_and_send_votes(hashed_txs, &tx_results, Some(gossip_vote_sender)); + bank_utils::find_and_send_votes(sanitized_txs, &tx_results, Some(gossip_vote_sender)); if let Some(transaction_status_sender) = transaction_status_sender { let txs = batch.transactions_iter().cloned().collect(); let post_balances = bank.collect_balances(batch); @@ -902,7 +902,7 @@ impl BankingStage { load_execute_time.as_us(), record_time.as_us(), commit_time.as_us(), - hashed_txs.len(), + sanitized_txs.len(), ); debug!( @@ -915,7 +915,7 @@ impl BankingStage { pub fn process_and_record_transactions( bank: &Arc, - txs: &[HashedTransaction], + txs: &[SanitizedTransaction], poh: &TransactionRecorder, chunk_offset: usize, transaction_status_sender: Option, @@ -924,7 +924,7 @@ impl BankingStage { let mut lock_time = Measure::start("lock_time"); // Once accounts are locked, other threads cannot encode transactions that will modify the // same account state - let batch = bank.prepare_hashed_batch(txs); + let batch = bank.prepare_sanitized_batch(txs); lock_time.stop(); let (result, mut retryable_txs) = Self::process_and_record_transactions_locked( @@ -959,7 +959,7 @@ impl BankingStage { fn process_transactions( bank: &Arc, bank_creation_time: &Instant, - transactions: &[HashedTransaction], + transactions: &[SanitizedTransaction], poh: &TransactionRecorder, transaction_status_sender: Option, gossip_vote_sender: &ReplayVoteSender, @@ -1062,7 +1062,7 @@ impl BankingStage { libsecp256k1_0_5_upgrade_enabled: bool, cost_tracker: &Arc>, banking_stage_stats: &BankingStageStats, - ) -> (Vec>, Vec, Vec) { + ) -> (Vec>, Vec, Vec) { let mut retryable_transaction_packet_indexes: Vec = vec![]; let verified_transactions_with_packet_indexes: Vec<_> = transaction_indexes @@ -1072,6 +1072,9 @@ impl BankingStage { let tx: Transaction = limited_deserialize(&p.data[0..p.meta.size]).ok()?; tx.verify_precompiles(libsecp256k1_0_5_upgrade_enabled) .ok()?; + let message_bytes = Self::packet_message(p)?; + let message_hash = Message::hash_raw_message(message_bytes); + let tx = SanitizedTransaction::try_create(Cow::Owned(tx), message_hash).ok()?; Some((tx, *tx_index)) }) .collect(); @@ -1081,7 +1084,7 @@ impl BankingStage { ); let mut cost_tracker_check_time = Measure::start("cost_tracker_check_time"); - let filtered_transactions_with_packet_indexes: Vec<_> = { + let (filtered_transactions, filter_transaction_packet_indexes) = { let cost_tracker_readonly = cost_tracker.read().unwrap(); verified_transactions_with_packet_indexes .into_iter() @@ -1094,24 +1097,10 @@ impl BankingStage { } Some((tx, tx_index)) }) - .collect() + .unzip() }; cost_tracker_check_time.stop(); - let (filtered_transactions, filter_transaction_packet_indexes) = - filtered_transactions_with_packet_indexes - .into_iter() - .filter_map(|(tx, tx_index)| { - let p = &msgs.packets[tx_index]; - let message_bytes = Self::packet_message(p)?; - let message_hash = Message::hash_raw_message(message_bytes); - Some(( - HashedTransaction::new(Cow::Owned(tx), message_hash), - tx_index, - )) - }) - .unzip(); - banking_stage_stats .cost_tracker_check_elapsed .fetch_add(cost_tracker_check_time.as_us(), Ordering::Relaxed); @@ -1130,7 +1119,7 @@ impl BankingStage { /// * `pending_indexes` - identifies which indexes in the `transactions` list are still pending fn filter_pending_packets_from_pending_txs( bank: &Arc, - transactions: &[HashedTransaction], + transactions: &[SanitizedTransaction], transaction_to_packet_indexes: &[usize], pending_indexes: &[usize], ) -> Vec { @@ -1218,17 +1207,7 @@ impl BankingStage { let mut cost_tracking_time = Measure::start("cost_tracking_time"); transactions.iter().enumerate().for_each(|(index, tx)| { if unprocessed_tx_indexes.iter().all(|&i| i != index) { - cost_tracker - .write() - .unwrap() - .add_transaction_cost(tx.transaction()) - .unwrap_or_else(|err| { - warn!( - "failed to track transaction cost, err {:?}, tx {:?}", - err, - tx.transaction() - ) - }); + cost_tracker.write().unwrap().add_transaction_cost(tx); } }); cost_tracking_time.stop(); @@ -1602,6 +1581,7 @@ mod tests { }; use solana_transaction_status::TransactionWithStatusMeta; use std::{ + convert::TryInto, net::SocketAddr, path::Path, sync::{ @@ -1817,7 +1797,7 @@ mod tests { if !entries.is_empty() { blockhash = entries.last().unwrap().hash; for entry in entries { - bank.process_transactions(&entry.transactions) + bank.process_transactions(entry.transactions.iter()) .iter() .for_each(|x| assert_eq!(*x, Ok(()))); } @@ -1931,7 +1911,7 @@ mod tests { let bank = Bank::new_no_wallclock_throttle(&genesis_config); for entry in &entries { - bank.process_transactions(&entry.transactions) + bank.process_transactions(entry.transactions.iter()) .iter() .for_each(|x| assert_eq!(*x, Ok(()))); } @@ -2223,7 +2203,8 @@ mod tests { let transactions = vec![ system_transaction::transfer(&mint_keypair, &pubkey, 1, genesis_config.hash()) - .into(), + .try_into() + .unwrap(), ]; let start = Arc::new(Instant::now()); @@ -2292,7 +2273,8 @@ mod tests { 2, genesis_config.hash(), ) - .into()]; + .try_into() + .unwrap()]; assert_matches!( BankingStage::process_and_record_transactions( @@ -2353,8 +2335,12 @@ mod tests { let pubkey1 = solana_sdk::pubkey::new_rand(); let transactions = vec![ - system_transaction::transfer(&mint_keypair, &pubkey, 1, genesis_config.hash()).into(), - system_transaction::transfer(&mint_keypair, &pubkey1, 1, genesis_config.hash()).into(), + system_transaction::transfer(&mint_keypair, &pubkey, 1, genesis_config.hash()) + .try_into() + .unwrap(), + system_transaction::transfer(&mint_keypair, &pubkey1, 1, genesis_config.hash()) + .try_into() + .unwrap(), ]; let start = Arc::new(Instant::now()); @@ -2467,7 +2453,8 @@ mod tests { let transactions = vec![ system_transaction::transfer(&mint_keypair, &pubkey, 1, genesis_config.hash()) - .into(), + .try_into() + .unwrap(), ]; let ledger_path = get_tmp_ledger_path!(); @@ -2544,7 +2531,11 @@ mod tests { let entry_3 = next_entry(&entry_2.hash, 1, vec![fail_tx.clone()]); let entries = vec![entry_1, entry_2, entry_3]; - let transactions = vec![success_tx.into(), ix_error_tx.into(), fail_tx.into()]; + let transactions = vec![ + success_tx.try_into().unwrap(), + ix_error_tx.try_into().unwrap(), + fail_tx.try_into().unwrap(), + ]; bank.transfer(4, &mint_keypair, &keypair1.pubkey()).unwrap(); let start = Arc::new(Instant::now()); diff --git a/core/src/cost_model.rs b/core/src/cost_model.rs index 3b88641b94..50660cd075 100644 --- a/core/src/cost_model.rs +++ b/core/src/cost_model.rs @@ -9,7 +9,7 @@ //! use crate::execute_cost_table::ExecuteCostTable; use log::*; -use solana_sdk::{pubkey::Pubkey, transaction::Transaction}; +use solana_sdk::{pubkey::Pubkey, sanitized_transaction::SanitizedTransaction}; use std::collections::HashMap; // Guestimated from mainnet-beta data, sigver averages 1us, average read 7us and average write 25us @@ -126,14 +126,11 @@ impl CostModel { ); } - pub fn calculate_cost( - &mut self, - transaction: &Transaction, - ) -> Result<&TransactionCost, CostModelError> { + pub fn calculate_cost(&mut self, transaction: &SanitizedTransaction) -> &TransactionCost { self.transaction_cost.reset(); // calculate transaction exeution cost - self.transaction_cost.execution_cost = self.find_transaction_cost(transaction)?; + self.transaction_cost.execution_cost = self.find_transaction_cost(transaction); // calculate account access cost let message = transaction.message(); @@ -159,7 +156,7 @@ impl CostModel { "transaction {:?} has cost {:?}", transaction, self.transaction_cost ); - Ok(&self.transaction_cost) + &self.transaction_cost } // To update or insert instruction cost to table. @@ -194,15 +191,10 @@ impl CostModel { } } - fn find_transaction_cost(&self, transaction: &Transaction) -> Result { + fn find_transaction_cost(&self, transaction: &SanitizedTransaction) -> u64 { let mut cost: u64 = 0; for instruction in &transaction.message().instructions { - // The Transaction may not be sanitized at this point - if instruction.program_id_index as usize >= transaction.message().account_keys.len() { - return Err(CostModelError::InvalidTransaction); - } - let program_id = transaction.message().account_keys[instruction.program_id_index as usize]; let instruction_cost = self.find_instruction_cost(&program_id); @@ -213,7 +205,7 @@ impl CostModel { ); cost += instruction_cost; } - Ok(cost) + cost } } @@ -232,8 +224,10 @@ mod tests { signature::{Keypair, Signer}, system_instruction::{self}, system_program, system_transaction, + transaction::Transaction, }; use std::{ + convert::{TryFrom, TryInto}, str::FromStr, sync::{Arc, RwLock}, thread::{self, JoinHandle}, @@ -279,8 +273,10 @@ mod tests { let (mint_keypair, start_hash) = test_setup(); let keypair = Keypair::new(); - let simple_transaction = - system_transaction::transfer(&mint_keypair, &keypair.pubkey(), 2, start_hash); + let simple_transaction: SanitizedTransaction = + system_transaction::transfer(&mint_keypair, &keypair.pubkey(), 2, start_hash) + .try_into() + .unwrap(); debug!( "system_transaction simple_transaction {:?}", simple_transaction @@ -295,7 +291,7 @@ mod tests { .unwrap(); assert_eq!( expected_cost, - testee.find_transaction_cost(&simple_transaction).unwrap() + testee.find_transaction_cost(&simple_transaction) ); } @@ -308,7 +304,9 @@ mod tests { let instructions = system_instruction::transfer_many(&mint_keypair.pubkey(), &[(key1, 1), (key2, 1)]); let message = Message::new(&instructions, Some(&mint_keypair.pubkey())); - let tx = Transaction::new(&[&mint_keypair], message, start_hash); + let tx: SanitizedTransaction = Transaction::new(&[&mint_keypair], message, start_hash) + .try_into() + .unwrap(); debug!("many transfer transaction {:?}", tx); // expected cost for two system transfer instructions @@ -319,7 +317,7 @@ mod tests { testee .upsert_instruction_cost(&system_program::id(), program_cost) .unwrap(); - assert_eq!(expected_cost, testee.find_transaction_cost(&tx).unwrap()); + assert_eq!(expected_cost, testee.find_transaction_cost(&tx)); } #[test] @@ -335,17 +333,19 @@ mod tests { CompiledInstruction::new(3, &(), vec![0, 1]), CompiledInstruction::new(4, &(), vec![0, 2]), ]; - let tx = Transaction::new_with_compiled_instructions( + let tx: SanitizedTransaction = Transaction::new_with_compiled_instructions( &[&mint_keypair], &[key1, key2], start_hash, vec![prog1, prog2], instructions, - ); + ) + .try_into() + .unwrap(); debug!("many random transaction {:?}", tx); let testee = CostModel::default(); - let result = testee.find_transaction_cost(&tx).unwrap(); + let result = testee.find_transaction_cost(&tx); // expected cost for two random/unknown program is let expected_cost = testee.instruction_execution_cost_table.get_mode() * 2; @@ -365,16 +365,18 @@ mod tests { CompiledInstruction::new(4, &(), vec![0, 2]), CompiledInstruction::new(5, &(), vec![1, 3]), ]; - let tx = Transaction::new_with_compiled_instructions( + let tx: SanitizedTransaction = Transaction::new_with_compiled_instructions( &[&signer1, &signer2], &[key1, key2], Hash::new_unique(), vec![prog1, prog2], instructions, - ); + ) + .try_into() + .unwrap(); let mut cost_model = CostModel::default(); - let tx_cost = cost_model.calculate_cost(&tx).unwrap(); + let tx_cost = cost_model.calculate_cost(&tx); assert_eq!(2 + 2, tx_cost.writable_accounts.len()); assert_eq!(signer1.pubkey(), tx_cost.writable_accounts[0]); assert_eq!(signer2.pubkey(), tx_cost.writable_accounts[1]); @@ -404,8 +406,10 @@ mod tests { #[test] fn test_cost_model_calculate_cost() { let (mint_keypair, start_hash) = test_setup(); - let tx = - system_transaction::transfer(&mint_keypair, &Keypair::new().pubkey(), 2, start_hash); + let tx: SanitizedTransaction = + system_transaction::transfer(&mint_keypair, &Keypair::new().pubkey(), 2, start_hash) + .try_into() + .unwrap(); let expected_account_cost = SIGNED_WRITABLE_ACCOUNT_ACCESS_COST + NON_SIGNED_WRITABLE_ACCOUNT_ACCESS_COST @@ -416,7 +420,7 @@ mod tests { cost_model .upsert_instruction_cost(&system_program::id(), expected_execution_cost) .unwrap(); - let tx_cost = cost_model.calculate_cost(&tx).unwrap(); + let tx_cost = cost_model.calculate_cost(&tx); assert_eq!(expected_account_cost, tx_cost.account_access_cost); assert_eq!(expected_execution_cost, tx_cost.execution_cost); assert_eq!(2, tx_cost.writable_accounts.len()); @@ -452,13 +456,16 @@ mod tests { CompiledInstruction::new(3, &(), vec![0, 1]), CompiledInstruction::new(4, &(), vec![0, 2]), ]; - let tx = Arc::new(Transaction::new_with_compiled_instructions( - &[&mint_keypair], - &[key1, key2], - start_hash, - vec![prog1, prog2], - instructions, - )); + let tx = Arc::new( + SanitizedTransaction::try_from(Transaction::new_with_compiled_instructions( + &[&mint_keypair], + &[key1, key2], + start_hash, + vec![prog1, prog2], + instructions, + )) + .unwrap(), + ); let number_threads = 10; let expected_account_cost = SIGNED_WRITABLE_ACCOUNT_ACCESS_COST @@ -484,7 +491,7 @@ mod tests { } else { thread::spawn(move || { let mut cost_model = cost_model.write().unwrap(); - let tx_cost = cost_model.calculate_cost(&tx).unwrap(); + let tx_cost = cost_model.calculate_cost(&tx); assert_eq!(3, tx_cost.writable_accounts.len()); assert_eq!(expected_account_cost, tx_cost.account_access_cost); }) diff --git a/core/src/cost_tracker.rs b/core/src/cost_tracker.rs index 7795c18a0a..34a65dec83 100644 --- a/core/src/cost_tracker.rs +++ b/core/src/cost_tracker.rs @@ -5,7 +5,7 @@ //! - add_transaction_cost(&tx), mutable function to accumulate `tx` cost to tracker. //! use crate::cost_model::{CostModel, CostModelError, TransactionCost}; -use solana_sdk::{clock::Slot, pubkey::Pubkey, transaction::Transaction}; +use solana_sdk::{clock::Slot, pubkey::Pubkey, sanitized_transaction::SanitizedTransaction}; use std::{ collections::HashMap, sync::{Arc, RwLock}, @@ -43,21 +43,21 @@ impl CostTracker { } } - pub fn would_transaction_fit(&self, transaction: &Transaction) -> Result<(), CostModelError> { + pub fn would_transaction_fit( + &self, + transaction: &SanitizedTransaction, + ) -> Result<(), CostModelError> { let mut cost_model = self.cost_model.write().unwrap(); - let tx_cost = cost_model.calculate_cost(transaction)?; + let tx_cost = cost_model.calculate_cost(transaction); self.would_fit( &tx_cost.writable_accounts, &(tx_cost.account_access_cost + tx_cost.execution_cost), ) } - pub fn add_transaction_cost( - &mut self, - transaction: &Transaction, - ) -> Result<(), CostModelError> { + pub fn add_transaction_cost(&mut self, transaction: &SanitizedTransaction) { let mut cost_model = self.cost_model.write().unwrap(); - let tx_cost = cost_model.calculate_cost(transaction)?; + let tx_cost = cost_model.calculate_cost(transaction); let cost = tx_cost.account_access_cost + tx_cost.execution_cost; for account_key in tx_cost.writable_accounts.iter() { *self @@ -66,7 +66,6 @@ impl CostTracker { .or_insert(0) += cost; } self.block_cost += cost; - Ok(()) } pub fn reset_if_new_bank(&mut self, slot: Slot) { diff --git a/entry/src/entry.rs b/entry/src/entry.rs index f347cd5609..856c0e1c56 100644 --- a/entry/src/entry.rs +++ b/entry/src/entry.rs @@ -18,12 +18,13 @@ use solana_perf::perf_libs; use solana_perf::recycler::Recycler; use solana_rayon_threadlimit::get_thread_count; use solana_sdk::hash::Hash; -use solana_sdk::hashed_transaction::HashedTransaction; use solana_sdk::packet::PACKET_DATA_SIZE; +use solana_sdk::sanitized_transaction::SanitizedTransaction; use solana_sdk::timing; -use solana_sdk::transaction::Transaction; +use solana_sdk::transaction::{Result, Transaction, TransactionError}; use std::borrow::Cow; use std::cell::RefCell; +use std::convert::TryFrom; use std::ffi::OsStr; use std::sync::mpsc::{Receiver, Sender}; use std::sync::Once; @@ -122,22 +123,23 @@ pub struct Entry { /// Typed entry to distinguish between transaction and tick entries pub enum EntryType<'a> { - Transactions(Vec>), + Transactions(Vec>), Tick(Hash), } -impl<'a> From<&'a Entry> for EntryType<'a> { - fn from(entry: &'a Entry) -> Self { +impl<'a> TryFrom<&'a Entry> for EntryType<'a> { + type Error = TransactionError; + fn try_from(entry: &'a Entry) -> Result { if entry.transactions.is_empty() { - EntryType::Tick(entry.hash) + Ok(EntryType::Tick(entry.hash)) } else { - EntryType::Transactions( + Ok(EntryType::Transactions( entry .transactions .iter() - .map(HashedTransaction::from) - .collect(), - ) + .map(SanitizedTransaction::try_from) + .collect::>()?, + )) } } } @@ -361,7 +363,7 @@ pub trait EntrySlice { skip_verification: bool, libsecp256k1_0_5_upgrade_enabled: bool, verify_tx_signatures_len: bool, - ) -> Option>>; + ) -> Result>>; } impl EntrySlice for [Entry] { @@ -517,24 +519,24 @@ impl EntrySlice for [Entry] { skip_verification: bool, libsecp256k1_0_5_upgrade_enabled: bool, verify_tx_signatures_len: bool, - ) -> Option>> { - let verify_and_hash = |tx: &'a Transaction| -> Option> { + ) -> Result>> { + let verify_and_hash = |tx: &'a Transaction| -> Result> { let message_hash = if !skip_verification { - let size = bincode::serialized_size(tx).ok()?; + let size = + bincode::serialized_size(tx).map_err(|_| TransactionError::SanitizeFailure)?; if size > PACKET_DATA_SIZE as u64 { - return None; + return Err(TransactionError::SanitizeFailure); } - tx.verify_precompiles(libsecp256k1_0_5_upgrade_enabled) - .ok()?; + tx.verify_precompiles(libsecp256k1_0_5_upgrade_enabled)?; if verify_tx_signatures_len && !tx.verify_signatures_len() { - return None; + return Err(TransactionError::SanitizeFailure); } - tx.verify_and_hash_message().ok()? + tx.verify_and_hash_message()? } else { tx.message().hash() }; - Some(HashedTransaction::new(Cow::Borrowed(tx), message_hash)) + SanitizedTransaction::try_create(Cow::Borrowed(tx), message_hash) }; PAR_THREAD_POOL.with(|thread_pool| { @@ -542,14 +544,14 @@ impl EntrySlice for [Entry] { self.par_iter() .map(|entry| { if entry.transactions.is_empty() { - Some(EntryType::Tick(entry.hash)) + Ok(EntryType::Tick(entry.hash)) } else { - Some(EntryType::Transactions( + Ok(EntryType::Transactions( entry .transactions .par_iter() .map(verify_and_hash) - .collect::>>()?, + .collect::>>()?, )) } }) @@ -920,27 +922,66 @@ mod tests { } tx }; - // No signatures. + // Too few signatures: Sanitization failure { let tx = make_transaction(TestCase::RemoveSignature); let entries = vec![next_entry(&recent_blockhash, 1, vec![tx])]; - assert!(entries[..] - .verify_and_hash_transactions(false, false, false) - .is_some()); - assert!(entries[..] - .verify_and_hash_transactions(false, false, true) - .is_none()); + assert_eq!( + entries[..] + .verify_and_hash_transactions(false, false, false) + .err(), + Some(TransactionError::SanitizeFailure), + ); } - // Too many signatures. + // Too many signatures: Sanitize failure only with feature switch { let tx = make_transaction(TestCase::AddSignature); let entries = vec![next_entry(&recent_blockhash, 1, vec![tx])]; assert!(entries[..] .verify_and_hash_transactions(false, false, false) - .is_some()); - assert!(entries[..] - .verify_and_hash_transactions(false, false, true) - .is_none()); + .is_ok()); + assert_eq!( + entries[..] + .verify_and_hash_transactions(false, false, true) + .err(), + Some(TransactionError::SanitizeFailure) + ); + } + } + + #[test] + fn test_verify_and_hash_transactions_load_duplicate_account() { + let mut rng = rand::thread_rng(); + let recent_blockhash = hash_new_rand(&mut rng); + let from_keypair = Keypair::new(); + let to_keypair = Keypair::new(); + let from_pubkey = from_keypair.pubkey(); + let to_pubkey = to_keypair.pubkey(); + + let make_transaction = || { + let mut message = Message::new( + &[system_instruction::transfer(&from_pubkey, &to_pubkey, 1)], + Some(&from_pubkey), + ); + let to_index = message + .account_keys + .iter() + .position(|k| k == &to_pubkey) + .unwrap(); + message.account_keys[to_index] = from_pubkey; + Transaction::new(&[&from_keypair], message, recent_blockhash) + }; + + // Duplicate account + { + let tx = make_transaction(); + let entries = vec![next_entry(&recent_blockhash, 1, vec![tx])]; + assert_eq!( + entries[..] + .verify_and_hash_transactions(false, false, false) + .err(), + Some(TransactionError::AccountLoadedTwice) + ); } } @@ -966,16 +1007,19 @@ mod tests { assert!(bincode::serialized_size(&tx).unwrap() <= PACKET_DATA_SIZE as u64); assert!(entries[..] .verify_and_hash_transactions(false, false, false) - .is_some()); + .is_ok()); } // Big transaction. { let tx = make_transaction(25); let entries = vec![next_entry(&recent_blockhash, 1, vec![tx.clone()])]; assert!(bincode::serialized_size(&tx).unwrap() > PACKET_DATA_SIZE as u64); - assert!(entries[..] - .verify_and_hash_transactions(false, false, false) - .is_none()); + assert_eq!( + entries[..] + .verify_and_hash_transactions(false, false, false) + .err(), + Some(TransactionError::SanitizeFailure) + ); } // Assert that verify fails as soon as serialized // size exceeds packet data size. @@ -986,7 +1030,7 @@ mod tests { bincode::serialized_size(&tx).unwrap() <= PACKET_DATA_SIZE as u64, entries[..] .verify_and_hash_transactions(false, false, false) - .is_some(), + .is_ok(), ); } } diff --git a/ledger-tool/src/main.rs b/ledger-tool/src/main.rs index 06d51522da..f762e0c4ce 100644 --- a/ledger-tool/src/main.rs +++ b/ledger-tool/src/main.rs @@ -43,6 +43,7 @@ use solana_sdk::{ native_token::{lamports_to_sol, sol_to_lamports, Sol}, pubkey::Pubkey, rent::Rent, + sanitized_transaction::SanitizedTransaction, shred_version::compute_shred_version, stake::{self, state::StakeState}, system_program, @@ -53,6 +54,7 @@ use solana_vote_program::{ vote_state::{self, VoteState}, }; use std::{ + borrow::Cow, collections::{BTreeMap, BTreeSet, HashMap, HashSet}, ffi::OsStr, fs::{self, File}, @@ -751,16 +753,19 @@ fn compute_slot_cost(blockstore: &Blockstore, slot: Slot) -> Result<(), String> let mut cost_model = cost_model.write().unwrap(); for transaction in &entry.transactions { programs += transaction.message().instructions.len(); - let tx_cost = match cost_model.calculate_cost(transaction) { - Err(err) => { - warn!( - "failed to calculate transaction cost, err {:?}, tx {:?}", - err, transaction - ); - continue; - } - Ok(cost) => cost, - }; + let transaction = + match SanitizedTransaction::try_create(Cow::Borrowed(transaction), Hash::default()) + { + Ok(tx) => tx, + Err(err) => { + warn!( + "failed to sanitize transaction, err {:?}, tx {:?}", + err, transaction + ); + continue; + } + }; + let tx_cost = cost_model.calculate_cost(&transaction); if cost_tracker.try_add(tx_cost).is_err() { println!( "Slot: {}, CostModel rejected transaction {:?}, stats {:?}!", diff --git a/ledger/src/blockstore_processor.rs b/ledger/src/blockstore_processor.rs index 179d50cf6c..6b18c39070 100644 --- a/ledger/src/blockstore_processor.rs +++ b/ledger/src/blockstore_processor.rs @@ -45,6 +45,7 @@ use solana_transaction_status::token_balances::{ use std::{ cell::RefCell, collections::{HashMap, HashSet}, + convert::TryFrom, path::PathBuf, result, sync::Arc, @@ -126,7 +127,11 @@ fn execute_batch( timings, ); - bank_utils::find_and_send_votes(batch.hashed_transactions(), &tx_results, replay_vote_sender); + bank_utils::find_and_send_votes( + batch.sanitized_transactions(), + &tx_results, + replay_vote_sender, + ); let TransactionResults { fee_collection_results, @@ -216,7 +221,10 @@ pub fn process_entries( replay_vote_sender: Option<&ReplayVoteSender>, ) -> Result<()> { let mut timings = ExecuteTimings::default(); - let mut entry_types: Vec<_> = entries.iter().map(EntryType::from).collect(); + let mut entry_types: Vec<_> = entries + .iter() + .map(EntryType::try_from) + .collect::>()?; let result = process_entries_with_callback( bank, &mut entry_types, @@ -276,7 +284,7 @@ fn process_entries_with_callback( loop { // try to lock the accounts - let batch = bank.prepare_hashed_batch(transactions); + let batch = bank.prepare_sanitized_batch(transactions); let first_lock_err = first_err(batch.lock_results()); // if locking worked @@ -791,18 +799,13 @@ pub fn confirm_slot( }; let check_start = Instant::now(); - let check_result = entries.verify_and_hash_transactions( + let mut entries = entries.verify_and_hash_transactions( skip_verification, bank.libsecp256k1_0_5_upgrade_enabled(), bank.verify_tx_signatures_len_enabled(), - ); - if check_result.is_none() { - warn!("Ledger proof of history failed at slot: {}", slot); - return Err(BlockError::InvalidEntryHash.into()); - } + )?; let transaction_duration_us = timing::duration_as_us(&check_start.elapsed()); - let mut entries = check_result.unwrap(); let mut replay_elapsed = Measure::start("replay_elapsed"); let mut execute_timings = ExecuteTimings::default(); // Note: This will shuffle entries' transactions in-place. @@ -2410,13 +2413,13 @@ pub mod tests { // Check all accounts are unlocked let txs1 = &entry_1_to_mint.transactions[..]; let txs2 = &entry_2_to_3_mint_to_1.transactions[..]; - let batch1 = bank.prepare_batch(txs1.iter()); + let batch1 = bank.prepare_batch(txs1.iter()).unwrap(); for result in batch1.lock_results() { assert!(result.is_ok()); } // txs1 and txs2 have accounts that conflict, so we must drop txs1 first drop(batch1); - let batch2 = bank.prepare_batch(txs2.iter()); + let batch2 = bank.prepare_batch(txs2.iter()).unwrap(); for result in batch2.lock_results() { assert!(result.is_ok()); } @@ -3173,15 +3176,14 @@ pub mod tests { bank.last_blockhash(), ); let account_not_found_sig = account_not_found_tx.signatures[0]; - let mut account_loaded_twice = system_transaction::transfer( + let invalid_blockhash_tx = system_transaction::transfer( &mint_keypair, &solana_sdk::pubkey::new_rand(), 42, - bank.last_blockhash(), + Hash::default(), ); - account_loaded_twice.message.account_keys[1] = mint_keypair.pubkey(); - let transactions = [account_not_found_tx, account_loaded_twice]; - let batch = bank.prepare_batch(transactions.iter()); + let transactions = [account_not_found_tx, invalid_blockhash_tx]; + let batch = bank.prepare_batch(transactions.iter()).unwrap(); let ( TransactionResults { fee_collection_results, @@ -3199,7 +3201,6 @@ pub mod tests { &mut ExecuteTimings::default(), ); let (err, signature) = get_first_error(&batch, fee_collection_results).unwrap(); - // First error found should be for the 2nd transaction, due to iteration_order assert_eq!(err.unwrap_err(), TransactionError::AccountNotFound); assert_eq!(signature, account_not_found_sig); } diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 0f491ef563..f139c3062d 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -293,7 +293,7 @@ fn process_transaction_and_record_inner( ) -> (Result<(), TransactionError>, Vec>) { let signature = tx.signatures.get(0).unwrap().clone(); let txs = vec![tx]; - let tx_batch = bank.prepare_batch(txs.iter()); + let tx_batch = bank.prepare_batch(txs.iter()).unwrap(); let (mut results, _, mut inner_instructions, _transaction_logs) = bank .load_execute_and_commit_transactions( &tx_batch, @@ -316,7 +316,7 @@ fn process_transaction_and_record_inner( } fn execute_transactions(bank: &Bank, txs: &[Transaction]) -> Vec { - let batch = bank.prepare_batch(txs.iter()); + let batch = bank.prepare_batch(txs.iter()).unwrap(); let mut timings = ExecuteTimings::default(); let mut mint_decimals = HashMap::new(); let tx_pre_token_balances = collect_token_balances(&bank, &batch, &mut mint_decimals); diff --git a/runtime/benches/accounts.rs b/runtime/benches/accounts.rs index 364707ddde..1b69685a6a 100644 --- a/runtime/benches/accounts.rs +++ b/runtime/benches/accounts.rs @@ -41,14 +41,6 @@ fn deposit_many(bank: &Bank, pubkeys: &mut Vec, num: usize) -> Result<() Ok(()) } -#[bench] -fn bench_has_duplicates(bencher: &mut Bencher) { - bencher.iter(|| { - let data = test::black_box([1, 2, 3]); - assert!(!Accounts::has_duplicates(&data)); - }) -} - #[bench] fn test_accounts_create(bencher: &mut Bencher) { let (genesis_config, _) = create_genesis_config(10_000); diff --git a/runtime/benches/bank.rs b/runtime/benches/bank.rs index 22f7b48c06..9442ddfe9a 100644 --- a/runtime/benches/bank.rs +++ b/runtime/benches/bank.rs @@ -84,7 +84,7 @@ pub fn create_native_loader_transactions( } fn sync_bencher(bank: &Arc, _bank_client: &BankClient, transactions: &[Transaction]) { - let results = bank.process_transactions(transactions); + let results = bank.process_transactions(transactions.iter()); assert!(results.iter().all(Result::is_ok)); } @@ -136,7 +136,7 @@ fn do_bench_transactions( let transactions = create_transactions(&bank_client, &mint_keypair); // Do once to fund accounts, load modules, etc... - let results = bank.process_transactions(&transactions); + let results = bank.process_transactions(transactions.iter()); assert!(results.iter().all(Result::is_ok)); bencher.iter(|| { diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 66792a1cef..d2cd586a6d 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -165,19 +165,6 @@ impl Accounts { } } - /// Return true if the slice has any duplicate elements - pub fn has_duplicates(xs: &[T]) -> bool { - // Note: This is an O(n^2) algorithm, but requires no heap allocations. The benchmark - // `bench_has_duplicates` in benches/message_processor.rs shows that this implementation is - // ~50 times faster than using HashSet for very short slices. - for i in 1..xs.len() { - if xs[i..].contains(&xs[i - 1]) { - return true; - } - } - false - } - fn construct_instructions_account(message: &Message) -> AccountSharedData { let mut data = message.serialize_instructions(); // add room for current instruction index. @@ -858,25 +845,13 @@ impl Accounts { #[must_use] #[allow(clippy::needless_collect)] pub fn lock_accounts<'a>(&self, txs: impl Iterator) -> Vec> { - use solana_sdk::sanitize::Sanitize; - let keys: Vec> = txs - .map(|tx| { - tx.sanitize().map_err(TransactionError::from)?; - - if Self::has_duplicates(&tx.message.account_keys) { - return Err(TransactionError::AccountLoadedTwice); - } - - Ok(tx.message().get_account_keys_by_lock_type()) - }) + let keys: Vec<_> = txs + .map(|tx| tx.message().get_account_keys_by_lock_type()) .collect(); let mut account_locks = &mut self.account_locks.lock().unwrap(); keys.into_iter() - .map(|result| match result { - Ok((writable_keys, readonly_keys)) => { - self.lock_account(&mut account_locks, writable_keys, readonly_keys) - } - Err(e) => Err(e), + .map(|(writable_keys, readonly_keys)| { + self.lock_account(&mut account_locks, writable_keys, readonly_keys) }) .collect() } @@ -2035,12 +2010,6 @@ mod tests { ); } - #[test] - fn test_has_duplicates() { - assert!(!Accounts::has_duplicates(&[1, 2])); - assert!(Accounts::has_duplicates(&[1, 2, 1])); - } - #[test] fn huge_clean() { solana_logger::setup(); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index fb6dfb63b6..48ac83021c 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -84,7 +84,6 @@ use solana_sdk::{ genesis_config::{ClusterType, GenesisConfig}, hard_forks::HardForks, hash::{extend_and_hash, hashv, Hash}, - hashed_transaction::{HashedTransaction, HashedTransactionSlice}, incinerator, inflation::Inflation, instruction::CompiledInstruction, @@ -97,7 +96,7 @@ use solana_sdk::{ program_utils::limited_deserialize, pubkey::Pubkey, recent_blockhashes_account, - sanitize::Sanitize, + sanitized_transaction::{SanitizedTransaction, SanitizedTransactionSlice}, signature::{Keypair, Signature}, slot_hashes::SlotHashes, slot_history::SlotHistory, @@ -2588,19 +2587,18 @@ impl Bank { fn update_transaction_statuses( &self, - hashed_txs: &[HashedTransaction], + sanitized_txs: &[SanitizedTransaction], res: &[TransactionExecutionResult], ) { let mut status_cache = self.src.status_cache.write().unwrap(); - assert_eq!(hashed_txs.len(), res.len()); - for (hashed_tx, (res, _nonce_rollback)) in hashed_txs.iter().zip(res) { - let tx = hashed_tx.transaction(); + assert_eq!(sanitized_txs.len(), res.len()); + for (tx, (res, _nonce_rollback)) in sanitized_txs.iter().zip(res) { if Self::can_commit(res) && !tx.signatures.is_empty() { // Add the message hash to the status cache to ensure that this message // won't be processed again with a different signature. status_cache.insert( &tx.message().recent_blockhash, - &hashed_tx.message_hash, + &tx.message_hash, self.slot(), res.clone(), ); @@ -2654,44 +2652,37 @@ impl Bank { pub fn prepare_batch<'a, 'b>( &'a self, txs: impl Iterator, - ) -> TransactionBatch<'a, 'b> { - let hashed_txs: Vec = txs.map(HashedTransaction::from).collect(); + ) -> Result> { + let sanitized_txs: Vec = txs + .map(SanitizedTransaction::try_from) + .collect::>()?; let lock_results = self .rc .accounts - .lock_accounts(hashed_txs.as_transactions_iter()); - TransactionBatch::new(lock_results, self, Cow::Owned(hashed_txs)) + .lock_accounts(sanitized_txs.as_transactions_iter()); + Ok(TransactionBatch::new( + lock_results, + self, + Cow::Owned(sanitized_txs), + )) } - pub fn prepare_hashed_batch<'a, 'b>( + pub fn prepare_sanitized_batch<'a, 'b>( &'a self, - hashed_txs: &'b [HashedTransaction], + sanitized_txs: &'b [SanitizedTransaction], ) -> TransactionBatch<'a, 'b> { let lock_results = self .rc .accounts - .lock_accounts(hashed_txs.as_transactions_iter()); - TransactionBatch::new(lock_results, self, Cow::Borrowed(hashed_txs)) + .lock_accounts(sanitized_txs.as_transactions_iter()); + TransactionBatch::new(lock_results, self, Cow::Borrowed(sanitized_txs)) } pub(crate) fn prepare_simulation_batch<'a, 'b>( &'a self, - tx: &'b Transaction, + tx: SanitizedTransaction<'b>, ) -> TransactionBatch<'a, 'b> { - let check_transaction = |tx: &Transaction| -> Result<()> { - tx.sanitize().map_err(TransactionError::from)?; - if Accounts::has_duplicates(&tx.message.account_keys) { - Err(TransactionError::AccountLoadedTwice) - } else { - Ok(()) - } - }; - - let mut batch = TransactionBatch::new( - vec![check_transaction(tx)], - self, - Cow::Owned(vec![HashedTransaction::from(tx)]), - ); + let mut batch = TransactionBatch::new(vec![Ok(())], self, Cow::Owned(vec![tx])); batch.needs_unlock = false; batch } @@ -2707,7 +2698,10 @@ impl Bank { ) { assert!(self.is_frozen(), "simulation bank must be frozen"); - let batch = self.prepare_simulation_batch(transaction); + let batch = match SanitizedTransaction::try_from(transaction) { + Ok(sanitized_tx) => self.prepare_simulation_batch(sanitized_tx), + Err(err) => return (Err(err), vec![], vec![]), + }; let mut timings = ExecuteTimings::default(); @@ -2795,11 +2789,11 @@ impl Bank { fn is_tx_already_processed( &self, - hashed_tx: &HashedTransaction, + sanitized_tx: &SanitizedTransaction, status_cache: &StatusCache>, ) -> bool { - let key = &hashed_tx.message_hash; - let transaction_blockhash = &hashed_tx.transaction().message().recent_blockhash; + let key = &sanitized_tx.message_hash; + let transaction_blockhash = &sanitized_tx.message().recent_blockhash; status_cache .get_status(key, transaction_blockhash, &self.ancestors) .is_some() @@ -2807,16 +2801,16 @@ impl Bank { fn check_status_cache( &self, - hashed_txs: &[HashedTransaction], + sanitized_txs: &[SanitizedTransaction], lock_results: Vec, error_counters: &mut ErrorCounters, ) -> Vec { let rcache = self.src.status_cache.read().unwrap(); - hashed_txs + sanitized_txs .iter() .zip(lock_results) - .map(|(hashed_tx, (lock_res, nonce_rollback))| { - if lock_res.is_ok() && self.is_tx_already_processed(hashed_tx, &rcache) { + .map(|(sanitized_tx, (lock_res, nonce_rollback))| { + if lock_res.is_ok() && self.is_tx_already_processed(sanitized_tx, &rcache) { error_counters.already_processed += 1; return (Err(TransactionError::AlreadyProcessed), None); } @@ -2881,22 +2875,23 @@ impl Bank { pub fn check_transactions( &self, - hashed_txs: &[HashedTransaction], + sanitized_txs: &[SanitizedTransaction], lock_results: &[Result<()>], max_age: usize, mut error_counters: &mut ErrorCounters, ) -> Vec { let age_results = self.check_age( - hashed_txs.as_transactions_iter(), + sanitized_txs.as_transactions_iter(), lock_results.to_vec(), max_age, &mut error_counters, ); - let cache_results = self.check_status_cache(hashed_txs, age_results, &mut error_counters); + let cache_results = + self.check_status_cache(sanitized_txs, age_results, &mut error_counters); if self.upgrade_epoch() { // Reject all non-vote transactions self.filter_by_vote_transactions( - hashed_txs.as_transactions_iter(), + sanitized_txs.as_transactions_iter(), cache_results, &mut error_counters, ) @@ -3131,9 +3126,9 @@ impl Bank { u64, u64, ) { - let hashed_txs = batch.hashed_transactions(); - debug!("processing transactions: {}", hashed_txs.len()); - inc_new_counter_info!("bank-process_transactions", hashed_txs.len()); + let sanitized_txs = batch.sanitized_transactions(); + debug!("processing transactions: {}", sanitized_txs.len()); + inc_new_counter_info!("bank-process_transactions", sanitized_txs.len()); let mut error_counters = ErrorCounters::default(); let retryable_txs: Vec<_> = batch @@ -3152,7 +3147,7 @@ impl Bank { let mut check_time = Measure::start("check_transactions"); let check_results = self.check_transactions( - hashed_txs, + sanitized_txs, batch.lock_results(), max_age, &mut error_counters, @@ -3162,7 +3157,7 @@ impl Bank { let mut load_time = Measure::start("accounts_load"); let mut loaded_txs = self.rc.accounts.load_accounts( &self.ancestors, - hashed_txs.as_transactions_iter(), + sanitized_txs.as_transactions_iter(), check_results, &self.blockhash_queue.read().unwrap(), &mut error_counters, @@ -3174,16 +3169,16 @@ impl Bank { let mut execution_time = Measure::start("execution_time"); let mut signature_count: u64 = 0; let mut inner_instructions: Vec> = - Vec::with_capacity(hashed_txs.len()); + Vec::with_capacity(sanitized_txs.len()); let mut transaction_log_messages: Vec>> = - Vec::with_capacity(hashed_txs.len()); + Vec::with_capacity(sanitized_txs.len()); let bpf_compute_budget = self .bpf_compute_budget .unwrap_or_else(BpfComputeBudget::new); let executed: Vec = loaded_txs .iter_mut() - .zip(hashed_txs.as_transactions_iter()) + .zip(sanitized_txs.as_transactions_iter()) .map(|(accs, tx)| match accs { (Err(e), _nonce_rollback) => { inner_instructions.push(None); @@ -3270,7 +3265,7 @@ impl Bank { check_time.as_us(), load_time.as_us(), execution_time.as_us(), - hashed_txs.len(), + sanitized_txs.len(), ); timings.check_us += check_time.as_us(); timings.load_us += load_time.as_us(); @@ -3281,8 +3276,7 @@ impl Bank { let transaction_log_collector_config = self.transaction_log_collector_config.read().unwrap(); - for (i, ((r, _nonce_rollback), hashed_tx)) in executed.iter().zip(hashed_txs).enumerate() { - let tx = hashed_tx.transaction(); + for (i, ((r, _nonce_rollback), tx)) in executed.iter().zip(sanitized_txs).enumerate() { if let Some(debug_keys) = &self.transaction_debug_keys { for key in &tx.message.account_keys { if debug_keys.contains(key) { @@ -3423,7 +3417,7 @@ impl Bank { pub fn commit_transactions( &self, - hashed_txs: &[HashedTransaction], + sanitized_txs: &[SanitizedTransaction], loaded_txs: &mut [TransactionLoadResult], executed: &[TransactionExecutionResult], tx_count: u64, @@ -3441,8 +3435,8 @@ impl Bank { inc_new_counter_info!("bank-process_transactions-txs", tx_count as usize); inc_new_counter_info!("bank-process_transactions-sigs", signature_count as usize); - if !hashed_txs.is_empty() { - let processed_tx_count = hashed_txs.len() as u64; + if !sanitized_txs.is_empty() { + let processed_tx_count = sanitized_txs.len() as u64; let failed_tx_count = processed_tx_count.saturating_sub(tx_count); self.transaction_error_count .fetch_add(failed_tx_count, Relaxed); @@ -3461,7 +3455,7 @@ impl Bank { let mut write_time = Measure::start("write_time"); self.rc.accounts.store_cached( self.slot(), - hashed_txs.as_transactions_iter(), + sanitized_txs.as_transactions_iter(), executed, loaded_txs, &self.rent_collector, @@ -3472,19 +3466,19 @@ impl Bank { let rent_debits = self.collect_rent(executed, loaded_txs); let overwritten_vote_accounts = - self.update_cached_accounts(hashed_txs.as_transactions_iter(), executed, loaded_txs); + self.update_cached_accounts(sanitized_txs.as_transactions_iter(), executed, loaded_txs); // once committed there is no way to unroll write_time.stop(); debug!( "store: {}us txs_len={}", write_time.as_us(), - hashed_txs.len() + sanitized_txs.len() ); timings.store_us += write_time.as_us(); - self.update_transaction_statuses(hashed_txs, executed); - let fee_collection_results = - self.filter_program_errors_and_collect_fee(hashed_txs.as_transactions_iter(), executed); + self.update_transaction_statuses(sanitized_txs, executed); + let fee_collection_results = self + .filter_program_errors_and_collect_fee(sanitized_txs.as_transactions_iter(), executed); TransactionResults { fee_collection_results, @@ -4115,7 +4109,7 @@ impl Bank { ); let results = self.commit_transactions( - batch.hashed_transactions(), + batch.sanitized_transactions(), &mut loaded_txs, &executed, tx_count, @@ -4136,19 +4130,35 @@ impl Bank { } /// Process a Transaction. This is used for unit tests and simply calls the vector - /// Bank::process_transactions method + /// Bank::process_transactions method. pub fn process_transaction(&self, tx: &Transaction) -> Result<()> { - let batch = self.prepare_batch(std::iter::once(tx)); - self.process_transaction_batch(&batch)[0].clone()?; + self.try_process_transactions(std::iter::once(tx))?[0].clone()?; tx.signatures .get(0) .map_or(Ok(()), |sig| self.get_signature_status(sig).unwrap()) } + /// Process multiple transaction in a single batch. This is used for benches and unit tests. + /// + /// # Panics + /// + /// Panics if any of the transactions do not pass sanitization checks. #[must_use] - pub fn process_transactions(&self, txs: &[Transaction]) -> Vec> { - let batch = self.prepare_batch(txs.iter()); - self.process_transaction_batch(&batch) + pub fn process_transactions<'a>( + &self, + txs: impl Iterator, + ) -> Vec> { + self.try_process_transactions(txs).unwrap() + } + + /// Process multiple transaction in a single batch. This is used for benches and unit tests. + /// Short circuits if any of the transactions do not pass sanitization checks. + pub fn try_process_transactions<'a>( + &self, + txs: impl Iterator, + ) -> Result>> { + let batch = self.prepare_batch(txs)?; + Ok(self.process_transaction_batch(&batch)) } #[must_use] @@ -5813,7 +5823,8 @@ pub(crate) mod tests { let t3 = system_transaction::transfer(&keypair5, &keypair6.pubkey(), 1, genesis_config.hash()); - let res = bank.process_transactions(&[t1.clone(), t2.clone(), t3]); + let txs = vec![t1.clone(), t2.clone(), t3]; + let res = bank.process_transactions(txs.iter()); assert_eq!(res.len(), 3); assert_eq!(res[0], Ok(())); @@ -5825,7 +5836,8 @@ pub(crate) mod tests { let rwlockguard_bank_hash = bank.hash.read().unwrap(); let bank_hash = rwlockguard_bank_hash.as_ref(); - let res = bank_with_success_txs.process_transactions(&[t2, t1]); + let txs = vec![t2, t1]; + let res = bank_with_success_txs.process_transactions(txs.iter()); assert_eq!(res.len(), 2); assert_eq!(res[0], Ok(())); @@ -6528,7 +6540,8 @@ pub(crate) mod tests { genesis_config.hash(), ); - let res = bank.process_transactions(&[t6, t5, t1, t2, t3, t4]); + let txs = vec![t6, t5, t1, t2, t3, t4]; + let res = bank.process_transactions(txs.iter()); assert_eq!(res.len(), 6); assert_eq!(res[0], Ok(())); @@ -7717,7 +7730,8 @@ pub(crate) mod tests { let t1 = system_transaction::transfer(&mint_keypair, &key1, 1, genesis_config.hash()); let t2 = system_transaction::transfer(&mint_keypair, &key2, 1, genesis_config.hash()); - let res = bank.process_transactions(&[t1.clone(), t2.clone()]); + let txs = vec![t1.clone(), t2.clone()]; + let res = bank.process_transactions(txs.iter()); assert_eq!(res.len(), 2); assert_eq!(res[0], Ok(())); @@ -8164,7 +8178,7 @@ pub(crate) mod tests { genesis_config.hash(), ); let txs = vec![tx0, tx1]; - let results = bank.process_transactions(&txs); + let results = bank.process_transactions(txs.iter()); assert!(results[1].is_err()); // Assert bad transactions aren't counted. @@ -8220,7 +8234,7 @@ pub(crate) mod tests { bank.last_blockhash(), ); let txs = vec![tx0, tx1]; - let results = bank.process_transactions(&txs); + let results = bank.process_transactions(txs.iter()); // If multiple transactions attempt to read the same account, they should succeed. // Vote authorized_voter and sysvar accounts are given read-only handling @@ -8241,7 +8255,7 @@ pub(crate) mod tests { bank.last_blockhash(), ); let txs = vec![tx0, tx1]; - let results = bank.process_transactions(&txs); + let results = bank.process_transactions(txs.iter()); // However, an account may not be locked as read-only and writable at the same time. assert_eq!(results[0], Ok(())); assert_eq!(results[1], Err(TransactionError::AccountInUse)); @@ -8258,7 +8272,7 @@ pub(crate) mod tests { system_transaction::transfer(&mint_keypair, &alice.pubkey(), 1, genesis_config.hash()); let pay_alice = vec![tx1]; - let lock_result = bank.prepare_batch(pay_alice.iter()); + let lock_result = bank.prepare_batch(pay_alice.iter()).unwrap(); let results_alice = bank .load_execute_and_commit_transactions( &lock_result, @@ -8311,7 +8325,7 @@ pub(crate) mod tests { let tx = Transaction::new(&[&key0], message, genesis_config.hash()); let txs = vec![tx]; - let batch0 = bank.prepare_batch(txs.iter()); + let batch0 = bank.prepare_batch(txs.iter()).unwrap(); assert!(batch0.lock_results()[0].is_ok()); // Try locking accounts, locking a previously read-only account as writable @@ -8329,7 +8343,7 @@ pub(crate) mod tests { let tx = Transaction::new(&[&key1], message, genesis_config.hash()); let txs = vec![tx]; - let batch1 = bank.prepare_batch(txs.iter()); + let batch1 = bank.prepare_batch(txs.iter()).unwrap(); assert!(batch1.lock_results()[0].is_err()); // Try locking a previously read-only account a 2nd time; should succeed @@ -8346,7 +8360,7 @@ pub(crate) mod tests { let tx = Transaction::new(&[&key2], message, genesis_config.hash()); let txs = vec![tx]; - let batch2 = bank.prepare_batch(txs.iter()); + let batch2 = bank.prepare_batch(txs.iter()).unwrap(); assert!(batch2.lock_results()[0].is_ok()); } @@ -10231,14 +10245,14 @@ pub(crate) mod tests { instructions, ); let txs = vec![tx0, tx1]; - let batch = bank0.prepare_batch(txs.iter()); + let batch = bank0.prepare_batch(txs.iter()).unwrap(); let balances = bank0.collect_balances(&batch); assert_eq!(balances.len(), 2); assert_eq!(balances[0], vec![8, 11, 1]); assert_eq!(balances[1], vec![8, 0, 1]); let txs: Vec<_> = txs.iter().rev().cloned().collect(); - let batch = bank0.prepare_batch(txs.iter()); + let batch = bank0.prepare_batch(txs.iter()).unwrap(); let balances = bank0.collect_balances(&batch); assert_eq!(balances.len(), 2); assert_eq!(balances[0], vec![8, 0, 1]); @@ -10272,7 +10286,7 @@ pub(crate) mod tests { let tx2 = system_transaction::transfer(&keypair1, &pubkey2, 12, blockhash); let txs = vec![tx0, tx1, tx2]; - let lock_result = bank0.prepare_batch(txs.iter()); + let lock_result = bank0.prepare_batch(txs.iter()).unwrap(); let (transaction_results, transaction_balances_set, inner_instructions, transaction_logs) = bank0.load_execute_and_commit_transactions( &lock_result, @@ -13335,10 +13349,9 @@ pub(crate) mod tests { let success_sig = tx0.signatures[0]; let tx1 = system_transaction::transfer(&sender1, &recipient1, 110, blockhash); // Should produce insufficient funds log let failure_sig = tx1.signatures[0]; - let mut invalid_tx = system_transaction::transfer(&sender1, &recipient1, 10, blockhash); - invalid_tx.message.header.num_required_signatures = 0; // this tx won't be processed because it has no signers - let txs = vec![invalid_tx, tx1, tx0]; - let batch = bank.prepare_batch(txs.iter()); + let tx2 = system_transaction::transfer(&sender0, &recipient0, 1, blockhash); + let txs = vec![tx0, tx1, tx2]; + let batch = bank.prepare_batch(txs.iter()).unwrap(); let log_results = bank .load_execute_and_commit_transactions( @@ -13351,9 +13364,9 @@ pub(crate) mod tests { ) .3; assert_eq!(log_results.len(), 3); - assert!(log_results[0].as_ref().is_none()); + assert!(log_results[0].as_ref().unwrap()[1].contains(&"success".to_string())); assert!(log_results[1].as_ref().unwrap()[2].contains(&"failed".to_string())); - assert!(log_results[2].as_ref().unwrap()[1].contains(&"success".to_string())); + assert!(log_results[2].as_ref().is_none()); let stored_logs = &bank.transaction_log_collector.read().unwrap().logs; let success_log_info = stored_logs diff --git a/runtime/src/bank_client.rs b/runtime/src/bank_client.rs index 55d29f7045..54cab1026d 100644 --- a/runtime/src/bank_client.rs +++ b/runtime/src/bank_client.rs @@ -280,7 +280,7 @@ impl BankClient { while let Ok(tx) = transaction_receiver.try_recv() { transactions.push(tx); } - let _ = bank.process_transactions(&transactions); + let _ = bank.try_process_transactions(transactions.iter()); } } diff --git a/runtime/src/bank_utils.rs b/runtime/src/bank_utils.rs index 286ecda44e..94a7b91afd 100644 --- a/runtime/src/bank_utils.rs +++ b/runtime/src/bank_utils.rs @@ -3,7 +3,7 @@ use crate::{ genesis_utils::{self, GenesisConfigInfo, ValidatorVoteKeypairs}, vote_sender_types::ReplayVoteSender, }; -use solana_sdk::{hashed_transaction::HashedTransaction, pubkey::Pubkey, signature::Signer}; +use solana_sdk::{pubkey::Pubkey, sanitized_transaction::SanitizedTransaction, signature::Signer}; use solana_vote_program::vote_transaction; pub fn setup_bank_and_vote_pubkeys(num_vote_accounts: usize, stake: u64) -> (Bank, Vec) { @@ -27,7 +27,7 @@ pub fn setup_bank_and_vote_pubkeys(num_vote_accounts: usize, stake: u64) -> (Ban } pub fn find_and_send_votes( - hashed_txs: &[HashedTransaction], + sanitized_txs: &[SanitizedTransaction], tx_results: &TransactionResults, vote_sender: Option<&ReplayVoteSender>, ) { @@ -41,7 +41,7 @@ pub fn find_and_send_votes( assert!(execution_results[old_account.transaction_result_index] .0 .is_ok()); - let transaction = hashed_txs[old_account.transaction_index].transaction(); + let transaction = &sanitized_txs[old_account.transaction_index]; if let Some(parsed_vote) = vote_transaction::parse_vote_transaction(transaction) { if parsed_vote.1.slots.last().is_some() { let _ = vote_sender.send(parsed_vote); diff --git a/runtime/src/transaction_batch.rs b/runtime/src/transaction_batch.rs index 10e1d40643..c76bdabef2 100644 --- a/runtime/src/transaction_batch.rs +++ b/runtime/src/transaction_batch.rs @@ -1,13 +1,16 @@ use crate::bank::Bank; -use solana_sdk::hashed_transaction::HashedTransaction; -use solana_sdk::transaction::{Result, Transaction}; +use solana_sdk::{ + sanitized_transaction::SanitizedTransaction, + transaction::{Result, Transaction}, +}; use std::borrow::Cow; +use std::ops::Deref; // Represents the results of trying to lock a set of accounts pub struct TransactionBatch<'a, 'b> { lock_results: Vec>, bank: &'a Bank, - hashed_txs: Cow<'b, [HashedTransaction<'b>]>, + sanitized_txs: Cow<'b, [SanitizedTransaction<'b>]>, pub(crate) needs_unlock: bool, } @@ -15,13 +18,13 @@ impl<'a, 'b> TransactionBatch<'a, 'b> { pub fn new( lock_results: Vec>, bank: &'a Bank, - hashed_txs: Cow<'b, [HashedTransaction<'b>]>, + sanitized_txs: Cow<'b, [SanitizedTransaction<'b>]>, ) -> Self { - assert_eq!(lock_results.len(), hashed_txs.len()); + assert_eq!(lock_results.len(), sanitized_txs.len()); Self { lock_results, bank, - hashed_txs, + sanitized_txs, needs_unlock: true, } } @@ -30,12 +33,12 @@ impl<'a, 'b> TransactionBatch<'a, 'b> { &self.lock_results } - pub fn hashed_transactions(&self) -> &[HashedTransaction] { - &self.hashed_txs + pub fn sanitized_transactions(&self) -> &[SanitizedTransaction] { + &self.sanitized_txs } pub fn transactions_iter(&self) -> impl Iterator { - self.hashed_txs.iter().map(|h| h.transaction()) + self.sanitized_txs.iter().map(Deref::deref) } pub fn bank(&self) -> &Bank { @@ -55,43 +58,49 @@ mod tests { use super::*; use crate::genesis_utils::{create_genesis_config_with_leader, GenesisConfigInfo}; use solana_sdk::{signature::Keypair, system_transaction}; + use std::convert::TryFrom; #[test] fn test_transaction_batch() { let (bank, txs) = setup(); // Test getting locked accounts - let batch = bank.prepare_batch(txs.iter()); + let batch = bank.prepare_batch(txs.iter()).unwrap(); // Grab locks assert!(batch.lock_results().iter().all(|x| x.is_ok())); // Trying to grab locks again should fail - let batch2 = bank.prepare_batch(txs.iter()); + let batch2 = bank.prepare_batch(txs.iter()).unwrap(); assert!(batch2.lock_results().iter().all(|x| x.is_err())); // Drop the first set of locks drop(batch); // Now grabbing locks should work again - let batch2 = bank.prepare_batch(txs.iter()); + let batch2 = bank.prepare_batch(txs.iter()).unwrap(); assert!(batch2.lock_results().iter().all(|x| x.is_ok())); } #[test] fn test_simulation_batch() { let (bank, txs) = setup(); + let txs = txs + .into_iter() + .map(SanitizedTransaction::try_from) + .collect::>>() + .unwrap(); // Prepare batch without locks - let batch = bank.prepare_simulation_batch(&txs[0]); + let batch = bank.prepare_simulation_batch(txs[0].clone()); assert!(batch.lock_results().iter().all(|x| x.is_ok())); // Grab locks - let batch2 = bank.prepare_batch(txs.iter()); + let batch2 = bank.prepare_sanitized_batch(&txs); assert!(batch2.lock_results().iter().all(|x| x.is_ok())); // Prepare another batch without locks - let batch3 = bank.prepare_simulation_batch(&txs[0]); + let batch3 = bank.prepare_simulation_batch(txs[0].clone()); assert!(batch3.lock_results().iter().all(|x| x.is_ok())); } diff --git a/sdk/benches/has_duplicates.rs b/sdk/benches/has_duplicates.rs new file mode 100644 index 0000000000..f1d2169d09 --- /dev/null +++ b/sdk/benches/has_duplicates.rs @@ -0,0 +1,13 @@ +#![feature(test)] + +extern crate test; +use solana_sdk::sanitized_transaction::SanitizedTransaction; +use test::Bencher; + +#[bench] +fn bench_has_duplicates(bencher: &mut Bencher) { + bencher.iter(|| { + let data = test::black_box([1, 2, 3]); + assert!(!SanitizedTransaction::has_duplicates(&data)); + }) +} diff --git a/sdk/src/hashed_transaction.rs b/sdk/src/hashed_transaction.rs deleted file mode 100644 index f478ab9fd5..0000000000 --- a/sdk/src/hashed_transaction.rs +++ /dev/null @@ -1,52 +0,0 @@ -#![cfg(feature = "full")] - -use crate::{hash::Hash, transaction::Transaction}; -use std::borrow::Cow; - -/// Transaction and the hash of its message -#[derive(Debug, Clone)] -pub struct HashedTransaction<'a> { - transaction: Cow<'a, Transaction>, - pub message_hash: Hash, -} - -impl<'a> HashedTransaction<'a> { - pub fn new(transaction: Cow<'a, Transaction>, message_hash: Hash) -> Self { - Self { - transaction, - message_hash, - } - } - - pub fn transaction(&self) -> &Transaction { - self.transaction.as_ref() - } -} - -impl<'a> From for HashedTransaction<'_> { - fn from(transaction: Transaction) -> Self { - Self { - message_hash: transaction.message().hash(), - transaction: Cow::Owned(transaction), - } - } -} - -impl<'a> From<&'a Transaction> for HashedTransaction<'a> { - fn from(transaction: &'a Transaction) -> Self { - Self { - message_hash: transaction.message().hash(), - transaction: Cow::Borrowed(transaction), - } - } -} - -pub trait HashedTransactionSlice<'a> { - fn as_transactions_iter(&'a self) -> Box + '_>; -} - -impl<'a> HashedTransactionSlice<'a> for [HashedTransaction<'a>] { - fn as_transactions_iter(&'a self) -> Box + '_> { - Box::new(self.iter().map(|h| h.transaction.as_ref())) - } -} diff --git a/sdk/src/lib.rs b/sdk/src/lib.rs index 292461176f..dbeae87ca4 100644 --- a/sdk/src/lib.rs +++ b/sdk/src/lib.rs @@ -26,7 +26,6 @@ pub mod feature_set; pub mod genesis_config; pub mod hard_forks; pub mod hash; -pub mod hashed_transaction; pub mod inflation; pub mod keyed_account; pub mod log; @@ -40,6 +39,7 @@ pub mod program_utils; pub mod pubkey; pub mod recent_blockhashes_account; pub mod rpc_port; +pub mod sanitized_transaction; pub mod secp256k1_instruction; pub mod shred_version; pub mod signature; diff --git a/sdk/src/sanitized_transaction.rs b/sdk/src/sanitized_transaction.rs new file mode 100644 index 0000000000..aaf9f0be5f --- /dev/null +++ b/sdk/src/sanitized_transaction.rs @@ -0,0 +1,87 @@ +#![cfg(feature = "full")] + +use crate::{ + hash::Hash, + sanitize::Sanitize, + transaction::{Result, Transaction, TransactionError}, +}; +use std::{borrow::Cow, convert::TryFrom, ops::Deref}; + +/// Sanitized transaction and the hash of its message +#[derive(Debug, Clone)] +pub struct SanitizedTransaction<'a> { + transaction: Cow<'a, Transaction>, + pub message_hash: Hash, +} + +impl<'a> SanitizedTransaction<'a> { + pub fn try_create(transaction: Cow<'a, Transaction>, message_hash: Hash) -> Result { + transaction.sanitize()?; + if Self::has_duplicates(&transaction.message.account_keys) { + return Err(TransactionError::AccountLoadedTwice); + } + + Ok(Self { + transaction, + message_hash, + }) + } + + /// Return true if the slice has any duplicate elements + pub fn has_duplicates(xs: &[T]) -> bool { + // Note: This is an O(n^2) algorithm, but requires no heap allocations. The benchmark + // `bench_has_duplicates` in benches/message_processor.rs shows that this implementation is + // ~50 times faster than using HashSet for very short slices. + for i in 1..xs.len() { + #[allow(clippy::integer_arithmetic)] + if xs[i..].contains(&xs[i - 1]) { + return true; + } + } + false + } +} + +impl Deref for SanitizedTransaction<'_> { + type Target = Transaction; + fn deref(&self) -> &Self::Target { + &self.transaction + } +} + +impl<'a> TryFrom for SanitizedTransaction<'_> { + type Error = TransactionError; + fn try_from(transaction: Transaction) -> Result { + let message_hash = transaction.message().hash(); + Self::try_create(Cow::Owned(transaction), message_hash) + } +} + +impl<'a> TryFrom<&'a Transaction> for SanitizedTransaction<'a> { + type Error = TransactionError; + fn try_from(transaction: &'a Transaction) -> Result { + let message_hash = transaction.message().hash(); + Self::try_create(Cow::Borrowed(transaction), message_hash) + } +} + +pub trait SanitizedTransactionSlice<'a> { + fn as_transactions_iter(&'a self) -> Box + '_>; +} + +impl<'a> SanitizedTransactionSlice<'a> for [SanitizedTransaction<'a>] { + fn as_transactions_iter(&'a self) -> Box + '_> { + Box::new(self.iter().map(Deref::deref)) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_has_duplicates() { + assert!(!SanitizedTransaction::has_duplicates(&[1, 2])); + assert!(SanitizedTransaction::has_duplicates(&[1, 2, 1])); + } +}