From 15f5371528fd216ae62a31bf70657631643237b3 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Thu, 1 May 2025 10:13:04 -0700 Subject: [PATCH 01/14] FIRST COMMIT basic impl --- Cargo.toml | 2 +- svm/src/account_loader.rs | 141 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index e8adb835d3d..5352eb9b884 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -180,7 +180,7 @@ check-cfg = [ ] [workspace.lints.rust] -warnings = "deny" +# XXX HANA warnings = "deny" # Clippy lint configuration that can not be applied in clippy.toml [workspace.lints.clippy] diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index ea4f133e24c..1542431f4d3 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -35,6 +35,10 @@ use { std::num::{NonZeroU32, Saturating}, }; +const HANA_FEATURE_GATE_PLACEHOLDER: bool = false; +const TRANSACTION_ACCOUNT_BASE_SIZE: usize = 64; +const ADDRESS_LOOKUP_TABLE_BASE_SIZE: usize = 8248; + // for the load instructions pub(crate) type TransactionRent = u64; pub(crate) type TransactionProgramIndices = Vec>; @@ -477,6 +481,143 @@ struct LoadedTransactionAccounts { pub(crate) loaded_accounts_data_size: u32, } +impl LoadedTransactionAccounts { + fn increase_calculated_data_size( + &mut self, + data_size_delta: usize, + requested_loaded_accounts_data_size_limit: NonZeroU32, + error_metrics: &mut TransactionErrorMetrics, + ) -> Result<()> { + let Ok(data_size_delta) = u32::try_from(data_size_delta) else { + error_metrics.max_loaded_accounts_data_size_exceeded += 1; + return Err(TransactionError::MaxLoadedAccountsDataSizeExceeded); + }; + + self.loaded_accounts_data_size = self + .loaded_accounts_data_size + .saturating_add(data_size_delta); + + if self.loaded_accounts_data_size > requested_loaded_accounts_data_size_limit.get() { + error_metrics.max_loaded_accounts_data_size_exceeded += 1; + Err(TransactionError::MaxLoadedAccountsDataSizeExceeded) + } else { + Ok(()) + } + } +} + +// HANA OK TODO how am i managing all this +// i was thinking just write a new load_transaction_accounts() from scratch +// and branch on the feature gate once and only once +// the loader handling is drastically simplified. other things maybe a bit trickier + +fn hana_load_transaction_accounts( + account_loader: &mut AccountLoader, + message: &impl SVMMessage, + loaded_fee_payer_account: LoadedTransactionAccount, + loaded_accounts_bytes_limit: NonZeroU32, + error_metrics: &mut TransactionErrorMetrics, + rent_collector: &dyn SVMRentCollector, +) -> Result { + let account_keys = message.account_keys(); + let mut additional_loaded_accounts: AHashSet = AHashSet::new(); + + let mut loaded_transaction_accounts = LoadedTransactionAccounts { + accounts: Vec::with_capacity(account_keys.len()), + program_indices: Vec::with_capacity(message.num_instructions()), + rent: 0, + rent_debits: RentDebits::default(), + loaded_accounts_data_size: 0, + }; + + // transactions pay a base fee per address lookup table + // HANA can there be more than one? who does this return usize instead of bool. forwards compat? + loaded_transaction_accounts.increase_calculated_data_size( + message + .num_lookup_tables() + .saturating_mul(ADDRESS_LOOKUP_TABLE_BASE_SIZE), + loaded_accounts_bytes_limit, + error_metrics, + )?; + + let mut collect_loaded_account = |key, loaded_account| -> Result<()> { + let LoadedTransactionAccount { + account, + loaded_size, + rent_collected, + } = loaded_account; + + loaded_transaction_accounts.increase_calculated_data_size( + loaded_size, + loaded_accounts_bytes_limit, + error_metrics, + )?; + + loaded_transaction_accounts.rent = loaded_transaction_accounts + .rent + .saturating_add(rent_collected); + + loaded_transaction_accounts + .rent_debits + .insert(key, rent_collected, account.lamports()); + + loaded_transaction_accounts.accounts.push((*key, account)); + + // HANA TODO check for a program data account here + + Ok(()) + }; + + // Since the fee payer is always the first account, collect it first. + // We can use it directly because it was already loaded during validation. + collect_loaded_account(message.fee_payer(), loaded_fee_payer_account)?; + + // Attempt to load and collect remaining non-fee payer accounts + for (account_index, account_key) in account_keys.iter().enumerate().skip(1) { + let loaded_account = load_transaction_account( + account_loader, + message, + account_key, + account_index, + rent_collector, + ); + collect_loaded_account(account_key, loaded_account)?; + } + + for (program_id, instruction) in message.program_instructions_iter() { + if native_loader::check_id(program_id) { + loaded_transaction_accounts.program_indices.push(vec![]); + continue; + } + + let Some(program_account) = account_loader.load_account(program_id) else { + error_metrics.account_not_found += 1; + return Err(TransactionError::ProgramAccountNotFound); + }; + + if !account_loader + .feature_set + .remove_accounts_executable_flag_checks + && !program_account.executable() + { + error_metrics.invalid_program_for_execution += 1; + return Err(TransactionError::InvalidProgramForExecution); + } + + let owner_id = program_account.owner(); + if !native_loader::check_id(owner_id) && !PROGRAM_OWNERS.contains(owner_id) { + error_metrics.invalid_program_for_execution += 1; + return Err(TransactionError::InvalidProgramForExecution); + } + + loaded_transaction_accounts + .program_indices + .push(vec![instruction.program_id_index as IndexOfAccount]); + } + + Ok(loaded_transaction_accounts) +} + fn load_transaction_accounts( account_loader: &mut AccountLoader, message: &impl SVMMessage, From 8f70b552653657cefc93bb3e3624a928c51bd74a Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Thu, 1 May 2025 10:20:45 -0700 Subject: [PATCH 02/14] wire it together --- svm/src/account_loader.rs | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 1542431f4d3..4f9707f406b 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -198,6 +198,12 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> { account_key: &Pubkey, is_writable: bool, ) -> Option { + let base_account_size = if HANA_FEATURE_GATE_PLACEHOLDER { + TRANSACTION_ACCOUNT_BASE_SIZE + } else { + 0 + }; + let account = self.load_account(account_key); // Inspect prior to collecting rent, since rent collection can modify the account. @@ -212,7 +218,7 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> { ); account.map(|account| LoadedTransactionAccount { - loaded_size: account.data().len(), + loaded_size: base_account_size.saturating_add(account.data().len()), account, rent_collected: 0, }) @@ -442,14 +448,25 @@ pub(crate) fn load_transaction( match validation_result { Err(e) => TransactionLoadResult::NotLoaded(e), Ok(tx_details) => { - let load_result = load_transaction_accounts( - account_loader, - message, - tx_details.loaded_fee_payer_account, - tx_details.loaded_accounts_bytes_limit, - error_metrics, - rent_collector, - ); + let load_result = if HANA_FEATURE_GATE_PLACEHOLDER { + hana_load_transaction_accounts( + account_loader, + message, + tx_details.loaded_fee_payer_account, + tx_details.loaded_accounts_bytes_limit, + error_metrics, + rent_collector, + ) + } else { + load_transaction_accounts( + account_loader, + message, + tx_details.loaded_fee_payer_account, + tx_details.loaded_accounts_bytes_limit, + error_metrics, + rent_collector, + ) + }; match load_result { Ok(loaded_tx_accounts) => TransactionLoadResult::Loaded(LoadedTransaction { From e016a91e94739cd9b0420bbf0a0929bd557ffb45 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Thu, 1 May 2025 11:01:36 -0700 Subject: [PATCH 03/14] add the loaderv3 magic --- svm/src/account_loader.rs | 95 +++++++++++++++++++++++++-------------- 1 file changed, 61 insertions(+), 34 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 4f9707f406b..6f77d774678 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -8,11 +8,13 @@ use { }, ahash::{AHashMap, AHashSet}, solana_account::{ - Account, AccountSharedData, ReadableAccount, WritableAccount, PROGRAM_OWNERS, + state_traits::StateMut, Account, AccountSharedData, ReadableAccount, WritableAccount, + PROGRAM_OWNERS, }, solana_fee_structure::FeeDetails, solana_instruction::{BorrowedAccountMeta, BorrowedInstruction}, solana_instructions_sysvar::construct_instructions_data, + solana_loader_v3_interface::state::UpgradeableLoaderState, solana_nonce::state::State as NonceState, solana_nonce_account::{get_system_account_kind, SystemAccountKind}, solana_program_runtime::execution_budget::{ @@ -23,7 +25,7 @@ use { solana_rent_collector::{CollectedInfo, RENT_EXEMPT_RENT_EPOCH}, solana_rent_debits::RentDebits, solana_sdk_ids::{ - native_loader, + bpf_loader_upgradeable, native_loader, sysvar::{self, slot_history}, }, solana_svm_callback::{AccountState, TransactionProcessingCallback}, @@ -523,11 +525,6 @@ impl LoadedTransactionAccounts { } } -// HANA OK TODO how am i managing all this -// i was thinking just write a new load_transaction_accounts() from scratch -// and branch on the feature gate once and only once -// the loader handling is drastically simplified. other things maybe a bit trickier - fn hana_load_transaction_accounts( account_loader: &mut AccountLoader, message: &impl SVMMessage, @@ -557,37 +554,67 @@ fn hana_load_transaction_accounts( error_metrics, )?; - let mut collect_loaded_account = |key, loaded_account| -> Result<()> { - let LoadedTransactionAccount { - account, - loaded_size, - rent_collected, - } = loaded_account; - - loaded_transaction_accounts.increase_calculated_data_size( - loaded_size, - loaded_accounts_bytes_limit, - error_metrics, - )?; - - loaded_transaction_accounts.rent = loaded_transaction_accounts - .rent - .saturating_add(rent_collected); - - loaded_transaction_accounts - .rent_debits - .insert(key, rent_collected, account.lamports()); - - loaded_transaction_accounts.accounts.push((*key, account)); + let mut collect_loaded_account = + |account_loader: &mut AccountLoader, key, loaded_account| -> Result<()> { + let LoadedTransactionAccount { + account, + loaded_size, + rent_collected, + } = loaded_account; + + loaded_transaction_accounts.increase_calculated_data_size( + loaded_size, + loaded_accounts_bytes_limit, + error_metrics, + )?; + + loaded_transaction_accounts.rent = loaded_transaction_accounts + .rent + .saturating_add(rent_collected); + + loaded_transaction_accounts + .rent_debits + .insert(key, rent_collected, account.lamports()); + + // If this is a valid LoaderV3 program... + if bpf_loader_upgradeable::check_id(account.owner()) { + if let Ok(UpgradeableLoaderState::Program { + programdata_address, + }) = account.state() + { + // ...and we won't count it as a transaction account, and haven't counted it already... + if !account_keys.iter().any(|key| programdata_address == *key) + && !additional_loaded_accounts.contains(&programdata_address) + { + // ...and it exists (if it doesn't, it is *not* a load failure)... + if let Some(programdata_account) = + account_loader.load_account(&programdata_address) + { + // ...count it toward this transaction's total size. + loaded_transaction_accounts.increase_calculated_data_size( + TRANSACTION_ACCOUNT_BASE_SIZE + .saturating_add(programdata_account.data().len()), + loaded_accounts_bytes_limit, + error_metrics, + )?; + additional_loaded_accounts.insert(programdata_address); + } + } + } + } - // HANA TODO check for a program data account here + loaded_transaction_accounts.accounts.push((*key, account)); - Ok(()) - }; + Ok(()) + }; // Since the fee payer is always the first account, collect it first. // We can use it directly because it was already loaded during validation. - collect_loaded_account(message.fee_payer(), loaded_fee_payer_account)?; + collect_loaded_account( + account_loader, + message.fee_payer(), + loaded_fee_payer_account, + )?; // Attempt to load and collect remaining non-fee payer accounts for (account_index, account_key) in account_keys.iter().enumerate().skip(1) { @@ -598,7 +625,7 @@ fn hana_load_transaction_accounts( account_index, rent_collector, ); - collect_loaded_account(account_key, loaded_account)?; + collect_loaded_account(account_loader, account_key, loaded_account)?; } for (program_id, instruction) in message.program_instructions_iter() { From cb4957bd18d6550451124afd2907adbb41990813 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 7 May 2025 01:09:09 -0700 Subject: [PATCH 04/14] cleanups --- svm/src/account_loader.rs | 81 ++++++++++++++++++++++++++------------- 1 file changed, 54 insertions(+), 27 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 6f77d774678..7d87cbf861c 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -450,25 +450,14 @@ pub(crate) fn load_transaction( match validation_result { Err(e) => TransactionLoadResult::NotLoaded(e), Ok(tx_details) => { - let load_result = if HANA_FEATURE_GATE_PLACEHOLDER { - hana_load_transaction_accounts( - account_loader, - message, - tx_details.loaded_fee_payer_account, - tx_details.loaded_accounts_bytes_limit, - error_metrics, - rent_collector, - ) - } else { - load_transaction_accounts( - account_loader, - message, - tx_details.loaded_fee_payer_account, - tx_details.loaded_accounts_bytes_limit, - error_metrics, - rent_collector, - ) - }; + let load_result = load_transaction_accounts( + account_loader, + message, + tx_details.loaded_fee_payer_account, + tx_details.loaded_accounts_bytes_limit, + error_metrics, + rent_collector, + ); match load_result { Ok(loaded_tx_accounts) => TransactionLoadResult::Loaded(LoadedTransaction { @@ -525,7 +514,36 @@ impl LoadedTransactionAccounts { } } -fn hana_load_transaction_accounts( +fn load_transaction_accounts( + account_loader: &mut AccountLoader, + message: &impl SVMMessage, + loaded_fee_payer_account: LoadedTransactionAccount, + loaded_accounts_bytes_limit: NonZeroU32, + error_metrics: &mut TransactionErrorMetrics, + rent_collector: &dyn SVMRentCollector, +) -> Result { + if HANA_FEATURE_GATE_PLACEHOLDER { + load_transaction_accounts_simd186( + account_loader, + message, + loaded_fee_payer_account, + loaded_accounts_bytes_limit, + error_metrics, + rent_collector, + ) + } else { + load_transaction_accounts_old( + account_loader, + message, + loaded_fee_payer_account, + loaded_accounts_bytes_limit, + error_metrics, + rent_collector, + ) + } +} + +fn load_transaction_accounts_simd186( account_loader: &mut AccountLoader, message: &impl SVMMessage, loaded_fee_payer_account: LoadedTransactionAccount, @@ -544,8 +562,7 @@ fn hana_load_transaction_accounts( loaded_accounts_data_size: 0, }; - // transactions pay a base fee per address lookup table - // HANA can there be more than one? who does this return usize instead of bool. forwards compat? + // Transactions pay a base fee per address lookup table. loaded_transaction_accounts.increase_calculated_data_size( message .num_lookup_tables() @@ -576,21 +593,31 @@ fn hana_load_transaction_accounts( .rent_debits .insert(key, rent_collected, account.lamports()); + // This has been annotated branch-by-branch because collapsing the logic is infeasible. + // Its purpose is to ensure programdata accounts are counted once and *only* once per + // transaction. By checking account_keys, we never double-count a programdata account + // that was explictly included in the transaction. We also use a hashset to gracefully + // handle cases that LoaderV3 presumably makes impossible, such as self-referential + // program accounts or multiply-referenced programdata accounts, for added safety. + // + // If in the future LoaderV3 programs are migrated to LoaderV4, this entire code block + // can be deleted. + // // If this is a valid LoaderV3 program... if bpf_loader_upgradeable::check_id(account.owner()) { if let Ok(UpgradeableLoaderState::Program { programdata_address, }) = account.state() { - // ...and we won't count it as a transaction account, and haven't counted it already... + // ...its programdata was not already counted and will not later be counted... if !account_keys.iter().any(|key| programdata_address == *key) && !additional_loaded_accounts.contains(&programdata_address) { - // ...and it exists (if it doesn't, it is *not* a load failure)... + // ...and the programdata account exists (if it doesn't, it is *not* a load failure)... if let Some(programdata_account) = account_loader.load_account(&programdata_address) { - // ...count it toward this transaction's total size. + // ...count programdata toward this transaction's total size. loaded_transaction_accounts.increase_calculated_data_size( TRANSACTION_ACCOUNT_BASE_SIZE .saturating_add(programdata_account.data().len()), @@ -616,7 +643,7 @@ fn hana_load_transaction_accounts( loaded_fee_payer_account, )?; - // Attempt to load and collect remaining non-fee payer accounts + // Attempt to load and collect remaining non-fee payer accounts. for (account_index, account_key) in account_keys.iter().enumerate().skip(1) { let loaded_account = load_transaction_account( account_loader, @@ -662,7 +689,7 @@ fn hana_load_transaction_accounts( Ok(loaded_transaction_accounts) } -fn load_transaction_accounts( +fn load_transaction_accounts_old( account_loader: &mut AccountLoader, message: &impl SVMMessage, loaded_fee_payer_account: LoadedTransactionAccount, From 25a0254902968d23b127614dc311677592631936 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 7 May 2025 01:45:53 -0700 Subject: [PATCH 05/14] add feature gate, fix some tests and mark others --- feature-set/src/lib.rs | 7 ++ svm-feature-set/src/lib.rs | 2 + svm/src/account_loader.rs | 123 +++++++++++++++++++++---------- svm/src/transaction_processor.rs | 4 + 4 files changed, 96 insertions(+), 40 deletions(-) diff --git a/feature-set/src/lib.rs b/feature-set/src/lib.rs index ffb965c48ba..655609ad2b3 100644 --- a/feature-set/src/lib.rs +++ b/feature-set/src/lib.rs @@ -153,6 +153,8 @@ impl FeatureSet { increase_tx_account_lock_limit: self.is_active(&increase_tx_account_lock_limit::id()), disable_rent_fees_collection: self.is_active(&disable_rent_fees_collection::id()), enable_extend_program_checked: self.is_active(&enable_extend_program_checked::id()), + formalize_loaded_transaction_data_size: self + .is_active(&formalize_loaded_transaction_data_size::id()), } } } @@ -1095,6 +1097,10 @@ pub mod enable_extend_program_checked { solana_pubkey::declare_id!("97QCmR4QtfeQsAti9srfHFk5uMRFP95CvXG8EGr615HM"); } +pub mod formalize_loaded_transaction_data_size { + solana_pubkey::declare_id!("DeS7sR48ZcFTUmt5FFEVDr1v1bh73aAbZiZq3SYr8Eh8"); +} + pub static FEATURE_NAMES: LazyLock> = LazyLock::new(|| { [ (secp256k1_program_enabled::id(), "secp256k1 program"), @@ -1330,6 +1336,7 @@ pub static FEATURE_NAMES: LazyLock> = LazyLock::n (mask_out_rent_epoch_in_vm_serialization::id(), "SIMD-0267: Sets rent_epoch to a constant in the VM"), (enshrine_slashing_program::id(), "SIMD-0204: Slashable event verification"), (enable_extend_program_checked::id(), "Enable ExtendProgramChecked instruction"), + (formalize_loaded_transaction_data_size::id(), "SIMD-0186: Loaded transaction data size specification"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter() diff --git a/svm-feature-set/src/lib.rs b/svm-feature-set/src/lib.rs index d7cb6aa445a..c421b8b9aac 100644 --- a/svm-feature-set/src/lib.rs +++ b/svm-feature-set/src/lib.rs @@ -36,6 +36,7 @@ pub struct SVMFeatureSet { pub increase_tx_account_lock_limit: bool, pub disable_rent_fees_collection: bool, pub enable_extend_program_checked: bool, + pub formalize_loaded_transaction_data_size: bool, } impl SVMFeatureSet { @@ -77,6 +78,7 @@ impl SVMFeatureSet { increase_tx_account_lock_limit: true, disable_rent_fees_collection: true, enable_extend_program_checked: true, + formalize_loaded_transaction_data_size: true, } } } diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 7d87cbf861c..70792f69e78 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -37,8 +37,12 @@ use { std::num::{NonZeroU32, Saturating}, }; -const HANA_FEATURE_GATE_PLACEHOLDER: bool = false; +// Per SIMD-0186, all accounts are assigned a base size of 64 bytes to cover +// the storage cost of metadata. const TRANSACTION_ACCOUNT_BASE_SIZE: usize = 64; + +// Per SIMD-0186, resolved address lookup tables are assigned a base size of 8248 +// bytes: 8192 bytes for the maximum table size plus 56 bytes for metadata. const ADDRESS_LOOKUP_TABLE_BASE_SIZE: usize = 8248; // for the load instructions @@ -200,7 +204,7 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> { account_key: &Pubkey, is_writable: bool, ) -> Option { - let base_account_size = if HANA_FEATURE_GATE_PLACEHOLDER { + let base_account_size = if self.feature_set.formalize_loaded_transaction_data_size { TRANSACTION_ACCOUNT_BASE_SIZE } else { 0 @@ -522,7 +526,10 @@ fn load_transaction_accounts( error_metrics: &mut TransactionErrorMetrics, rent_collector: &dyn SVMRentCollector, ) -> Result { - if HANA_FEATURE_GATE_PLACEHOLDER { + if account_loader + .feature_set + .formalize_loaded_transaction_data_size + { load_transaction_accounts_simd186( account_loader, message, @@ -962,8 +969,10 @@ mod tests { solana_transaction_context::{TransactionAccount, TransactionContext}, solana_transaction_error::{TransactionError, TransactionResult as Result}, std::{borrow::Cow, cell::RefCell, collections::HashMap, fs::File, io::Read}, + test_case::test_case, }; + // HANA enable all by default #[derive(Clone, Default)] struct TestCallbacks { accounts_map: HashMap, @@ -1055,22 +1064,9 @@ mod tests { )) } - fn load_accounts_aux_test( - tx: Transaction, - accounts: &[TransactionAccount], - error_metrics: &mut TransactionErrorMetrics, - ) -> TransactionLoadResult { - load_accounts_with_features_and_rent( - tx, - accounts, - &RentCollector::default(), - error_metrics, - SVMFeatureSet::all_enabled(), - ) - } - - #[test] - fn test_load_accounts_unknown_program_id() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_load_accounts_unknown_program_id(formalize_loaded_transaction_data_size: bool) { let mut accounts: Vec = Vec::new(); let mut error_metrics = TransactionErrorMetrics::default(); @@ -1093,7 +1089,16 @@ mod tests { instructions, ); - let load_results = load_accounts_aux_test(tx, &accounts, &mut error_metrics); + let mut feature_set = SVMFeatureSet::all_enabled(); + feature_set.formalize_loaded_transaction_data_size = formalize_loaded_transaction_data_size; + + let load_results = load_accounts_with_features_and_rent( + tx, + &accounts, + &RentCollector::default(), + &mut error_metrics, + feature_set, + ); assert_eq!(error_metrics.account_not_found.0, 1); assert!(matches!( @@ -1105,8 +1110,9 @@ mod tests { )); } - #[test] - fn test_load_accounts_no_loaders() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_load_accounts_no_loaders(formalize_loaded_transaction_data_size: bool) { let mut accounts: Vec = Vec::new(); let mut error_metrics = TransactionErrorMetrics::default(); @@ -1131,12 +1137,15 @@ mod tests { instructions, ); + let mut feature_set = SVMFeatureSet::all_enabled(); + feature_set.formalize_loaded_transaction_data_size = formalize_loaded_transaction_data_size; + let loaded_accounts = load_accounts_with_features_and_rent( tx, &accounts, &RentCollector::default(), &mut error_metrics, - SVMFeatureSet::all_enabled(), + feature_set, ); assert_eq!(error_metrics.account_not_found.0, 0); @@ -1152,8 +1161,9 @@ mod tests { } } - #[test] - fn test_load_accounts_bad_owner() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_load_accounts_bad_owner(formalize_loaded_transaction_data_size: bool) { let mut accounts: Vec = Vec::new(); let mut error_metrics = TransactionErrorMetrics::default(); @@ -1165,7 +1175,6 @@ mod tests { accounts.push((key0, account)); let mut account = AccountSharedData::new(40, 1, &Pubkey::default()); - account.set_owner(bpf_loader_upgradeable::id()); account.set_executable(true); accounts.push((key1, account)); @@ -1178,20 +1187,41 @@ mod tests { instructions, ); - let load_results = load_accounts_aux_test(tx, &accounts, &mut error_metrics); + let mut feature_set = SVMFeatureSet::all_enabled(); + feature_set.formalize_loaded_transaction_data_size = formalize_loaded_transaction_data_size; - assert_eq!(error_metrics.account_not_found.0, 1); - assert!(matches!( - load_results, - TransactionLoadResult::FeesOnly(FeesOnlyTransaction { - load_error: TransactionError::ProgramAccountNotFound, - .. - }), - )); + let load_results = load_accounts_with_features_and_rent( + tx, + &accounts, + &RentCollector::default(), + &mut error_metrics, + feature_set, + ); + + if formalize_loaded_transaction_data_size { + assert_eq!(error_metrics.invalid_program_for_execution.0, 1); + assert!(matches!( + load_results, + TransactionLoadResult::FeesOnly(FeesOnlyTransaction { + load_error: TransactionError::InvalidProgramForExecution, + .. + }), + )); + } else { + assert_eq!(error_metrics.account_not_found.0, 1); + assert!(matches!( + load_results, + TransactionLoadResult::FeesOnly(FeesOnlyTransaction { + load_error: TransactionError::ProgramAccountNotFound, + .. + }), + )); + } } - #[test] - fn test_load_accounts_not_executable() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_load_accounts_not_executable(formalize_loaded_transaction_data_size: bool) { let mut accounts: Vec = Vec::new(); let mut error_metrics = TransactionErrorMetrics::default(); @@ -1216,6 +1246,8 @@ mod tests { let mut feature_set = SVMFeatureSet::all_enabled(); feature_set.remove_accounts_executable_flag_checks = false; + feature_set.formalize_loaded_transaction_data_size = formalize_loaded_transaction_data_size; + let load_results = load_accounts_with_features_and_rent( tx, &accounts, @@ -1234,8 +1266,9 @@ mod tests { )); } - #[test] - fn test_load_accounts_multiple_loaders() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_load_accounts_multiple_loaders(formalize_loaded_transaction_data_size: bool) { let mut accounts: Vec = Vec::new(); let mut error_metrics = TransactionErrorMetrics::default(); @@ -1272,12 +1305,15 @@ mod tests { instructions, ); + let mut feature_set = SVMFeatureSet::all_enabled(); + feature_set.formalize_loaded_transaction_data_size = formalize_loaded_transaction_data_size; + let loaded_accounts = load_accounts_with_features_and_rent( tx, &accounts, &RentCollector::default(), &mut error_metrics, - SVMFeatureSet::all_enabled(), + feature_set, ); assert_eq!(error_metrics.account_not_found.0, 0); @@ -1632,6 +1668,7 @@ mod tests { ); } + // HANA fails with feature #[test] fn test_load_transaction_accounts_native_loader() { let key1 = Keypair::new(); @@ -1782,6 +1819,7 @@ mod tests { ); } + // HANA fails with feature #[test] fn test_load_transaction_accounts_native_loader_owner() { let key1 = Keypair::new(); @@ -1951,6 +1989,7 @@ mod tests { ); } + // HANA fails with feature #[test] fn test_load_transaction_accounts_program_success_complete() { let key1 = Keypair::new(); @@ -2026,6 +2065,7 @@ mod tests { ); } + // HANA fails with feature #[test] fn test_load_transaction_accounts_program_builtin_saturating_add() { let key1 = Keypair::new(); @@ -2496,6 +2536,9 @@ mod tests { assert_eq!(actual_inspected_accounts, expected_inspected_accounts,); } + // HANA fails with feature, need a new version of this + // our new one is much less arbitrary so probably a totally new test + // just generate accounts knowing what the sizes should be #[test] fn test_load_transaction_accounts_data_sizes() { let mut mock_bank = TestCallbacks::default(); diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 6e7c4229c74..70dcc2b0ca1 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -1147,6 +1147,7 @@ mod tests { } } + // HANA enable all by default #[derive(Default, Clone)] struct MockBankCallback { account_shared_data: Arc>>, @@ -2036,6 +2037,7 @@ mod tests { assert_eq!(entry, Arc::new(program)); } + // HANA fails with feature #[test] fn test_validate_transaction_fee_payer_exact_balance() { let lamports_per_signature = 5000; @@ -2130,6 +2132,7 @@ mod tests { ); } + // HANA fails with feature #[test] fn test_validate_transaction_fee_payer_rent_paying() { let lamports_per_signature = 5000; @@ -2381,6 +2384,7 @@ mod tests { assert_eq!(result, Err(TransactionError::DuplicateInstruction(1u8))); } + // HANA fails with feature #[test] fn test_validate_transaction_fee_payer_is_nonce() { let lamports_per_signature = 5000; From 91600c7dd2c8eb96947c76809b9fd201c3eb43d7 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 7 May 2025 02:09:01 -0700 Subject: [PATCH 06/14] fix realloc integration test --- svm/tests/integration_test.rs | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/svm/tests/integration_test.rs b/svm/tests/integration_test.rs index d0eae11f560..80768774e76 100644 --- a/svm/tests/integration_test.rs +++ b/svm/tests/integration_test.rs @@ -4,8 +4,8 @@ use { crate::mock_bank::{ create_custom_loader, deploy_program_with_upgrade_authority, program_address, - register_builtins, MockBankCallback, MockForkGraph, EXECUTION_EPOCH, EXECUTION_SLOT, - WALLCLOCK_TIME, + program_data_size, register_builtins, MockBankCallback, MockForkGraph, EXECUTION_EPOCH, + EXECUTION_SLOT, WALLCLOCK_TIME, }, agave_feature_set::{self as feature_set, FeatureSet}, solana_account::{AccountSharedData, ReadableAccount, WritableAccount, PROGRAM_OWNERS}, @@ -2193,14 +2193,20 @@ fn simd83_fee_payer_deallocate() -> Vec { vec![test_entry] } -fn simd83_account_reallocate() -> Vec { +fn simd83_account_reallocate(formalize_loaded_transaction_data_size: bool) -> Vec { let mut test_entries = vec![]; let program_name = "write-to-account"; let program_id = program_address(program_name); + let program_size = program_data_size(program_name); let mut common_test_entry = SvmTestEntry::default(); common_test_entry.add_initial_program(program_name); + if !formalize_loaded_transaction_data_size { + common_test_entry + .disabled_features + .push(feature_set::formalize_loaded_transaction_data_size::id()); + } let fee_payer_keypair = Keypair::new(); let fee_payer = fee_payer_keypair.pubkey(); @@ -2223,11 +2229,20 @@ fn simd83_account_reallocate() -> Vec { let target_start_size = 100; common_test_entry.add_initial_account(target, &mk_target(target_start_size)); + // we set a budget that is enough pre-large-realloc but not enough post-large-realloc + // the relevant feature counts programdata size, so if enabled, we add breathing room + // this test has nothing to do with the feature + let size_budget = Some(if formalize_loaded_transaction_data_size { + (program_size + MAX_PERMITTED_DATA_INCREASE) as u32 + } else { + MAX_PERMITTED_DATA_INCREASE as u32 + }); + let print_transaction = WriteProgramInstruction::Print.create_transaction( program_id, &fee_payer_keypair, target, - Some(MAX_PERMITTED_DATA_INCREASE.try_into().unwrap()), + size_budget, ); common_test_entry.decrease_expected_lamports(&fee_payer, LAMPORTS_PER_SIGNATURE * 2); @@ -2338,7 +2353,8 @@ fn program_cache_update_tombstone() -> Vec { #[test_case(simd83_nonce_reuse(true))] #[test_case(simd83_account_deallocate())] #[test_case(simd83_fee_payer_deallocate())] -#[test_case(simd83_account_reallocate())] +#[test_case(simd83_account_reallocate(false))] +#[test_case(simd83_account_reallocate(true))] #[test_case(program_cache_update_tombstone())] fn svm_integration(test_entries: Vec) { for test_entry in test_entries { From 97aaee081f81e9796d922fbc470638e2d9c6a45c Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 7 May 2025 03:11:27 -0700 Subject: [PATCH 07/14] fix account loader tests --- svm/src/account_loader.rs | 160 ++++++++++++++++++++++++++++---------- 1 file changed, 120 insertions(+), 40 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 70792f69e78..da7a2c31659 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -972,8 +972,7 @@ mod tests { test_case::test_case, }; - // HANA enable all by default - #[derive(Clone, Default)] + #[derive(Clone)] struct TestCallbacks { accounts_map: HashMap, #[allow(clippy::type_complexity)] @@ -982,6 +981,16 @@ mod tests { feature_set: SVMFeatureSet, } + impl Default for TestCallbacks { + fn default() -> Self { + Self { + accounts_map: HashMap::default(), + inspected_accounts: RefCell::default(), + feature_set: SVMFeatureSet::all_enabled(), + } + } + } + impl InvokeContextCallback for TestCallbacks {} impl TransactionProcessingCallback for TestCallbacks { @@ -1668,9 +1677,9 @@ mod tests { ); } - // HANA fails with feature - #[test] - fn test_load_transaction_accounts_native_loader() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_load_transaction_accounts_native_loader(formalize_loaded_transaction_data_size: bool) { let key1 = Keypair::new(); let message = Message { account_keys: vec![key1.pubkey(), native_loader::id()], @@ -1685,6 +1694,8 @@ mod tests { let sanitized_message = new_unchecked_sanitized_message(message); let mut mock_bank = TestCallbacks::default(); + mock_bank.feature_set.formalize_loaded_transaction_data_size = + formalize_loaded_transaction_data_size; mock_bank .accounts_map .insert(native_loader::id(), AccountSharedData::default()); @@ -1702,11 +1713,19 @@ mod tests { vec![Signature::new_unique()], false, ); + + let base_account_size = if formalize_loaded_transaction_data_size { + TRANSACTION_ACCOUNT_BASE_SIZE + } else { + 0 + }; + let result = load_transaction_accounts( &mut account_loader, sanitized_transaction.message(), LoadedTransactionAccount { account: fee_payer_account.clone(), + loaded_size: base_account_size, ..LoadedTransactionAccount::default() }, MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES, @@ -1714,6 +1733,8 @@ mod tests { &RentCollector::default(), ); + let loaded_accounts_data_size = base_account_size as u32 * 2; + assert_eq!( result.unwrap(), LoadedTransactionAccounts { @@ -1727,7 +1748,7 @@ mod tests { program_indices: vec![vec![]], rent: 0, rent_debits: RentDebits::default(), - loaded_accounts_data_size: 0, + loaded_accounts_data_size, } ); } @@ -1819,9 +1840,11 @@ mod tests { ); } - // HANA fails with feature - #[test] - fn test_load_transaction_accounts_native_loader_owner() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_load_transaction_accounts_native_loader_owner( + formalize_loaded_transaction_data_size: bool, + ) { let key1 = Keypair::new(); let key2 = Keypair::new(); @@ -1838,6 +1861,8 @@ mod tests { let sanitized_message = new_unchecked_sanitized_message(message); let mut mock_bank = TestCallbacks::default(); + mock_bank.feature_set.formalize_loaded_transaction_data_size = + formalize_loaded_transaction_data_size; let mut account_data = AccountSharedData::default(); account_data.set_owner(native_loader::id()); account_data.set_lamports(1); @@ -1858,11 +1883,19 @@ mod tests { vec![Signature::new_unique()], false, ); + + let base_account_size = if formalize_loaded_transaction_data_size { + TRANSACTION_ACCOUNT_BASE_SIZE + } else { + 0 + }; + let result = load_transaction_accounts( &mut account_loader, sanitized_transaction.message(), LoadedTransactionAccount { account: fee_payer_account.clone(), + loaded_size: base_account_size, ..LoadedTransactionAccount::default() }, MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES, @@ -1870,6 +1903,8 @@ mod tests { &RentCollector::default(), ); + let loaded_accounts_data_size = base_account_size as u32 * 2; + assert_eq!( result.unwrap(), LoadedTransactionAccounts { @@ -1883,7 +1918,7 @@ mod tests { program_indices: vec![vec![1]], rent: 0, rent_debits: RentDebits::default(), - loaded_accounts_data_size: 0, + loaded_accounts_data_size, } ); } @@ -1989,12 +2024,13 @@ mod tests { ); } - // HANA fails with feature - #[test] - fn test_load_transaction_accounts_program_success_complete() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_load_transaction_accounts_program_success_complete( + formalize_loaded_transaction_data_size: bool, + ) { let key1 = Keypair::new(); let key2 = Keypair::new(); - let key3 = Keypair::new(); let message = Message { account_keys: vec![key2.pubkey(), key1.pubkey()], @@ -2009,10 +2045,12 @@ mod tests { let sanitized_message = new_unchecked_sanitized_message(message); let mut mock_bank = TestCallbacks::default(); + mock_bank.feature_set.formalize_loaded_transaction_data_size = + formalize_loaded_transaction_data_size; let mut account_data = AccountSharedData::default(); account_data.set_lamports(1); account_data.set_executable(true); - account_data.set_owner(key3.pubkey()); + account_data.set_owner(bpf_loader::id()); mock_bank.accounts_map.insert(key1.pubkey(), account_data); let mut fee_payer_account = AccountSharedData::default(); @@ -2025,7 +2063,9 @@ mod tests { account_data.set_lamports(1); account_data.set_executable(true); account_data.set_owner(native_loader::id()); - mock_bank.accounts_map.insert(key3.pubkey(), account_data); + mock_bank + .accounts_map + .insert(bpf_loader::id(), account_data); let mut account_loader = (&mock_bank).into(); let mut error_metrics = TransactionErrorMetrics::default(); @@ -2035,11 +2075,19 @@ mod tests { vec![Signature::new_unique()], false, ); + + let base_account_size = if formalize_loaded_transaction_data_size { + TRANSACTION_ACCOUNT_BASE_SIZE + } else { + 0 + }; + let result = load_transaction_accounts( &mut account_loader, sanitized_transaction.message(), LoadedTransactionAccount { account: fee_payer_account.clone(), + loaded_size: base_account_size, ..LoadedTransactionAccount::default() }, MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES, @@ -2047,6 +2095,8 @@ mod tests { &RentCollector::default(), ); + let loaded_accounts_data_size = base_account_size as u32 * 2; + assert_eq!( result.unwrap(), LoadedTransactionAccounts { @@ -2060,21 +2110,22 @@ mod tests { program_indices: vec![vec![1]], rent: 0, rent_debits: RentDebits::default(), - loaded_accounts_data_size: 0, + loaded_accounts_data_size, } ); } - // HANA fails with feature - #[test] - fn test_load_transaction_accounts_program_builtin_saturating_add() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_load_transaction_accounts_program_builtin_saturating_add( + formalize_loaded_transaction_data_size: bool, + ) { let key1 = Keypair::new(); let key2 = Keypair::new(); let key3 = Keypair::new(); - let key4 = Keypair::new(); let message = Message { - account_keys: vec![key2.pubkey(), key1.pubkey(), key4.pubkey()], + account_keys: vec![key2.pubkey(), key1.pubkey(), key3.pubkey()], header: MessageHeader::default(), instructions: vec![ CompiledInstruction { @@ -2093,10 +2144,12 @@ mod tests { let sanitized_message = new_unchecked_sanitized_message(message); let mut mock_bank = TestCallbacks::default(); + mock_bank.feature_set.formalize_loaded_transaction_data_size = + formalize_loaded_transaction_data_size; let mut account_data = AccountSharedData::default(); account_data.set_lamports(1); account_data.set_executable(true); - account_data.set_owner(key3.pubkey()); + account_data.set_owner(bpf_loader::id()); mock_bank.accounts_map.insert(key1.pubkey(), account_data); let mut fee_payer_account = AccountSharedData::default(); @@ -2109,7 +2162,9 @@ mod tests { account_data.set_lamports(1); account_data.set_executable(true); account_data.set_owner(native_loader::id()); - mock_bank.accounts_map.insert(key3.pubkey(), account_data); + mock_bank + .accounts_map + .insert(bpf_loader::id(), account_data); let mut account_loader = (&mock_bank).into(); let mut error_metrics = TransactionErrorMetrics::default(); @@ -2119,11 +2174,19 @@ mod tests { vec![Signature::new_unique()], false, ); + + let base_account_size = if formalize_loaded_transaction_data_size { + TRANSACTION_ACCOUNT_BASE_SIZE + } else { + 0 + }; + let result = load_transaction_accounts( &mut account_loader, sanitized_transaction.message(), LoadedTransactionAccount { account: fee_payer_account.clone(), + loaded_size: base_account_size, ..LoadedTransactionAccount::default() }, MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES, @@ -2131,6 +2194,8 @@ mod tests { &RentCollector::default(), ); + let loaded_accounts_data_size = base_account_size as u32 * 2; + let mut account_data = AccountSharedData::default(); account_data.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); assert_eq!( @@ -2142,12 +2207,12 @@ mod tests { key1.pubkey(), mock_bank.accounts_map[&key1.pubkey()].clone() ), - (key4.pubkey(), account_data), + (key3.pubkey(), account_data), ], program_indices: vec![vec![1], vec![1]], rent: 0, rent_debits: RentDebits::default(), - loaded_accounts_data_size: 0, + loaded_accounts_data_size, } ); } @@ -2217,15 +2282,15 @@ mod tests { ); } - #[test] - fn test_load_accounts_success() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_load_accounts_success(formalize_loaded_transaction_data_size: bool) { let key1 = Keypair::new(); let key2 = Keypair::new(); let key3 = Keypair::new(); - let key4 = Keypair::new(); let message = Message { - account_keys: vec![key2.pubkey(), key1.pubkey(), key4.pubkey()], + account_keys: vec![key2.pubkey(), key1.pubkey(), key3.pubkey()], header: MessageHeader::default(), instructions: vec![ CompiledInstruction { @@ -2244,10 +2309,12 @@ mod tests { let sanitized_message = new_unchecked_sanitized_message(message); let mut mock_bank = TestCallbacks::default(); + mock_bank.feature_set.formalize_loaded_transaction_data_size = + formalize_loaded_transaction_data_size; let mut account_data = AccountSharedData::default(); account_data.set_lamports(1); account_data.set_executable(true); - account_data.set_owner(key3.pubkey()); + account_data.set_owner(bpf_loader::id()); mock_bank.accounts_map.insert(key1.pubkey(), account_data); let mut fee_payer_account = AccountSharedData::default(); @@ -2260,7 +2327,9 @@ mod tests { account_data.set_lamports(1); account_data.set_executable(true); account_data.set_owner(native_loader::id()); - mock_bank.accounts_map.insert(key3.pubkey(), account_data); + mock_bank + .accounts_map + .insert(bpf_loader::id(), account_data); let mut account_loader = (&mock_bank).into(); let mut error_metrics = TransactionErrorMetrics::default(); @@ -2270,9 +2339,17 @@ mod tests { vec![Signature::new_unique()], false, ); + + let base_account_size = if formalize_loaded_transaction_data_size { + TRANSACTION_ACCOUNT_BASE_SIZE + } else { + 0 + }; + let validation_result = Ok(ValidatedTransactionDetails { loaded_fee_payer_account: LoadedTransactionAccount { account: fee_payer_account, + loaded_size: base_account_size, ..LoadedTransactionAccount::default() }, ..ValidatedTransactionDetails::default() @@ -2286,6 +2363,8 @@ mod tests { &RentCollector::default(), ); + let loaded_accounts_data_size = base_account_size as u32 * 2; + let mut account_data = AccountSharedData::default(); account_data.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); @@ -2304,7 +2383,7 @@ mod tests { key1.pubkey(), mock_bank.accounts_map[&key1.pubkey()].clone() ), - (key4.pubkey(), account_data), + (key3.pubkey(), account_data), ], program_indices: vec![vec![1], vec![1]], fee_details: FeeDetails::default(), @@ -2312,7 +2391,7 @@ mod tests { compute_budget: SVMTransactionExecutionBudget::default(), rent: 0, rent_debits: RentDebits::default(), - loaded_accounts_data_size: 0, + loaded_accounts_data_size, } ); } @@ -2536,12 +2615,10 @@ mod tests { assert_eq!(actual_inspected_accounts, expected_inspected_accounts,); } - // HANA fails with feature, need a new version of this - // our new one is much less arbitrary so probably a totally new test - // just generate accounts knowing what the sizes should be #[test] fn test_load_transaction_accounts_data_sizes() { let mut mock_bank = TestCallbacks::default(); + mock_bank.feature_set.formalize_loaded_transaction_data_size = false; let loader_v2 = bpf_loader::id(); let loader_v3 = bpf_loader_upgradeable::id(); @@ -2623,10 +2700,13 @@ mod tests { let mut program_accounts = HashMap::new(); program_accounts.insert(program1, (&loader_v2, 0)); program_accounts.insert(program2, (&loader_v3, 0)); - let feature_set = SVMFeatureSet::default(); let test_transaction_data_size = |transaction, expected_size| { - let mut account_loader = - AccountLoader::new_with_loaded_accounts_capacity(None, &mock_bank, &feature_set, 0); + let mut account_loader = AccountLoader::new_with_loaded_accounts_capacity( + None, + &mock_bank, + &mock_bank.feature_set, + 0, + ); let loaded_transaction_accounts = load_transaction_accounts( &mut account_loader, From fd12e4ddb26c5c00a303c0023aa03027d811edec Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 7 May 2025 03:34:42 -0700 Subject: [PATCH 08/14] fix transaction processor tests --- svm/src/account_loader.rs | 2 +- svm/src/transaction_processor.rs | 77 ++++++++++++++++++++++++-------- 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index da7a2c31659..ee7b3fd2507 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -39,7 +39,7 @@ use { // Per SIMD-0186, all accounts are assigned a base size of 64 bytes to cover // the storage cost of metadata. -const TRANSACTION_ACCOUNT_BASE_SIZE: usize = 64; +pub(crate) const TRANSACTION_ACCOUNT_BASE_SIZE: usize = 64; // Per SIMD-0186, resolved address lookup tables are assigned a base size of 8248 // bytes: 8192 bytes for the maximum table size plus 56 bytes for metadata. diff --git a/svm/src/transaction_processor.rs b/svm/src/transaction_processor.rs index 70dcc2b0ca1..7d00785a6fa 100644 --- a/svm/src/transaction_processor.rs +++ b/svm/src/transaction_processor.rs @@ -1099,7 +1099,10 @@ mod tests { use { super::*, crate::{ - account_loader::{LoadedTransactionAccount, ValidatedTransactionDetails}, + account_loader::{ + LoadedTransactionAccount, ValidatedTransactionDetails, + TRANSACTION_ACCOUNT_BASE_SIZE, + }, nonce_info::NonceInfo, rollback_accounts::RollbackAccounts, }, @@ -1147,8 +1150,7 @@ mod tests { } } - // HANA enable all by default - #[derive(Default, Clone)] + #[derive(Clone)] struct MockBankCallback { account_shared_data: Arc>>, #[allow(clippy::type_complexity)] @@ -1157,6 +1159,16 @@ mod tests { feature_set: SVMFeatureSet, } + impl Default for MockBankCallback { + fn default() -> Self { + Self { + account_shared_data: Arc::default(), + inspected_accounts: Arc::default(), + feature_set: SVMFeatureSet::all_enabled(), + } + } + } + impl InvokeContextCallback for MockBankCallback {} impl TransactionProcessingCallback for MockBankCallback { @@ -2037,9 +2049,11 @@ mod tests { assert_eq!(entry, Arc::new(program)); } - // HANA fails with feature - #[test] - fn test_validate_transaction_fee_payer_exact_balance() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_validate_transaction_fee_payer_exact_balance( + formalize_loaded_transaction_data_size: bool, + ) { let lamports_per_signature = 5000; let message = new_unchecked_sanitized_message(Message::new_with_blockhash( &[ @@ -2077,10 +2091,12 @@ mod tests { ); let mut mock_accounts = HashMap::new(); mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); - let mock_bank = MockBankCallback { + let mut mock_bank = MockBankCallback { account_shared_data: Arc::new(RwLock::new(mock_accounts)), ..Default::default() }; + mock_bank.feature_set.formalize_loaded_transaction_data_size = + formalize_loaded_transaction_data_size; let mut account_loader = (&mock_bank).into(); let mut error_counters = TransactionErrorMetrics::default(); @@ -2109,6 +2125,12 @@ mod tests { account }; + let base_account_size = if formalize_loaded_transaction_data_size { + TRANSACTION_ACCOUNT_BASE_SIZE + } else { + 0 + }; + assert_eq!( result, Ok(ValidatedTransactionDetails { @@ -2124,7 +2146,7 @@ mod tests { .loaded_accounts_data_size_limit, fee_details: FeeDetails::new(transaction_fee, priority_fee), loaded_fee_payer_account: LoadedTransactionAccount { - loaded_size: fee_payer_account.data().len(), + loaded_size: base_account_size + fee_payer_account.data().len(), account: post_validation_fee_payer_account, rent_collected: fee_payer_rent_debit, }, @@ -2132,9 +2154,11 @@ mod tests { ); } - // HANA fails with feature - #[test] - fn test_validate_transaction_fee_payer_rent_paying() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_validate_transaction_fee_payer_rent_paying( + formalize_loaded_transaction_data_size: bool, + ) { let lamports_per_signature = 5000; let message = new_unchecked_sanitized_message(Message::new_with_blockhash( &[], @@ -2159,10 +2183,13 @@ mod tests { let mut mock_accounts = HashMap::new(); mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); - let mock_bank = MockBankCallback { + let mut mock_bank = MockBankCallback { account_shared_data: Arc::new(RwLock::new(mock_accounts)), ..Default::default() }; + mock_bank.feature_set.formalize_loaded_transaction_data_size = + formalize_loaded_transaction_data_size; + mock_bank.feature_set.disable_rent_fees_collection = false; let mut account_loader = (&mock_bank).into(); let mut error_counters = TransactionErrorMetrics::default(); @@ -2186,6 +2213,12 @@ mod tests { account }; + let base_account_size = if formalize_loaded_transaction_data_size { + TRANSACTION_ACCOUNT_BASE_SIZE + } else { + 0 + }; + assert_eq!( result, Ok(ValidatedTransactionDetails { @@ -2201,7 +2234,7 @@ mod tests { .loaded_accounts_data_size_limit, fee_details: FeeDetails::new(transaction_fee, 0), loaded_fee_payer_account: LoadedTransactionAccount { - loaded_size: fee_payer_account.data().len(), + loaded_size: base_account_size + fee_payer_account.data().len(), account: post_validation_fee_payer_account, rent_collected: fee_payer_rent_debit, } @@ -2384,9 +2417,9 @@ mod tests { assert_eq!(result, Err(TransactionError::DuplicateInstruction(1u8))); } - // HANA fails with feature - #[test] - fn test_validate_transaction_fee_payer_is_nonce() { + #[test_case(false; "informal_loaded_size")] + #[test_case(true; "simd186_loaded_size")] + fn test_validate_transaction_fee_payer_is_nonce(formalize_loaded_transaction_data_size: bool) { let lamports_per_signature = 5000; let rent_collector = RentCollector::default(); let compute_unit_limit = 1000u64; @@ -2425,10 +2458,12 @@ mod tests { let mut mock_accounts = HashMap::new(); mock_accounts.insert(*fee_payer_address, fee_payer_account.clone()); - let mock_bank = MockBankCallback { + let mut mock_bank = MockBankCallback { account_shared_data: Arc::new(RwLock::new(mock_accounts)), ..Default::default() }; + mock_bank.feature_set.formalize_loaded_transaction_data_size = + formalize_loaded_transaction_data_size; let mut account_loader = (&mock_bank).into(); let mut error_counters = TransactionErrorMetrics::default(); @@ -2461,6 +2496,12 @@ mod tests { account }; + let base_account_size = if formalize_loaded_transaction_data_size { + TRANSACTION_ACCOUNT_BASE_SIZE + } else { + 0 + }; + assert_eq!( result, Ok(ValidatedTransactionDetails { @@ -2476,7 +2517,7 @@ mod tests { .loaded_accounts_data_size_limit, fee_details: FeeDetails::new(transaction_fee, priority_fee), loaded_fee_payer_account: LoadedTransactionAccount { - loaded_size: fee_payer_account.data().len(), + loaded_size: base_account_size + fee_payer_account.data().len(), account: post_validation_fee_payer_account, rent_collected: 0, } From 9936c1d187f34f67b4c479925e405fdb12a3e8de Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 7 May 2025 03:54:09 -0700 Subject: [PATCH 09/14] note to self --- runtime/src/bank/tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 8087e4b2f8e..1c2695a8552 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -8400,6 +8400,7 @@ fn test_program_is_native_loader() { ); } +// HANA this breaks, expects UnsupportedProgramId but we produce InvalidProgramForExecution #[test] fn test_invoke_non_program_account_owned_by_a_builtin() { let (genesis_config, mint_keypair) = create_genesis_config(10000000); @@ -13383,6 +13384,7 @@ fn test_deploy_last_epoch_slot() { assert_eq!(result_with_feature_enabled, Ok(())); } +// HANA this breaks, expects UnsupportedProgramId but we produce InvalidProgramForExecution #[test] fn test_loader_v3_to_v4_migration() { solana_logger::setup(); From 09b243e2266f62fa444dc55c3fd278737d9e7c6d Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Fri, 9 May 2025 11:38:48 -0700 Subject: [PATCH 10/14] add coverage to runtime tests --- runtime/src/bank/tests.rs | 62 ++++++++++++++++++++++++++------------- 1 file changed, 42 insertions(+), 20 deletions(-) diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 1c2695a8552..e0bca2c6219 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -7177,11 +7177,15 @@ fn test_bank_load_program() { } #[allow(deprecated)] -#[test] -fn test_bpf_loader_upgradeable_deploy_with_max_len() { +#[test_case(false; "informal_loaded_size")] +#[test_case(true; "simd186_loaded_size")] +fn test_bpf_loader_upgradeable_deploy_with_max_len(formalize_loaded_transaction_data_size: bool) { let (genesis_config, mint_keypair) = create_genesis_config_no_tx_fee(1_000_000_000); let mut bank = Bank::new_for_tests(&genesis_config); bank.feature_set = Arc::new(FeatureSet::all_enabled()); + if !formalize_loaded_transaction_data_size { + bank.deactivate_feature(&feature_set::formalize_loaded_transaction_data_size::id()); + } let (bank, bank_forks) = bank.wrap_with_bank_forks_for_tests(); let mut bank_client = BankClient::new_shared(bank.clone()); @@ -8376,10 +8380,15 @@ fn test_timestamp_fast() { } } -#[test] -fn test_program_is_native_loader() { +#[test_case(false; "informal_loaded_size")] +#[test_case(true; "simd186_loaded_size")] +fn test_program_is_native_loader(formalize_loaded_transaction_data_size: bool) { let (genesis_config, mint_keypair) = create_genesis_config(50000); - let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); + let mut bank = Bank::new_for_tests(&genesis_config); + if formalize_loaded_transaction_data_size { + bank.activate_feature(&feature_set::formalize_loaded_transaction_data_size::id()); + } + let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); let tx = Transaction::new_signed_with_payer( &[Instruction::new_with_bincode( @@ -8400,12 +8409,17 @@ fn test_program_is_native_loader() { ); } -// HANA this breaks, expects UnsupportedProgramId but we produce InvalidProgramForExecution -#[test] -fn test_invoke_non_program_account_owned_by_a_builtin() { +#[test_case(false; "informal_loaded_size")] +#[test_case(true; "simd186_loaded_size")] +fn test_invoke_non_program_account_owned_by_a_builtin( + formalize_loaded_transaction_data_size: bool, +) { let (genesis_config, mint_keypair) = create_genesis_config(10000000); let mut bank = Bank::new_for_tests(&genesis_config); bank.activate_feature(&feature_set::remove_accounts_executable_flag_checks::id()); + if formalize_loaded_transaction_data_size { + bank.activate_feature(&feature_set::formalize_loaded_transaction_data_size::id()); + } let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); let bogus_program = Pubkey::new_unique(); @@ -8432,13 +8446,12 @@ fn test_invoke_non_program_account_owned_by_a_builtin() { &[&mint_keypair, &created_account_keypair], bank.last_blockhash(), ); - assert_eq!( - bank.process_transaction(&tx), - Err(TransactionError::InstructionError( - 0, - InstructionError::UnsupportedProgramId - )) - ); + let expected_error = if formalize_loaded_transaction_data_size { + TransactionError::InvalidProgramForExecution + } else { + TransactionError::InstructionError(0, InstructionError::UnsupportedProgramId) + }; + assert_eq!(bank.process_transaction(&tx), Err(expected_error),); } #[test] @@ -12225,8 +12238,11 @@ fn test_is_in_slot_hashes_history() { assert!(!new_bank.is_in_slot_hashes_history(&0)); } -#[test] -fn test_feature_activation_loaded_programs_cache_preparation_phase() { +#[test_case(false; "informal_loaded_size")] +#[test_case(true; "simd186_loaded_size")] +fn test_feature_activation_loaded_programs_cache_preparation_phase( + formalize_loaded_transaction_data_size: bool, +) { solana_logger::setup(); // Bank Setup @@ -12235,6 +12251,9 @@ fn test_feature_activation_loaded_programs_cache_preparation_phase() { let mut feature_set = FeatureSet::all_enabled(); feature_set.deactivate(&feature_set::disable_sbpf_v0_execution::id()); feature_set.deactivate(&feature_set::reenable_sbpf_v0_execution::id()); + if !formalize_loaded_transaction_data_size { + feature_set.deactivate(&feature_set::formalize_loaded_transaction_data_size::id()); + } bank.feature_set = Arc::new(feature_set); let (root_bank, bank_forks) = bank.wrap_with_bank_forks_for_tests(); @@ -13384,9 +13403,9 @@ fn test_deploy_last_epoch_slot() { assert_eq!(result_with_feature_enabled, Ok(())); } -// HANA this breaks, expects UnsupportedProgramId but we produce InvalidProgramForExecution -#[test] -fn test_loader_v3_to_v4_migration() { +#[test_case(false; "informal_loaded_size")] +#[test_case(true; "simd186_loaded_size")] +fn test_loader_v3_to_v4_migration(formalize_loaded_transaction_data_size: bool) { solana_logger::setup(); // Bank Setup @@ -13397,6 +13416,9 @@ fn test_loader_v3_to_v4_migration() { ); let mut bank = Bank::new_for_tests(&genesis_config); bank.activate_feature(&feature_set::remove_accounts_executable_flag_checks::id()); + if formalize_loaded_transaction_data_size { + bank.activate_feature(&feature_set::formalize_loaded_transaction_data_size::id()); + } let (bank, bank_forks) = bank.wrap_with_bank_forks_for_tests(); let fee_calculator = genesis_config.fee_rate_governor.create_fee_calculator(); let mut next_slot = 1; From d7d7dc2863142ddbb2fb2d7816fbe4bf20d5e107 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Tue, 13 May 2025 06:01:05 -0700 Subject: [PATCH 11/14] add test, cleanup --- Cargo.toml | 2 +- svm/src/account_loader.rs | 200 +++++++++++++++++++++++++++++++++++--- 2 files changed, 185 insertions(+), 17 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 5352eb9b884..e8adb835d3d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -180,7 +180,7 @@ check-cfg = [ ] [workspace.lints.rust] -# XXX HANA warnings = "deny" +warnings = "deny" # Clippy lint configuration that can not be applied in clippy.toml [workspace.lints.clippy] diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index ee7b3fd2507..44807abf908 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -779,22 +779,6 @@ fn load_transaction_accounts_old( } if !validated_loaders.contains(owner_id) { - // NOTE there are several feature gate activations that affect this code: - // * `remove_accounts_executable_flag_checks`: this implicitly makes system, vote, stake, et al valid loaders - // it is impossible to mark an account executable and also have it be owned by one of them - // so, with the feature disabled, we always fail the executable check if they are a program id owner - // however, with the feature enabled, any account owned by an account owned by native loader is a "program" - // this is benign (any such transaction will fail at execution) but it affects which transactions pay fees - // * `enable_transaction_loading_failure_fees`: loading failures behave the same as execution failures - // at this point we can restrict valid loaders to those contained in `PROGRAM_OWNERS` - // since any other pseudo-loader owner is destined to fail at execution - // * SIMD-186: explicitly defines a sensible transaction data size algorithm - // at this point we stop counting loaders toward transaction data size entirely - // - // when _all three_ of `remove_accounts_executable_flag_checks`, `enable_transaction_loading_failure_fees`, - // and SIMD-186 are active, we do not need to load loaders at all to comply with consensus rules - // we may verify program ids are owned by `PROGRAM_OWNERS` purely as an optimization - // this could even be done before loading the rest of the accounts for a transaction if let Some(owner_account) = account_loader.load_account(owner_id) { if !native_loader::check_id(owner_account.owner()) || (!account_loader @@ -938,6 +922,7 @@ mod tests { super::*, crate::transaction_account_state_info::TransactionAccountStateInfo, agave_reserved_account_keys::ReservedAccountKeys, + rand0_7::prelude::*, solana_account::{Account, AccountSharedData, ReadableAccount, WritableAccount}, solana_epoch_schedule::EpochSchedule, solana_hash::Hash, @@ -2979,4 +2964,187 @@ mod tests { assert_eq!(account_loader.load_account(&fee_payer), None); assert_eq!(account_loader.get_account_shared_data(&fee_payer), None); } + + // note all magic numbers (how many accounts, how many instructions, how big to size buffers) are arbitrary + // other than trying not to swamp programs with blank accounts and keep transaction size below the 64mb limit + #[test_case(false; "executable_mandatory")] + #[test_case(true; "executable_optional")] + fn test_load_transaction_accounts_data_sizes_simd186( + remove_accounts_executable_flag_checks: bool, + ) { + let mut rng = rand0_7::thread_rng(); + let mut mock_bank = TestCallbacks::default(); + mock_bank.feature_set.remove_accounts_executable_flag_checks = + remove_accounts_executable_flag_checks; + + // arbitrary accounts + for _ in 0..128 { + let account = AccountSharedData::create( + 1, + vec![0; rng.gen_range(0, 128)], + Pubkey::new_unique(), + rng.gen(), + u64::MAX, + ); + mock_bank.accounts_map.insert(Pubkey::new_unique(), account); + } + + // fee-payers + let mut fee_payers = vec![]; + for _ in 0..8 { + let fee_payer = Pubkey::new_unique(); + let account = AccountSharedData::create( + LAMPORTS_PER_SOL, + vec![0; rng.gen_range(0, 32)], + system_program::id(), + rng.gen(), + u64::MAX, + ); + mock_bank.accounts_map.insert(fee_payer, account); + fee_payers.push(fee_payer); + } + + // programs + let mut loader_owned_accounts = vec![]; + let mut programdata_tracker = AHashMap::new(); + for loader in PROGRAM_OWNERS { + for _ in 0..16 { + let program_id = Pubkey::new_unique(); + let mut account = AccountSharedData::create( + 1, + vec![0; rng.gen_range(0, 512)], + *loader, + !remove_accounts_executable_flag_checks || rng.gen(), + u64::MAX, + ); + + // give half loaderv3 accounts (if theyre long enough) a valid programdata + // a quarter a dead pointer and a quarter nothing + // we set executable like a program because after the flag is disabled... + // ...programdata and buffer accounts can be used as program ids without aborting loading + // this will always fail at execution but we are merely testing the data size accounting here + if *loader == bpf_loader_upgradeable::id() && account.data().len() >= 64 { + let programdata_address = Pubkey::new_unique(); + let has_programdata = rng.gen(); + + if has_programdata { + let programdata_account = AccountSharedData::create( + 1, + vec![0; rng.gen_range(0, 512)], + *loader, + !remove_accounts_executable_flag_checks || rng.gen(), + u64::MAX, + ); + programdata_tracker.insert( + program_id, + (programdata_address, programdata_account.data().len()), + ); + mock_bank + .accounts_map + .insert(programdata_address, programdata_account); + loader_owned_accounts.push(programdata_address); + } + + if has_programdata || rng.gen() { + account + .set_state(&UpgradeableLoaderState::Program { + programdata_address, + }) + .unwrap(); + } + } + + mock_bank.accounts_map.insert(program_id, account); + loader_owned_accounts.push(program_id); + } + } + + let mut all_accounts = mock_bank.accounts_map.keys().copied().collect::>(); + let mut account_loader = (&mock_bank).into(); + + // now generate arbitrary transactions using this accounts + // we ensure valid fee-payers and that all program ids are loader-owned + // otherwise any account can appear anywhere + // some edge cases we hope to hit (not necessarily all in every run): + // * programs used multiple times as program ids and/or normal accounts are counted once + // * loaderv3 programdata used explicitly zero one or multiple times is counted once + // * loaderv3 programs with missing programdata are allowed through + // * loaderv3 programdata used as program id does nothing weird + // * loaderv3 programdata used as a regular account does nothing weird + // * the programdata conditions hold regardless of ordering + for _ in 0..1024 { + let mut instructions = vec![]; + for _ in 0..rng.gen_range(1, 8) { + let mut accounts = vec![]; + for _ in 0..rng.gen_range(1, 16) { + all_accounts.shuffle(&mut rng); + let pubkey = all_accounts[0]; + + accounts.push(AccountMeta { + pubkey, + is_writable: rng.gen(), + is_signer: rng.gen() && rng.gen(), + }); + } + + loader_owned_accounts.shuffle(&mut rng); + let program_id = loader_owned_accounts[0]; + instructions.push(Instruction { + accounts, + program_id, + data: vec![], + }); + } + + fee_payers.shuffle(&mut rng); + let fee_payer = fee_payers[0]; + let fee_payer_account = mock_bank.accounts_map.get(&fee_payer).cloned().unwrap(); + + let transaction = SanitizedTransaction::from_transaction_for_tests( + Transaction::new_with_payer(&instructions, Some(&fee_payer)), + ); + + let mut expected_size = 0; + let mut counted_programdatas = transaction + .account_keys() + .iter() + .copied() + .collect::>(); + + for pubkey in transaction.account_keys().iter() { + let account = mock_bank.accounts_map.get(pubkey).unwrap(); + expected_size += TRANSACTION_ACCOUNT_BASE_SIZE + account.data().len(); + + if let Some((programdata_address, programdata_size)) = + programdata_tracker.get(pubkey) + { + if counted_programdatas.get(programdata_address).is_none() { + expected_size += TRANSACTION_ACCOUNT_BASE_SIZE + programdata_size; + counted_programdatas.insert(*programdata_address); + } + } + } + + assert!(expected_size <= MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES.get() as usize); + + let loaded_transaction_accounts = load_transaction_accounts( + &mut account_loader, + &transaction, + LoadedTransactionAccount { + loaded_size: TRANSACTION_ACCOUNT_BASE_SIZE + fee_payer_account.data().len(), + account: fee_payer_account, + rent_collected: 0, + }, + MAX_LOADED_ACCOUNTS_DATA_SIZE_BYTES, + &mut TransactionErrorMetrics::default(), + &RentCollector::default(), + ) + .unwrap(); + + assert_eq!( + loaded_transaction_accounts.loaded_accounts_data_size, + expected_size as u32, + ); + } + } } From 1fff4c01b9228e3612654616a34e20010b46afeb Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Fri, 30 May 2025 04:38:42 -0700 Subject: [PATCH 12/14] cover new accounts in test --- svm/src/account_loader.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 44807abf908..82869d8400c 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -3060,6 +3060,13 @@ mod tests { } let mut all_accounts = mock_bank.accounts_map.keys().copied().collect::>(); + + // append some to-be-created accounts + // this is to test that their size is 0 rather than 64 + for _ in 0..32 { + all_accounts.push(Pubkey::new_unique()); + } + let mut account_loader = (&mock_bank).into(); // now generate arbitrary transactions using this accounts @@ -3112,8 +3119,9 @@ mod tests { .collect::>(); for pubkey in transaction.account_keys().iter() { - let account = mock_bank.accounts_map.get(pubkey).unwrap(); - expected_size += TRANSACTION_ACCOUNT_BASE_SIZE + account.data().len(); + if let Some(account) = mock_bank.accounts_map.get(pubkey) { + expected_size += TRANSACTION_ACCOUNT_BASE_SIZE + account.data().len(); + }; if let Some((programdata_address, programdata_size)) = programdata_tracker.get(pubkey) From 34e58c99ee0f90d5de544151e1d88be975b4055c Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Sun, 1 Jun 2025 23:28:07 -0700 Subject: [PATCH 13/14] make usage of native loader error early --- runtime/src/bank/tests.rs | 42 +++++++++++++------- svm/src/account_loader.rs | 80 ++++++++++++++++++++++++++------------- 2 files changed, 82 insertions(+), 40 deletions(-) diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index e0bca2c6219..58f0ed0a1c9 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -8400,13 +8400,16 @@ fn test_program_is_native_loader(formalize_loaded_transaction_data_size: bool) { &[&mint_keypair], bank.last_blockhash(), ); - assert_eq!( - bank.process_transaction(&tx), - Err(TransactionError::InstructionError( - 0, - InstructionError::UnsupportedProgramId - )) - ); + + let err = bank.process_transaction(&tx).unwrap_err(); + if formalize_loaded_transaction_data_size { + assert_eq!(err, TransactionError::InvalidProgramForExecution,); + } else { + assert_eq!( + err, + TransactionError::InstructionError(0, InstructionError::UnsupportedProgramId) + ); + } } #[test_case(false; "informal_loaded_size")] @@ -10577,8 +10580,9 @@ fn test_calculate_fee_secp256k1() { assert_eq!(calculate_test_fee(&message, 1, &fee_structure,), 11); } -#[test] -fn test_an_empty_instruction_without_program() { +#[test_case(false; "informal_loaded_size")] +#[test_case(true; "simd186_loaded_size")] +fn test_an_empty_instruction_without_program(formalize_loaded_transaction_data_size: bool) { let (genesis_config, mint_keypair) = create_genesis_config_no_tx_fee_no_rent(1); let destination = solana_pubkey::new_rand(); let mut ix = system_instruction::transfer(&mint_keypair.pubkey(), &destination, 0); @@ -10586,11 +10590,21 @@ fn test_an_empty_instruction_without_program() { let message = Message::new(&[ix], Some(&mint_keypair.pubkey())); let tx = Transaction::new(&[&mint_keypair], message, genesis_config.hash()); - let (bank, _bank_forks) = Bank::new_with_bank_forks_for_tests(&genesis_config); - assert_eq!( - bank.process_transaction(&tx).unwrap_err(), - TransactionError::InstructionError(0, InstructionError::UnsupportedProgramId), - ); + let mut bank = Bank::new_for_tests(&genesis_config); + if !formalize_loaded_transaction_data_size { + bank.deactivate_feature(&feature_set::formalize_loaded_transaction_data_size::id()); + } + let (bank, _bank_forks) = bank.wrap_with_bank_forks_for_tests(); + + let err = bank.process_transaction(&tx).unwrap_err(); + if formalize_loaded_transaction_data_size { + assert_eq!(err, TransactionError::InvalidProgramForExecution,); + } else { + assert_eq!( + err, + TransactionError::InstructionError(0, InstructionError::UnsupportedProgramId) + ); + } } #[test] diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 82869d8400c..3517d39f529 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -664,8 +664,8 @@ fn load_transaction_accounts_simd186( for (program_id, instruction) in message.program_instructions_iter() { if native_loader::check_id(program_id) { - loaded_transaction_accounts.program_indices.push(vec![]); - continue; + error_metrics.invalid_program_for_execution += 1; + return Err(TransactionError::InvalidProgramForExecution); } let Some(program_account) = account_loader.load_account(program_id) else { @@ -1144,14 +1144,25 @@ mod tests { assert_eq!(error_metrics.account_not_found.0, 0); match &loaded_accounts { - TransactionLoadResult::Loaded(loaded_transaction) => { + TransactionLoadResult::Loaded(loaded_transaction) + if !formalize_loaded_transaction_data_size => + { + assert_eq!(error_metrics.invalid_program_for_execution.0, 0); assert_eq!(loaded_transaction.accounts.len(), 3); assert_eq!(loaded_transaction.accounts[0].1, accounts[0].1); assert_eq!(loaded_transaction.program_indices.len(), 1); assert_eq!(loaded_transaction.program_indices[0].len(), 0); } - TransactionLoadResult::FeesOnly(fees_only_tx) => panic!("{}", fees_only_tx.load_error), - TransactionLoadResult::NotLoaded(e) => panic!("{e}"), + TransactionLoadResult::FeesOnly(fees_only_tx) + if formalize_loaded_transaction_data_size => + { + assert_eq!(error_metrics.invalid_program_for_execution.0, 1); + assert_eq!( + fees_only_tx.load_error, + TransactionError::InvalidProgramForExecution + ); + } + result => panic!("unexpected result: {:?}", result), } } @@ -1391,17 +1402,28 @@ mod tests { let keypair = Keypair::new(); let account = AccountSharedData::new(1_000_000, 0, &Pubkey::default()); + let mut program_account = AccountSharedData::default(); + program_account.set_lamports(1); + program_account.set_executable(true); + program_account.set_owner(native_loader::id()); + let instructions = vec![CompiledInstruction::new(2, &(), vec![0])]; let tx = Transaction::new_with_compiled_instructions( &[&keypair], &[slot_history_id], Hash::default(), - vec![native_loader::id()], + vec![bpf_loader::id()], instructions, ); - let loaded_accounts = - load_accounts_no_store(&[(keypair.pubkey(), account)], tx, Some(&account_overrides)); + let loaded_accounts = load_accounts_no_store( + &[ + (keypair.pubkey(), account), + (bpf_loader::id(), program_account), + ], + tx, + Some(&account_overrides), + ); match &loaded_accounts { TransactionLoadResult::Loaded(loaded_transaction) => { assert_eq!(loaded_transaction.accounts[0].0, keypair.pubkey()); @@ -1718,24 +1740,30 @@ mod tests { &RentCollector::default(), ); - let loaded_accounts_data_size = base_account_size as u32 * 2; - - assert_eq!( - result.unwrap(), - LoadedTransactionAccounts { - accounts: vec![ - (key1.pubkey(), fee_payer_account), - ( - native_loader::id(), - mock_bank.accounts_map[&native_loader::id()].clone() - ) - ], - program_indices: vec![vec![]], - rent: 0, - rent_debits: RentDebits::default(), - loaded_accounts_data_size, - } - ); + if formalize_loaded_transaction_data_size { + assert_eq!( + result.unwrap_err(), + TransactionError::InvalidProgramForExecution + ); + } else { + let loaded_accounts_data_size = base_account_size as u32 * 2; + assert_eq!( + result.unwrap(), + LoadedTransactionAccounts { + accounts: vec![ + (key1.pubkey(), fee_payer_account), + ( + native_loader::id(), + mock_bank.accounts_map[&native_loader::id()].clone() + ) + ], + program_indices: vec![vec![]], + rent: 0, + rent_debits: RentDebits::default(), + loaded_accounts_data_size, + } + ); + } } #[test] From b7f5ed5da88a023e026e5a1476abf32ad585d423 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 4 Jun 2025 00:19:02 -0700 Subject: [PATCH 14/14] remove native loader special case --- runtime/src/bank/tests.rs | 4 ++-- svm/src/account_loader.rs | 14 ++++---------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 58f0ed0a1c9..9efcb3cd466 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -8403,7 +8403,7 @@ fn test_program_is_native_loader(formalize_loaded_transaction_data_size: bool) { let err = bank.process_transaction(&tx).unwrap_err(); if formalize_loaded_transaction_data_size { - assert_eq!(err, TransactionError::InvalidProgramForExecution,); + assert_eq!(err, TransactionError::ProgramAccountNotFound); } else { assert_eq!( err, @@ -10598,7 +10598,7 @@ fn test_an_empty_instruction_without_program(formalize_loaded_transaction_data_s let err = bank.process_transaction(&tx).unwrap_err(); if formalize_loaded_transaction_data_size { - assert_eq!(err, TransactionError::InvalidProgramForExecution,); + assert_eq!(err, TransactionError::ProgramAccountNotFound); } else { assert_eq!( err, diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 3517d39f529..7b367e3aca4 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -663,11 +663,6 @@ fn load_transaction_accounts_simd186( } for (program_id, instruction) in message.program_instructions_iter() { - if native_loader::check_id(program_id) { - error_metrics.invalid_program_for_execution += 1; - return Err(TransactionError::InvalidProgramForExecution); - } - let Some(program_account) = account_loader.load_account(program_id) else { error_metrics.account_not_found += 1; return Err(TransactionError::ProgramAccountNotFound); @@ -1142,12 +1137,11 @@ mod tests { feature_set, ); - assert_eq!(error_metrics.account_not_found.0, 0); match &loaded_accounts { TransactionLoadResult::Loaded(loaded_transaction) if !formalize_loaded_transaction_data_size => { - assert_eq!(error_metrics.invalid_program_for_execution.0, 0); + assert_eq!(error_metrics.account_not_found.0, 0); assert_eq!(loaded_transaction.accounts.len(), 3); assert_eq!(loaded_transaction.accounts[0].1, accounts[0].1); assert_eq!(loaded_transaction.program_indices.len(), 1); @@ -1156,10 +1150,10 @@ mod tests { TransactionLoadResult::FeesOnly(fees_only_tx) if formalize_loaded_transaction_data_size => { - assert_eq!(error_metrics.invalid_program_for_execution.0, 1); + assert_eq!(error_metrics.account_not_found.0, 1); assert_eq!( fees_only_tx.load_error, - TransactionError::InvalidProgramForExecution + TransactionError::ProgramAccountNotFound, ); } result => panic!("unexpected result: {:?}", result), @@ -1743,7 +1737,7 @@ mod tests { if formalize_loaded_transaction_data_size { assert_eq!( result.unwrap_err(), - TransactionError::InvalidProgramForExecution + TransactionError::ProgramAccountNotFound, ); } else { let loaded_accounts_data_size = base_account_size as u32 * 2;