diff --git a/core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs b/core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs index a60702b34af..a6bd32d8eac 100644 --- a/core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs +++ b/core/src/banking_stage/transaction_scheduler/receive_and_buffer.rs @@ -667,7 +667,7 @@ impl TransactionViewReceiveAndBuffer { transaction_account_lock_limit: usize, ) -> Result { // Parsing and basic sanitization checks - let Ok(view) = SanitizedTransactionView::try_new_sanitized(bytes) else { + let Ok(view) = SanitizedTransactionView::try_new_sanitized(bytes, working_bank.feature_set.is_active(&agave_feature_set::static_instruction_limit::id())) else { return Err(PacketHandlingError::Sanitization); }; diff --git a/core/src/banking_stage/transaction_scheduler/transaction_state_container.rs b/core/src/banking_stage/transaction_scheduler/transaction_state_container.rs index bfd1419dcf1..dbed1199f06 100644 --- a/core/src/banking_stage/transaction_scheduler/transaction_state_container.rs +++ b/core/src/banking_stage/transaction_scheduler/transaction_state_container.rs @@ -437,7 +437,7 @@ mod tests { let reserved_addresses = HashSet::default(); let packet_parser = |data, priority, cost| { - let view = SanitizedTransactionView::try_new_sanitized(data).unwrap(); + let view = SanitizedTransactionView::try_new_sanitized(data, true).unwrap(); let view = RuntimeTransaction::>::try_from( view, MessageHash::Compute, diff --git a/core/src/forwarding_stage.rs b/core/src/forwarding_stage.rs index 60e0f2bfd7d..52ba069719b 100644 --- a/core/src/forwarding_stage.rs +++ b/core/src/forwarding_stage.rs @@ -324,19 +324,22 @@ impl // Perform basic sanitization checks and calculate priority. // If any steps fail, drop the packet. - let Some(priority) = SanitizedTransactionView::try_new_sanitized(packet_data) + let Some(priority) = SanitizedTransactionView::try_new_sanitized( + packet_data, + bank.feature_set + .is_active(&agave_feature_set::static_instruction_limit::id()), + ) + .map_err(|_| ()) + .and_then(|transaction| { + RuntimeTransaction::>::try_from( + transaction, + MessageHash::Compute, + Some(packet.meta().is_simple_vote_tx()), + ) .map_err(|_| ()) - .and_then(|transaction| { - RuntimeTransaction::>::try_from( - transaction, - MessageHash::Compute, - Some(packet.meta().is_simple_vote_tx()), - ) - .map_err(|_| ()) - }) - .ok() - .and_then(|transaction| calculate_priority(&transaction, bank)) - else { + }) + .ok() + .and_then(|transaction| calculate_priority(&transaction, bank)) else { self.metrics.votes_dropped_on_receive += vote_count; self.metrics.non_votes_dropped_on_receive += non_vote_count; continue; diff --git a/feature-set/src/lib.rs b/feature-set/src/lib.rs index 0524e423d94..29946256601 100644 --- a/feature-set/src/lib.rs +++ b/feature-set/src/lib.rs @@ -1126,6 +1126,10 @@ pub mod enforce_fixed_fec_set { solana_pubkey::declare_id!("fixfecLZYMfkGzwq6NJA11Yw6KYztzXiK9QcL3K78in"); } +pub mod static_instruction_limit { + solana_pubkey::declare_id!("64ixypL1HPu8WtJhNSMb9mSgfFaJvsANuRkTbHyuLfnx"); +} + pub static FEATURE_NAMES: LazyLock> = LazyLock::new(|| { [ (secp256k1_program_enabled::id(), "secp256k1 program"), @@ -2035,6 +2039,10 @@ pub static FEATURE_NAMES: LazyLock> = LazyLock::n enforce_fixed_fec_set::id(), "SIMD-0317: Enforce 32 data + 32 coding shreds", ), + ( + static_instruction_limit::id(), + "SIMD-0160: static instruction limit", + ), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/perf/src/sigverify.rs b/perf/src/sigverify.rs index 118a26f6802..28481b4511f 100644 --- a/perf/src/sigverify.rs +++ b/perf/src/sigverify.rs @@ -45,6 +45,8 @@ struct PacketOffsets { pub msg_start: u32, pub pubkey_start: u32, pub pubkey_len: u32, + pub instruction_len: u32, + pub instruction_start: u32, } impl PacketOffsets { @@ -54,6 +56,8 @@ impl PacketOffsets { msg_start: u32, pubkey_start: u32, pubkey_len: u32, + instruction_len: u32, + instruction_start: u32, ) -> Self { Self { sig_len, @@ -61,6 +65,8 @@ impl PacketOffsets { msg_start, pubkey_start, pubkey_len, + instruction_len, + instruction_start, } } } @@ -261,7 +267,7 @@ fn do_get_packet_offsets( .checked_add(pubkey_len_size) .ok_or(PacketError::InvalidPubkeyLen)?; - let _ = pubkey_len + let blockhash_offset = pubkey_len .checked_mul(size_of::()) .and_then(|v| v.checked_add(pubkey_start)) .filter(|v| *v <= packet.meta().size) @@ -271,6 +277,37 @@ fn do_get_packet_offsets( return Err(PacketError::InvalidPubkeyLen); } + // read instruction length + let instructions_len_offset = blockhash_offset + .checked_add(size_of::()) + .ok_or(PacketError::InvalidLen)?; + + // Packet should have at least 1 more byte for instructions.len + let _ = instructions_len_offset + .checked_add(1usize) + .filter(|v| *v <= packet.meta().size) + .ok_or(PacketError::InvalidLen)?; + + let (instruction_len, instruction_len_size) = packet + .data(instructions_len_offset..) + .and_then(|bytes| decode_shortu16_len(bytes).ok()) + .ok_or(PacketError::InvalidLen)?; + + // SIMD-0160: skip if has more than 64 top instructions + if instruction_len > 64 { + return Err(PacketError::InvalidProgramLen); + } + + let instruction_start = instructions_len_offset + .checked_add(instruction_len_size) + .ok_or(PacketError::InvalidLen)?; + + // Packet should have at least 1 more byte for one instructions_program_id + let _ = instruction_start + .checked_add(1usize) + .filter(|v| *v <= packet.meta().size) + .ok_or(PacketError::InvalidLen)?; + let sig_start = current_offset .checked_add(sig_size) .ok_or(PacketError::InvalidLen)?; @@ -280,6 +317,9 @@ fn do_get_packet_offsets( let pubkey_start = current_offset .checked_add(pubkey_start) .ok_or(PacketError::InvalidLen)?; + let instruction_start = current_offset + .checked_add(instruction_start) + .ok_or(PacketError::InvalidLen)?; Ok(PacketOffsets::new( u32::try_from(sig_len_untrusted)?, @@ -287,6 +327,8 @@ fn do_get_packet_offsets( u32::try_from(msg_start)?, u32::try_from(pubkey_start)?, u32::try_from(pubkey_len)?, + u32::try_from(instruction_len)?, + u32::try_from(instruction_start)?, )) } @@ -303,7 +345,7 @@ fn get_packet_offsets( } } // force sigverify to fail by returning zeros - PacketOffsets::new(0, 0, 0, 0, 0) + PacketOffsets::new(0, 0, 0, 0, 0, 0, 0) } fn check_for_simple_vote_transaction( @@ -330,35 +372,13 @@ fn check_for_simple_vote_transaction( .checked_sub(current_offset) .ok_or(PacketError::InvalidLen)?; - let instructions_len_offset = (packet_offsets.pubkey_len as usize) - .checked_mul(size_of::()) - .and_then(|v| v.checked_add(pubkey_start)) - .and_then(|v| v.checked_add(size_of::())) - .ok_or(PacketError::InvalidLen)?; - - // Packet should have at least 1 more byte for instructions.len - let _ = instructions_len_offset - .checked_add(1usize) - .filter(|v| *v <= packet.meta().size) - .ok_or(PacketError::InvalidLen)?; - - let (instruction_len, instruction_len_size) = packet - .data(instructions_len_offset..) - .and_then(|bytes| decode_shortu16_len(bytes).ok()) - .ok_or(PacketError::InvalidLen)?; // skip if has more than 1 instruction - if instruction_len != 1 { + if packet_offsets.instruction_len != 1 { return Err(PacketError::InvalidProgramLen); } - let instruction_start = instructions_len_offset - .checked_add(instruction_len_size) - .ok_or(PacketError::InvalidLen)?; - - // Packet should have at least 1 more byte for one instructions_program_id - let _ = instruction_start - .checked_add(1usize) - .filter(|v| *v <= packet.meta().size) + let instruction_start = (packet_offsets.instruction_start as usize) + .checked_sub(current_offset) .ok_or(PacketError::InvalidLen)?; let instruction_program_id_index: usize = usize::from( @@ -681,7 +701,9 @@ mod tests { PACKETS_PER_BATCH, }, sigverify::{self, PacketOffsets}, - test_tx::{new_test_vote_tx, test_multisig_tx, test_tx}, + test_tx::{ + new_test_tx_with_number_of_ixs, new_test_vote_tx, test_multisig_tx, test_tx, + }, }, bincode::{deserialize, serialize}, bytes::{BufMut, Bytes, BytesMut}, @@ -691,11 +713,12 @@ mod tests { solana_message::{compiled_instruction::CompiledInstruction, Message, MessageHeader}, solana_signature::Signature, solana_signer::Signer, - solana_transaction::Transaction, + solana_transaction::{versioned::VersionedTransaction, Transaction}, std::{ iter::repeat_with, sync::atomic::{AtomicU64, Ordering}, }, + test_case::test_case, }; const SIG_OFFSET: usize = 1; @@ -996,6 +1019,7 @@ mod tests { let offsets = sigverify::do_get_packet_offsets(packet.as_ref(), 0).unwrap(); let expected_offsets = { legacy_offsets.pubkey_start += 1; + legacy_offsets.instruction_start += 1; legacy_offsets }; @@ -1033,6 +1057,8 @@ mod tests { packet_offsets.msg_start - packet_offsets.sig_start, packet_offsets.pubkey_start - packet_offsets.msg_start, packet_offsets.pubkey_len, + packet_offsets.instruction_len, + packet_offsets.instruction_start - packet_offsets.pubkey_start, ) } @@ -1040,23 +1066,23 @@ mod tests { fn test_get_packet_offsets() { assert_eq!( get_packet_offsets_from_tx(test_tx(), 0), - PacketOffsets::new(1, 1, 64, 4, 2) + PacketOffsets::new(1, 1, 64, 4, 2, 1, 97) ); assert_eq!( get_packet_offsets_from_tx(test_tx(), 100), - PacketOffsets::new(1, 1, 64, 4, 2) + PacketOffsets::new(1, 1, 64, 4, 2, 1, 97) ); // Ensure we're not indexing packet by the `current_offset` parameter. assert_eq!( get_packet_offsets_from_tx(test_tx(), 1_000_000), - PacketOffsets::new(1, 1, 64, 4, 2) + PacketOffsets::new(1, 1, 64, 4, 2, 1, 97) ); // Ensure we're returning sig_len, not sig_size. assert_eq!( get_packet_offsets_from_tx(test_multisig_tx(), 0), - PacketOffsets::new(2, 1, 128, 4, 4) + PacketOffsets::new(2, 1, 128, 4, 4, 1, 161) ); } @@ -1799,4 +1825,32 @@ mod tests { ] ) } + + #[test_case(false, false; "ok_ixs_legacy")] + #[test_case(true, false; "too_many_ixs_legacy")] + #[test_case(false, true; "ok_ixs_versioned")] + #[test_case(true, true; "too_many_ixs_versioned")] + fn test_number_of_instructions(too_many_ixs: bool, is_versioned_tx: bool) { + let mut number_of_ixs = 64; + if too_many_ixs { + number_of_ixs += 1; + } + + let packet = if is_versioned_tx { + let tx: VersionedTransaction = new_test_tx_with_number_of_ixs(number_of_ixs); + BytesPacket::from_data(None, tx.clone()).unwrap() + } else { + let tx: Transaction = new_test_tx_with_number_of_ixs(number_of_ixs); + BytesPacket::from_data(None, tx.clone()).unwrap() + }; + + if too_many_ixs { + assert_eq!( + do_get_packet_offsets(packet.as_ref(), 0), + Err(PacketError::InvalidProgramLen), + ); + } else { + assert!(do_get_packet_offsets(packet.as_ref(), 0).is_ok()); + } + } } diff --git a/perf/src/test_tx.rs b/perf/src/test_tx.rs index 0ec18a070e1..59d36af7ff5 100644 --- a/perf/src/test_tx.rs +++ b/perf/src/test_tx.rs @@ -3,11 +3,15 @@ use { solana_clock::Slot, solana_hash::Hash, solana_keypair::Keypair, - solana_message::compiled_instruction::CompiledInstruction, + solana_message::{ + compiled_instruction::CompiledInstruction, v0::Message as MessageV0, AccountMeta, + Instruction, Message, VersionedMessage, + }, + solana_pubkey::Pubkey, solana_sdk_ids::{stake, system_program}, solana_signer::Signer, solana_system_interface::instruction::SystemInstruction, - solana_transaction::Transaction, + solana_transaction::{versioned::VersionedTransaction, Transaction}, solana_vote::vote_transaction, solana_vote_program::vote_state::TowerSync, }; @@ -69,3 +73,46 @@ where switch_proof_hash, ) } + +pub fn new_test_tx_with_number_of_ixs(number_of_ixs: usize) -> T +where + T: FromTestTx, +{ + let program_id = Pubkey::new_unique(); + let account = Pubkey::new_unique(); + + let mut instructions = Vec::with_capacity(number_of_ixs); + for i in 0..number_of_ixs { + instructions.push(Instruction { + program_id, + accounts: vec![AccountMeta::new(account, false)], + data: vec![i as u8], + }); + } + + let payer = Keypair::new(); + let blockhash = Hash::new_unique(); + + T::from_test_tx(&payer, blockhash, instructions) +} + +/// Trait that unifies building legacy vs v0 transactions +pub trait FromTestTx: Sized { + fn from_test_tx(payer: &Keypair, blockhash: Hash, ixs: Vec) -> Self; +} + +impl FromTestTx for Transaction { + fn from_test_tx(payer: &Keypair, blockhash: Hash, ixs: Vec) -> Self { + let msg = Message::new(&ixs, Some(&payer.pubkey())); + Transaction::new(&[payer], msg, blockhash) + } +} + +impl FromTestTx for VersionedTransaction { + fn from_test_tx(payer: &Keypair, _blockhash: Hash, ixs: Vec) -> Self { + let msg_v0 = + MessageV0::try_compile(&payer.pubkey(), &ixs, &[], Hash::new_unique()).unwrap(); + let versioned = VersionedMessage::V0(msg_v0); + VersionedTransaction::try_new(versioned, &[payer]).unwrap() + } +} diff --git a/runtime-transaction/src/runtime_transaction/transaction_view.rs b/runtime-transaction/src/runtime_transaction/transaction_view.rs index 5f41103cf2e..20ff7e92ed1 100644 --- a/runtime-transaction/src/runtime_transaction/transaction_view.rs +++ b/runtime-transaction/src/runtime_transaction/transaction_view.rs @@ -220,7 +220,7 @@ mod tests { let hash = Hash::new_unique(); let transaction = - SanitizedTransactionView::try_new_sanitized(&serialized_transaction[..]).unwrap(); + SanitizedTransactionView::try_new_sanitized(&serialized_transaction[..], true).unwrap(); let static_runtime_transaction = RuntimeTransaction::>::try_from( transaction, @@ -252,7 +252,8 @@ mod tests { reserved_account_keys: &HashSet, ) { let bytes = bincode::serialize(&original_transaction).unwrap(); - let transaction_view = SanitizedTransactionView::try_new_sanitized(&bytes[..]).unwrap(); + let transaction_view = + SanitizedTransactionView::try_new_sanitized(&bytes[..], true).unwrap(); let runtime_transaction = RuntimeTransaction::>::try_from( transaction_view, MessageHash::Compute, @@ -318,7 +319,8 @@ mod tests { ) { let bytes = bincode::serialize(&original_transaction.to_versioned_transaction()).unwrap(); - let transaction_view = SanitizedTransactionView::try_new_sanitized(&bytes[..]).unwrap(); + let transaction_view = + SanitizedTransactionView::try_new_sanitized(&bytes[..], true).unwrap(); let runtime_transaction = RuntimeTransaction::>::try_from( transaction_view, MessageHash::Compute, diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index f82edfcf0b6..b2b45e160e4 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -4656,6 +4656,14 @@ impl Bank { } let message_hash = if verification_mode == TransactionVerificationMode::FullVerification { + // SIMD-0160, check instruction limit before signature verificaton + if self + .feature_set + .is_active(&agave_feature_set::static_instruction_limit::id()) + && tx.message.instructions().len() > 64 + { + return Err(solana_transaction_error::TransactionError::SanitizeFailure); + } tx.verify_and_hash_message()? } else { tx.message.hash() diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index e4070fb2c19..5b2cb9cf3dc 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -9133,6 +9133,51 @@ fn test_verify_transactions_packet_data_size() { } } +#[test_case(false; "pre_simd160_static_instruction_limit")] +#[test_case(true; "simd160_static_instruction_limit")] +fn test_verify_transactions_instruction_limit(simd_0160_enabled: bool) { + let GenesisConfigInfo { genesis_config, .. } = + create_genesis_config_with_leader(42, &solana_pubkey::new_rand(), 42); + let mut bank = Bank::new_for_tests(&genesis_config); + if !simd_0160_enabled { + bank.deactivate_feature(&feature_set::static_instruction_limit::id()); + } + + let recent_blockhash = Hash::new_unique(); + let keypair = Keypair::new(); + let pubkey = keypair.pubkey(); + let ix_count = 65; + let ixs: Vec<_> = std::iter::repeat_with(|| CompiledInstruction { + program_id_index: 1, + accounts: vec![0], + data: vec![], + }) + .take(ix_count) + .collect(); + let message = Message::new_with_compiled_instructions( + 1, + 0, + 1, + vec![pubkey, Pubkey::new_unique()], + recent_blockhash, + ixs, + ); + let tx = Transaction::new(&[&keypair], message, recent_blockhash); + + assert!(bincode::serialized_size(&tx).unwrap() <= PACKET_DATA_SIZE as u64); + + if simd_0160_enabled { + assert_matches!( + bank.verify_transaction(tx.into(), TransactionVerificationMode::FullVerification), + Err(TransactionError::SanitizeFailure) + ); + } else { + assert!(bank + .verify_transaction(tx.into(), TransactionVerificationMode::FullVerification) + .is_ok()); + } +} + #[test] fn test_check_reserved_keys() { let (genesis_config, _mint_keypair) = create_genesis_config(1); diff --git a/transaction-view/benches/transaction_view.rs b/transaction-view/benches/transaction_view.rs index 815cf59d407..ef19f2df734 100644 --- a/transaction-view/benches/transaction_view.rs +++ b/transaction-view/benches/transaction_view.rs @@ -64,7 +64,8 @@ fn bench_transactions_parsing( group.bench_function("TransactionView (Sanitized)", |c| { c.iter(|| { for bytes in serialized_transactions.iter() { - let _ = TransactionView::try_new_sanitized(black_box(bytes.as_ref())).unwrap(); + let _ = + TransactionView::try_new_sanitized(black_box(bytes.as_ref()), true).unwrap(); } }); }); diff --git a/transaction-view/src/resolved_transaction_view.rs b/transaction-view/src/resolved_transaction_view.rs index 584c95c9a4b..379f9cab6d5 100644 --- a/transaction-view/src/resolved_transaction_view.rs +++ b/transaction-view/src/resolved_transaction_view.rs @@ -284,7 +284,7 @@ mod tests { }), }; let bytes = bincode::serialize(&transaction).unwrap(); - let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref()).unwrap(); + let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref(), true).unwrap(); let result = ResolvedTransactionView::try_new(view, None, &HashSet::default()); assert!(matches!( result, @@ -315,7 +315,7 @@ mod tests { }), }; let bytes = bincode::serialize(&transaction).unwrap(); - let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref()).unwrap(); + let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref(), true).unwrap(); let result = ResolvedTransactionView::try_new(view, Some(loaded_addresses), &HashSet::default()); assert!(matches!( @@ -352,7 +352,7 @@ mod tests { }), }; let bytes = bincode::serialize(&transaction).unwrap(); - let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref()).unwrap(); + let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref(), true).unwrap(); let result = ResolvedTransactionView::try_new(view, Some(loaded_addresses), &HashSet::default()); assert!(matches!( @@ -402,7 +402,7 @@ mod tests { }; let transaction = create_transaction_with_keys(static_keys, &loaded_addresses); let bytes = bincode::serialize(&transaction).unwrap(); - let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref()).unwrap(); + let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref(), true).unwrap(); let resolved_view = ResolvedTransactionView::try_new( view, Some(loaded_addresses), @@ -425,7 +425,7 @@ mod tests { }; let transaction = create_transaction_with_keys(static_keys, &loaded_addresses); let bytes = bincode::serialize(&transaction).unwrap(); - let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref()).unwrap(); + let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref(), true).unwrap(); let resolved_view = ResolvedTransactionView::try_new( view, Some(loaded_addresses), @@ -448,7 +448,7 @@ mod tests { }; let transaction = create_transaction_with_keys(static_keys, &loaded_addresses); let bytes = bincode::serialize(&transaction).unwrap(); - let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref()).unwrap(); + let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref(), true).unwrap(); let resolved_view = ResolvedTransactionView::try_new( view, Some(loaded_addresses), @@ -509,7 +509,7 @@ mod tests { let static_keys = vec![key0, key1, key2]; let transaction = create_transaction_with_static_keys(static_keys, &loaded_addresses); let bytes = bincode::serialize(&transaction).unwrap(); - let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref()).unwrap(); + let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref(), true).unwrap(); let resolved_view = ResolvedTransactionView::try_new( view, Some(loaded_addresses.clone()), @@ -528,7 +528,7 @@ mod tests { let static_keys = vec![key0, key1, bpf_loader_upgradeable::ID]; let transaction = create_transaction_with_static_keys(static_keys, &loaded_addresses); let bytes = bincode::serialize(&transaction).unwrap(); - let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref()).unwrap(); + let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref(), true).unwrap(); let resolved_view = ResolvedTransactionView::try_new( view, Some(loaded_addresses.clone()), @@ -551,7 +551,7 @@ mod tests { }; let transaction = create_transaction_with_static_keys(static_keys, &loaded_addresses); let bytes = bincode::serialize(&transaction).unwrap(); - let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref()).unwrap(); + let view = SanitizedTransactionView::try_new_sanitized(bytes.as_ref(), true).unwrap(); let resolved_view = ResolvedTransactionView::try_new( view, diff --git a/transaction-view/src/sanitize.rs b/transaction-view/src/sanitize.rs index 992a24e3086..96bcd3a0b22 100644 --- a/transaction-view/src/sanitize.rs +++ b/transaction-view/src/sanitize.rs @@ -4,10 +4,13 @@ use crate::{ transaction_view::UnsanitizedTransactionView, }; -pub(crate) fn sanitize(view: &UnsanitizedTransactionView) -> Result<()> { +pub(crate) fn sanitize( + view: &UnsanitizedTransactionView, + enable_static_instruction_limit: bool, +) -> Result<()> { sanitize_signatures(view)?; sanitize_account_access(view)?; - sanitize_instructions(view)?; + sanitize_instructions(view, enable_static_instruction_limit)?; sanitize_address_table_lookups(view) } @@ -51,7 +54,15 @@ fn sanitize_account_access(view: &UnsanitizedTransactionView) -> Result<()> { +fn sanitize_instructions( + view: &UnsanitizedTransactionView, + enable_static_instruction_limit: bool, +) -> Result<()> { + // SIMD-160: transaction can not have more than 64 top level instructions + if enable_static_instruction_limit && view.num_instructions() > 64 { + return Err(TransactionViewError::SanitizeError); + } + // already verified there is at least one static account. let max_program_id_index = view.num_static_account_keys().wrapping_sub(1); // verified that there are no more than 256 accounts in `sanitize_account_access` @@ -172,7 +183,7 @@ mod tests { let transaction = multiple_transfers(); let data = bincode::serialize(&transaction).unwrap(); let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); - assert!(view.sanitize().is_ok()); + assert!(view.sanitize(true).is_ok()); } #[test] @@ -379,7 +390,7 @@ mod tests { ); let data = bincode::serialize(&transaction).unwrap(); let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); - assert!(sanitize_instructions(&view).is_ok()); + assert!(sanitize_instructions(&view, true).is_ok()); let transaction = create_v0_transaction( num_signatures, @@ -390,7 +401,7 @@ mod tests { ); let data = bincode::serialize(&transaction).unwrap(); let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); - assert!(sanitize_instructions(&view).is_ok()); + assert!(sanitize_instructions(&view, true).is_ok()); } for instruction_index in 0..valid_instructions.len() { @@ -407,7 +418,7 @@ mod tests { let data = bincode::serialize(&transaction).unwrap(); let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); assert_eq!( - sanitize_instructions(&view), + sanitize_instructions(&view, true), Err(TransactionViewError::SanitizeError) ); } @@ -426,7 +437,7 @@ mod tests { let data = bincode::serialize(&transaction).unwrap(); let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); assert_eq!( - sanitize_instructions(&view), + sanitize_instructions(&view, true), Err(TransactionViewError::SanitizeError) ); } @@ -444,7 +455,7 @@ mod tests { let data = bincode::serialize(&transaction).unwrap(); let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); assert_eq!( - sanitize_instructions(&view), + sanitize_instructions(&view, true), Err(TransactionViewError::SanitizeError) ); } @@ -464,7 +475,7 @@ mod tests { let data = bincode::serialize(&transaction).unwrap(); let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); assert_eq!( - sanitize_instructions(&view), + sanitize_instructions(&view, true), Err(TransactionViewError::SanitizeError) ); } @@ -488,11 +499,49 @@ mod tests { let data = bincode::serialize(&transaction).unwrap(); let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); assert_eq!( - sanitize_instructions(&view), + sanitize_instructions(&view, true), Err(TransactionViewError::SanitizeError) ); } } + + // SIMD-0160, too many instructions are invalid + { + let too_many_instructions: Vec<_> = valid_instructions + .iter() + .cycle() + .take(65) + .cloned() + .collect(); + let transaction = create_legacy_transaction( + num_signatures, + header, + account_keys.clone(), + too_many_instructions.clone(), + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_instructions(&view, true), + Err(TransactionViewError::SanitizeError) + ); + assert!(sanitize_instructions(&view, false).is_ok()); + + let transaction = create_v0_transaction( + num_signatures, + header, + account_keys.clone(), + too_many_instructions.clone(), + atls.clone(), + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_instructions(&view, true), + Err(TransactionViewError::SanitizeError) + ); + assert!(sanitize_instructions(&view, false).is_ok()); + } } #[test] diff --git a/transaction-view/src/transaction_view.rs b/transaction-view/src/transaction_view.rs index f15db6b95a9..47b7091ceb4 100644 --- a/transaction-view/src/transaction_view.rs +++ b/transaction-view/src/transaction_view.rs @@ -36,8 +36,11 @@ impl TransactionView { } /// Sanitizes the transaction view, returning a sanitized view on success. - pub fn sanitize(self) -> Result> { - sanitize(&self)?; + pub fn sanitize( + self, + enable_static_instruction_limit: bool, + ) -> Result> { + sanitize(&self, enable_static_instruction_limit)?; Ok(SanitizedTransactionView { data: self.data, frame: self.frame, @@ -47,9 +50,9 @@ impl TransactionView { impl TransactionView { /// Creates a new `TransactionView`, running sanitization checks. - pub fn try_new_sanitized(data: D) -> Result { + pub fn try_new_sanitized(data: D, enable_static_instruction_limit: bool) -> Result { let unsanitized_view = TransactionView::try_new_unsanitized(data)?; - unsanitized_view.sanitize() + unsanitized_view.sanitize(enable_static_instruction_limit) } }