Skip to content

Commit

Permalink
excise the account cache
Browse files Browse the repository at this point in the history
  • Loading branch information
@2501babe
2501babe committed Oct 31, 2024
1 parent 49b3287 commit af8ef47
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 258 deletions.
238 changes: 13 additions & 225 deletions svm/src/account_loader.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,12 @@ use {
rollback_accounts::RollbackAccounts,
transaction_error_metrics::TransactionErrorMetrics,
transaction_processing_callback::{AccountState, TransactionProcessingCallback},
transaction_processing_result::ProcessedTransaction,
},
ahash::AHashMap,
itertools::Itertools,
solana_compute_budget::compute_budget_limits::ComputeBudgetLimits,
solana_feature_set::{self as feature_set, FeatureSet},
solana_program_runtime::loaded_programs::{ProgramCacheEntry, ProgramCacheForTxBatch},
solana_sdk::{
account::{Account, AccountSharedData, ReadableAccount, WritableAccount, PROGRAM_OWNERS},
account::{Account, AccountSharedData, ReadableAccount, WritableAccount},
fee::FeeDetails,
native_loader,
nonce::State as NonceState,
Expand Down Expand Up @@ -102,20 +99,17 @@ pub struct FeesOnlyTransaction {
pub(crate) struct AccountLoader<'a, CB: TransactionProcessingCallback> {
pub(crate) program_cache: ProgramCacheForTxBatch,
account_overrides: Option<&'a AccountOverrides>,
account_cache: AHashMap<Pubkey, AccountCacheItem>,
callbacks: &'a CB,
}
impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> {
pub fn new_with_account_cache_capacity(
pub fn new(
callbacks: &'a CB,
account_overrides: Option<&'a AccountOverrides>,
program_cache: ProgramCacheForTxBatch,
capacity: usize,
) -> AccountLoader<'a, CB> {
Self {
program_cache,
account_overrides,
account_cache: AHashMap::with_capacity(capacity),
callbacks,
}
}
Expand All @@ -126,73 +120,18 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> {
usage_pattern: AccountUsagePattern,
) -> Option<LoadedTransactionAccount> {
let is_writable = usage_pattern == AccountUsagePattern::Writable;
let is_invisible = usage_pattern == AccountUsagePattern::ReadOnlyInvisible;

if let Some(cache_item) = self.account_cache.get_mut(account_key) {
// Non-builtin accounts owned by NativeLoader are never loaded into the cache.
let is_owned_by_loader = PROGRAM_OWNERS.iter().contains(cache_item.account.owner());

if !cache_item.inspected_as_writable {
// Inspecting a read-only account is harmless. Inspecting a writable
// account must be done before rent collection, and must never be done
// again in-batch, because inspection is intended to preserve the
// state prior to any writes.
self.callbacks.inspect_account(
account_key,
AccountState::Alive(&cache_item.account),
is_writable,
);
cache_item.inspected_as_writable = cache_item.inspected_as_writable || is_writable;
}
let is_invisible_read = usage_pattern == AccountUsagePattern::ReadOnlyInvisible;

if cache_item.account.lamports() == 0 {
None
} else if let Some(program) = (is_invisible && is_owned_by_loader)
.then_some(())
.and_then(|_| self.program_cache.find(account_key))
{
// NOTE if this account could be found in the program cache, we must
// substitute its size here, to perserve an oddity with transaction size
// calculations. However, we CANNOT skip the accounts cache check,
// in case the account was non-executable and modified in-batch.
//
// We must also check the current owner, in case this account was closed
// and reopened, in which case we ignore the stale program cache entry.
//
// When we properly check if program cache entries are executable,
// we can go to the program cache first and remove this branch, because
// executable accounts are immutable.
Some(LoadedTransactionAccount {
loaded_size: program.account_size,
account: account_shared_data_from_program(&program),
rent_collected: 0,
})
} else {
Some(LoadedTransactionAccount {
loaded_size: cache_item.account.data().len(),
account: cache_item.account.clone(),
rent_collected: 0,
})
}
} else if let Some(account_override) = self
if let Some(account_override) = self
.account_overrides
.and_then(|overrides| overrides.get(account_key))
{
// We mark `inspected_as_writable` because overrides are never inspected.
self.account_cache.insert(
*account_key,
AccountCacheItem {
account: account_override.clone(),
inspected_as_writable: true,
},
);

Some(LoadedTransactionAccount {
loaded_size: account_override.data().len(),
account: account_override.clone(),
rent_collected: 0,
})
} else if let Some(program) = is_invisible
} else if let Some(program) = is_invisible_read
.then_some(())
.and_then(|_| self.program_cache.find(account_key))
{
Expand All @@ -210,14 +149,6 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> {
self.callbacks
.inspect_account(account_key, AccountState::Alive(&account), is_writable);

self.account_cache.insert(
*account_key,
AccountCacheItem {
account: account.clone(),
inspected_as_writable: is_writable,
},
);

Some(LoadedTransactionAccount {
loaded_size: account.data().len(),
account,
Expand All @@ -230,101 +161,6 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> {
None
}
}

pub fn update_accounts_for_executed_tx(
&mut self,
message: &impl SVMMessage,
processed_transaction: &ProcessedTransaction,
) {
match processed_transaction {
ProcessedTransaction::Executed(executed_tx) => {
if executed_tx.execution_details.status.is_ok() {
self.update_accounts_for_successful_tx(
message,
&executed_tx.loaded_transaction.accounts,
);
} else {
self.update_accounts_for_failed_tx(
message,
&executed_tx.loaded_transaction.rollback_accounts,
);
}
}
ProcessedTransaction::FeesOnly(fees_only_tx) => {
self.update_accounts_for_failed_tx(message, &fees_only_tx.rollback_accounts);
}
}
}

pub fn update_accounts_for_failed_tx(
&mut self,
message: &impl SVMMessage,
rollback_accounts: &RollbackAccounts,
) {
let fee_payer_address = message.fee_payer();
match rollback_accounts {
RollbackAccounts::FeePayerOnly { fee_payer_account } => {
self.update_account(fee_payer_address, fee_payer_account);
}
RollbackAccounts::SameNonceAndFeePayer { nonce } => {
self.update_account(nonce.address(), nonce.account());
}
RollbackAccounts::SeparateNonceAndFeePayer {
nonce,
fee_payer_account,
} => {
self.update_account(nonce.address(), nonce.account());
self.update_account(fee_payer_address, fee_payer_account);
}
}
}

fn update_accounts_for_successful_tx(
&mut self,
message: &impl SVMMessage,
transaction_accounts: &[TransactionAccount],
) {
for (i, (address, account)) in (0..message.account_keys().len()).zip(transaction_accounts) {
if !message.is_writable(i) {
continue;
}

// Accounts that are invoked and also not passed as an instruction
// account to a program don't need to be stored because it's assumed
// to be impossible for a committable transaction to modify an
// invoked account if said account isn't passed to some program.
if message.is_invoked(i) && !message.is_instruction_account(i) {
continue;
}

self.update_account(address, account);
}
}

fn update_account(&mut self, account_key: &Pubkey, account: &AccountSharedData) {
// If an account has been dropped, we must hide the data from any future transactions.
// We NEVER remove items from cache, or else we will fetch stale data from accounts-db.
// Because `update_account()` is only called on writable accounts using the same rules
// as commit, this (correctly) does not shadow rent-paying zero-lamport read-only accounts.
let account = if account.lamports() == 0 {
&AccountSharedData::default()
} else {
account
};

// Accounts are inspected during loading, and we only update writable accounts.
// Ergo, `inspected_as_writable` is always true.
self.account_cache
.entry(*account_key)
.and_modify(|cache_item| {
debug_assert!(cache_item.inspected_as_writable);
cache_item.account = account.clone();
})
.or_insert_with(|| AccountCacheItem {
account: account.clone(),
inspected_as_writable: true,
});
}
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
Expand All @@ -346,12 +182,6 @@ impl AccountUsagePattern {
}
}

#[derive(Debug, Clone)]
struct AccountCacheItem {
account: AccountSharedData,
inspected_as_writable: bool,
}

/// Collect rent from an account if rent is still enabled and regardless of
/// whether rent is enabled, set the rent epoch to u64::MAX if the account is
/// rent exempt.
Expand Down Expand Up @@ -771,7 +601,7 @@ mod tests {
transaction_context::{TransactionAccount, TransactionContext},
},
std::{borrow::Cow, cell::RefCell, collections::HashMap, sync::Arc},
test_case::test_matrix,
test_case::test_case,
};

#[derive(Clone, Default)]
Expand Down Expand Up @@ -811,12 +641,7 @@ mod tests {

impl<'a> From<&'a TestCallbacks> for AccountLoader<'a, TestCallbacks> {
fn from(callbacks: &'a TestCallbacks) -> AccountLoader<'a, TestCallbacks> {
AccountLoader::new_with_account_cache_capacity(
callbacks,
None,
ProgramCacheForTxBatch::default(),
callbacks.accounts_map.len(),
)
AccountLoader::new(callbacks, None, ProgramCacheForTxBatch::default())
}
}

Expand Down Expand Up @@ -1146,11 +971,10 @@ mod tests {
accounts_map,
..Default::default()
};
let mut account_loader = AccountLoader::new_with_account_cache_capacity(
let mut account_loader = AccountLoader::new(
&callbacks,
account_overrides,
ProgramCacheForTxBatch::default(),
accounts.len(),
);
load_transaction(
&mut account_loader,
Expand Down Expand Up @@ -1559,8 +1383,7 @@ mod tests {
let mut loaded_programs = ProgramCacheForTxBatch::default();
loaded_programs.replenish(key2.pubkey(), Arc::new(ProgramCacheEntry::default()));

let mut account_loader =
AccountLoader::new_with_account_cache_capacity(&mock_bank, None, loaded_programs, 1);
let mut account_loader = AccountLoader::new(&mock_bank, None, loaded_programs);

let sanitized_transaction = SanitizedTransaction::new_for_tests(
sanitized_message,
Expand All @@ -1584,17 +1407,11 @@ mod tests {
// it creates a mock AccountSharedData with the executable flag set to true
// however, it does not check whether these accounts are actually executable before doing so
// this affects consensus: a transaction that uses a cached non-executable program executes and fails
// but if the transaction gets the program from accounts-db, it will be dropped during account loading
// but if the transaction gets the program from accounts-db, the transaction will be dropped during account loading
// this test enforces the current behavior, so that future account loader changes do not break consensus
//
// account cache has its own code path that accesses program cache, so we test hit and miss
// we also test bpf_loader and bpf_loader_upgradeable, because accounts owned by the latter can behave strangely
// all cases should produce the same results
#[test_matrix([bpf_loader::id(), bpf_loader_upgradeable::id()], [false, true])]
fn test_load_transaction_accounts_program_account_executable_bypass(
program_owner: Pubkey,
clear_account_cache: bool,
) {
#[test_case(bpf_loader::id())]
#[test_case(bpf_loader_upgradeable::id())]
fn test_load_transaction_accounts_program_account_executable_bypass(program_owner: Pubkey) {
let mut mock_bank = TestCallbacks::default();
let account_keypair = Keypair::new();
let program_keypair = Keypair::new();
Expand Down Expand Up @@ -1663,10 +1480,6 @@ mod tests {
Arc::new(ProgramCacheEntry::default()),
);

if clear_account_cache {
account_loader.account_cache = AHashMap::default();
}

let result = load_transaction_accounts(
&mut account_loader,
transaction.message(),
Expand Down Expand Up @@ -1711,10 +1524,6 @@ mod tests {
Hash::default(),
));

if clear_account_cache {
account_loader.account_cache = AHashMap::default();
}

let result = load_transaction_accounts(
&mut account_loader,
transaction.message(),
Expand All @@ -1738,10 +1547,6 @@ mod tests {
account_keypair.pubkey(),
);

if clear_account_cache {
account_loader.account_cache = AHashMap::default();
}

let result = load_transaction_accounts(
&mut account_loader,
transaction.message(),
Expand Down Expand Up @@ -2486,23 +2291,6 @@ mod tests {
assert_eq!(account.lamports(), 0);
}

#[test]
fn test_load_account_dropped() {
let mock_bank = TestCallbacks::default();
let mut account_loader: AccountLoader<TestCallbacks> = (&mock_bank).into();

let address = Pubkey::new_unique();
let account = AccountSharedData::default();
let cache_item = AccountCacheItem {
account,
inspected_as_writable: false,
};
account_loader.account_cache.insert(address, cache_item);

let result = account_loader.load_account(&address, AccountUsagePattern::ReadOnlyInvisible);
assert!(result.is_none());
}

// Ensure `TransactionProcessingCallback::inspect_account()` is called when
// loading accounts for transaction processing.
#[test]
Expand Down
Loading

0 comments on commit af8ef47

Please sign in to comment.