Skip to content

Commit

Permalink
fix account inspecting for new load
Browse files Browse the repository at this point in the history
  • Loading branch information
@2501babe
2501babe committed Sep 7, 2024
1 parent 3562efe commit d9ece0b
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 103 deletions.
62 changes: 25 additions & 37 deletions svm/src/account_loader.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use {
nonce_info::NonceInfo,
rollback_accounts::RollbackAccounts,
transaction_error_metrics::TransactionErrorMetrics,
transaction_processing_callback::{/* XXX AccountState, */ TransactionProcessingCallback,},
transaction_processing_callback::{AccountState, TransactionProcessingCallback},
},
itertools::Itertools,
solana_compute_budget::compute_budget_limits::ComputeBudgetLimits,
Expand Down Expand Up @@ -228,24 +228,35 @@ pub(crate) fn load_accounts<CB: TransactionProcessingCallback>(
.map(|(tx, _)| tx)
.collect();

let account_keys: Vec<_> = checked_messages
.iter()
.flat_map(|m| m.account_keys().iter())
.unique()
.collect();
let mut account_key_map = HashMap::new();
for message in checked_messages.iter() {
for (account_index, account_key) in message.account_keys().iter().enumerate() {
if message.is_writable(account_index) {
account_key_map.insert(account_key, true);
} else {
account_key_map.entry(account_key).or_insert(false);
}
}
}

for key in account_keys {
if solana_sdk::sysvar::instructions::check_id(key) {
for (account_key, is_writable) in account_key_map {
if solana_sdk::sysvar::instructions::check_id(account_key) {
continue;
} else if let Some(account_override) =
account_overrides.and_then(|overrides| overrides.get(key))
account_overrides.and_then(|overrides| overrides.get(account_key))
{
accounts_map.insert(*key, account_override.clone());
} else if let Some(account) = callbacks.get_account_shared_data(key) {
accounts_map.insert(*key, account);
accounts_map.insert(*account_key, account_override.clone());
} else if let Some(account) = callbacks.get_account_shared_data(account_key) {
callbacks.inspect_account(account_key, AccountState::Alive(&account), is_writable);

accounts_map.insert(*account_key, account);
}
// XXX we do not insert the default account here because we need to know if its not found later
// it raises the question however of whether we need to track if a program account is created in the batch
// XXX HANA FIXME UPDATE check with brooks on monday........ i dont know why we need to do this
else {
callbacks.inspect_account(account_key, AccountState::Dead, is_writable);
}
}

// XXX it is possible we want to fully validate programs for messages here
Expand Down Expand Up @@ -522,18 +533,6 @@ fn load_transaction_account(
.cloned() // XXX new clone
.map(|mut account| {
let rent_collected = if is_writable {
// Inspect the account prior to collecting rent, since
// rent collection can modify the account.
/* XXX HANA i messaged brooks asking what this is... newly added yesterday 8/22
debug_assert!(!was_inspected);
callbacks.inspect_account(
account_key,
AccountState::Alive(&account),
is_writable,
);
was_inspected = true;
*/

collect_rent_from_account(
feature_set,
rent_collector,
Expand Down Expand Up @@ -566,17 +565,6 @@ fn load_transaction_account(
})
};

/* XXX as noted above
if !was_inspected {
let account_state = if account_found {
AccountState::Alive(&loaded_account.account)
} else {
AccountState::Dead
};
callbacks.inspect_account(account_key, account_state, is_writable);
}
*/

Ok((loaded_account, account_found))
}

Expand Down Expand Up @@ -650,7 +638,7 @@ mod tests {
super::*,
crate::{
transaction_account_state_info::TransactionAccountStateInfo,
transaction_processing_callback::{AccountState, TransactionProcessingCallback},
transaction_processing_callback::TransactionProcessingCallback,
},
nonce::state::Versions as NonceVersions,
solana_compute_budget::{compute_budget::ComputeBudget, compute_budget_limits},
Expand Down Expand Up @@ -2276,7 +2264,7 @@ mod tests {
actual_inspected_accounts.sort_unstable_by(|a, b| a.0.cmp(&b.0));

let mut expected_inspected_accounts = vec![
// *not* key0, since it is loaded during fee payer validation
(address0, vec![(Some(account0), true)]),
(address1, vec![(Some(account1), true)]),
(address2, vec![(None, true)]),
(address3, vec![(Some(account3), false)]),
Expand Down
69 changes: 3 additions & 66 deletions svm/src/transaction_processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use {
transaction_account_state_info::TransactionAccountStateInfo,
transaction_error_metrics::TransactionErrorMetrics,
transaction_execution_result::{ExecutedTransaction, TransactionExecutionDetails},
transaction_processing_callback::{AccountState, TransactionProcessingCallback},
transaction_processing_callback::TransactionProcessingCallback,
transaction_processing_result::{ProcessedTransaction, TransactionProcessingResult},
},
log::debug,
Expand Down Expand Up @@ -281,7 +281,7 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
.feature_set
.is_active(&feature_set::enable_transaction_loading_failure_fees::id());

for (tx, check_result) in sanitized_txs.into_iter().zip(check_results) {
for (tx, check_result) in sanitized_txs.iter().zip(check_results) {
let validate_result = check_result.and_then(|tx_details| {
// XXX this shouldnt take callback or override
self.validate_transaction_fee_payer(
Expand Down Expand Up @@ -448,12 +448,6 @@ impl<FG: ForkGraph> TransactionBatchProcessor<FG> {
return Err(TransactionError::AccountNotFound);
};

callbacks.inspect_account(
fee_payer_address,
AccountState::Alive(&fee_payer_account),
true, // <-- is_writable
);

let fee_payer_loaded_rent_epoch = fee_payer_account.rent_epoch();
let fee_payer_rent_debit = collect_rent_from_account(
feature_set,
Expand Down Expand Up @@ -1014,7 +1008,7 @@ mod tests {
super::*,
crate::{
account_loader::ValidatedTransactionDetails, nonce_info::NonceInfo,
rollback_accounts::RollbackAccounts,
rollback_accounts::RollbackAccounts, transaction_processing_callback::AccountState,
},
solana_compute_budget::compute_budget_limits::ComputeBudgetLimits,
solana_program_runtime::loaded_programs::{BlockRelation, ProgramCacheEntryType},
Expand Down Expand Up @@ -2374,61 +2368,4 @@ mod tests {
result.err()
);
}

// Ensure `TransactionProcessingCallback::inspect_account()` is called when
// validating the fee payer, since that's when the fee payer account is loaded.
#[test]
fn test_inspect_account_fee_payer() {
let fee_payer_address = Pubkey::new_unique();
let fee_payer_account = AccountSharedData::new_rent_epoch(
123_000_000_000,
0,
&Pubkey::default(),
RENT_EXEMPT_RENT_EPOCH,
);
let mock_bank = MockBankCallback::default();
mock_bank
.account_shared_data
.write()
.unwrap()
.insert(fee_payer_address, fee_payer_account.clone());

let message = new_unchecked_sanitized_message(Message::new_with_blockhash(
&[
ComputeBudgetInstruction::set_compute_unit_limit(2000u32),
ComputeBudgetInstruction::set_compute_unit_price(1_000_000_000),
],
Some(&fee_payer_address),
&Hash::new_unique(),
));
let batch_processor = TransactionBatchProcessor::<TestForkGraph>::default();
batch_processor
.validate_transaction_fee_payer(
&mock_bank,
None,
&message,
CheckedTransactionDetails {
nonce: None,
lamports_per_signature: 5000,
},
&FeatureSet::default(),
&FeeStructure::default(),
&RentCollector::default(),
&mut TransactionErrorMetrics::default(),
)
.unwrap();

// ensure the fee payer is an inspected account
let actual_inspected_accounts: Vec<_> = mock_bank
.inspected_accounts
.read()
.unwrap()
.iter()
.map(|(k, v)| (*k, v.clone()))
.collect();
assert_eq!(
actual_inspected_accounts.as_slice(),
&[(fee_payer_address, vec![(Some(fee_payer_account), true)])],
);
}
}

0 comments on commit d9ece0b

Please sign in to comment.