From 138d5ed5b0d21e8e0a287748e05678040e830943 Mon Sep 17 00:00:00 2001 From: "Jeff Washington (jwash)" Date: Thu, 15 Sep 2022 08:46:32 -0700 Subject: [PATCH] don't return zero lamport accounts from 'load' with feature (#27793) * don't return zero lamport accounts from 'load' * add feature * rename --- runtime/src/accounts.rs | 59 +++++++++++++++++------- runtime/src/accounts_db.rs | 29 +++++++++--- runtime/src/bank/address_lookup_table.rs | 12 +++++ sdk/src/feature_set.rs | 5 ++ 4 files changed, 82 insertions(+), 23 deletions(-) diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index d432e209fb14dc..2fe1b2b45c04f6 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -4,7 +4,7 @@ use { account_rent_state::{check_rent_state_with_account, RentState}, accounts_db::{ AccountShrinkThreshold, AccountsAddRootTiming, AccountsDb, AccountsDbConfig, - BankHashInfo, LoadHint, LoadedAccount, ScanStorageResult, + BankHashInfo, LoadHint, LoadZeroLamports, LoadedAccount, ScanStorageResult, ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS, ACCOUNTS_DB_CONFIG_FOR_TESTING, }, accounts_index::{ @@ -35,8 +35,8 @@ use { bpf_loader_upgradeable::{self, UpgradeableLoaderState}, clock::{BankId, Slot, INITIAL_RENT_EPOCH}, feature_set::{ - self, remove_deprecated_request_unit_ix, use_default_units_in_fee_calculation, - FeatureSet, + self, remove_deprecated_request_unit_ix, return_none_for_zero_lamport_accounts, + use_default_units_in_fee_calculation, FeatureSet, }, fee::FeeStructure, genesis_config::ClusterType, @@ -258,6 +258,13 @@ impl Accounts { feature_set: &FeatureSet, account_overrides: Option<&AccountOverrides>, ) -> Result { + let load_zero_lamports = + if feature_set.is_active(&return_none_for_zero_lamport_accounts::id()) { + LoadZeroLamports::None + } else { + LoadZeroLamports::SomeWithZeroLamportAccount + }; + // Copy all the accounts let message = tx.message(); // NOTE: this check will never fail because `tx` is sanitized @@ -293,7 +300,7 @@ impl Accounts { (account_override.clone(), 0) } else { self.accounts_db - .load_with_fixed_root(ancestors, key) + .load_with_fixed_root(ancestors, key, load_zero_lamports) .map(|(mut account, _)| { if message.is_writable(i) { let rent_due = rent_collector @@ -342,9 +349,12 @@ impl Accounts { programdata_address, }) = account.state() { - if let Some((programdata_account, _)) = self - .accounts_db - .load_with_fixed_root(ancestors, &programdata_address) + if let Some((programdata_account, _)) = + self.accounts_db.load_with_fixed_root( + ancestors, + &programdata_address, + load_zero_lamports, + ) { account_deps .push((programdata_address, programdata_account)); @@ -388,6 +398,7 @@ impl Accounts { &mut accounts, instruction.program_id_index as IndexOfAccount, error_counters, + load_zero_lamports, ) }) .collect::>>>()?; @@ -459,6 +470,7 @@ impl Accounts { accounts: &mut Vec, mut program_account_index: IndexOfAccount, error_counters: &mut TransactionErrorMetrics, + load_zero_lamports: LoadZeroLamports, ) -> Result> { let mut account_indices = Vec::new(); let mut program_id = match accounts.get(program_account_index as usize) { @@ -476,10 +488,11 @@ impl Accounts { } depth += 1; - program_account_index = match self - .accounts_db - .load_with_fixed_root(ancestors, &program_id) - { + program_account_index = match self.accounts_db.load_with_fixed_root( + ancestors, + &program_id, + load_zero_lamports, + ) { Some((program_account, _)) => { let account_index = accounts.len() as IndexOfAccount; accounts.push((program_id, program_account)); @@ -505,10 +518,11 @@ impl Accounts { programdata_address, }) = program.state() { - let programdata_account_index = match self - .accounts_db - .load_with_fixed_root(ancestors, &programdata_address) - { + let programdata_account_index = match self.accounts_db.load_with_fixed_root( + ancestors, + &programdata_address, + load_zero_lamports, + ) { Some((programdata_account, _)) => { let account_index = accounts.len() as IndexOfAccount; accounts.push((programdata_address, programdata_account)); @@ -606,10 +620,15 @@ impl Accounts { ancestors: &Ancestors, address_table_lookup: &MessageAddressTableLookup, slot_hashes: &SlotHashes, + load_zero_lamports: LoadZeroLamports, ) -> std::result::Result { let table_account = self .accounts_db - .load_with_fixed_root(ancestors, &address_table_lookup.account_key) + .load_with_fixed_root( + ancestors, + &address_table_lookup.account_key, + load_zero_lamports, + ) .map(|(account, _rent)| account) .ok_or(AddressLookupError::LookupTableAccountNotFound)?; @@ -2077,6 +2096,7 @@ mod tests { &ancestors, &address_table_lookup, &SlotHashes::default(), + LoadZeroLamports::SomeWithZeroLamportAccount, ), Err(AddressLookupError::LookupTableAccountNotFound), ); @@ -2094,7 +2114,8 @@ mod tests { ); let invalid_table_key = Pubkey::new_unique(); - let invalid_table_account = AccountSharedData::default(); + let mut invalid_table_account = AccountSharedData::default(); + invalid_table_account.set_lamports(1); accounts.store_slow_uncached(0, &invalid_table_key, &invalid_table_account); let address_table_lookup = MessageAddressTableLookup { @@ -2108,6 +2129,7 @@ mod tests { &ancestors, &address_table_lookup, &SlotHashes::default(), + LoadZeroLamports::SomeWithZeroLamportAccount, ), Err(AddressLookupError::InvalidAccountOwner), ); @@ -2140,6 +2162,7 @@ mod tests { &ancestors, &address_table_lookup, &SlotHashes::default(), + LoadZeroLamports::SomeWithZeroLamportAccount, ), Err(AddressLookupError::InvalidAccountData), ); @@ -2184,6 +2207,7 @@ mod tests { &ancestors, &address_table_lookup, &SlotHashes::default(), + LoadZeroLamports::SomeWithZeroLamportAccount, ), Ok(LoadedAddresses { writable: vec![table_addresses[0]], @@ -2498,6 +2522,7 @@ mod tests { &mut vec![(keypair.pubkey(), account)], 0, &mut error_counters, + LoadZeroLamports::SomeWithZeroLamportAccount, ), Err(TransactionError::ProgramAccountNotFound) ); diff --git a/runtime/src/accounts_db.rs b/runtime/src/accounts_db.rs index c8a393fa4bb2fb..ff53e5f325b9aa 100644 --- a/runtime/src/accounts_db.rs +++ b/runtime/src/accounts_db.rs @@ -145,6 +145,17 @@ pub enum StoreReclaims { Ignore, } +/// specifies how to return zero lamport accounts +/// This will only be useful until a feature activation occurs. +#[derive(Clone, Copy)] +pub enum LoadZeroLamports { + /// return None if loaded account has zero lamports + None, + /// return Some(account with zero lamports) if loaded account has zero lamports + /// Today this is the default. With feature activation, this will no longer be possible. + SomeWithZeroLamportAccount, +} + // the current best way to add filler accounts is gradually. // In other scenarios, such as monitoring catchup with large # of accounts, it may be useful to be able to // add filler accounts at the beginning, so that code path remains but won't execute at the moment. @@ -4586,8 +4597,15 @@ impl AccountsDb { &self, ancestors: &Ancestors, pubkey: &Pubkey, + load_zero_lamports: LoadZeroLamports, ) -> Option<(AccountSharedData, Slot)> { self.load(ancestors, pubkey, LoadHint::FixedMaxRoot) + .filter(|(account, _)| { + matches!( + load_zero_lamports, + LoadZeroLamports::SomeWithZeroLamportAccount + ) || !account.is_zero_lamport() + }) } pub fn load_without_fixed_root( @@ -13859,13 +13877,13 @@ pub mod tests { assert_eq!(db.read_only_accounts_cache.cache_len(), 0); let account = db - .load_with_fixed_root(&Ancestors::default(), &account_key) + .load_with_fixed_root(&Ancestors::default(), &account_key, LoadZeroLamports::None) .map(|(account, _)| account) .unwrap(); assert_eq!(account.lamports(), 1); assert_eq!(db.read_only_accounts_cache.cache_len(), 1); let account = db - .load_with_fixed_root(&Ancestors::default(), &account_key) + .load_with_fixed_root(&Ancestors::default(), &account_key, LoadZeroLamports::None) .map(|(account, _)| account) .unwrap(); assert_eq!(account.lamports(), 1); @@ -13873,10 +13891,9 @@ pub mod tests { db.store_cached((2, &[(&account_key, &zero_lamport_account)][..]), None); assert_eq!(db.read_only_accounts_cache.cache_len(), 1); let account = db - .load_with_fixed_root(&Ancestors::default(), &account_key) - .map(|(account, _)| account) - .unwrap(); - assert_eq!(account.lamports(), 0); + .load_with_fixed_root(&Ancestors::default(), &account_key, LoadZeroLamports::None) + .map(|(account, _)| account); + assert!(account.is_none()); assert_eq!(db.read_only_accounts_cache.cache_len(), 1); } diff --git a/runtime/src/bank/address_lookup_table.rs b/runtime/src/bank/address_lookup_table.rs index 3916177afd5668..0a4bc732f45ba6 100644 --- a/runtime/src/bank/address_lookup_table.rs +++ b/runtime/src/bank/address_lookup_table.rs @@ -1,7 +1,9 @@ use { super::Bank, + crate::accounts_db::LoadZeroLamports, solana_address_lookup_table_program::error::AddressLookupError, solana_sdk::{ + feature_set::return_none_for_zero_lamport_accounts, message::v0::{LoadedAddresses, MessageAddressTableLookup}, transaction::{AddressLoader, Result as TransactionResult, TransactionError}, }, @@ -16,6 +18,15 @@ impl AddressLoader for &Bank { return Err(TransactionError::UnsupportedVersion); } + let load_zero_lamports = if self + .feature_set + .is_active(&return_none_for_zero_lamport_accounts::id()) + { + LoadZeroLamports::None + } else { + LoadZeroLamports::SomeWithZeroLamportAccount + }; + let slot_hashes = self .sysvar_cache .read() @@ -30,6 +41,7 @@ impl AddressLoader for &Bank { &self.ancestors, address_table_lookup, &slot_hashes, + load_zero_lamports, ) }) .collect::>()?) diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 2e6c9154c5aa58..1c6ca47ff5bf53 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -514,6 +514,10 @@ pub mod cap_accounts_data_allocations_per_transaction { solana_sdk::declare_id!("9gxu85LYRAcZL38We8MYJ4A9AwgBBPtVBAqebMcT1241"); } +pub mod return_none_for_zero_lamport_accounts { + solana_sdk::declare_id!("7K5HFrS1WAq6ND7RQbShXZXbtAookyTfaDQPTJNuZpze"); +} + pub mod epoch_accounts_hash { solana_sdk::declare_id!("5GpmAKxaGsWWbPp4bNXFLJxZVvG92ctxf7jQnzTQjF3n"); } @@ -645,6 +649,7 @@ lazy_static! { (stop_sibling_instruction_search_at_parent::id(), "stop the search in get_processed_sibling_instruction when the parent instruction is reached #27289"), (vote_state_update_root_fix::id(), "fix root in vote state updates #27361"), (cap_accounts_data_allocations_per_transaction::id(), "cap accounts data allocations per transaction #27375"), + (return_none_for_zero_lamport_accounts::id(), "return none for zero lamport accounts #27800"), (epoch_accounts_hash::id(), "enable epoch accounts hash calculation #27539"), (remove_deprecated_request_unit_ix::id(), "remove support for RequestUnitsDeprecated instruction #27500"), /*************** ADD NEW FEATURES HERE ***************/