diff --git a/transaction-view/benches/transaction_view.rs b/transaction-view/benches/transaction_view.rs index 79a4393be5207e..10901023908fcd 100644 --- a/transaction-view/benches/transaction_view.rs +++ b/transaction-view/benches/transaction_view.rs @@ -15,7 +15,7 @@ use { signature::Keypair, signer::Signer, system_instruction, - transaction::VersionedTransaction, + transaction::{SanitizedVersionedTransaction, VersionedTransaction}, }, }; @@ -41,11 +41,30 @@ fn bench_transactions_parsing( }); }); + // Legacy Transaction Parsing and Sanitize checks + group.bench_function("SanitizedVersionedTransaction", |c| { + c.iter(|| { + for bytes in serialized_transactions.iter() { + let tx = bincode::deserialize::(black_box(bytes)).unwrap(); + let _ = SanitizedVersionedTransaction::try_new(tx).unwrap(); + } + }); + }); + // New Transaction Parsing group.bench_function("TransactionView", |c| { c.iter(|| { for bytes in serialized_transactions.iter() { - let _ = TransactionView::try_new(black_box(bytes.as_ref())).unwrap(); + let _ = TransactionView::try_new_unsanitized(black_box(bytes.as_ref())).unwrap(); + } + }); + }); + + // New Transaction Parsing and Sanitize checks + 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(); } }); }); diff --git a/transaction-view/src/address_table_lookup_meta.rs b/transaction-view/src/address_table_lookup_meta.rs index bbce7817cda5fc..01f3b2cab831de 100644 --- a/transaction-view/src/address_table_lookup_meta.rs +++ b/transaction-view/src/address_table_lookup_meta.rs @@ -4,7 +4,7 @@ use { advance_offset_for_array, advance_offset_for_type, check_remaining, optimized_read_compressed_u16, read_byte, read_slice_data, read_type, }, - result::{Result, TransactionParsingError}, + result::{Result, TransactionViewError}, }, solana_sdk::{hash::Hash, packet::PACKET_DATA_SIZE, pubkey::Pubkey, signature::Signature}, solana_svm_transaction::message_address_table_lookup::SVMMessageAddressTableLookup, @@ -51,6 +51,10 @@ pub(crate) struct AddressTableLookupMeta { pub(crate) num_address_table_lookups: u8, /// The offset to the first address table lookup in the transaction. pub(crate) offset: u16, + /// The total number of writable lookup accounts in the transaction. + pub(crate) total_writable_lookup_accounts: u16, + /// The total number of readonly lookup accounts in the transaction. + pub(crate) total_readonly_lookup_accounts: u16, } impl AddressTableLookupMeta { @@ -66,7 +70,7 @@ impl AddressTableLookupMeta { const _: () = assert!(MAX_ATLS_PER_PACKET & 0b1000_0000 == 0); let num_address_table_lookups = read_byte(bytes, offset)?; if num_address_table_lookups > MAX_ATLS_PER_PACKET { - return Err(TransactionParsingError); + return Err(TransactionViewError::ParseError); } // Check that the remaining bytes are enough to hold the ATLs. @@ -80,6 +84,13 @@ impl AddressTableLookupMeta { // length is less than u16::MAX, so we can safely cast to u16. let address_table_lookups_offset = *offset as u16; + // Check that there is no chance of overflow when calculating the total + // number of writable and readonly lookup accounts using a u32. + const _: () = + assert!(u16::MAX as usize * MAX_ATLS_PER_PACKET as usize <= u32::MAX as usize); + let mut total_writable_lookup_accounts: u32 = 0; + let mut total_readonly_lookup_accounts: u32 = 0; + // The ATLs do not have a fixed size. So we must iterate over // each ATL to find the total size of the ATLs in the packet, // and check for any malformed ATLs or buffer overflows. @@ -94,16 +105,24 @@ impl AddressTableLookupMeta { // Read the number of write indexes, and then update the offset. let num_write_accounts = optimized_read_compressed_u16(bytes, offset)?; + total_writable_lookup_accounts = + total_writable_lookup_accounts.wrapping_add(u32::from(num_write_accounts)); advance_offset_for_array::(bytes, offset, num_write_accounts)?; // Read the number of read indexes, and then update the offset. let num_read_accounts = optimized_read_compressed_u16(bytes, offset)?; - advance_offset_for_array::(bytes, offset, num_read_accounts)? + total_readonly_lookup_accounts = + total_readonly_lookup_accounts.wrapping_add(u32::from(num_read_accounts)); + advance_offset_for_array::(bytes, offset, num_read_accounts)?; } Ok(Self { num_address_table_lookups, offset: address_table_lookups_offset, + total_writable_lookup_accounts: u16::try_from(total_writable_lookup_accounts) + .map_err(|_| TransactionViewError::SanitizeError)?, + total_readonly_lookup_accounts: u16::try_from(total_readonly_lookup_accounts) + .map_err(|_| TransactionViewError::SanitizeError)?, }) } } @@ -195,6 +214,8 @@ mod tests { assert_eq!(meta.num_address_table_lookups, 0); assert_eq!(meta.offset, 1); assert_eq!(offset, bytes.len()); + assert_eq!(meta.total_writable_lookup_accounts, 0); + assert_eq!(meta.total_readonly_lookup_accounts, 0); } #[test] @@ -221,6 +242,8 @@ mod tests { assert_eq!(meta.num_address_table_lookups, 1); assert_eq!(meta.offset, 1); assert_eq!(offset, bytes.len()); + assert_eq!(meta.total_writable_lookup_accounts, 3); + assert_eq!(meta.total_readonly_lookup_accounts, 3); } #[test] @@ -234,7 +257,7 @@ mod tests { MessageAddressTableLookup { account_key: Pubkey::new_unique(), writable_indexes: vec![1, 2, 3], - readonly_indexes: vec![4, 5, 6], + readonly_indexes: vec![4, 5], }, ])) .unwrap(); @@ -243,6 +266,8 @@ mod tests { assert_eq!(meta.num_address_table_lookups, 2); assert_eq!(meta.offset, 1); assert_eq!(offset, bytes.len()); + assert_eq!(meta.total_writable_lookup_accounts, 6); + assert_eq!(meta.total_readonly_lookup_accounts, 5); } #[test] diff --git a/transaction-view/src/bytes.rs b/transaction-view/src/bytes.rs index 9e2724e3cac6de..6a147b69c2038e 100644 --- a/transaction-view/src/bytes.rs +++ b/transaction-view/src/bytes.rs @@ -1,4 +1,4 @@ -use crate::result::{Result, TransactionParsingError}; +use crate::result::{Result, TransactionViewError}; /// Check that the buffer has at least `len` bytes remaining starting at /// `offset`. Returns Err if the buffer is too short. @@ -12,7 +12,7 @@ use crate::result::{Result, TransactionParsingError}; #[inline(always)] pub fn check_remaining(bytes: &[u8], offset: usize, num_bytes: usize) -> Result<()> { if num_bytes > bytes.len().wrapping_sub(offset) { - Err(TransactionParsingError) + Err(TransactionViewError::ParseError) } else { Ok(()) } @@ -24,7 +24,10 @@ pub fn check_remaining(bytes: &[u8], offset: usize, num_bytes: usize) -> Result< pub fn read_byte(bytes: &[u8], offset: &mut usize) -> Result { // Implicitly checks that the offset is within bounds, no need // to call `check_remaining` explicitly here. - let value = bytes.get(*offset).copied().ok_or(TransactionParsingError); + let value = bytes + .get(*offset) + .copied() + .ok_or(TransactionViewError::ParseError); *offset = offset.wrapping_add(1); value } @@ -49,10 +52,10 @@ pub fn read_compressed_u16(bytes: &[u8], offset: &mut usize) -> Result { // to call check_remaining explicitly here. let byte = *bytes .get(offset.wrapping_add(i)) - .ok_or(TransactionParsingError)?; + .ok_or(TransactionViewError::ParseError)?; // non-minimal encoding or overflow if (i > 0 && byte == 0) || (i == 2 && byte > 3) { - return Err(TransactionParsingError); + return Err(TransactionViewError::ParseError); } result |= ((byte & 0x7F) as u16) << shift; shift = shift.wrapping_add(7); @@ -86,7 +89,7 @@ pub fn optimized_read_compressed_u16(bytes: &[u8], offset: &mut usize) -> Result let mut result = 0u16; // First byte - let byte1 = *bytes.get(*offset).ok_or(TransactionParsingError)?; + let byte1 = *bytes.get(*offset).ok_or(TransactionViewError::ParseError)?; result |= (byte1 & 0x7F) as u16; if byte1 & 0x80 == 0 { *offset = offset.wrapping_add(1); @@ -96,9 +99,9 @@ pub fn optimized_read_compressed_u16(bytes: &[u8], offset: &mut usize) -> Result // Second byte let byte2 = *bytes .get(offset.wrapping_add(1)) - .ok_or(TransactionParsingError)?; + .ok_or(TransactionViewError::ParseError)?; if byte2 == 0 || byte2 & 0x80 != 0 { - return Err(TransactionParsingError); // non-minimal encoding or overflow + return Err(TransactionViewError::ParseError); // non-minimal encoding or overflow } result |= ((byte2 & 0x7F) as u16) << 7; *offset = offset.wrapping_add(2); diff --git a/transaction-view/src/lib.rs b/transaction-view/src/lib.rs index def04240b2aab7..4058c88fa83034 100644 --- a/transaction-view/src/lib.rs +++ b/transaction-view/src/lib.rs @@ -8,6 +8,7 @@ mod address_table_lookup_meta; mod instructions_meta; mod message_header_meta; pub mod result; +mod sanitize; mod signature_meta; pub mod static_account_keys_meta; pub mod transaction_data; diff --git a/transaction-view/src/message_header_meta.rs b/transaction-view/src/message_header_meta.rs index dfc04766958a28..b9f40e3cb7ef11 100644 --- a/transaction-view/src/message_header_meta.rs +++ b/transaction-view/src/message_header_meta.rs @@ -1,7 +1,7 @@ use { crate::{ bytes::read_byte, - result::{Result, TransactionParsingError}, + result::{Result, TransactionViewError}, }, solana_sdk::message::MESSAGE_VERSION_PREFIX, }; @@ -49,7 +49,7 @@ impl MessageHeaderMeta { let version = message_prefix & !MESSAGE_VERSION_PREFIX; match version { 0 => (TransactionVersion::V0, read_byte(bytes, offset)?), - _ => return Err(TransactionParsingError), + _ => return Err(TransactionViewError::ParseError), } } else { // Legacy transaction. The `message_prefix` that was just read is diff --git a/transaction-view/src/result.rs b/transaction-view/src/result.rs index 1997a784b73650..b94c6b26e63a58 100644 --- a/transaction-view/src/result.rs +++ b/transaction-view/src/result.rs @@ -1,3 +1,8 @@ #[derive(Debug, PartialEq, Eq)] -pub struct TransactionParsingError; -pub type Result = core::result::Result; // no distinction between errors for now +#[repr(u8)] // repr(u8) is used to ensure that the enum is represented as a single byte in memory. +pub enum TransactionViewError { + ParseError, + SanitizeError, +} + +pub type Result = core::result::Result; diff --git a/transaction-view/src/sanitize.rs b/transaction-view/src/sanitize.rs new file mode 100644 index 00000000000000..b1aff7bb70cdd7 --- /dev/null +++ b/transaction-view/src/sanitize.rs @@ -0,0 +1,544 @@ +use crate::{ + result::{Result, TransactionViewError}, + transaction_data::TransactionData, + transaction_view::UnsanitizedTransactionView, +}; + +pub(crate) fn sanitize(view: &UnsanitizedTransactionView) -> Result<()> { + sanitize_signatures(view)?; + sanitize_account_access(view)?; + sanitize_instructions(view)?; + sanitize_address_table_lookups(view) +} + +fn sanitize_signatures(view: &UnsanitizedTransactionView) -> Result<()> { + // Check the required number of signatures matches the number of signatures. + if view.num_signatures() != view.num_required_signatures() { + return Err(TransactionViewError::SanitizeError); + } + + // Each signature is associated with a unique static public key. + // Check that there are at least as many static account keys as signatures. + if view.num_static_account_keys() < view.num_signatures() { + return Err(TransactionViewError::SanitizeError); + } + + Ok(()) +} + +fn sanitize_account_access(view: &UnsanitizedTransactionView) -> Result<()> { + // Check there is no overlap of signing area and readonly non-signing area. + // We have already checked that `num_required_signatures` is less than or equal to `num_static_account_keys`, + // so it is safe to use wrapping arithmetic. + if view.num_readonly_unsigned_accounts() + > view + .num_static_account_keys() + .wrapping_sub(view.num_required_signatures()) + { + return Err(TransactionViewError::SanitizeError); + } + + // Check there is at least 1 writable fee-payer account. + if view.num_readonly_signed_accounts() >= view.num_required_signatures() { + return Err(TransactionViewError::SanitizeError); + } + + // Check there are not more than 256 accounts. + if total_number_of_accounts(view) > 256 { + return Err(TransactionViewError::SanitizeError); + } + + Ok(()) +} + +fn sanitize_instructions(view: &UnsanitizedTransactionView) -> Result<()> { + // 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` + let max_account_index = total_number_of_accounts(view).wrapping_sub(1) as u8; + + for instruction in view.instructions_iter() { + // Check that program indexes are static account keys. + if instruction.program_id_index > max_program_id_index { + return Err(TransactionViewError::SanitizeError); + } + + // Check that the program index is not the fee-payer. + if instruction.program_id_index == 0 { + return Err(TransactionViewError::SanitizeError); + } + + // Check that all account indexes are valid. + for account_index in instruction.accounts.iter().copied() { + if account_index > max_account_index { + return Err(TransactionViewError::SanitizeError); + } + } + } + + Ok(()) +} + +fn sanitize_address_table_lookups( + view: &UnsanitizedTransactionView, +) -> Result<()> { + for address_table_lookup in view.address_table_lookup_iter() { + // Check that there is at least one account lookup. + if address_table_lookup.writable_indexes.is_empty() + && address_table_lookup.readonly_indexes.is_empty() + { + return Err(TransactionViewError::SanitizeError); + } + } + + Ok(()) +} + +fn total_number_of_accounts(view: &UnsanitizedTransactionView) -> u16 { + u16::from(view.num_static_account_keys()) + .saturating_add(view.total_writable_lookup_accounts()) + .saturating_add(view.total_readonly_lookup_accounts()) +} + +#[cfg(test)] +mod tests { + use { + super::*, + crate::transaction_view::TransactionView, + solana_sdk::{ + hash::Hash, + instruction::CompiledInstruction, + message::{ + v0::{self, MessageAddressTableLookup}, + Message, MessageHeader, VersionedMessage, + }, + pubkey::Pubkey, + signature::Signature, + system_instruction, + transaction::VersionedTransaction, + }, + }; + + fn create_legacy_transaction( + num_signatures: u8, + header: MessageHeader, + account_keys: Vec, + instructions: Vec, + ) -> VersionedTransaction { + VersionedTransaction { + signatures: vec![Signature::default(); num_signatures as usize], + message: VersionedMessage::Legacy(Message { + header, + account_keys, + recent_blockhash: Hash::default(), + instructions, + }), + } + } + + fn create_v0_transaction( + num_signatures: u8, + header: MessageHeader, + account_keys: Vec, + instructions: Vec, + address_table_lookups: Vec, + ) -> VersionedTransaction { + VersionedTransaction { + signatures: vec![Signature::default(); num_signatures as usize], + message: VersionedMessage::V0(v0::Message { + header, + account_keys, + recent_blockhash: Hash::default(), + instructions, + address_table_lookups, + }), + } + } + + fn multiple_transfers() -> VersionedTransaction { + let payer = Pubkey::new_unique(); + VersionedTransaction { + signatures: vec![Signature::default()], // 1 signature to be valid. + message: VersionedMessage::Legacy(Message::new( + &[ + system_instruction::transfer(&payer, &Pubkey::new_unique(), 1), + system_instruction::transfer(&payer, &Pubkey::new_unique(), 1), + ], + Some(&payer), + )), + } + } + + #[test] + fn test_sanitize_multiple_transfers() { + 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()); + } + + #[test] + fn test_sanitize_signatures() { + // Too few signatures. + { + let transaction = create_legacy_transaction( + 1, + MessageHeader { + num_required_signatures: 2, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 0, + }, + (0..3).map(|_| Pubkey::new_unique()).collect(), + vec![], + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_signatures(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Too many signatures. + { + let transaction = create_legacy_transaction( + 2, + MessageHeader { + num_required_signatures: 1, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 0, + }, + (0..3).map(|_| Pubkey::new_unique()).collect(), + vec![], + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_signatures(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Not enough static accounts. + { + let transaction = create_legacy_transaction( + 2, + MessageHeader { + num_required_signatures: 2, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 0, + }, + (0..1).map(|_| Pubkey::new_unique()).collect(), + vec![], + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_signatures(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Not enough static accounts - with look up accounts + { + let transaction = create_v0_transaction( + 2, + MessageHeader { + num_required_signatures: 2, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 0, + }, + (0..1).map(|_| Pubkey::new_unique()).collect(), + vec![], + vec![MessageAddressTableLookup { + account_key: Pubkey::new_unique(), + writable_indexes: vec![0, 1, 2, 3, 4, 5], + readonly_indexes: vec![6, 7, 8], + }], + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_signatures(&view), + Err(TransactionViewError::SanitizeError) + ); + } + } + + #[test] + fn test_sanitize_account_access() { + // Overlap of signing and readonly non-signing accounts. + { + let transaction = create_legacy_transaction( + 1, + MessageHeader { + num_required_signatures: 1, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 2, + }, + (0..2).map(|_| Pubkey::new_unique()).collect(), + vec![], + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_account_access(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Not enough writable accounts. + { + let transaction = create_legacy_transaction( + 1, + MessageHeader { + num_required_signatures: 1, + num_readonly_signed_accounts: 1, + num_readonly_unsigned_accounts: 0, + }, + (0..2).map(|_| Pubkey::new_unique()).collect(), + vec![], + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_account_access(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Too many accounts. + { + let transaction = create_v0_transaction( + 2, + MessageHeader { + num_required_signatures: 2, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 0, + }, + (0..1).map(|_| Pubkey::new_unique()).collect(), + vec![], + vec![ + MessageAddressTableLookup { + account_key: Pubkey::new_unique(), + writable_indexes: (0..100).collect(), + readonly_indexes: (100..200).collect(), + }, + MessageAddressTableLookup { + account_key: Pubkey::new_unique(), + writable_indexes: (100..200).collect(), + readonly_indexes: (0..100).collect(), + }, + ], + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_account_access(&view), + Err(TransactionViewError::SanitizeError) + ); + } + } + + #[test] + fn test_sanitize_instructions() { + let num_signatures = 1; + let header = MessageHeader { + num_required_signatures: 1, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 1, + }; + let account_keys = vec![ + Pubkey::new_unique(), + Pubkey::new_unique(), + Pubkey::new_unique(), + ]; + let valid_instructions = vec![ + CompiledInstruction { + program_id_index: 1, + accounts: vec![0, 1], + data: vec![1, 2, 3], + }, + CompiledInstruction { + program_id_index: 2, + accounts: vec![1, 0], + data: vec![3, 2, 1, 4], + }, + ]; + let atls = vec![MessageAddressTableLookup { + account_key: Pubkey::new_unique(), + writable_indexes: vec![0, 1], + readonly_indexes: vec![2], + }]; + + // Verify that the unmodified transaction(s) are valid/sanitized. + { + let transaction = create_legacy_transaction( + num_signatures, + header, + account_keys.clone(), + valid_instructions.clone(), + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert!(sanitize_instructions(&view).is_ok()); + + let transaction = create_v0_transaction( + num_signatures, + header, + account_keys.clone(), + valid_instructions.clone(), + atls.clone(), + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert!(sanitize_instructions(&view).is_ok()); + } + + for instruction_index in 0..valid_instructions.len() { + // Invalid program index. + { + let mut instructions = valid_instructions.clone(); + instructions[instruction_index].program_id_index = account_keys.len() as u8; + let transaction = create_legacy_transaction( + num_signatures, + header, + account_keys.clone(), + instructions, + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_instructions(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Invalid program index with lookups. + { + let mut instructions = valid_instructions.clone(); + instructions[instruction_index].program_id_index = account_keys.len() as u8; + let transaction = create_v0_transaction( + num_signatures, + header, + account_keys.clone(), + instructions, + atls.clone(), + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_instructions(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Program index is fee-payer. + { + let mut instructions = valid_instructions.clone(); + instructions[instruction_index].program_id_index = 0; + let transaction = create_legacy_transaction( + num_signatures, + header, + account_keys.clone(), + instructions, + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_instructions(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Invalid account index. + { + let mut instructions = valid_instructions.clone(); + instructions[instruction_index] + .accounts + .push(account_keys.len() as u8); + let transaction = create_legacy_transaction( + num_signatures, + header, + account_keys.clone(), + instructions, + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_instructions(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Invalid account index with v0. + { + let num_lookup_accounts = + atls[0].writable_indexes.len() + atls[0].readonly_indexes.len(); + let total_accounts = (account_keys.len() + num_lookup_accounts) as u8; + let mut instructions = valid_instructions.clone(); + instructions[instruction_index] + .accounts + .push(total_accounts); + let transaction = create_v0_transaction( + num_signatures, + header, + account_keys.clone(), + instructions, + atls.clone(), + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_instructions(&view), + Err(TransactionViewError::SanitizeError) + ); + } + } + } + + #[test] + fn test_sanitize_address_table_lookups() { + fn create_transaction(empty_index: usize) -> VersionedTransaction { + let payer = Pubkey::new_unique(); + let mut address_table_lookups = vec![ + MessageAddressTableLookup { + account_key: Pubkey::new_unique(), + writable_indexes: vec![0, 1], + readonly_indexes: vec![], + }, + MessageAddressTableLookup { + account_key: Pubkey::new_unique(), + writable_indexes: vec![0, 1], + readonly_indexes: vec![], + }, + ]; + address_table_lookups[empty_index].writable_indexes.clear(); + create_v0_transaction( + 1, + MessageHeader { + num_required_signatures: 1, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 0, + }, + vec![payer], + vec![], + address_table_lookups, + ) + } + + for empty_index in 0..2 { + let transaction = create_transaction(empty_index); + assert_eq!( + transaction.message.address_table_lookups().unwrap().len(), + 2 + ); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_address_table_lookups(&view), + Err(TransactionViewError::SanitizeError) + ); + } + } +} diff --git a/transaction-view/src/signature_meta.rs b/transaction-view/src/signature_meta.rs index 8d98554e195a11..227649483ccfe3 100644 --- a/transaction-view/src/signature_meta.rs +++ b/transaction-view/src/signature_meta.rs @@ -1,7 +1,7 @@ use { crate::{ bytes::{advance_offset_for_array, read_byte}, - result::{Result, TransactionParsingError}, + result::{Result, TransactionViewError}, }, solana_sdk::{packet::PACKET_DATA_SIZE, pubkey::Pubkey, signature::Signature}, }; @@ -35,7 +35,7 @@ impl SignatureMeta { let num_signatures = read_byte(bytes, offset)?; if num_signatures == 0 || num_signatures > MAX_SIGNATURES_PER_PACKET { - return Err(TransactionParsingError); + return Err(TransactionViewError::ParseError); } let signature_offset = *offset as u16; diff --git a/transaction-view/src/static_account_keys_meta.rs b/transaction-view/src/static_account_keys_meta.rs index f1f3b64f42bf83..46bd6fb5babf5d 100644 --- a/transaction-view/src/static_account_keys_meta.rs +++ b/transaction-view/src/static_account_keys_meta.rs @@ -1,7 +1,7 @@ use { crate::{ bytes::{advance_offset_for_array, read_byte}, - result::{Result, TransactionParsingError}, + result::{Result, TransactionViewError}, }, solana_sdk::{packet::PACKET_DATA_SIZE, pubkey::Pubkey}, }; @@ -30,7 +30,7 @@ impl StaticAccountKeysMeta { let num_static_accounts = read_byte(bytes, offset)?; if num_static_accounts == 0 || num_static_accounts > MAX_STATIC_ACCOUNTS_PER_PACKET { - return Err(TransactionParsingError); + return Err(TransactionViewError::ParseError); } // We also know that the offset must be less than 3 here, since the diff --git a/transaction-view/src/transaction_meta.rs b/transaction-view/src/transaction_meta.rs index 39fe9d5700fc5a..67a9179b894734 100644 --- a/transaction-view/src/transaction_meta.rs +++ b/transaction-view/src/transaction_meta.rs @@ -4,7 +4,7 @@ use { bytes::advance_offset_for_type, instructions_meta::{InstructionsIterator, InstructionsMeta}, message_header_meta::{MessageHeaderMeta, TransactionVersion}, - result::{Result, TransactionParsingError}, + result::{Result, TransactionViewError}, signature_meta::SignatureMeta, static_account_keys_meta::StaticAccountKeysMeta, }, @@ -46,13 +46,15 @@ impl TransactionMeta { TransactionVersion::Legacy => AddressTableLookupMeta { num_address_table_lookups: 0, offset: 0, + total_writable_lookup_accounts: 0, + total_readonly_lookup_accounts: 0, }, TransactionVersion::V0 => AddressTableLookupMeta::try_new(bytes, &mut offset)?, }; // Verify that the entire transaction was parsed. if offset != bytes.len() { - return Err(TransactionParsingError); + return Err(TransactionViewError::ParseError); } Ok(Self { @@ -105,6 +107,16 @@ impl TransactionMeta { self.address_table_lookup.num_address_table_lookups } + /// Return the number of writable lookup accounts in the transaction. + pub(crate) fn total_writable_lookup_accounts(&self) -> u16 { + self.address_table_lookup.total_writable_lookup_accounts + } + + /// Return the number of readonly lookup accounts in the transaction. + pub(crate) fn total_readonly_lookup_accounts(&self) -> u16 { + self.address_table_lookup.total_readonly_lookup_accounts + } + /// Return the offset to the message. pub(crate) fn message_offset(&self) -> u16 { self.message_header.offset diff --git a/transaction-view/src/transaction_view.rs b/transaction-view/src/transaction_view.rs index 6ee705e6c5560e..e1b4a98d37f56b 100644 --- a/transaction-view/src/transaction_view.rs +++ b/transaction-view/src/transaction_view.rs @@ -2,11 +2,16 @@ use { crate::{ address_table_lookup_meta::AddressTableLookupIterator, instructions_meta::InstructionsIterator, message_header_meta::TransactionVersion, - result::Result, transaction_data::TransactionData, transaction_meta::TransactionMeta, + result::Result, sanitize::sanitize, transaction_data::TransactionData, + transaction_meta::TransactionMeta, }, solana_sdk::{hash::Hash, pubkey::Pubkey, signature::Signature}, }; +// alias for convenience +pub type UnsanitizedTransactionView = TransactionView; +pub type SanitizedTransactionView = TransactionView; + /// A view into a serialized transaction. /// /// This struct provides access to the transaction data without @@ -14,19 +19,37 @@ use { /// about the layout of the serialized transaction. /// The owned `data` is abstracted through the `TransactionData` trait, /// so that different containers for the serialized transaction can be used. -pub struct TransactionView { +pub struct TransactionView { data: D, meta: TransactionMeta, } -impl TransactionView { - /// Creates a new `TransactionView` from the given serialized transaction data. - /// Returns an error if the data does not meet the expected format. - pub fn try_new(data: D) -> Result { +impl TransactionView { + /// Creates a new `TransactionView` without running sanitization checks. + pub fn try_new_unsanitized(data: D) -> Result { let meta = TransactionMeta::try_new(data.data())?; Ok(Self { data, meta }) } + /// Sanitizes the transaction view, returning a sanitized view on success. + pub fn sanitize(self) -> Result> { + sanitize(&self)?; + Ok(SanitizedTransactionView { + data: self.data, + meta: self.meta, + }) + } +} + +impl TransactionView { + /// Creates a new `TransactionView`, running sanitization checks. + pub fn try_new_sanitized(data: D) -> Result { + let unsanitized_view = TransactionView::try_new_unsanitized(data)?; + unsanitized_view.sanitize() + } +} + +impl TransactionView { /// Return the number of signatures in the transaction. pub fn num_signatures(&self) -> u8 { self.meta.num_signatures() @@ -67,6 +90,16 @@ impl TransactionView { self.meta.num_address_table_lookups() } + /// Return the number of writable lookup accounts in the transaction. + pub fn total_writable_lookup_accounts(&self) -> u16 { + self.meta.total_writable_lookup_accounts() + } + + /// Return the number of readonly lookup accounts in the transaction. + pub fn total_readonly_lookup_accounts(&self) -> u16 { + self.meta.total_readonly_lookup_accounts() + } + /// Return the slice of signatures in the transaction. pub fn signatures(&self) -> &[Signature] { let data = self.data(); @@ -130,7 +163,7 @@ mod tests { fn verify_transaction_view_meta(tx: &VersionedTransaction) { let bytes = bincode::serialize(tx).unwrap(); - let view = TransactionView::try_new(bytes.as_ref()).unwrap(); + let view = TransactionView::try_new_unsanitized(bytes.as_ref()).unwrap(); assert_eq!(view.num_signatures(), tx.signatures.len() as u8);