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

Move filter_executable_program_accounts to bank.rs #34004

Merged
merged 1 commit into from
Nov 9, 2023
Merged

Move filter_executable_program_accounts to bank.rs #34004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
272 changes: 1 addition & 271 deletions accounts-db/src/accounts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,7 @@ use {
solana_system_program::{get_system_account_kind, SystemAccountKind},
std::{
cmp::Reverse,
collections::{
hash_map::{self, Entry},
BinaryHeap, HashMap, HashSet,
},
collections::{hash_map, BinaryHeap, HashMap, HashSet},
num::NonZeroUsize,
ops::RangeBounds,
path::PathBuf,
Expand Down Expand Up @@ -638,59 +635,6 @@ impl Accounts {
)
}

/// Returns a hash map of executable program accounts (program accounts that are not writable
/// in the given transactions), and their owners, for the transactions with a valid
/// blockhash or nonce.
pub fn filter_executable_program_accounts<'a>(
&self,
ancestors: &Ancestors,
txs: &[SanitizedTransaction],
lock_results: &mut [TransactionCheckResult],
program_owners: &'a [Pubkey],
hash_queue: &BlockhashQueue,
) -> HashMap<Pubkey, (&'a Pubkey, u64)> {
let mut result: HashMap<Pubkey, (&'a Pubkey, u64)> = HashMap::new();
lock_results.iter_mut().zip(txs).for_each(|etx| {
if let ((Ok(()), nonce), tx) = etx {
if nonce
.as_ref()
.map(|nonce| nonce.lamports_per_signature())
.unwrap_or_else(|| {
hash_queue.get_lamports_per_signature(tx.message().recent_blockhash())
})
.is_some()
{
tx.message()
.account_keys()
.iter()
.for_each(|key| match result.entry(*key) {
Entry::Occupied(mut entry) => {
let (_, count) = entry.get_mut();
saturating_add_assign!(*count, 1);
}
Entry::Vacant(entry) => {
if let Ok(index) = self.accounts_db.account_matches_owners(
ancestors,
key,
program_owners,
) {
program_owners
.get(index)
.map(|owner| entry.insert((owner, 1)));
}
}
});
} else {
// If the transaction's nonce account was not valid, and blockhash is not found,
// the transaction will fail to process. Let's not load any programs from the
// transaction, and update the status of the transaction.
*etx.0 = (Err(TransactionError::BlockhashNotFound), None);
}
}
});
result
}

#[allow(clippy::too_many_arguments)]
pub fn load_accounts(
&self,
Expand Down Expand Up @@ -2000,220 +1944,6 @@ mod tests {
);
}

#[test]
fn test_filter_executable_program_accounts() {
let mut tx_accounts: Vec<TransactionAccount> = Vec::new();

let keypair1 = Keypair::new();
let keypair2 = Keypair::new();

let non_program_pubkey1 = Pubkey::new_unique();
let non_program_pubkey2 = Pubkey::new_unique();
let program1_pubkey = Pubkey::new_unique();
let program2_pubkey = Pubkey::new_unique();
let account1_pubkey = Pubkey::new_unique();
let account2_pubkey = Pubkey::new_unique();
let account3_pubkey = Pubkey::new_unique();
let account4_pubkey = Pubkey::new_unique();

let account5_pubkey = Pubkey::new_unique();

tx_accounts.push((
non_program_pubkey1,
AccountSharedData::new(1, 10, &account5_pubkey),
));
tx_accounts.push((
non_program_pubkey2,
AccountSharedData::new(1, 10, &account5_pubkey),
));
tx_accounts.push((
program1_pubkey,
AccountSharedData::new(40, 1, &account5_pubkey),
));
tx_accounts.push((
program2_pubkey,
AccountSharedData::new(40, 1, &account5_pubkey),
));
tx_accounts.push((
account1_pubkey,
AccountSharedData::new(1, 10, &non_program_pubkey1),
));
tx_accounts.push((
account2_pubkey,
AccountSharedData::new(1, 10, &non_program_pubkey2),
));
tx_accounts.push((
account3_pubkey,
AccountSharedData::new(40, 1, &program1_pubkey),
));
tx_accounts.push((
account4_pubkey,
AccountSharedData::new(40, 1, &program2_pubkey),
));

let accounts = Accounts::new_with_config_for_tests(
Vec::new(),
&ClusterType::Development,
AccountSecondaryIndexes::default(),
AccountShrinkThreshold::default(),
);
for tx_account in tx_accounts.iter() {
accounts.store_for_tests(0, &tx_account.0, &tx_account.1);
}

let mut hash_queue = BlockhashQueue::new(100);

let tx1 = Transaction::new_with_compiled_instructions(
&[&keypair1],
&[non_program_pubkey1],
Hash::new_unique(),
vec![account1_pubkey, account2_pubkey, account3_pubkey],
vec![CompiledInstruction::new(1, &(), vec![0])],
);
hash_queue.register_hash(&tx1.message().recent_blockhash, 0);
let sanitized_tx1 = SanitizedTransaction::from_transaction_for_tests(tx1);

let tx2 = Transaction::new_with_compiled_instructions(
&[&keypair2],
&[non_program_pubkey2],
Hash::new_unique(),
vec![account4_pubkey, account3_pubkey, account2_pubkey],
vec![CompiledInstruction::new(1, &(), vec![0])],
);
hash_queue.register_hash(&tx2.message().recent_blockhash, 0);
let sanitized_tx2 = SanitizedTransaction::from_transaction_for_tests(tx2);

let ancestors = vec![(0, 0)].into_iter().collect();
let owners = &[program1_pubkey, program2_pubkey];
let programs = accounts.filter_executable_program_accounts(
&ancestors,
&[sanitized_tx1, sanitized_tx2],
&mut [(Ok(()), None), (Ok(()), None)],
owners,
&hash_queue,
);

// The result should contain only account3_pubkey, and account4_pubkey as the program accounts
assert_eq!(programs.len(), 2);
assert_eq!(
programs
.get(&account3_pubkey)
.expect("failed to find the program account"),
&(&program1_pubkey, 2)
);
assert_eq!(
programs
.get(&account4_pubkey)
.expect("failed to find the program account"),
&(&program2_pubkey, 1)
);
}

#[test]
fn test_filter_executable_program_accounts_invalid_blockhash() {
let mut tx_accounts: Vec<TransactionAccount> = Vec::new();

let keypair1 = Keypair::new();
let keypair2 = Keypair::new();

let non_program_pubkey1 = Pubkey::new_unique();
let non_program_pubkey2 = Pubkey::new_unique();
let program1_pubkey = Pubkey::new_unique();
let program2_pubkey = Pubkey::new_unique();
let account1_pubkey = Pubkey::new_unique();
let account2_pubkey = Pubkey::new_unique();
let account3_pubkey = Pubkey::new_unique();
let account4_pubkey = Pubkey::new_unique();

let account5_pubkey = Pubkey::new_unique();

tx_accounts.push((
non_program_pubkey1,
AccountSharedData::new(1, 10, &account5_pubkey),
));
tx_accounts.push((
non_program_pubkey2,
AccountSharedData::new(1, 10, &account5_pubkey),
));
tx_accounts.push((
program1_pubkey,
AccountSharedData::new(40, 1, &account5_pubkey),
));
tx_accounts.push((
program2_pubkey,
AccountSharedData::new(40, 1, &account5_pubkey),
));
tx_accounts.push((
account1_pubkey,
AccountSharedData::new(1, 10, &non_program_pubkey1),
));
tx_accounts.push((
account2_pubkey,
AccountSharedData::new(1, 10, &non_program_pubkey2),
));
tx_accounts.push((
account3_pubkey,
AccountSharedData::new(40, 1, &program1_pubkey),
));
tx_accounts.push((
account4_pubkey,
AccountSharedData::new(40, 1, &program2_pubkey),
));

let accounts = Accounts::new_with_config_for_tests(
Vec::new(),
&ClusterType::Development,
AccountSecondaryIndexes::default(),
AccountShrinkThreshold::default(),
);
for tx_account in tx_accounts.iter() {
accounts.store_for_tests(0, &tx_account.0, &tx_account.1);
}

let mut hash_queue = BlockhashQueue::new(100);

let tx1 = Transaction::new_with_compiled_instructions(
&[&keypair1],
&[non_program_pubkey1],
Hash::new_unique(),
vec![account1_pubkey, account2_pubkey, account3_pubkey],
vec![CompiledInstruction::new(1, &(), vec![0])],
);
hash_queue.register_hash(&tx1.message().recent_blockhash, 0);
let sanitized_tx1 = SanitizedTransaction::from_transaction_for_tests(tx1);

let tx2 = Transaction::new_with_compiled_instructions(
&[&keypair2],
&[non_program_pubkey2],
Hash::new_unique(),
vec![account4_pubkey, account3_pubkey, account2_pubkey],
vec![CompiledInstruction::new(1, &(), vec![0])],
);
// Let's not register blockhash from tx2. This should cause the tx2 to fail
let sanitized_tx2 = SanitizedTransaction::from_transaction_for_tests(tx2);

let ancestors = vec![(0, 0)].into_iter().collect();
let owners = &[program1_pubkey, program2_pubkey];
let mut lock_results = vec![(Ok(()), None), (Ok(()), None)];
let programs = accounts.filter_executable_program_accounts(
&ancestors,
&[sanitized_tx1, sanitized_tx2],
&mut lock_results,
owners,
&hash_queue,
);

// The result should contain only account3_pubkey as the program accounts
assert_eq!(programs.len(), 1);
assert_eq!(
programs
.get(&account3_pubkey)
.expect("failed to find the program account"),
&(&program1_pubkey, 1)
);
assert_eq!(lock_results[1].0, Err(TransactionError::BlockhashNotFound));
}

#[test]
fn test_load_accounts_multiple_loaders() {
let mut accounts: Vec<TransactionAccount> = Vec::new();
Expand Down
58 changes: 56 additions & 2 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ use {
std::{
borrow::Cow,
cell::RefCell,
collections::{HashMap, HashSet},
collections::{hash_map::Entry, HashMap, HashSet},
convert::TryFrom,
fmt, mem,
ops::{AddAssign, RangeInclusive},
Expand Down Expand Up @@ -5116,6 +5116,60 @@ impl Bank {
loaded_programs_for_txs
}

/// Returns a hash map of executable program accounts (program accounts that are not writable
/// in the given transactions), and their owners, for the transactions with a valid
/// blockhash or nonce.
fn filter_executable_program_accounts<'a>(
&self,
ancestors: &Ancestors,
txs: &[SanitizedTransaction],
lock_results: &mut [TransactionCheckResult],
program_owners: &'a [Pubkey],
hash_queue: &BlockhashQueue,
) -> HashMap<Pubkey, (&'a Pubkey, u64)> {
let mut result: HashMap<Pubkey, (&'a Pubkey, u64)> = HashMap::new();
lock_results.iter_mut().zip(txs).for_each(|etx| {
if let ((Ok(()), nonce), tx) = etx {
if nonce
.as_ref()
.map(|nonce| nonce.lamports_per_signature())
.unwrap_or_else(|| {
hash_queue.get_lamports_per_signature(tx.message().recent_blockhash())
})
.is_some()
{
tx.message()
.account_keys()
.iter()
.for_each(|key| match result.entry(*key) {
Entry::Occupied(mut entry) => {
let (_, count) = entry.get_mut();
saturating_add_assign!(*count, 1);
}
Entry::Vacant(entry) => {
if let Ok(index) = self
.rc
.accounts
.accounts_db
Copy link
Contributor

Choose a reason for hiding this comment

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

As in load_accounts, the only member of self we need for this function is Arc<AccountsDb>.

I was thinking of maintaining the accounts related functions as standalone and receive the Arc<AccountsDb> as a parameter. Do you think this is also valid for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the final SVM implementation we'll need a trait here instead of a specific class/object. It might be worth keeping the change simple for now. Let's see what trait definition will look like and adjust accordingly.

.account_matches_owners(ancestors, key, program_owners)
{
program_owners
.get(index)
.map(|owner| entry.insert((owner, 1)));
}
}
});
} else {
// If the transaction's nonce account was not valid, and blockhash is not found,
// the transaction will fail to process. Let's not load any programs from the
// transaction, and update the status of the transaction.
*etx.0 = (Err(TransactionError::BlockhashNotFound), None);
}
}
});
result
}

#[allow(clippy::type_complexity)]
pub fn load_and_execute_transactions(
&self,
Expand Down Expand Up @@ -5183,7 +5237,7 @@ impl Bank {
bpf_loader_deprecated::id(),
loader_v4::id(),
];
let mut program_accounts_map = self.rc.accounts.filter_executable_program_accounts(
let mut program_accounts_map = self.filter_executable_program_accounts(
&self.ancestors,
sanitized_txs,
&mut check_results,
Expand Down
Loading