Refactor - Simplify program accounts in transaction loading (#29728)
* Refactors the "!validated_fee_payer" case from an "else" branch to an early "return". * Moves the early return upward. * Removes empty entries. * Adds account_found_and_dep_index. * cargo fmt. * Replaces call site of load_executable_accounts(). * Adjusts number of total loaded accounts in test_load_accounts_multiple_loaders(). * Removes test_accounts_account_not_found(). * Removes load_executable_accounts(). * Refactor back to built-in loader ownership chain loop.
This commit is contained in:
Alexander Meißner
GitHub
parent
5e35823b66
commit
aa2e3487ba
|
|
@ -258,6 +258,7 @@ impl Accounts {
|
|||
let mut tx_rent: TransactionRent = 0;
|
||||
let message = tx.message();
|
||||
let account_keys = message.account_keys();
|
||||
let mut account_found_and_dep_index = Vec::with_capacity(account_keys.len());
|
||||
let mut account_deps = Vec::with_capacity(account_keys.len());
|
||||
let mut rent_debits = RentDebits::default();
|
||||
|
||||
|
|
@ -267,113 +268,119 @@ impl Accounts {
|
|||
.iter()
|
||||
.enumerate()
|
||||
.map(|(i, key)| {
|
||||
let account = if !message.is_non_loader_key(i) {
|
||||
// Fill in an empty account for the program slots.
|
||||
AccountSharedData::default()
|
||||
let mut account_found = true;
|
||||
let mut account_dep_index = None;
|
||||
#[allow(clippy::collapsible_else_if)]
|
||||
let account = if solana_sdk::sysvar::instructions::check_id(key) {
|
||||
Self::construct_instructions_account(
|
||||
message,
|
||||
feature_set
|
||||
.is_active(&feature_set::instructions_sysvar_owned_by_sysvar::id()),
|
||||
)
|
||||
} else {
|
||||
#[allow(clippy::collapsible_else_if)]
|
||||
if solana_sdk::sysvar::instructions::check_id(key) {
|
||||
Self::construct_instructions_account(
|
||||
message,
|
||||
feature_set
|
||||
.is_active(&feature_set::instructions_sysvar_owned_by_sysvar::id()),
|
||||
)
|
||||
let (mut account, rent) = if let Some(account_override) =
|
||||
account_overrides.and_then(|overrides| overrides.get(key))
|
||||
{
|
||||
(account_override.clone(), 0)
|
||||
} else {
|
||||
let (mut account, rent) = if let Some(account_override) =
|
||||
account_overrides.and_then(|overrides| overrides.get(key))
|
||||
{
|
||||
(account_override.clone(), 0)
|
||||
} else {
|
||||
self.accounts_db
|
||||
.load_with_fixed_root(ancestors, key)
|
||||
.map(|(mut account, _)| {
|
||||
if message.is_writable(i) {
|
||||
let rent_due = rent_collector
|
||||
.collect_from_existing_account(
|
||||
key,
|
||||
&mut account,
|
||||
self.accounts_db.filler_account_suffix.as_ref(),
|
||||
set_exempt_rent_epoch_max,
|
||||
)
|
||||
.rent_amount;
|
||||
(account, rent_due)
|
||||
} else {
|
||||
(account, 0)
|
||||
}
|
||||
})
|
||||
.unwrap_or_else(|| {
|
||||
let mut default_account = AccountSharedData::default();
|
||||
if set_exempt_rent_epoch_max {
|
||||
// All new accounts must be rent-exempt (enforced in Bank::execute_loaded_transaction).
|
||||
// Currently, rent collection sets rent_epoch to u64::MAX, but initializing the account
|
||||
// with this field already set would allow us to skip rent collection for these accounts.
|
||||
default_account.set_rent_epoch(u64::MAX);
|
||||
}
|
||||
(default_account, 0)
|
||||
})
|
||||
};
|
||||
self.accounts_db
|
||||
.load_with_fixed_root(ancestors, key)
|
||||
.map(|(mut account, _)| {
|
||||
if message.is_writable(i) {
|
||||
let rent_due = rent_collector
|
||||
.collect_from_existing_account(
|
||||
key,
|
||||
&mut account,
|
||||
self.accounts_db.filler_account_suffix.as_ref(),
|
||||
set_exempt_rent_epoch_max,
|
||||
)
|
||||
.rent_amount;
|
||||
(account, rent_due)
|
||||
} else {
|
||||
(account, 0)
|
||||
}
|
||||
})
|
||||
.unwrap_or_else(|| {
|
||||
account_found = false;
|
||||
let mut default_account = AccountSharedData::default();
|
||||
if set_exempt_rent_epoch_max {
|
||||
// All new accounts must be rent-exempt (enforced in Bank::execute_loaded_transaction).
|
||||
// Currently, rent collection sets rent_epoch to u64::MAX, but initializing the account
|
||||
// with this field already set would allow us to skip rent collection for these accounts.
|
||||
default_account.set_rent_epoch(u64::MAX);
|
||||
}
|
||||
(default_account, 0)
|
||||
})
|
||||
};
|
||||
|
||||
if !validated_fee_payer {
|
||||
if i != 0 {
|
||||
warn!("Payer index should be 0! {:?}", tx);
|
||||
}
|
||||
|
||||
Self::validate_fee_payer(
|
||||
key,
|
||||
&mut account,
|
||||
i as IndexOfAccount,
|
||||
error_counters,
|
||||
rent_collector,
|
||||
feature_set,
|
||||
fee,
|
||||
)?;
|
||||
|
||||
validated_fee_payer = true;
|
||||
if !validated_fee_payer && message.is_non_loader_key(i) {
|
||||
if i != 0 {
|
||||
warn!("Payer index should be 0! {:?}", tx);
|
||||
}
|
||||
|
||||
if bpf_loader_upgradeable::check_id(account.owner()) {
|
||||
if message.is_writable(i) && !message.is_upgradeable_loader_present() {
|
||||
error_counters.invalid_writable_account += 1;
|
||||
return Err(TransactionError::InvalidWritableAccount);
|
||||
}
|
||||
Self::validate_fee_payer(
|
||||
key,
|
||||
&mut account,
|
||||
i as IndexOfAccount,
|
||||
error_counters,
|
||||
rent_collector,
|
||||
feature_set,
|
||||
fee,
|
||||
)?;
|
||||
|
||||
if account.executable() {
|
||||
// The upgradeable loader requires the derived ProgramData account
|
||||
if let Ok(UpgradeableLoaderState::Program {
|
||||
programdata_address,
|
||||
}) = account.state()
|
||||
{
|
||||
if let Some((programdata_account, _)) = self
|
||||
.accounts_db
|
||||
.load_with_fixed_root(ancestors, &programdata_address)
|
||||
{
|
||||
account_deps
|
||||
.push((programdata_address, programdata_account));
|
||||
} else {
|
||||
error_counters.account_not_found += 1;
|
||||
return Err(TransactionError::ProgramAccountNotFound);
|
||||
}
|
||||
} else {
|
||||
error_counters.invalid_program_for_execution += 1;
|
||||
return Err(TransactionError::InvalidProgramForExecution);
|
||||
}
|
||||
}
|
||||
} else if account.executable() && message.is_writable(i) {
|
||||
validated_fee_payer = true;
|
||||
}
|
||||
|
||||
if bpf_loader_upgradeable::check_id(account.owner()) {
|
||||
if message.is_writable(i) && !message.is_upgradeable_loader_present() {
|
||||
error_counters.invalid_writable_account += 1;
|
||||
return Err(TransactionError::InvalidWritableAccount);
|
||||
}
|
||||
|
||||
tx_rent += rent;
|
||||
rent_debits.insert(key, rent, account.lamports());
|
||||
|
||||
account
|
||||
if account.executable() {
|
||||
// The upgradeable loader requires the derived ProgramData account
|
||||
if let Ok(UpgradeableLoaderState::Program {
|
||||
programdata_address,
|
||||
}) = account.state()
|
||||
{
|
||||
if let Some((programdata_account, _)) = self
|
||||
.accounts_db
|
||||
.load_with_fixed_root(ancestors, &programdata_address)
|
||||
{
|
||||
account_dep_index =
|
||||
Some(account_keys.len().saturating_add(account_deps.len())
|
||||
as IndexOfAccount);
|
||||
account_deps.push((programdata_address, programdata_account));
|
||||
} else {
|
||||
error_counters.account_not_found += 1;
|
||||
return Err(TransactionError::ProgramAccountNotFound);
|
||||
}
|
||||
} else {
|
||||
error_counters.invalid_program_for_execution += 1;
|
||||
return Err(TransactionError::InvalidProgramForExecution);
|
||||
}
|
||||
}
|
||||
} else if account.executable() && message.is_writable(i) {
|
||||
error_counters.invalid_writable_account += 1;
|
||||
return Err(TransactionError::InvalidWritableAccount);
|
||||
}
|
||||
|
||||
tx_rent += rent;
|
||||
rent_debits.insert(key, rent, account.lamports());
|
||||
|
||||
account
|
||||
};
|
||||
|
||||
account_found_and_dep_index.push((account_found, account_dep_index));
|
||||
Ok((*key, account))
|
||||
})
|
||||
.collect::<Result<Vec<_>>>()?;
|
||||
|
||||
if !validated_fee_payer {
|
||||
error_counters.account_not_found += 1;
|
||||
return Err(TransactionError::AccountNotFound);
|
||||
}
|
||||
|
||||
// Appends the account_deps at the end of the accounts,
|
||||
// this way they can be accessed in a uniform way.
|
||||
// At places where only the accounts are needed,
|
||||
|
|
@ -381,30 +388,70 @@ impl Accounts {
|
|||
// accounts.iter().take(message.account_keys.len())
|
||||
accounts.append(&mut account_deps);
|
||||
|
||||
if validated_fee_payer {
|
||||
let program_indices = message
|
||||
.instructions()
|
||||
.iter()
|
||||
.map(|instruction| {
|
||||
self.load_executable_accounts(
|
||||
ancestors,
|
||||
&mut accounts,
|
||||
instruction.program_id_index as IndexOfAccount,
|
||||
error_counters,
|
||||
)
|
||||
})
|
||||
.collect::<Result<Vec<Vec<IndexOfAccount>>>>()?;
|
||||
|
||||
Ok(LoadedTransaction {
|
||||
accounts,
|
||||
program_indices,
|
||||
rent: tx_rent,
|
||||
rent_debits,
|
||||
let builtins_start_index = accounts.len();
|
||||
let program_indices = message
|
||||
.instructions()
|
||||
.iter()
|
||||
.map(|instruction| {
|
||||
let mut account_indices = Vec::new();
|
||||
let mut program_index = instruction.program_id_index as usize;
|
||||
for _ in 0..5 {
|
||||
let (program_id, program_account) = accounts
|
||||
.get(program_index)
|
||||
.ok_or(TransactionError::ProgramAccountNotFound)?;
|
||||
let (account_found, account_dep_index) = account_found_and_dep_index
|
||||
.get(program_index)
|
||||
.unwrap_or(&(true, None));
|
||||
if native_loader::check_id(program_id) {
|
||||
return Ok(account_indices);
|
||||
}
|
||||
if !account_found {
|
||||
error_counters.account_not_found += 1;
|
||||
return Err(TransactionError::ProgramAccountNotFound);
|
||||
}
|
||||
if !program_account.executable() {
|
||||
error_counters.invalid_program_for_execution += 1;
|
||||
return Err(TransactionError::InvalidProgramForExecution);
|
||||
}
|
||||
account_indices.insert(0, program_index as IndexOfAccount);
|
||||
if let Some(account_index) = account_dep_index {
|
||||
account_indices.insert(0, *account_index);
|
||||
}
|
||||
let owner_id = program_account.owner();
|
||||
if native_loader::check_id(owner_id) {
|
||||
return Ok(account_indices);
|
||||
}
|
||||
program_index = if let Some(owner_index) = accounts
|
||||
.get(builtins_start_index..)
|
||||
.ok_or(TransactionError::ProgramAccountNotFound)?
|
||||
.iter()
|
||||
.position(|(key, _)| key == owner_id)
|
||||
{
|
||||
builtins_start_index.saturating_add(owner_index)
|
||||
} else {
|
||||
let owner_index = accounts.len();
|
||||
if let Some((program_account, _)) =
|
||||
self.accounts_db.load_with_fixed_root(ancestors, owner_id)
|
||||
{
|
||||
accounts.push((*owner_id, program_account));
|
||||
} else {
|
||||
error_counters.account_not_found += 1;
|
||||
return Err(TransactionError::ProgramAccountNotFound);
|
||||
}
|
||||
owner_index
|
||||
};
|
||||
}
|
||||
error_counters.call_chain_too_deep += 1;
|
||||
Err(TransactionError::CallChainTooDeep)
|
||||
})
|
||||
} else {
|
||||
error_counters.account_not_found += 1;
|
||||
Err(TransactionError::AccountNotFound)
|
||||
}
|
||||
.collect::<Result<Vec<Vec<IndexOfAccount>>>>()?;
|
||||
|
||||
Ok(LoadedTransaction {
|
||||
accounts,
|
||||
program_indices,
|
||||
rent: tx_rent,
|
||||
rent_debits,
|
||||
})
|
||||
}
|
||||
|
||||
fn validate_fee_payer(
|
||||
|
|
@ -453,84 +500,6 @@ impl Accounts {
|
|||
)
|
||||
}
|
||||
|
||||
fn load_executable_accounts(
|
||||
&self,
|
||||
ancestors: &Ancestors,
|
||||
accounts: &mut Vec<TransactionAccount>,
|
||||
mut program_account_index: IndexOfAccount,
|
||||
error_counters: &mut TransactionErrorMetrics,
|
||||
) -> Result<Vec<IndexOfAccount>> {
|
||||
let mut account_indices = Vec::new();
|
||||
let mut program_id = match accounts.get(program_account_index as usize) {
|
||||
Some(program_account) => program_account.0,
|
||||
None => {
|
||||
error_counters.account_not_found += 1;
|
||||
return Err(TransactionError::ProgramAccountNotFound);
|
||||
}
|
||||
};
|
||||
let mut depth = 0;
|
||||
while !native_loader::check_id(&program_id) {
|
||||
if depth >= 5 {
|
||||
error_counters.call_chain_too_deep += 1;
|
||||
return Err(TransactionError::CallChainTooDeep);
|
||||
}
|
||||
depth += 1;
|
||||
|
||||
program_account_index = match self
|
||||
.accounts_db
|
||||
.load_with_fixed_root(ancestors, &program_id)
|
||||
{
|
||||
Some((program_account, _)) => {
|
||||
let account_index = accounts.len() as IndexOfAccount;
|
||||
accounts.push((program_id, program_account));
|
||||
account_index
|
||||
}
|
||||
None => {
|
||||
error_counters.account_not_found += 1;
|
||||
return Err(TransactionError::ProgramAccountNotFound);
|
||||
}
|
||||
};
|
||||
let program = &accounts[program_account_index as usize].1;
|
||||
if !program.executable() {
|
||||
error_counters.invalid_program_for_execution += 1;
|
||||
return Err(TransactionError::InvalidProgramForExecution);
|
||||
}
|
||||
|
||||
// Add loader to chain
|
||||
let program_owner = *program.owner();
|
||||
account_indices.insert(0, program_account_index);
|
||||
if bpf_loader_upgradeable::check_id(&program_owner) {
|
||||
// The upgradeable loader requires the derived ProgramData account
|
||||
if let Ok(UpgradeableLoaderState::Program {
|
||||
programdata_address,
|
||||
}) = program.state()
|
||||
{
|
||||
let programdata_account_index = match self
|
||||
.accounts_db
|
||||
.load_with_fixed_root(ancestors, &programdata_address)
|
||||
{
|
||||
Some((programdata_account, _)) => {
|
||||
let account_index = accounts.len() as IndexOfAccount;
|
||||
accounts.push((programdata_address, programdata_account));
|
||||
account_index
|
||||
}
|
||||
None => {
|
||||
error_counters.account_not_found += 1;
|
||||
return Err(TransactionError::ProgramAccountNotFound);
|
||||
}
|
||||
};
|
||||
account_indices.insert(0, programdata_account_index);
|
||||
} else {
|
||||
error_counters.invalid_program_for_execution += 1;
|
||||
return Err(TransactionError::InvalidProgramForExecution);
|
||||
}
|
||||
}
|
||||
|
||||
program_id = program_owner;
|
||||
}
|
||||
Ok(account_indices)
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub fn load_accounts(
|
||||
&self,
|
||||
|
|
@ -1952,7 +1921,7 @@ mod tests {
|
|||
assert_eq!(loaded_accounts.len(), 1);
|
||||
match &loaded_accounts[0] {
|
||||
(Ok(loaded_transaction), _nonce) => {
|
||||
assert_eq!(loaded_transaction.accounts.len(), 6);
|
||||
assert_eq!(loaded_transaction.accounts.len(), 4);
|
||||
assert_eq!(loaded_transaction.accounts[0].1, accounts[0].1);
|
||||
assert_eq!(loaded_transaction.program_indices.len(), 2);
|
||||
assert_eq!(loaded_transaction.program_indices[0].len(), 1);
|
||||
|
|
@ -2403,34 +2372,6 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_accounts_account_not_found() {
|
||||
let accounts = Accounts::new_with_config_for_tests(
|
||||
Vec::new(),
|
||||
&ClusterType::Development,
|
||||
AccountSecondaryIndexes::default(),
|
||||
AccountShrinkThreshold::default(),
|
||||
);
|
||||
let mut error_counters = TransactionErrorMetrics::default();
|
||||
let ancestors = vec![(0, 0)].into_iter().collect();
|
||||
|
||||
let keypair = Keypair::new();
|
||||
let mut account = AccountSharedData::new(1, 0, &Pubkey::default());
|
||||
account.set_executable(true);
|
||||
accounts.store_for_tests(0, &keypair.pubkey(), &account);
|
||||
|
||||
assert_eq!(
|
||||
accounts.load_executable_accounts(
|
||||
&ancestors,
|
||||
&mut vec![(keypair.pubkey(), account)],
|
||||
0,
|
||||
&mut error_counters,
|
||||
),
|
||||
Err(TransactionError::ProgramAccountNotFound)
|
||||
);
|
||||
assert_eq!(error_counters.account_not_found, 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_accounts_empty_bank_hash() {
|
||||
let accounts = Accounts::new_with_config_for_tests(
|
||||
|
|
|
|||
Loading…
Reference in New Issue