Skip to content

Commit

Permalink
stop trying to outsmart loader bugs
Browse files Browse the repository at this point in the history
  • Loading branch information
@2501babe
2501babe committed Nov 4, 2024
1 parent 61fbf29 commit 343ce1b
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 91 deletions.
156 changes: 72 additions & 84 deletions svm/src/account_loader.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use {
sysvar::{
self,
instructions::{construct_instructions_data, BorrowedAccountMeta, BorrowedInstruction},
slot_history,
},
transaction::{Result, TransactionError},
transaction_context::{IndexOfAccount, TransactionAccount},
Expand All @@ -34,6 +35,8 @@ use {
std::num::NonZeroU32,
};

const FEATURE_GATE_PLACEHOLDER: bool = false;

// for the load instructions
pub(crate) type TransactionRent = u64;
pub(crate) type TransactionProgramIndices = Vec<Vec<IndexOfAccount>>;
Expand Down Expand Up @@ -97,15 +100,9 @@ pub struct FeesOnlyTransaction {
pub fee_details: FeeDetails,
}

// XXX im going to go insane how the fuck is this supposed to work
// we cant let the program cache preempt the account cache if accounts are closed
// that means we want to go with starrys idea of creating a new program cache for every tx
// so we want to impl the trait for

#[cfg_attr(feature = "dev-context-only-utils", derive(Clone))]
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,
}
Expand All @@ -116,10 +113,24 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> {
program_cache: ProgramCacheForTxBatch,
capacity: usize,
) -> AccountLoader<'a, CB> {
let mut account_cache = AHashMap::with_capacity(capacity);

if let Some(slot_history) =
account_overrides.and_then(|overrides| overrides.get(&slot_history::id()))
{
// We set `inspected_as_writable` to `true` because this never needs to be inspected.
account_cache.insert(
slot_history::id(),
AccountCacheItem {
account: slot_history.clone(),
inspected_as_writable: true,
},
);
}

Self {
program_cache,
account_overrides,
account_cache: AHashMap::with_capacity(capacity),
account_cache,
callbacks,
}
}
Expand All @@ -132,42 +143,17 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> {
let is_writable = usage_pattern == AccountUsagePattern::Writable;
let is_invisible_read = usage_pattern == AccountUsagePattern::ReadOnlyInvisible;

if let Some(account_override) = self
.account_overrides
.and_then(|overrides| overrides.get(account_key))
{
// Ensure account overrides preempt program cache. This is only for
// abundance of caution: account overrides is an overly general mechanism
// to replace a specific sysvar during simulation. It may be removed in
// the future.
if let Some(cache_item) = self.account_cache.get(account_key) {
Some(LoadedTransactionAccount {
loaded_size: cache_item.account.data().len(),
account: cache_item.account.clone(),
rent_collected: 0,
})
} else {
// Account overrides never need to be inspected. It would be a grave error for
// the runtime to pass a writable override for a non-simulated transaction.
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_read
if let Some(program) = is_invisible_read
.then_some(())
.and_then(|_| self.program_cache.find(account_key))
.filter(|program| {
if FEATURE_GATE_PLACEHOLDER {
!program.is_tombstone()
} else {
true
}
})
{
self.callbacks.get_account_shared_data(account_key)?;
// Optimization to skip loading of accounts which are only used as
// programs in top-level instructions and not passed as instruction accounts.
Some(LoadedTransactionAccount {
Expand All @@ -185,6 +171,51 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> {
}
}

fn get_and_inspect_account_shared_data(
&mut self,
account_key: &Pubkey,
is_writable: bool,
) -> Option<AccountSharedData> {
if let Some(cache_item) = self.account_cache.get_mut(account_key) {
if is_writable && !cache_item.inspected_as_writable {
// Inspecting a read-only account is a no-op. But 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;
}

Some(cache_item.account.clone())
} else if let Some(account) = self.callbacks.get_account_shared_data(account_key) {
// Inspect the account prior to collecting rent, since
// rent collection can modify the account.
self.callbacks
.inspect_account(account_key, AccountState::Alive(&account), is_writable);

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

Some(account)
} else {
self.callbacks
.inspect_account(account_key, AccountState::Dead, is_writable);

None
}
}

pub fn update_accounts_for_executed_tx(
&mut self,
message: &impl SVMMessage,
Expand Down Expand Up @@ -229,49 +260,6 @@ impl<'a, CB: TransactionProcessingCallback> AccountLoader<'a, CB> {
}
}

fn get_and_inspect_account_shared_data(
&mut self,
account_key: &Pubkey,
is_writable: bool,
) -> Option<AccountSharedData> {
if let Some(cache_item) = self.account_cache.get_mut(account_key) {
if is_writable && !cache_item.inspected_as_writable {
// Inspecting a read-only account is a no-op. But 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;
}

Some(cache_item.account.clone())
} else if let Some(account) = self.callbacks.get_account_shared_data(account_key) {
// Inspect the account prior to collecting rent, since
// rent collection can modify the account.
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(account)
} else {
self.callbacks
.inspect_account(account_key, AccountState::Dead, is_writable);

None
}
}

fn update_accounts_for_successful_tx(
&mut self,
message: &impl SVMMessage,
Expand Down
24 changes: 17 additions & 7 deletions svm/src/transaction_processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ use {
},
};

const FEATURE_GATE_PLACEHOLDER: bool = false;

/// A list of log messages emitted during a transaction
pub type TransactionLogMessages = Vec<String>;

Expand Down Expand Up @@ -356,9 +358,11 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
TransactionLoadResult::NotLoaded(err) => Err(err),
TransactionLoadResult::FeesOnly(fees_only_tx) => {
if enable_transaction_loading_failure_fees {
// Update loaded accounts cache with nonce and fee-payer
account_loader
.update_accounts_for_failed_tx(tx, &fees_only_tx.rollback_accounts);
if FEATURE_GATE_PLACEHOLDER {
// Update loaded accounts cache with nonce and fee-payer
account_loader
.update_accounts_for_failed_tx(tx, &fees_only_tx.rollback_accounts);
}

Ok(ProcessedTransaction::FeesOnly(Box::new(fees_only_tx)))
} else {
Expand All @@ -376,10 +380,16 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
config,
);

// Update loaded accounts cache with account states which might have changed.
// Also update local progrma cache with modifications made by the transaction,
// if it executed successfully.
account_loader.update_accounts_for_executed_tx(tx, &executed_tx);
if FEATURE_GATE_PLACEHOLDER {
// Update loaded accounts cache with account states which might have changed.
// Also update local progrma cache with modifications made by the transaction,
// if it executed successfully.
account_loader.update_accounts_for_executed_tx(tx, &executed_tx);
} else {
account_loader
.program_cache
.merge(&executed_tx.programs_modified_by_tx);
}

Ok(ProcessedTransaction::Executed(Box::new(executed_tx)))
}
Expand Down

0 comments on commit 343ce1b

Please sign in to comment.