From 46493f8bd297678be5265556ab06ddd3fb440d0f Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Thu, 14 Nov 2024 03:53:42 -0800 Subject: [PATCH 1/5] svm: skip appending loaders to loaded accounts --- runtime/src/account_saver.rs | 2 +- svm/src/account_loader.rs | 85 ++++++++++++++++++++++-------------- 2 files changed, 54 insertions(+), 33 deletions(-) diff --git a/runtime/src/account_saver.rs b/runtime/src/account_saver.rs index 4d7be1150b89d1..fa4df885200d83 100644 --- a/runtime/src/account_saver.rs +++ b/runtime/src/account_saver.rs @@ -110,7 +110,7 @@ fn collect_accounts_for_successful_tx<'a, T: SVMMessage>( transaction_ref: Option<&'a SanitizedTransaction>, transaction_accounts: &'a [TransactionAccount], ) { - for (i, (address, account)) in (0..transaction.account_keys().len()).zip(transaction_accounts) { + for (i, (address, account)) in transaction_accounts.iter().enumerate() { if !transaction.is_writable(i) { continue; } diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 091351d1889b70..7a201d52e43bdd 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -12,7 +12,7 @@ use { solana_feature_set::{self as feature_set, FeatureSet}, solana_program_runtime::loaded_programs::ProgramCacheForTxBatch, solana_sdk::{ - account::{Account, AccountSharedData, ReadableAccount, WritableAccount}, + account::{Account, AccountSharedData, ReadableAccount, WritableAccount, PROGRAM_OWNERS}, fee::FeeDetails, native_loader, nonce::State as NonceState, @@ -32,7 +32,11 @@ use { solana_svm_rent_collector::svm_rent_collector::SVMRentCollector, solana_svm_transaction::svm_message::SVMMessage, solana_system_program::{get_system_account_kind, SystemAccountKind}, - std::{collections::HashMap, num::NonZeroU32, sync::Arc}, + std::{ + collections::{HashMap, HashSet}, + num::NonZeroU32, + sync::Arc, + }, }; // for the load instructions @@ -431,11 +435,11 @@ fn load_transaction_accounts( let mut tx_rent: TransactionRent = 0; let account_keys = message.account_keys(); let mut accounts = Vec::with_capacity(account_keys.len()); - let mut accounts_found = Vec::with_capacity(account_keys.len()); + let mut validated_loaders = HashSet::with_capacity(PROGRAM_OWNERS.len()); // TODO AHashSet let mut rent_debits = RentDebits::default(); let mut accumulated_accounts_data_size: u32 = 0; - let mut collect_loaded_account = |key, (loaded_account, found)| -> Result<()> { + let mut collect_loaded_account = |key, loaded_account| -> Result<()> { let LoadedTransactionAccount { account, loaded_size, @@ -453,29 +457,25 @@ fn load_transaction_accounts( rent_debits.insert(key, rent_collected, account.lamports()); accounts.push((*key, account)); - accounts_found.push(found); Ok(()) }; - // Since the fee payer is always the first account, collect it first. Note - // that account overrides are already applied during fee payer validation so - // it's fine to use the fee payer directly here rather than checking account - // overrides again. - collect_loaded_account(message.fee_payer(), (loaded_fee_payer_account, true))?; + // 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, account_found) = load_transaction_account( + let loaded_account = load_transaction_account( account_loader, message, account_key, account_index, rent_collector, - )?; - collect_loaded_account(account_key, (loaded_account, account_found))?; + ); + collect_loaded_account(account_key, loaded_account)?; } - let builtins_start_index = accounts.len(); let program_indices = message .instructions_iter() .map(|instruction| { @@ -489,7 +489,9 @@ fn load_transaction_accounts( return Ok(account_indices); } - let account_found = accounts_found.get(program_index).unwrap_or(&true); + let account_found = account_loader + .load_account(program_id, AccountUsagePattern::ReadOnlyInvisible) + .is_some(); if !account_found { error_metrics.account_not_found += 1; return Err(TransactionError::ProgramAccountNotFound); @@ -508,17 +510,38 @@ fn load_transaction_accounts( if native_loader::check_id(owner_id) { return Ok(account_indices); } - if !accounts - .get(builtins_start_index..) - .ok_or(TransactionError::ProgramAccountNotFound)? - .iter() - .any(|(key, _)| key == owner_id) - { - // NOTE this will be changed to a `load_account()` call in a PR immediately following this one - // we want to stop pushing loaders on the accounts vec, but tests need to change if we do that - // and this PR is complex enough that we want to code review any new behaviors separately - if let Some(owner_account) = - account_loader.callbacks.get_account_shared_data(owner_id) + if !validated_loaders.contains(owner_id) { + // NOTE we load as `ReadOnlyInstruction` to bypass the program cache + // since the program cache would incorrectly mark a user-created native-owned account as executable + // this is just to be careful; an account owned by a native-owned non-program cannot be set executable + // + // there are a panopoly of fetaure gate activations that affect this code, in generally benign ways: + // * `remove_accounts_executable_flag_checks`: incorrect executable flag from program cache no longer matters + // we should still avoid program cache because it changes transaction size + // albeit in a consensus-safe manner because it would result from feature activation + // * `disable_account_loader_special_case`: program cache codepath goes away entirely + // at this point the instruction vs invisible distinction ceases to affect loading + // however we might not remove it, because... + // * SIMD-163: allows non-instruction account keys to be used for CPI + // the instruction/invisible distinction may be useful again, to optimize loading, but maybe not + // * `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 + // in theory we could do this now with no change in behavior + // but then the behavior _would_ change when `remove_accounts_executable_flag_checks` activates + // * 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(LoadedTransactionAccount { + account: owner_account, + loaded_size: owner_size, + .. + }) = + account_loader.load_account(owner_id, AccountUsagePattern::ReadOnlyInstruction) { if !native_loader::check_id(owner_account.owner()) || (!account_loader @@ -531,11 +554,11 @@ fn load_transaction_accounts( } accumulate_and_check_loaded_account_data_size( &mut accumulated_accounts_data_size, - owner_account.data().len(), + owner_size, compute_budget_limits.loaded_accounts_bytes, error_metrics, )?; - accounts.push((*owner_id, owner_account)); + validated_loaders.insert(*owner_id); } else { error_metrics.account_not_found += 1; return Err(TransactionError::ProgramAccountNotFound); @@ -560,8 +583,7 @@ fn load_transaction_account( account_key: &Pubkey, account_index: usize, rent_collector: &dyn SVMRentCollector, -) -> Result<(LoadedTransactionAccount, bool)> { - let mut account_found = true; +) -> LoadedTransactionAccount { let usage_pattern = AccountUsagePattern::new(message, account_index); let loaded_account = if solana_sdk::sysvar::instructions::check_id(account_key) { @@ -588,7 +610,6 @@ fn load_transaction_account( loaded_account } else { - account_found = false; let mut default_account = AccountSharedData::default(); // All new accounts must be rent-exempt (enforced in Bank::execute_loaded_transaction). // Currently, rent collection sets rent_epoch to u64::MAX, but initializing the account @@ -601,7 +622,7 @@ fn load_transaction_account( } }; - Ok((loaded_account, account_found)) + loaded_account } fn account_shared_data_from_program( From 017abacdd623f1cfcc1f1275fe4e1cf10c81448c Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Thu, 14 Nov 2024 04:03:45 -0800 Subject: [PATCH 2/5] fix tests --- svm/src/account_loader.rs | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 7a201d52e43bdd..b0c886edd9045a 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -1093,7 +1093,7 @@ mod tests { assert_eq!(error_metrics.account_not_found, 0); match &loaded_accounts { TransactionLoadResult::Loaded(loaded_transaction) => { - assert_eq!(loaded_transaction.accounts.len(), 4); + assert_eq!(loaded_transaction.accounts.len(), 3); assert_eq!(loaded_transaction.accounts[0].1, accounts[0].1); assert_eq!(loaded_transaction.program_indices.len(), 2); assert_eq!(loaded_transaction.program_indices[0], &[1]); @@ -1601,7 +1601,6 @@ mod tests { accounts: vec![ (account_keypair.pubkey(), account_data.clone()), (program_keypair.pubkey(), cached_program), - (loader_v2, loader_data), ], program_indices: vec![vec![1]], rent: 0, @@ -1982,10 +1981,6 @@ mod tests { key1.pubkey(), mock_bank.accounts_map[&key1.pubkey()].clone() ), - ( - key3.pubkey(), - mock_bank.accounts_map[&key3.pubkey()].clone() - ), ], program_indices: vec![vec![1]], rent: 0, @@ -2070,10 +2065,6 @@ mod tests { mock_bank.accounts_map[&key1.pubkey()].clone() ), (key4.pubkey(), account_data), - ( - key3.pubkey(), - mock_bank.accounts_map[&key3.pubkey()].clone() - ), ], program_indices: vec![vec![1], vec![1]], rent: 0, @@ -2232,10 +2223,6 @@ mod tests { mock_bank.accounts_map[&key1.pubkey()].clone() ), (key4.pubkey(), account_data), - ( - key3.pubkey(), - mock_bank.accounts_map[&key3.pubkey()].clone() - ), ], program_indices: vec![vec![1], vec![1]], fee_details: FeeDetails::default(), @@ -2284,7 +2271,7 @@ mod tests { assert!(matches!( load_result, TransactionLoadResult::FeesOnly(FeesOnlyTransaction { - load_error: TransactionError::InvalidProgramForExecution, + load_error: TransactionError::ProgramAccountNotFound, .. }), )); @@ -2400,7 +2387,7 @@ mod tests { let mut account3 = AccountSharedData::default(); account3.set_lamports(4_000_000_000); account3.set_executable(true); - account3.set_owner(native_loader::id()); + account3.set_owner(bpf_loader::id()); mock_bank.accounts_map.insert(address3, account3.clone()); let mut account_loader = (&mock_bank).into(); @@ -2456,11 +2443,17 @@ mod tests { .collect(); actual_inspected_accounts.sort_unstable_by(|a, b| a.0.cmp(&b.0)); + // TODO i think this goes back to normal once account cache is in + // remember i changed owner above let mut expected_inspected_accounts = vec![ // *not* key0, since it is loaded during fee payer validation (address1, vec![(Some(account1), true)]), (address2, vec![(None, true)]), - (address3, vec![(Some(account3), false)]), + ( + address3, + vec![(Some(account3.clone()), false), (Some(account3), false)], + ), + (bpf_loader::id(), vec![(None, false)]), ]; expected_inspected_accounts.sort_unstable_by(|a, b| a.0.cmp(&b.0)); From 88c5c63d4657cdeb7cda124a225332df2c7d0be4 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 20 Nov 2024 06:04:23 -0800 Subject: [PATCH 3/5] rebase on simd83 loader --- svm/src/account_loader.rs | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index b0c886edd9045a..ba358760470968 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -7,7 +7,7 @@ use { transaction_execution_result::ExecutedTransaction, transaction_processing_callback::{AccountState, TransactionProcessingCallback}, }, - ahash::AHashMap, + ahash::{AHashMap, AHashSet}, solana_compute_budget::compute_budget_limits::ComputeBudgetLimits, solana_feature_set::{self as feature_set, FeatureSet}, solana_program_runtime::loaded_programs::ProgramCacheForTxBatch, @@ -32,11 +32,7 @@ use { solana_svm_rent_collector::svm_rent_collector::SVMRentCollector, solana_svm_transaction::svm_message::SVMMessage, solana_system_program::{get_system_account_kind, SystemAccountKind}, - std::{ - collections::{HashMap, HashSet}, - num::NonZeroU32, - sync::Arc, - }, + std::{collections::HashMap, num::NonZeroU32, sync::Arc}, }; // for the load instructions @@ -252,7 +248,7 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> { message: &impl SVMMessage, transaction_accounts: &[TransactionAccount], ) { - for (i, (address, account)) in (0..message.account_keys().len()).zip(transaction_accounts) { + for (i, (address, account)) in transaction_accounts.iter().enumerate() { if !message.is_writable(i) { continue; } @@ -435,7 +431,7 @@ fn load_transaction_accounts( let mut tx_rent: TransactionRent = 0; let account_keys = message.account_keys(); let mut accounts = Vec::with_capacity(account_keys.len()); - let mut validated_loaders = HashSet::with_capacity(PROGRAM_OWNERS.len()); // TODO AHashSet + let mut validated_loaders = AHashSet::with_capacity(PROGRAM_OWNERS.len()); let mut rent_debits = RentDebits::default(); let mut accumulated_accounts_data_size: u32 = 0; @@ -511,9 +507,9 @@ fn load_transaction_accounts( return Ok(account_indices); } if !validated_loaders.contains(owner_id) { - // NOTE we load as `ReadOnlyInstruction` to bypass the program cache + // NOTE we load the program owner as `ReadOnlyInstruction` to bypass the program cache // since the program cache would incorrectly mark a user-created native-owned account as executable - // this is just to be careful; an account owned by a native-owned non-program cannot be set executable + // this preserves consensus until `disable_account_loader_special_case` is active, after which it doesnt matter // // there are a panopoly of fetaure gate activations that affect this code, in generally benign ways: // * `remove_accounts_executable_flag_checks`: incorrect executable flag from program cache no longer matters @@ -521,14 +517,10 @@ fn load_transaction_accounts( // albeit in a consensus-safe manner because it would result from feature activation // * `disable_account_loader_special_case`: program cache codepath goes away entirely // at this point the instruction vs invisible distinction ceases to affect loading - // however we might not remove it, because... - // * SIMD-163: allows non-instruction account keys to be used for CPI - // the instruction/invisible distinction may be useful again, to optimize loading, but maybe not + // keeping the distinction may be useful for SIMD-163 (cpi caller restriction), but maybe not // * `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 - // in theory we could do this now with no change in behavior - // but then the behavior _would_ change when `remove_accounts_executable_flag_checks` activates // * SIMD-186: explicitly defines a sensible transaction data size algorithm // at this point we stop counting loaders toward transaction data size entirely // @@ -1770,6 +1762,7 @@ mod tests { let mut mock_bank = TestCallbacks::default(); let mut account_data = AccountSharedData::default(); account_data.set_owner(native_loader::id()); + account_data.set_lamports(1); account_data.set_executable(true); mock_bank.accounts_map.insert(key1.pubkey(), account_data); @@ -1883,6 +1876,7 @@ mod tests { let sanitized_message = new_unchecked_sanitized_message(message); let mut mock_bank = TestCallbacks::default(); let mut account_data = AccountSharedData::default(); + account_data.set_lamports(1); account_data.set_executable(true); account_data.set_owner(key3.pubkey()); mock_bank.accounts_map.insert(key1.pubkey(), account_data); @@ -1937,6 +1931,7 @@ mod tests { let sanitized_message = new_unchecked_sanitized_message(message); let mut mock_bank = TestCallbacks::default(); let mut account_data = AccountSharedData::default(); + account_data.set_lamports(1); account_data.set_executable(true); account_data.set_owner(key3.pubkey()); mock_bank.accounts_map.insert(key1.pubkey(), account_data); @@ -1948,6 +1943,7 @@ mod tests { .insert(key2.pubkey(), fee_payer_account.clone()); let mut account_data = AccountSharedData::default(); + 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); @@ -2018,6 +2014,7 @@ mod tests { let sanitized_message = new_unchecked_sanitized_message(message); let mut mock_bank = TestCallbacks::default(); let mut account_data = AccountSharedData::default(); + account_data.set_lamports(1); account_data.set_executable(true); account_data.set_owner(key3.pubkey()); mock_bank.accounts_map.insert(key1.pubkey(), account_data); @@ -2029,6 +2026,7 @@ mod tests { .insert(key2.pubkey(), fee_payer_account.clone()); let mut account_data = AccountSharedData::default(); + 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); @@ -2082,6 +2080,7 @@ mod tests { let last_block_hash = Hash::new_unique(); let mut system_data = AccountSharedData::default(); + system_data.set_lamports(1); system_data.set_executable(true); system_data.set_owner(native_loader::id()); bank.accounts_map @@ -2165,6 +2164,7 @@ mod tests { let sanitized_message = new_unchecked_sanitized_message(message); let mut mock_bank = TestCallbacks::default(); let mut account_data = AccountSharedData::default(); + account_data.set_lamports(1); account_data.set_executable(true); account_data.set_owner(key3.pubkey()); mock_bank.accounts_map.insert(key1.pubkey(), account_data); @@ -2176,6 +2176,7 @@ mod tests { .insert(key2.pubkey(), fee_payer_account.clone()); let mut account_data = AccountSharedData::default(); + 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); @@ -2443,8 +2444,6 @@ mod tests { .collect(); actual_inspected_accounts.sort_unstable_by(|a, b| a.0.cmp(&b.0)); - // TODO i think this goes back to normal once account cache is in - // remember i changed owner above let mut expected_inspected_accounts = vec![ // *not* key0, since it is loaded during fee payer validation (address1, vec![(Some(account1), true)]), From d369ad568df3276c4661613342b025a4650cc8f2 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Wed, 20 Nov 2024 06:39:10 -0800 Subject: [PATCH 4/5] stop scanning the accounts vec entirely --- svm/src/account_loader.rs | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index ba358760470968..0a27b5ae512255 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -473,25 +473,24 @@ fn load_transaction_accounts( } let program_indices = message - .instructions_iter() - .map(|instruction| { + .program_instructions_iter() + .map(|(program_id, instruction)| { let mut account_indices = Vec::with_capacity(2); - let program_index = instruction.program_id_index as usize; - // This command may never return error, because the transaction is sanitized - let (program_id, program_account) = accounts - .get(program_index) - .ok_or(TransactionError::ProgramAccountNotFound)?; if native_loader::check_id(program_id) { return Ok(account_indices); } - let account_found = account_loader - .load_account(program_id, AccountUsagePattern::ReadOnlyInvisible) - .is_some(); - if !account_found { + let program_index = instruction.program_id_index as usize; + let program_usage_pattern = AccountUsagePattern::new(message, program_index); + + let Some(LoadedTransactionAccount { + account: program_account, + .. + }) = account_loader.load_account(program_id, program_usage_pattern) + else { error_metrics.account_not_found += 1; return Err(TransactionError::ProgramAccountNotFound); - } + }; if !account_loader .feature_set @@ -502,10 +501,12 @@ fn load_transaction_accounts( return Err(TransactionError::InvalidProgramForExecution); } account_indices.insert(0, program_index as IndexOfAccount); + let owner_id = program_account.owner(); if native_loader::check_id(owner_id) { return Ok(account_indices); } + if !validated_loaders.contains(owner_id) { // NOTE we load the program owner as `ReadOnlyInstruction` to bypass the program cache // since the program cache would incorrectly mark a user-created native-owned account as executable From 67b42507c7401793c2e96c335778ed3dc8a13413 Mon Sep 17 00:00:00 2001 From: hana <81144685+2501babe@users.noreply.github.com> Date: Mon, 25 Nov 2024 00:02:39 -0800 Subject: [PATCH 5/5] revert account keys length logic --- runtime/src/account_saver.rs | 2 +- svm/src/account_loader.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/src/account_saver.rs b/runtime/src/account_saver.rs index fa4df885200d83..4d7be1150b89d1 100644 --- a/runtime/src/account_saver.rs +++ b/runtime/src/account_saver.rs @@ -110,7 +110,7 @@ fn collect_accounts_for_successful_tx<'a, T: SVMMessage>( transaction_ref: Option<&'a SanitizedTransaction>, transaction_accounts: &'a [TransactionAccount], ) { - for (i, (address, account)) in transaction_accounts.iter().enumerate() { + for (i, (address, account)) in (0..transaction.account_keys().len()).zip(transaction_accounts) { if !transaction.is_writable(i) { continue; } diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 0a27b5ae512255..c64b7f38527c9c 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -248,7 +248,7 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> { message: &impl SVMMessage, transaction_accounts: &[TransactionAccount], ) { - for (i, (address, account)) in transaction_accounts.iter().enumerate() { + for (i, (address, account)) in (0..message.account_keys().len()).zip(transaction_accounts) { if !message.is_writable(i) { continue; }