Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calls inspect_account() on the fee payer #2718

Merged
merged 6 commits into from
Aug 31, 2024
Merged

Calls inspect_account() on the fee payer #2718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
31b1b5f
Calls inspect_account() on the fee payer
brooksprumo Aug 23, 2024
eb553b9
pr: adds tests
brooksprumo Aug 27, 2024
2238a6c
pr: add integration test
brooksprumo Aug 28, 2024
5ee617f
clippy
brooksprumo Aug 29, 2024
a2556cc
pr: check non-writable accounts in integration test
brooksprumo Aug 30, 2024
b19d98c
updates to handle the refactor
brooksprumo Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 122 additions & 3 deletions svm/src/account_loader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,12 +608,15 @@ mod tests {
transaction::{Result, SanitizedTransaction, Transaction, TransactionError},
transaction_context::{TransactionAccount, TransactionContext},
},
std::{borrow::Cow, collections::HashMap, sync::Arc},
std::{borrow::Cow, cell::RefCell, collections::HashMap, sync::Arc},
};

#[derive(Default)]
struct TestCallbacks {
accounts_map: HashMap<Pubkey, AccountSharedData>,
#[allow(clippy::type_complexity)]
inspected_accounts:
RefCell<HashMap<Pubkey, Vec<(Option<AccountSharedData>, /* is_writable */ bool)>>>,
}

impl TransactionProcessingCallback for TestCallbacks {
Expand All @@ -624,6 +627,23 @@ mod tests {
fn get_account_shared_data(&self, pubkey: &Pubkey) -> Option<AccountSharedData> {
self.accounts_map.get(pubkey).cloned()
}

fn inspect_account(
&self,
address: &Pubkey,
account_state: AccountState,
is_writable: bool,
) {
let account = match account_state {
AccountState::Dead => None,
AccountState::Alive(account) => Some(account.clone()),
};
self.inspected_accounts
.borrow_mut()
.entry(*address)
.or_default()
.push((account, is_writable));
}
}

fn load_accounts_with_features_and_rent(
Expand All @@ -640,7 +660,10 @@ mod tests {
for (pubkey, account) in accounts {
accounts_map.insert(*pubkey, account.clone());
}
let callbacks = TestCallbacks { accounts_map };
let callbacks = TestCallbacks {
accounts_map,
..Default::default()
};
load_accounts(
&callbacks,
&[sanitized_tx],
Expand Down Expand Up @@ -929,7 +952,10 @@ mod tests {
for (pubkey, account) in accounts {
accounts_map.insert(*pubkey, account.clone());
}
let callbacks = TestCallbacks { accounts_map };
let callbacks = TestCallbacks {
accounts_map,
..Default::default()
};
load_accounts(
&callbacks,
&[tx],
Expand Down Expand Up @@ -2108,4 +2134,97 @@ mod tests {
assert_eq!(account.rent_epoch(), 0);
assert_eq!(account.lamports(), 0);
}

// Ensure `TransactionProcessingCallback::inspect_account()` is called when
// loading accounts for transaction processing.
#[test]
fn test_inspect_account_non_fee_payer() {
let mut mock_bank = TestCallbacks::default();

let address0 = Pubkey::new_unique(); // <-- fee payer
let address1 = Pubkey::new_unique(); // <-- initially alive
let address2 = Pubkey::new_unique(); // <-- initially dead
let address3 = Pubkey::new_unique(); // <-- program

let mut account0 = AccountSharedData::default();
account0.set_lamports(1_000_000_000);
mock_bank.accounts_map.insert(address0, account0.clone());

let mut account1 = AccountSharedData::default();
account1.set_lamports(2_000_000_000);
mock_bank.accounts_map.insert(address1, account1.clone());

// account2 *not* added to the bank's accounts_map

let mut account3 = AccountSharedData::default();
account3.set_lamports(4_000_000_000);
account3.set_executable(true);
account3.set_owner(native_loader::id());
mock_bank.accounts_map.insert(address3, account3.clone());

let message = Message {
account_keys: vec![address0, address1, address2, address3],
header: MessageHeader::default(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess this default header is causing all accounts to be loaded as writable because it means there are 0 signers and 0 readonly non-signers? And the program is loaded as readonly because it was demoted by demote_program_id? Do you mind adding comments describing this behavior?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can add a comment. I'm not familiar with these specifics; why is that information germane to this test?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's important that account inspection correctly captures whether an account is loaded with a write lock, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think no, not this test. This test only cares that the accounts are being inspected. The writability is assumed to be correct. (I imagine there are tests that are test this, yes?)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that seems reasonable to me

instructions: vec![
CompiledInstruction {
program_id_index: 3,
accounts: vec![0],
data: vec![],
},
CompiledInstruction {
program_id_index: 3,
accounts: vec![1, 2],
data: vec![],
},
CompiledInstruction {
program_id_index: 3,
accounts: vec![1],
data: vec![],
},
],
recent_blockhash: Hash::new_unique(),
};
let sanitized_message = new_unchecked_sanitized_message(message);
let sanitized_transaction = SanitizedTransaction::new_for_tests(
sanitized_message,
vec![Signature::new_unique()],
false,
);
let validation_result = Ok(ValidatedTransactionDetails {
loaded_fee_payer_account: LoadedTransactionAccount {
account: account0.clone(),
..LoadedTransactionAccount::default()
},
..ValidatedTransactionDetails::default()
});
let _load_results = load_accounts(
&mock_bank,
&[sanitized_transaction],
vec![validation_result],
&mut TransactionErrorMetrics::default(),
None,
&FeatureSet::default(),
&RentCollector::default(),
&ProgramCacheForTxBatch::default(),
);

// ensure the loaded accounts are inspected
let mut actual_inspected_accounts: Vec<_> = mock_bank
.inspected_accounts
.borrow()
.iter()
.map(|(k, v)| (*k, v.clone()))
.collect();
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
(address1, vec![(Some(account1), true)]),
(address2, vec![(None, true)]),
(address3, vec![(Some(account3), false)]),
];
expected_inspected_accounts.sort_unstable_by(|a, b| a.0.cmp(&b.0));

assert_eq!(actual_inspected_accounts, expected_inspected_accounts,);
}
}
94 changes: 93 additions & 1 deletion svm/src/transaction_processor.rs
View file
Open in desktop
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::TransactionProcessingCallback,
transaction_processing_callback::{AccountState, TransactionProcessingCallback},
transaction_processing_result::{ProcessedTransaction, TransactionProcessingResult},
},
log::debug,
Expand Down Expand Up @@ -434,6 +434,12 @@ 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 @@ -1034,6 +1040,9 @@ mod tests {
#[derive(Default, Clone)]
pub struct MockBankCallback {
pub account_shared_data: Arc<RwLock<HashMap<Pubkey, AccountSharedData>>>,
#[allow(clippy::type_complexity)]
pub inspected_accounts:
Arc<RwLock<HashMap<Pubkey, Vec<(Option<AccountSharedData>, /* is_writable */ bool)>>>>,
}

impl TransactionProcessingCallback for MockBankCallback {
Expand Down Expand Up @@ -1065,6 +1074,24 @@ mod tests {
.unwrap()
.insert(*program_id, account_data);
}

fn inspect_account(
&self,
address: &Pubkey,
account_state: AccountState,
is_writable: bool,
) {
let account = match account_state {
AccountState::Dead => None,
AccountState::Alive(account) => Some(account.clone()),
};
self.inspected_accounts
.write()
.unwrap()
.entry(*address)
.or_default()
.push((account, is_writable));
}
}

#[test]
Expand Down Expand Up @@ -1853,6 +1880,7 @@ mod tests {
mock_accounts.insert(*fee_payer_address, fee_payer_account.clone());
let mock_bank = MockBankCallback {
account_shared_data: Arc::new(RwLock::new(mock_accounts)),
..Default::default()
};

let mut error_counters = TransactionErrorMetrics::default();
Expand Down Expand Up @@ -1930,6 +1958,7 @@ mod tests {
mock_accounts.insert(*fee_payer_address, fee_payer_account.clone());
let mock_bank = MockBankCallback {
account_shared_data: Arc::new(RwLock::new(mock_accounts)),
..Default::default()
};

let mut error_counters = TransactionErrorMetrics::default();
Expand Down Expand Up @@ -2014,6 +2043,7 @@ mod tests {
mock_accounts.insert(*fee_payer_address, fee_payer_account.clone());
let mock_bank = MockBankCallback {
account_shared_data: Arc::new(RwLock::new(mock_accounts)),
..Default::default()
};

let mut error_counters = TransactionErrorMetrics::default();
Expand Down Expand Up @@ -2051,6 +2081,7 @@ mod tests {
mock_accounts.insert(*fee_payer_address, fee_payer_account.clone());
let mock_bank = MockBankCallback {
account_shared_data: Arc::new(RwLock::new(mock_accounts)),
..Default::default()
};

let mut error_counters = TransactionErrorMetrics::default();
Expand Down Expand Up @@ -2086,6 +2117,7 @@ mod tests {
mock_accounts.insert(*fee_payer_address, fee_payer_account.clone());
let mock_bank = MockBankCallback {
account_shared_data: Arc::new(RwLock::new(mock_accounts)),
..Default::default()
};

let mut error_counters = TransactionErrorMetrics::default();
Expand Down Expand Up @@ -2177,6 +2209,7 @@ mod tests {
mock_accounts.insert(*fee_payer_address, fee_payer_account.clone());
let mock_bank = MockBankCallback {
account_shared_data: Arc::new(RwLock::new(mock_accounts)),
..Default::default()
};

let mut error_counters = TransactionErrorMetrics::default();
Expand Down Expand Up @@ -2242,6 +2275,7 @@ mod tests {
mock_accounts.insert(*fee_payer_address, fee_payer_account.clone());
let mock_bank = MockBankCallback {
account_shared_data: Arc::new(RwLock::new(mock_accounts)),
..Default::default()
};

let mut error_counters = TransactionErrorMetrics::default();
Expand Down Expand Up @@ -2294,6 +2328,7 @@ mod tests {

let mock_bank = MockBankCallback {
account_shared_data: Arc::new(RwLock::new(mock_accounts)),
..Default::default()
};

let mut error_counters = TransactionErrorMetrics::default();
Expand All @@ -2318,4 +2353,61 @@ 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)])],
);
}
}
Loading