From 829368a263ec38c6dbfd59bc8f7366c2e124fc06 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Tue, 23 Jul 2024 13:42:22 +0000 Subject: [PATCH 1/4] refactor: load transaction accounts --- svm/src/account_loader.rs | 58 +++++++++++++++++++++--------------- svm/src/account_overrides.rs | 4 ++- 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index b62833e404004b..5914c99e39635d 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -208,30 +208,42 @@ fn load_transaction_accounts( .unique() .collect::>(); + let mut fee_payer_account = Some(( + tx_details.fee_payer_account.data().len(), + tx_details.fee_payer_account, + tx_details.fee_payer_rent_debit, + )); + let mut accounts = account_keys .iter() .enumerate() .map(|(i, key)| { let mut account_found = true; - #[allow(clippy::collapsible_else_if)] - let account = if solana_sdk::sysvar::instructions::check_id(key) { - construct_instructions_account(message) - } else { - let is_fee_payer = i == 0; - let instruction_account = u8::try_from(i) - .map(|i| instruction_accounts.contains(&&i)) - .unwrap_or(false); - let (account_size, account, rent) = if is_fee_payer { + let is_instruction_account = u8::try_from(i) + .map(|i| instruction_accounts.contains(&&i)) + .unwrap_or(false); + let (account_size, account, rent) = + if let Some(fee_payer_account) = fee_payer_account.take() { + // Since the fee payer is always the first account, load 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. + fee_payer_account + } else if solana_sdk::sysvar::instructions::check_id(key) { + // Since the instructions sysvar is constructed by the SVM + // and modified for each transaction instruction, it cannot + // be overridden. ( - tx_details.fee_payer_account.data().len(), - tx_details.fee_payer_account.clone(), - tx_details.fee_payer_rent_debit, + 0, /* loaded size */ + construct_instructions_account(message), + 0, /* collected rent */ ) } else if let Some(account_override) = account_overrides.and_then(|overrides| overrides.get(key)) { (account_override.data().len(), account_override.clone(), 0) - } else if let Some(program) = (!instruction_account && !message.is_writable(i)) + } else if let Some(program) = (!is_instruction_account && !message.is_writable(i)) .then_some(()) .and_then(|_| loaded_programs.find(key)) { @@ -270,18 +282,16 @@ fn load_transaction_accounts( (default_account.data().len(), default_account, 0) }) }; - accumulate_and_check_loaded_account_data_size( - &mut accumulated_accounts_data_size, - account_size, - tx_details.compute_budget_limits.loaded_accounts_bytes, - error_metrics, - )?; - tx_rent += rent; - rent_debits.insert(key, rent, account.lamports()); - - account - }; + accumulate_and_check_loaded_account_data_size( + &mut accumulated_accounts_data_size, + account_size, + tx_details.compute_budget_limits.loaded_accounts_bytes, + error_metrics, + )?; + + tx_rent += rent; + rent_debits.insert(key, rent, account.lamports()); accounts_found.push(account_found); Ok((*key, account)) diff --git a/svm/src/account_overrides.rs b/svm/src/account_overrides.rs index 8a205a798f66b1..7628b82f85d88e 100644 --- a/svm/src/account_overrides.rs +++ b/svm/src/account_overrides.rs @@ -3,7 +3,9 @@ use { std::collections::HashMap, }; -/// Encapsulates overridden accounts, typically used for transaction simulations +/// Encapsulates overridden accounts, typically used for transaction +/// simulations. Account overrides are currently not used when loading the +/// durable nonce account or when constructing the instructions sysvar account. #[derive(Default)] pub struct AccountOverrides { accounts: HashMap, From f38ba7a55b066283da869bffd9ed1f263545bcd7 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Thu, 8 Aug 2024 01:39:37 +0000 Subject: [PATCH 2/4] feedback --- svm/src/account_loader.rs | 159 +++++++++++++++++++------------------- 1 file changed, 80 insertions(+), 79 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 5914c99e39635d..5405ac69ceb5b2 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -198,6 +198,7 @@ fn load_transaction_accounts( ) -> Result { 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 rent_debits = RentDebits::default(); let mut accumulated_accounts_data_size: u32 = 0; @@ -208,81 +209,8 @@ fn load_transaction_accounts( .unique() .collect::>(); - let mut fee_payer_account = Some(( - tx_details.fee_payer_account.data().len(), - tx_details.fee_payer_account, - tx_details.fee_payer_rent_debit, - )); - - let mut accounts = account_keys - .iter() - .enumerate() - .map(|(i, key)| { - let mut account_found = true; - let is_instruction_account = u8::try_from(i) - .map(|i| instruction_accounts.contains(&&i)) - .unwrap_or(false); - let (account_size, account, rent) = - if let Some(fee_payer_account) = fee_payer_account.take() { - // Since the fee payer is always the first account, load 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. - fee_payer_account - } else if solana_sdk::sysvar::instructions::check_id(key) { - // Since the instructions sysvar is constructed by the SVM - // and modified for each transaction instruction, it cannot - // be overridden. - ( - 0, /* loaded size */ - construct_instructions_account(message), - 0, /* collected rent */ - ) - } else if let Some(account_override) = - account_overrides.and_then(|overrides| overrides.get(key)) - { - (account_override.data().len(), account_override.clone(), 0) - } else if let Some(program) = (!is_instruction_account && !message.is_writable(i)) - .then_some(()) - .and_then(|_| loaded_programs.find(key)) - { - callbacks - .get_account_shared_data(key) - .ok_or(TransactionError::AccountNotFound)?; - // Optimization to skip loading of accounts which are only used as - // programs in top-level instructions and not passed as instruction accounts. - let program_account = account_shared_data_from_program(&program); - (program.account_size, program_account, 0) - } else { - callbacks - .get_account_shared_data(key) - .map(|mut account| { - if message.is_writable(i) { - let rent_due = collect_rent_from_account( - feature_set, - rent_collector, - key, - &mut account, - ) - .rent_amount; - - (account.data().len(), account, rent_due) - } else { - (account.data().len(), account, 0) - } - }) - .unwrap_or_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 - // with this field already set would allow us to skip rent collection for these accounts. - default_account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); - (default_account.data().len(), default_account, 0) - }) - }; - + let mut collect_account = + |key: &Pubkey, account_size: usize, account: AccountSharedData, rent: u64| -> Result<()> { accumulate_and_check_loaded_account_data_size( &mut accumulated_accounts_data_size, account_size, @@ -293,10 +221,83 @@ fn load_transaction_accounts( tx_rent += rent; rent_debits.insert(key, rent, account.lamports()); - accounts_found.push(account_found); - Ok((*key, account)) - }) - .collect::>>()?; + accounts.push((*key, account)); + accounts_found.push(true); + 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_account( + message.fee_payer(), + tx_details.fee_payer_account.data().len(), + tx_details.fee_payer_account, + tx_details.fee_payer_rent_debit, + )?; + + // Attempt to load and collect remaining non-fee payer accounts + for (i, key) in account_keys.iter().enumerate().skip(1) { + let mut account_found = true; + let is_instruction_account = u8::try_from(i) + .map(|i| instruction_accounts.contains(&&i)) + .unwrap_or(false); + let (account_size, account, rent) = if solana_sdk::sysvar::instructions::check_id(key) { + // Since the instructions sysvar is constructed by the SVM + // and modified for each transaction instruction, it cannot + // be overridden. + ( + 0, /* loaded size */ + construct_instructions_account(message), + 0, /* collected rent */ + ) + } else if let Some(account_override) = + account_overrides.and_then(|overrides| overrides.get(key)) + { + (account_override.data().len(), account_override.clone(), 0) + } else if let Some(program) = (!is_instruction_account && !message.is_writable(i)) + .then_some(()) + .and_then(|_| loaded_programs.find(key)) + { + callbacks + .get_account_shared_data(key) + .ok_or(TransactionError::AccountNotFound)?; + // Optimization to skip loading of accounts which are only used as + // programs in top-level instructions and not passed as instruction accounts. + let program_account = account_shared_data_from_program(&program); + (program.account_size, program_account, 0) + } else { + callbacks + .get_account_shared_data(key) + .map(|mut account| { + if message.is_writable(i) { + let rent_due = collect_rent_from_account( + feature_set, + rent_collector, + key, + &mut account, + ) + .rent_amount; + + (account.data().len(), account, rent_due) + } else { + (account.data().len(), account, 0) + } + }) + .unwrap_or_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 + // with this field already set would allow us to skip rent collection for these accounts. + default_account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH); + (default_account.data().len(), default_account, 0) + }) + }; + + collect_account(key, account_size, account, rent)?; + } let builtins_start_index = accounts.len(); let program_indices = message From aa46dda4719e1e7046e793e625094e0465c7df2a Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Thu, 8 Aug 2024 01:45:54 +0000 Subject: [PATCH 3/4] fix account found --- svm/src/account_loader.rs | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 5405ac69ceb5b2..966de14867f689 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -209,22 +209,21 @@ fn load_transaction_accounts( .unique() .collect::>(); - let mut collect_account = - |key: &Pubkey, account_size: usize, account: AccountSharedData, rent: u64| -> Result<()> { - accumulate_and_check_loaded_account_data_size( - &mut accumulated_accounts_data_size, - account_size, - tx_details.compute_budget_limits.loaded_accounts_bytes, - error_metrics, - )?; - - tx_rent += rent; - rent_debits.insert(key, rent, account.lamports()); - - accounts.push((*key, account)); - accounts_found.push(true); - Ok(()) - }; + let mut collect_account = |key, account_size, account, rent, account_found| -> Result<()> { + accumulate_and_check_loaded_account_data_size( + &mut accumulated_accounts_data_size, + account_size, + tx_details.compute_budget_limits.loaded_accounts_bytes, + error_metrics, + )?; + + tx_rent += rent; + rent_debits.insert(key, rent, account.lamports()); + + accounts.push((*key, account)); + accounts_found.push(account_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 @@ -235,6 +234,7 @@ fn load_transaction_accounts( tx_details.fee_payer_account.data().len(), tx_details.fee_payer_account, tx_details.fee_payer_rent_debit, + true, // account_found )?; // Attempt to load and collect remaining non-fee payer accounts @@ -296,7 +296,7 @@ fn load_transaction_accounts( }) }; - collect_account(key, account_size, account, rent)?; + collect_account(key, account_size, account, rent, account_found)?; } let builtins_start_index = accounts.len(); From ae2791a221bcc9442a86bab263882beefff9df91 Mon Sep 17 00:00:00 2001 From: Justin Starry Date: Thu, 8 Aug 2024 02:04:33 +0000 Subject: [PATCH 4/4] fix clippy --- svm/src/account_loader.rs | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/svm/src/account_loader.rs b/svm/src/account_loader.rs index 966de14867f689..2b2e31aacb8fd1 100644 --- a/svm/src/account_loader.rs +++ b/svm/src/account_loader.rs @@ -209,21 +209,22 @@ fn load_transaction_accounts( .unique() .collect::>(); - let mut collect_account = |key, account_size, account, rent, account_found| -> Result<()> { - accumulate_and_check_loaded_account_data_size( - &mut accumulated_accounts_data_size, - account_size, - tx_details.compute_budget_limits.loaded_accounts_bytes, - error_metrics, - )?; - - tx_rent += rent; - rent_debits.insert(key, rent, account.lamports()); - - accounts.push((*key, account)); - accounts_found.push(account_found); - Ok(()) - }; + let mut collect_account = + |key, account_size, account: AccountSharedData, rent, account_found| -> Result<()> { + accumulate_and_check_loaded_account_data_size( + &mut accumulated_accounts_data_size, + account_size, + tx_details.compute_budget_limits.loaded_accounts_bytes, + error_metrics, + )?; + + tx_rent += rent; + rent_debits.insert(key, rent, account.lamports()); + + accounts.push((*key, account)); + accounts_found.push(account_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