Skip to content

Commit

Permalink
rebase on simd83 loader
Browse files Browse the repository at this point in the history
  • Loading branch information
@2501babe
2501babe committed Nov 25, 2024
1 parent ce78b40 commit 6f56c11
Showing 1 changed file with 16 additions and 17 deletions.
33 changes: 16 additions & 17 deletions svm/src/account_loader.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -435,7 +431,7 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
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;

Expand Down Expand Up @@ -511,24 +507,20 @@ fn load_transaction_accounts<CB: TransactionProcessingCallback>(
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
// 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
// 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
//
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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)]),
Expand Down

0 comments on commit 6f56c11

Please sign in to comment.