From 9e479b2aec8062c0d554678f48f990c9994197cf Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Tue, 27 Aug 2024 10:06:26 -0500 Subject: [PATCH 01/12] Track number of lookup accounts --- .../src/address_table_lookup_meta.rs | 27 ++++++++++++++++++- transaction-view/src/transaction_meta.rs | 12 +++++++++ transaction-view/src/transaction_view.rs | 10 +++++++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/transaction-view/src/address_table_lookup_meta.rs b/transaction-view/src/address_table_lookup_meta.rs index bbce7817cda5fc..d53e9e7aa48b26 100644 --- a/transaction-view/src/address_table_lookup_meta.rs +++ b/transaction-view/src/address_table_lookup_meta.rs @@ -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 { @@ -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)?; + 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(|_| TransactionParsingError)?, + total_readonly_lookup_accounts: u16::try_from(total_readonly_lookup_accounts) + .map_err(|_| TransactionParsingError)?, }) } } @@ -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/transaction_meta.rs b/transaction-view/src/transaction_meta.rs index 39fe9d5700fc5a..9bfef5d09686da 100644 --- a/transaction-view/src/transaction_meta.rs +++ b/transaction-view/src/transaction_meta.rs @@ -46,6 +46,8 @@ 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)?, }; @@ -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..c423439ef5334c 100644 --- a/transaction-view/src/transaction_view.rs +++ b/transaction-view/src/transaction_view.rs @@ -67,6 +67,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(); From 99d147dabf502198a0f8a5b4287e4bcc205294f9 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Tue, 27 Aug 2024 09:57:49 -0500 Subject: [PATCH 02/12] sanitization checks --- transaction-view/src/lib.rs | 1 + transaction-view/src/sanitize.rs | 105 +++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 transaction-view/src/sanitize.rs diff --git a/transaction-view/src/lib.rs b/transaction-view/src/lib.rs index def04240b2aab7..6943f4ed3334f2 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; +pub mod sanitize; mod signature_meta; pub mod static_account_keys_meta; pub mod transaction_data; diff --git a/transaction-view/src/sanitize.rs b/transaction-view/src/sanitize.rs new file mode 100644 index 00000000000000..1e1c7b8a1f6788 --- /dev/null +++ b/transaction-view/src/sanitize.rs @@ -0,0 +1,105 @@ +use crate::{transaction_data::TransactionData, transaction_view::TransactionView}; + +pub struct SanitizeError; + +pub fn sanitize(view: &TransactionView) -> Result<(), SanitizeError> { + sanitize_signatures(view)?; + sanitize_account_access(view)?; + sanitize_instructions(view)?; + sanitize_address_table_lookups(view)?; + + Ok(()) +} + +fn sanitize_signatures(view: &TransactionView) -> Result<(), SanitizeError> { + // Check the required number of signatures matches the number of signatures. + if view.num_signatures() != view.num_required_signatures() { + return Err(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(SanitizeError); + } + + Ok(()) +} + +fn sanitize_account_access( + view: &TransactionView, +) -> Result<(), SanitizeError> { + // 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(SanitizeError); + } + + // Check there is at least 1 writable fee-payer account. + if view.num_readonly_signed_accounts() >= view.num_required_signatures() { + return Err(SanitizeError); + } + + // Check there are not more than 256 accounts. + if total_number_of_accounts(view) > 256 { + return Err(SanitizeError); + } + + Ok(()) +} + +fn sanitize_instructions( + view: &TransactionView, +) -> Result<(), 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` + 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(SanitizeError); + } + + // Check that the program index is not the fee-payer. + if instruction.program_id_index == 0 { + return Err(SanitizeError); + } + + // Check that all account indexes are valid. + for account_index in instruction.accounts.iter().copied() { + if account_index > max_account_index { + return Err(SanitizeError); + } + } + } + + Ok(()) +} + +fn sanitize_address_table_lookups( + view: &TransactionView, +) -> Result<(), SanitizeError> { + 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(SanitizeError); + } + } + + Ok(()) +} + +fn total_number_of_accounts(view: &TransactionView) -> u16 { + u16::from(view.num_static_account_keys()) + .saturating_add(view.total_writable_lookup_accounts()) + .saturating_add(view.total_readonly_lookup_accounts()) +} From 375e7d14d42c5b0d1d9f69269f4cf64ee1b018d8 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Tue, 27 Aug 2024 10:24:33 -0500 Subject: [PATCH 03/12] error-type split sanitize and parse --- .../src/address_table_lookup_meta.rs | 8 ++-- transaction-view/src/bytes.rs | 19 +++++---- transaction-view/src/message_header_meta.rs | 4 +- transaction-view/src/result.rs | 9 +++- transaction-view/src/sanitize.rs | 42 +++++++++---------- transaction-view/src/signature_meta.rs | 4 +- .../src/static_account_keys_meta.rs | 4 +- transaction-view/src/transaction_meta.rs | 4 +- 8 files changed, 49 insertions(+), 45 deletions(-) diff --git a/transaction-view/src/address_table_lookup_meta.rs b/transaction-view/src/address_table_lookup_meta.rs index d53e9e7aa48b26..9ebab7d78c6ada 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, @@ -70,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. @@ -120,9 +120,9 @@ impl AddressTableLookupMeta { num_address_table_lookups, offset: address_table_lookups_offset, total_writable_lookup_accounts: u16::try_from(total_writable_lookup_accounts) - .map_err(|_| TransactionParsingError)?, + .map_err(|_| TransactionViewError::SanitizeError)?, total_readonly_lookup_accounts: u16::try_from(total_readonly_lookup_accounts) - .map_err(|_| TransactionParsingError)?, + .map_err(|_| TransactionViewError::SanitizeError)?, }) } } 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/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 index 1e1c7b8a1f6788..743fd18695e4f2 100644 --- a/transaction-view/src/sanitize.rs +++ b/transaction-view/src/sanitize.rs @@ -1,8 +1,10 @@ -use crate::{transaction_data::TransactionData, transaction_view::TransactionView}; +use crate::{ + result::{Result, TransactionViewError}, + transaction_data::TransactionData, + transaction_view::TransactionView, +}; -pub struct SanitizeError; - -pub fn sanitize(view: &TransactionView) -> Result<(), SanitizeError> { +pub fn sanitize(view: &TransactionView) -> Result<()> { sanitize_signatures(view)?; sanitize_account_access(view)?; sanitize_instructions(view)?; @@ -11,24 +13,22 @@ pub fn sanitize(view: &TransactionView) -> Result<(), Sani Ok(()) } -fn sanitize_signatures(view: &TransactionView) -> Result<(), SanitizeError> { +fn sanitize_signatures(view: &TransactionView) -> Result<()> { // Check the required number of signatures matches the number of signatures. if view.num_signatures() != view.num_required_signatures() { - return Err(SanitizeError); + 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(SanitizeError); + return Err(TransactionViewError::SanitizeError); } Ok(()) } -fn sanitize_account_access( - view: &TransactionView, -) -> Result<(), SanitizeError> { +fn sanitize_account_access(view: &TransactionView) -> 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. @@ -37,25 +37,23 @@ fn sanitize_account_access( .num_static_account_keys() .wrapping_sub(view.num_required_signatures()) { - return Err(SanitizeError); + 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(SanitizeError); + return Err(TransactionViewError::SanitizeError); } // Check there are not more than 256 accounts. if total_number_of_accounts(view) > 256 { - return Err(SanitizeError); + return Err(TransactionViewError::SanitizeError); } Ok(()) } -fn sanitize_instructions( - view: &TransactionView, -) -> Result<(), SanitizeError> { +fn sanitize_instructions(view: &TransactionView) -> 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` @@ -64,18 +62,18 @@ fn sanitize_instructions( 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(SanitizeError); + return Err(TransactionViewError::SanitizeError); } // Check that the program index is not the fee-payer. if instruction.program_id_index == 0 { - return Err(SanitizeError); + 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(SanitizeError); + return Err(TransactionViewError::SanitizeError); } } } @@ -83,15 +81,13 @@ fn sanitize_instructions( Ok(()) } -fn sanitize_address_table_lookups( - view: &TransactionView, -) -> Result<(), SanitizeError> { +fn sanitize_address_table_lookups(view: &TransactionView) -> 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(SanitizeError); + return 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 9bfef5d09686da..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, }, @@ -54,7 +54,7 @@ impl TransactionMeta { // Verify that the entire transaction was parsed. if offset != bytes.len() { - return Err(TransactionParsingError); + return Err(TransactionViewError::ParseError); } Ok(Self { From d292aff9a88d5592178a6e68126dfd3f3d3d9aa0 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Tue, 27 Aug 2024 10:36:44 -0500 Subject: [PATCH 04/12] generic over sanitized --- transaction-view/benches/transaction_view.rs | 2 +- transaction-view/src/sanitize.rs | 35 +++++++++++++------- transaction-view/src/transaction_view.rs | 28 +++++++++++----- 3 files changed, 43 insertions(+), 22 deletions(-) diff --git a/transaction-view/benches/transaction_view.rs b/transaction-view/benches/transaction_view.rs index 79a4393be5207e..f4b53b8981cda4 100644 --- a/transaction-view/benches/transaction_view.rs +++ b/transaction-view/benches/transaction_view.rs @@ -45,7 +45,7 @@ fn bench_transactions_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(); } }); }); diff --git a/transaction-view/src/sanitize.rs b/transaction-view/src/sanitize.rs index 743fd18695e4f2..26c1e6a72725fa 100644 --- a/transaction-view/src/sanitize.rs +++ b/transaction-view/src/sanitize.rs @@ -4,16 +4,25 @@ use crate::{ transaction_view::TransactionView, }; -pub fn sanitize(view: &TransactionView) -> Result<()> { - sanitize_signatures(view)?; - sanitize_account_access(view)?; - sanitize_instructions(view)?; - sanitize_address_table_lookups(view)?; - - Ok(()) +// alias for convenience +type UnsanitizedTransactionView = TransactionView; +type SanitizedTransactionView = TransactionView; + +pub fn sanitize( + view: UnsanitizedTransactionView, +) -> Result> { + sanitize_signatures(&view)?; + sanitize_account_access(&view)?; + sanitize_instructions(&view)?; + sanitize_address_table_lookups(&view)?; + + Ok(SanitizedTransactionView { + data: view.data, + meta: view.meta, + }) } -fn sanitize_signatures(view: &TransactionView) -> Result<()> { +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); @@ -28,7 +37,7 @@ fn sanitize_signatures(view: &TransactionView) -> Result<( Ok(()) } -fn sanitize_account_access(view: &TransactionView) -> Result<()> { +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. @@ -53,7 +62,7 @@ fn sanitize_account_access(view: &TransactionView) -> Resu Ok(()) } -fn sanitize_instructions(view: &TransactionView) -> Result<()> { +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` @@ -81,7 +90,9 @@ fn sanitize_instructions(view: &TransactionView) -> Result Ok(()) } -fn sanitize_address_table_lookups(view: &TransactionView) -> Result<()> { +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() @@ -94,7 +105,7 @@ fn sanitize_address_table_lookups(view: &TransactionView) Ok(()) } -fn total_number_of_accounts(view: &TransactionView) -> u16 { +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()) diff --git a/transaction-view/src/transaction_view.rs b/transaction-view/src/transaction_view.rs index c423439ef5334c..c74bf7fa8e5dca 100644 --- a/transaction-view/src/transaction_view.rs +++ b/transaction-view/src/transaction_view.rs @@ -2,7 +2,8 @@ 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}, }; @@ -14,19 +15,28 @@ 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 { - data: D, - meta: TransactionMeta, +pub struct TransactionView { + pub(crate) data: D, + pub(crate) 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 }) } +} + +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)?; + sanitize(unsanitized_view) + } +} +impl TransactionView { /// Return the number of signatures in the transaction. pub fn num_signatures(&self) -> u8 { self.meta.num_signatures() @@ -140,7 +150,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); From 2eab4fd29c81625f928fe93d1b6f4409805a9d2e Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Tue, 27 Aug 2024 11:30:50 -0500 Subject: [PATCH 05/12] sanitize tests --- .../src/address_table_lookup_meta.rs | 2 +- transaction-view/src/lib.rs | 10 +- transaction-view/src/sanitize.rs | 284 ++++++++++++++++++ transaction-view/src/transaction_meta.rs | 12 +- 4 files changed, 296 insertions(+), 12 deletions(-) diff --git a/transaction-view/src/address_table_lookup_meta.rs b/transaction-view/src/address_table_lookup_meta.rs index 9ebab7d78c6ada..01f3b2cab831de 100644 --- a/transaction-view/src/address_table_lookup_meta.rs +++ b/transaction-view/src/address_table_lookup_meta.rs @@ -113,7 +113,7 @@ impl AddressTableLookupMeta { let num_read_accounts = optimized_read_compressed_u16(bytes, offset)?; total_readonly_lookup_accounts = total_readonly_lookup_accounts.wrapping_add(u32::from(num_read_accounts)); - advance_offset_for_array::(bytes, offset, num_read_accounts)? + advance_offset_for_array::(bytes, offset, num_read_accounts)?; } Ok(Self { diff --git a/transaction-view/src/lib.rs b/transaction-view/src/lib.rs index 6943f4ed3334f2..f26ca2ad1c6150 100644 --- a/transaction-view/src/lib.rs +++ b/transaction-view/src/lib.rs @@ -4,13 +4,13 @@ pub mod bytes; #[cfg(not(feature = "dev-context-only-utils"))] mod bytes; -mod address_table_lookup_meta; -mod instructions_meta; -mod message_header_meta; +pub(crate) mod address_table_lookup_meta; +pub(crate) mod instructions_meta; +pub(crate) mod message_header_meta; pub mod result; pub mod sanitize; -mod signature_meta; +pub(crate) mod signature_meta; pub mod static_account_keys_meta; pub mod transaction_data; -mod transaction_meta; +pub(crate) mod transaction_meta; pub mod transaction_view; diff --git a/transaction-view/src/sanitize.rs b/transaction-view/src/sanitize.rs index 26c1e6a72725fa..8c85cf967bc505 100644 --- a/transaction-view/src/sanitize.rs +++ b/transaction-view/src/sanitize.rs @@ -110,3 +110,287 @@ fn total_number_of_accounts(view: &UnsanitizedTransactionView TransactionMeta { + TransactionMeta { + signature: SignatureMeta { + num_signatures: 1, + offset: 1, + }, + message_header: MessageHeaderMeta { + offset: 0, + version: TransactionVersion::Legacy, + num_required_signatures: 1, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 0, + }, + static_account_keys: StaticAccountKeysMeta { + num_static_accounts: 1, + offset: 2, + }, + recent_blockhash_offset: 3, + instructions: InstructionsMeta { + num_instructions: 0, + offset: 4, + }, + address_table_lookup: AddressTableLookupMeta { + num_address_table_lookups: 0, + offset: 5, + total_writable_lookup_accounts: 0, + total_readonly_lookup_accounts: 0, + }, + } + } + + 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_dummy() { + // The dummy metadata object should pass all sanitization checks. + // Otherwise the tests which modify it are incorrect. + let view = UnsanitizedTransactionView { + data: &[][..], + meta: dummy_metadata(), + }; + assert!(sanitize(view).is_ok()); + } + + #[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!(sanitize(view).is_ok()); + } + + #[test] + fn test_sanitize_signatures() { + // Too few signatures. + { + let mut meta = dummy_metadata(); + meta.signature.num_signatures = 0; + let view = UnsanitizedTransactionView { + data: &[][..], + meta, + }; + assert_eq!( + sanitize_signatures(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Too many signatures. + { + let mut meta = dummy_metadata(); + meta.signature.num_signatures = 2; + let view = UnsanitizedTransactionView { + data: &[][..], + meta, + }; + assert_eq!( + sanitize_signatures(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Not enough static accounts. + { + let mut meta = dummy_metadata(); + meta.signature.num_signatures = 2; + meta.message_header.num_required_signatures = 2; + meta.static_account_keys.num_static_accounts = 1; + let view = UnsanitizedTransactionView { + data: &[][..], + meta, + }; + assert_eq!( + sanitize_signatures(&view), + Err(TransactionViewError::SanitizeError) + ); + } + } + + #[test] + fn test_sanitize_account_access() { + // Overlap of signing and readonly non-signing accounts. + { + let mut meta = dummy_metadata(); + meta.message_header.num_readonly_unsigned_accounts = 2; + meta.static_account_keys.num_static_accounts = 2; + let view = UnsanitizedTransactionView { + data: &[][..], + meta, + }; + assert_eq!( + sanitize_account_access(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Not enough writable accounts. + { + let mut meta = dummy_metadata(); + meta.message_header.num_readonly_signed_accounts = 1; + meta.static_account_keys.num_static_accounts = 2; + let view = UnsanitizedTransactionView { + data: &[][..], + meta, + }; + assert_eq!( + sanitize_account_access(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Too many accounts. + { + let mut meta = dummy_metadata(); + meta.static_account_keys.num_static_accounts = 1; + meta.address_table_lookup.total_writable_lookup_accounts = 200; + meta.address_table_lookup.total_readonly_lookup_accounts = 200; + let view = UnsanitizedTransactionView { + data: &[][..], + meta, + }; + assert_eq!( + sanitize_account_access(&view), + Err(TransactionViewError::SanitizeError) + ); + } + } + + #[test] + fn test_sanitize_instructions() { + let transaction = multiple_transfers(); + let data = bincode::serialize(&transaction).unwrap(); + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + + let instruction_size = + bincode::serialized_size(&transaction.message.instructions()[0]).unwrap(); + for instruction_index in 0..usize::from(view.num_instructions()) { + let instruction_offset = usize::from(view.meta.instructions.offset) + + instruction_index * instruction_size as usize; + + // Invalid program index. + { + let mut data = data.clone(); + data[instruction_offset] = 4; + 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 data = data.clone(); + data[instruction_offset] = 0; + let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + assert_eq!( + sanitize_instructions(&view), + Err(TransactionViewError::SanitizeError) + ); + } + + // Invalid account index. + { + let mut data = data.clone(); + // one byte for program index, one byte for number of accounts + data[instruction_offset + 2] = 7; + 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(); + VersionedTransaction { + signatures: vec![Signature::default()], // 1 signature to be valid. + message: VersionedMessage::V0(v0::Message { + header: MessageHeader { + num_required_signatures: 1, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 0, + }, + account_keys: vec![payer], + recent_blockhash: Hash::default(), + instructions: vec![], + address_table_lookups: vec![ + MessageAddressTableLookup { + account_key: Pubkey::new_unique(), + writable_indexes: if empty_index == 0 { vec![] } else { vec![0, 1] }, + readonly_indexes: vec![], + }, + MessageAddressTableLookup { + account_key: Pubkey::new_unique(), + writable_indexes: if empty_index == 1 { vec![] } else { vec![0, 1] }, + readonly_indexes: vec![], + }, + ], + }), + } + } + + 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/transaction_meta.rs b/transaction-view/src/transaction_meta.rs index 67a9179b894734..8e392d02b58bc8 100644 --- a/transaction-view/src/transaction_meta.rs +++ b/transaction-view/src/transaction_meta.rs @@ -13,17 +13,17 @@ use { pub(crate) struct TransactionMeta { /// Signature metadata. - signature: SignatureMeta, + pub(crate) signature: SignatureMeta, /// Message header metadata. - message_header: MessageHeaderMeta, + pub(crate) message_header: MessageHeaderMeta, /// Static account keys metadata. - static_account_keys: StaticAccountKeysMeta, + pub(crate) static_account_keys: StaticAccountKeysMeta, /// Recent blockhash offset. - recent_blockhash_offset: u16, + pub(crate) recent_blockhash_offset: u16, /// Instructions metadata. - instructions: InstructionsMeta, + pub(crate) instructions: InstructionsMeta, /// Address table lookup metadata. - address_table_lookup: AddressTableLookupMeta, + pub(crate) address_table_lookup: AddressTableLookupMeta, } impl TransactionMeta { From e8937cdb4bb4e65db14bd00c6405e185e68a5b43 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Tue, 27 Aug 2024 11:40:07 -0500 Subject: [PATCH 06/12] add bench for parse and sanitize --- transaction-view/benches/transaction_view.rs | 21 +++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/transaction-view/benches/transaction_view.rs b/transaction-view/benches/transaction_view.rs index f4b53b8981cda4..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,6 +41,16 @@ 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(|| { @@ -49,6 +59,15 @@ fn bench_transactions_parsing( } }); }); + + // 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(); + } + }); + }); } fn minimum_sized_transactions() -> Vec { From 2eebb4d73173e902fbded89611f786088fa5d8a3 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Wed, 28 Aug 2024 08:22:30 -0500 Subject: [PATCH 07/12] sanitize as member function of UnsanitizedTransactionView --- transaction-view/src/sanitize.rs | 35 +++++++++++------------- transaction-view/src/transaction_view.rs | 9 ++++-- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/transaction-view/src/sanitize.rs b/transaction-view/src/sanitize.rs index 8c85cf967bc505..036b91fcd89e22 100644 --- a/transaction-view/src/sanitize.rs +++ b/transaction-view/src/sanitize.rs @@ -1,25 +1,21 @@ use crate::{ result::{Result, TransactionViewError}, transaction_data::TransactionData, - transaction_view::TransactionView, + transaction_view::{SanitizedTransactionView, UnsanitizedTransactionView}, }; -// alias for convenience -type UnsanitizedTransactionView = TransactionView; -type SanitizedTransactionView = TransactionView; - -pub fn sanitize( - view: UnsanitizedTransactionView, -) -> Result> { - sanitize_signatures(&view)?; - sanitize_account_access(&view)?; - sanitize_instructions(&view)?; - sanitize_address_table_lookups(&view)?; - - Ok(SanitizedTransactionView { - data: view.data, - meta: view.meta, - }) +impl UnsanitizedTransactionView { + pub fn sanitize(self) -> Result> { + sanitize_signatures(&self)?; + sanitize_account_access(&self)?; + sanitize_instructions(&self)?; + sanitize_address_table_lookups(&self)?; + + Ok(SanitizedTransactionView { + data: self.data, + meta: self.meta, + }) + } } fn sanitize_signatures(view: &UnsanitizedTransactionView) -> Result<()> { @@ -122,6 +118,7 @@ mod tests { signature_meta::SignatureMeta, static_account_keys_meta::StaticAccountKeysMeta, transaction_meta::TransactionMeta, + transaction_view::TransactionView, }, solana_sdk::{ hash::Hash, @@ -193,7 +190,7 @@ mod tests { data: &[][..], meta: dummy_metadata(), }; - assert!(sanitize(view).is_ok()); + assert!(view.sanitize().is_ok()); } #[test] @@ -201,7 +198,7 @@ mod tests { let transaction = multiple_transfers(); let data = bincode::serialize(&transaction).unwrap(); let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); - assert!(sanitize(view).is_ok()); + assert!(view.sanitize().is_ok()); } #[test] diff --git a/transaction-view/src/transaction_view.rs b/transaction-view/src/transaction_view.rs index c74bf7fa8e5dca..f2448b21e1c837 100644 --- a/transaction-view/src/transaction_view.rs +++ b/transaction-view/src/transaction_view.rs @@ -2,12 +2,15 @@ use { crate::{ address_table_lookup_meta::AddressTableLookupIterator, instructions_meta::InstructionsIterator, message_header_meta::TransactionVersion, - result::Result, sanitize::sanitize, transaction_data::TransactionData, - transaction_meta::TransactionMeta, + result::Result, 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 @@ -32,7 +35,7 @@ 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)?; - sanitize(unsanitized_view) + unsanitized_view.sanitize() } } From c9c47a5d03c2df0f5d2f772c32ea608bc744fda3 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Wed, 28 Aug 2024 10:04:17 -0500 Subject: [PATCH 08/12] no more dummy meta --- transaction-view/src/sanitize.rs | 283 ++++++++++++++++++------------- 1 file changed, 165 insertions(+), 118 deletions(-) diff --git a/transaction-view/src/sanitize.rs b/transaction-view/src/sanitize.rs index 036b91fcd89e22..6f74a396452125 100644 --- a/transaction-view/src/sanitize.rs +++ b/transaction-view/src/sanitize.rs @@ -111,17 +111,10 @@ fn total_number_of_accounts(view: &UnsanitizedTransactionView TransactionMeta { - TransactionMeta { - signature: SignatureMeta { - num_signatures: 1, - offset: 1, - }, - message_header: MessageHeaderMeta { - offset: 0, - version: TransactionVersion::Legacy, - num_required_signatures: 1, - num_readonly_signed_accounts: 0, - num_readonly_unsigned_accounts: 0, - }, - static_account_keys: StaticAccountKeysMeta { - num_static_accounts: 1, - offset: 2, - }, - recent_blockhash_offset: 3, - instructions: InstructionsMeta { - num_instructions: 0, - offset: 4, - }, - address_table_lookup: AddressTableLookupMeta { - num_address_table_lookups: 0, - offset: 5, - total_writable_lookup_accounts: 0, - total_readonly_lookup_accounts: 0, - }, + 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, + }), } } @@ -182,17 +176,6 @@ mod tests { } } - #[test] - fn test_sanitize_dummy() { - // The dummy metadata object should pass all sanitization checks. - // Otherwise the tests which modify it are incorrect. - let view = UnsanitizedTransactionView { - data: &[][..], - meta: dummy_metadata(), - }; - assert!(view.sanitize().is_ok()); - } - #[test] fn test_sanitize_multiple_transfers() { let transaction = multiple_transfers(); @@ -205,12 +188,18 @@ mod tests { fn test_sanitize_signatures() { // Too few signatures. { - let mut meta = dummy_metadata(); - meta.signature.num_signatures = 0; - let view = UnsanitizedTransactionView { - data: &[][..], - meta, - }; + 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) @@ -219,12 +208,18 @@ mod tests { // Too many signatures. { - let mut meta = dummy_metadata(); - meta.signature.num_signatures = 2; - let view = UnsanitizedTransactionView { - data: &[][..], - meta, - }; + 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) @@ -233,14 +228,43 @@ mod tests { // Not enough static accounts. { - let mut meta = dummy_metadata(); - meta.signature.num_signatures = 2; - meta.message_header.num_required_signatures = 2; - meta.static_account_keys.num_static_accounts = 1; - let view = UnsanitizedTransactionView { - data: &[][..], - meta, - }; + 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) @@ -252,13 +276,18 @@ mod tests { fn test_sanitize_account_access() { // Overlap of signing and readonly non-signing accounts. { - let mut meta = dummy_metadata(); - meta.message_header.num_readonly_unsigned_accounts = 2; - meta.static_account_keys.num_static_accounts = 2; - let view = UnsanitizedTransactionView { - data: &[][..], - meta, - }; + 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) @@ -267,13 +296,18 @@ mod tests { // Not enough writable accounts. { - let mut meta = dummy_metadata(); - meta.message_header.num_readonly_signed_accounts = 1; - meta.static_account_keys.num_static_accounts = 2; - let view = UnsanitizedTransactionView { - data: &[][..], - meta, - }; + 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) @@ -282,14 +316,30 @@ mod tests { // Too many accounts. { - let mut meta = dummy_metadata(); - meta.static_account_keys.num_static_accounts = 1; - meta.address_table_lookup.total_writable_lookup_accounts = 200; - meta.address_table_lookup.total_readonly_lookup_accounts = 200; - let view = UnsanitizedTransactionView { - data: &[][..], - meta, - }; + 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) @@ -349,31 +399,28 @@ mod tests { fn test_sanitize_address_table_lookups() { fn create_transaction(empty_index: usize) -> VersionedTransaction { let payer = Pubkey::new_unique(); - VersionedTransaction { - signatures: vec![Signature::default()], // 1 signature to be valid. - message: VersionedMessage::V0(v0::Message { - header: MessageHeader { - num_required_signatures: 1, - num_readonly_signed_accounts: 0, - num_readonly_unsigned_accounts: 0, + create_v0_transaction( + 1, + MessageHeader { + num_required_signatures: 1, + num_readonly_signed_accounts: 0, + num_readonly_unsigned_accounts: 0, + }, + vec![payer], + vec![], + vec![ + MessageAddressTableLookup { + account_key: Pubkey::new_unique(), + writable_indexes: if empty_index == 0 { vec![] } else { vec![0, 1] }, + readonly_indexes: vec![], }, - account_keys: vec![payer], - recent_blockhash: Hash::default(), - instructions: vec![], - address_table_lookups: vec![ - MessageAddressTableLookup { - account_key: Pubkey::new_unique(), - writable_indexes: if empty_index == 0 { vec![] } else { vec![0, 1] }, - readonly_indexes: vec![], - }, - MessageAddressTableLookup { - account_key: Pubkey::new_unique(), - writable_indexes: if empty_index == 1 { vec![] } else { vec![0, 1] }, - readonly_indexes: vec![], - }, - ], - }), - } + MessageAddressTableLookup { + account_key: Pubkey::new_unique(), + writable_indexes: if empty_index == 1 { vec![] } else { vec![0, 1] }, + readonly_indexes: vec![], + }, + ], + ) } for empty_index in 0..2 { From 11630b45273be5cbec545b03f2a79cb0a300f27e Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Wed, 28 Aug 2024 10:22:30 -0500 Subject: [PATCH 09/12] rework tests --- transaction-view/src/sanitize.rs | 165 +++++++++++++++++------ transaction-view/src/transaction_view.rs | 16 ++- 2 files changed, 138 insertions(+), 43 deletions(-) diff --git a/transaction-view/src/sanitize.rs b/transaction-view/src/sanitize.rs index 6f74a396452125..e981d3247e162a 100644 --- a/transaction-view/src/sanitize.rs +++ b/transaction-view/src/sanitize.rs @@ -1,21 +1,14 @@ use crate::{ result::{Result, TransactionViewError}, transaction_data::TransactionData, - transaction_view::{SanitizedTransactionView, UnsanitizedTransactionView}, + transaction_view::UnsanitizedTransactionView, }; -impl UnsanitizedTransactionView { - pub fn sanitize(self) -> Result> { - sanitize_signatures(&self)?; - sanitize_account_access(&self)?; - sanitize_instructions(&self)?; - sanitize_address_table_lookups(&self)?; - - Ok(SanitizedTransactionView { - data: self.data, - meta: self.meta, - }) - } +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<()> { @@ -349,20 +342,71 @@ mod tests { #[test] fn test_sanitize_instructions() { - let transaction = multiple_transfers(); - let data = bincode::serialize(&transaction).unwrap(); - let view = TransactionView::try_new_unsanitized(data.as_ref()).unwrap(); + 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 instruction_size = - bincode::serialized_size(&transaction.message.instructions()[0]).unwrap(); - for instruction_index in 0..usize::from(view.num_instructions()) { - let instruction_offset = usize::from(view.meta.instructions.offset) - + instruction_index * instruction_size as usize; + 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 data = data.clone(); - data[instruction_offset] = 4; + 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), @@ -372,8 +416,15 @@ mod tests { // Program index is fee-payer. { - let mut data = data.clone(); - data[instruction_offset] = 0; + 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), @@ -383,9 +434,41 @@ mod tests { // Invalid account index. { - let mut data = data.clone(); - // one byte for program index, one byte for number of accounts - data[instruction_offset + 2] = 7; + 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), @@ -399,6 +482,19 @@ mod tests { 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 { @@ -408,18 +504,7 @@ mod tests { }, vec![payer], vec![], - vec![ - MessageAddressTableLookup { - account_key: Pubkey::new_unique(), - writable_indexes: if empty_index == 0 { vec![] } else { vec![0, 1] }, - readonly_indexes: vec![], - }, - MessageAddressTableLookup { - account_key: Pubkey::new_unique(), - writable_indexes: if empty_index == 1 { vec![] } else { vec![0, 1] }, - readonly_indexes: vec![], - }, - ], + address_table_lookups, ) } diff --git a/transaction-view/src/transaction_view.rs b/transaction-view/src/transaction_view.rs index f2448b21e1c837..e1b4a98d37f56b 100644 --- a/transaction-view/src/transaction_view.rs +++ b/transaction-view/src/transaction_view.rs @@ -2,7 +2,8 @@ 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}, }; @@ -19,8 +20,8 @@ pub type SanitizedTransactionView = TransactionView; /// The owned `data` is abstracted through the `TransactionData` trait, /// so that different containers for the serialized transaction can be used. pub struct TransactionView { - pub(crate) data: D, - pub(crate) meta: TransactionMeta, + data: D, + meta: TransactionMeta, } impl TransactionView { @@ -29,6 +30,15 @@ impl TransactionView { 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 { From 1fb746c5a4e0fc3509194549f86b0b85094cc924 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Wed, 28 Aug 2024 10:25:44 -0500 Subject: [PATCH 10/12] modules private where they can be --- transaction-view/src/lib.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/transaction-view/src/lib.rs b/transaction-view/src/lib.rs index f26ca2ad1c6150..4058c88fa83034 100644 --- a/transaction-view/src/lib.rs +++ b/transaction-view/src/lib.rs @@ -4,13 +4,13 @@ pub mod bytes; #[cfg(not(feature = "dev-context-only-utils"))] mod bytes; -pub(crate) mod address_table_lookup_meta; -pub(crate) mod instructions_meta; -pub(crate) mod message_header_meta; +mod address_table_lookup_meta; +mod instructions_meta; +mod message_header_meta; pub mod result; -pub mod sanitize; -pub(crate) mod signature_meta; +mod sanitize; +mod signature_meta; pub mod static_account_keys_meta; pub mod transaction_data; -pub(crate) mod transaction_meta; +mod transaction_meta; pub mod transaction_view; From 6d02337340cb10fd65e1225f382fad97ab218c52 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Thu, 29 Aug 2024 08:05:00 -0500 Subject: [PATCH 11/12] privatize TransactionMeta fields --- transaction-view/src/transaction_meta.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/transaction-view/src/transaction_meta.rs b/transaction-view/src/transaction_meta.rs index 8e392d02b58bc8..67a9179b894734 100644 --- a/transaction-view/src/transaction_meta.rs +++ b/transaction-view/src/transaction_meta.rs @@ -13,17 +13,17 @@ use { pub(crate) struct TransactionMeta { /// Signature metadata. - pub(crate) signature: SignatureMeta, + signature: SignatureMeta, /// Message header metadata. - pub(crate) message_header: MessageHeaderMeta, + message_header: MessageHeaderMeta, /// Static account keys metadata. - pub(crate) static_account_keys: StaticAccountKeysMeta, + static_account_keys: StaticAccountKeysMeta, /// Recent blockhash offset. - pub(crate) recent_blockhash_offset: u16, + recent_blockhash_offset: u16, /// Instructions metadata. - pub(crate) instructions: InstructionsMeta, + instructions: InstructionsMeta, /// Address table lookup metadata. - pub(crate) address_table_lookup: AddressTableLookupMeta, + address_table_lookup: AddressTableLookupMeta, } impl TransactionMeta { From a1600ca7656bcccb90dcba014738e05b281191c5 Mon Sep 17 00:00:00 2001 From: Andrew Fitzgerald Date: Thu, 29 Aug 2024 08:26:32 -0500 Subject: [PATCH 12/12] Add test for invalid program index with lookups --- transaction-view/src/sanitize.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/transaction-view/src/sanitize.rs b/transaction-view/src/sanitize.rs index e981d3247e162a..b1aff7bb70cdd7 100644 --- a/transaction-view/src/sanitize.rs +++ b/transaction-view/src/sanitize.rs @@ -414,6 +414,25 @@ mod tests { ); } + // 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();