blockworks-foundation
/
solana
mirror of https://github.com/blockworks-foundation/solana.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Refactor: Remove `visit_each_account_once()` (#25532) 

* Removes is_duplicate().

* Replaces use sites of visit_each_account_once().

* Removes visit_each_account_once().

* cargo fmt

* Adds comments to clarify the order of pre_accounts and instruction_accounts.

* Simplify control flow.
This commit is contained in:
Alexander Meißner 2022-05-26 20:55:58 +02:00 committed by GitHub
parent 86d308ae50
commit 41988807d3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 114 additions and 218 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

163
program-runtime/src/invoke_context.rs
View File

@ -370,31 +370,29 @@ impl<'a> InvokeContext<'a> {
}
self.pre_accounts = Vec::with_capacity(instruction_accounts.len());
let mut work = |_index_in_instruction: usize,
instruction_account: &InstructionAccount| {
if instruction_account.index_in_transaction
< self.transaction_context.get_number_of_accounts()
{
let account = self
.transaction_context
.get_account_at_index(instruction_account.index_in_transaction)?
.borrow()
.clone();
self.pre_accounts.push(PreAccount::new(
self.transaction_context.get_key_of_account_at_index(
instruction_account.index_in_transaction,
)?,
account,
));
return Ok(());
for (index_in_instruction, instruction_account) in
instruction_accounts.iter().enumerate()
{
if index_in_instruction != instruction_account.index_in_callee {
continue; // Skip duplicate account
}
Err(InstructionError::MissingAccount)
};
visit_each_account_once(
instruction_accounts,
&mut work,
InstructionError::NotEnoughAccountKeys,
)?;
if instruction_account.index_in_transaction
>= self.transaction_context.get_number_of_accounts()
{
return Err(InstructionError::MissingAccount);
}
let account = self
.transaction_context
.get_account_at_index(instruction_account.index_in_transaction)?
.borrow()
.clone();
self.pre_accounts.push(PreAccount::new(
self.transaction_context
.get_key_of_account_at_index(instruction_account.index_in_transaction)?,
account,
));
}
} else {
let contains = (0..self
.transaction_context
@ -481,6 +479,9 @@ impl<'a> InvokeContext<'a> {
}
/// Verify the results of an instruction
///
/// Note: `instruction_accounts` must be the same as passed to `InvokeContext::push()`,
/// so that they match the order of `pre_accounts`.
fn verify(
&mut self,
instruction_accounts: &[InstructionAccount],
@ -507,7 +508,10 @@ impl<'a> InvokeContext<'a> {
// Verify the per-account instruction results
let (mut pre_sum, mut post_sum) = (0_u128, 0_u128);
let mut pre_account_index = 0;
let mut work = |_index_in_instruction: usize, instruction_account: &InstructionAccount| {
for (index_in_instruction, instruction_account) in instruction_accounts.iter().enumerate() {
if index_in_instruction != instruction_account.index_in_callee {
continue; // Skip duplicate account
}
{
// Verify account has no outstanding references
let _ = self
@ -560,14 +564,7 @@ impl<'a> InvokeContext<'a> {
self.accounts_data_meter
.adjust_delta_unchecked(data_len_delta);
}
Ok(())
};
visit_each_account_once(
instruction_accounts,
&mut work,
InstructionError::NotEnoughAccountKeys,
)?;
}
// Verify that the total sum of all the lamports did not change
if pre_sum != post_sum {
@ -577,6 +574,9 @@ impl<'a> InvokeContext<'a> {
}
/// Verify and update PreAccount state based on program execution
///
/// Note: `instruction_accounts` must be the same as passed to `InvokeContext::push()`,
/// so that they match the order of `pre_accounts`.
fn verify_and_update(
&mut self,
instruction_accounts: &[InstructionAccount],
@ -592,7 +592,10 @@ impl<'a> InvokeContext<'a> {
// Verify the per-account instruction results
let (mut pre_sum, mut post_sum) = (0_u128, 0_u128);
let mut work = |_index_in_instruction: usize, instruction_account: &InstructionAccount| {
for (index_in_instruction, instruction_account) in instruction_accounts.iter().enumerate() {
if index_in_instruction != instruction_account.index_in_callee {
continue; // Skip duplicate account
}
if instruction_account.index_in_transaction
< transaction_context.get_number_of_accounts()
{
@ -659,17 +662,11 @@ impl<'a> InvokeContext<'a> {
.adjust_delta_unchecked(data_len_delta);
}
return Ok(());
break;
}
}
}
Err(InstructionError::MissingAccount)
};
visit_each_account_once(
instruction_accounts,
&mut work,
InstructionError::NotEnoughAccountKeys,
)?;
}
// Verify that the total sum of all the lamports did not change
if pre_sum != post_sum {
@ -741,7 +738,8 @@ impl<'a> InvokeContext<'a> {
) -> Result<(Vec<InstructionAccount>, Vec<usize>), InstructionError> {
// Finds the index of each account in the instruction by its pubkey.
// Then normalizes / unifies the privileges of duplicate accounts.
// Note: This works like visit_each_account_once() and is an O(n^2) algorithm too.
// Note: This is an O(n^2) algorithm,
// but performed on a very small slice and requires no heap allocations.
let instruction_context = self.transaction_context.get_current_instruction_context()?;
let mut deduplicated_instruction_accounts: Vec<InstructionAccount> = Vec::new();
let mut duplicate_indicies = Vec::with_capacity(instruction.accounts.len());
@ -1265,32 +1263,6 @@ pub fn mock_process_instruction(
transaction_accounts
}
/// Visit each unique instruction account index once
pub fn visit_each_account_once<E>(
instruction_accounts: &[InstructionAccount],
work: &mut dyn FnMut(usize, &InstructionAccount) -> Result<(), E>,
inner_error: E,
) -> Result<(), E> {
// Note: This is an O(n^2) algorithm,
// but performed on a very small slice and requires no heap allocations
'root: for (index, instruction_account) in instruction_accounts.iter().enumerate() {
match instruction_accounts.get(..index) {
Some(range) => {
for (before_index, before) in range.iter().enumerate() {
if before.index_in_transaction == instruction_account.index_in_transaction {
debug_assert_eq!(before_index, instruction_account.index_in_callee);
continue 'root; // skip dups
}
}
debug_assert_eq!(index, instruction_account.index_in_callee);
work(index, instruction_account)?;
}
None => return Err(inner_error),
}
}
Ok(())
}
#[cfg(test)]
mod tests {
use {
@ -1316,59 +1288,6 @@ mod tests {
},
}
#[test]
fn test_visit_each_account_once() {
let do_work = |accounts: &[InstructionAccount]| -> (usize, usize, usize) {
let mut unique_entries = 0;
let mut index_sum_a = 0;
let mut index_sum_b = 0;
let mut work = |index_in_instruction: usize, entry: &InstructionAccount| {
unique_entries += 1;
index_sum_a += index_in_instruction;
index_sum_b += entry.index_in_transaction;
Ok(())
};
visit_each_account_once(accounts, &mut work, InstructionError::NotEnoughAccountKeys)
.unwrap();
(unique_entries, index_sum_a, index_sum_b)
};
assert_eq!(
(3, 3, 19),
do_work(&[
InstructionAccount {
index_in_transaction: 7,
index_in_caller: 0,
index_in_callee: 0,
is_signer: false,
is_writable: false,
},
InstructionAccount {
index_in_transaction: 3,
index_in_caller: 1,
index_in_callee: 1,
is_signer: false,
is_writable: false,
},
InstructionAccount {
index_in_transaction: 9,
index_in_caller: 2,
index_in_callee: 2,
is_signer: false,
is_writable: false,
},
InstructionAccount {
index_in_transaction: 3,
index_in_caller: 1,
index_in_callee: 1,
is_signer: false,
is_writable: false,
},
])
);
}
#[test]
fn test_program_entry_debug() {
#[allow(clippy::unnecessary_wraps)]

30
programs/bpf_loader/src/serialization.rs
View File

@ -14,24 +14,6 @@ use {
std::{io::prelude::*, mem::size_of},
};
/// Look for a duplicate account and return its position if found
pub fn is_duplicate(
instruction_context: &InstructionContext,
index_in_instruction: usize,
) -> Option<usize> {
let index_in_transaction = instruction_context.get_index_in_transaction(index_in_instruction);
let old_approach = (instruction_context.get_number_of_program_accounts()..index_in_instruction)
.position(|index_in_instruction| {
instruction_context.get_index_in_transaction(index_in_instruction)
== index_in_transaction
});
let result = instruction_context
.is_duplicate(index_in_instruction)
.unwrap_or(None);
debug_assert_eq!(result, old_approach);
old_approach
}
pub fn serialize_parameters(
transaction_context: &TransactionContext,
instruction_context: &InstructionContext,
@ -97,7 +79,7 @@ pub fn serialize_parameters_unaligned(
for index_in_instruction in instruction_context.get_number_of_program_accounts()
..instruction_context.get_number_of_accounts()
{
let duplicate = is_duplicate(instruction_context, index_in_instruction);
let duplicate = instruction_context.is_duplicate(index_in_instruction)?;
size += 1; // dup
if duplicate.is_none() {
let data_len = instruction_context
@ -125,7 +107,7 @@ pub fn serialize_parameters_unaligned(
for index_in_instruction in instruction_context.get_number_of_program_accounts()
..instruction_context.get_number_of_accounts()
{
let duplicate = is_duplicate(instruction_context, index_in_instruction);
let duplicate = instruction_context.is_duplicate(index_in_instruction)?;
if let Some(position) = duplicate {
v.write_u8(position as u8)
.map_err(|_| InstructionError::InvalidArgument)?;
@ -179,7 +161,7 @@ pub fn deserialize_parameters_unaligned(
..instruction_context.get_number_of_accounts())
.zip(account_lengths.iter())
{
let duplicate = is_duplicate(instruction_context, index_in_instruction);
let duplicate = instruction_context.is_duplicate(index_in_instruction)?;
start += 1; // is_dup
if duplicate.is_none() {
let mut borrowed_account = instruction_context
@ -217,7 +199,7 @@ pub fn serialize_parameters_aligned(
for index_in_instruction in instruction_context.get_number_of_program_accounts()
..instruction_context.get_number_of_accounts()
{
let duplicate = is_duplicate(instruction_context, index_in_instruction);
let duplicate = instruction_context.is_duplicate(index_in_instruction)?;
size += 1; // dup
if duplicate.is_some() {
size += 7; // padding to 64-bit aligned
@ -251,7 +233,7 @@ pub fn serialize_parameters_aligned(
for index_in_instruction in instruction_context.get_number_of_program_accounts()
..instruction_context.get_number_of_accounts()
{
let duplicate = is_duplicate(instruction_context, index_in_instruction);
let duplicate = instruction_context.is_duplicate(index_in_instruction)?;
if let Some(position) = duplicate {
v.write_u8(position as u8)
.map_err(|_| InstructionError::InvalidArgument)?;
@ -316,7 +298,7 @@ pub fn deserialize_parameters_aligned(
..instruction_context.get_number_of_accounts())
.zip(account_lengths.iter())
{
let duplicate = is_duplicate(instruction_context, index_in_instruction);
let duplicate = instruction_context.is_duplicate(index_in_instruction)?;
start += size_of::<u8>(); // position
if duplicate.is_some() {
start += 7; // padding to 64-bit aligned

139
programs/bpf_loader/src/syscalls.rs
View File

@ -3,7 +3,7 @@ use {
crate::{allocator_bump::BpfAllocator, BpfError},
solana_program_runtime::{
ic_logger_msg, ic_msg,
invoke_context::{visit_each_account_once, ComputeMeter, InvokeContext},
invoke_context::{ComputeMeter, InvokeContext},
stable_log,
timings::ExecuteTimings,
},
@ -2964,80 +2964,75 @@ where
))?;
accounts.push((*program_account_index, None));
visit_each_account_once::<EbpfError<BpfError>>(
instruction_accounts,
&mut |_index: usize, instruction_account: &InstructionAccount| {
let account = invoke_context
.transaction_context
.get_account_at_index(instruction_account.index_in_transaction)
.map_err(SyscallError::InstructionError)?;
let account_key = invoke_context
.transaction_context
.get_key_of_account_at_index(instruction_account.index_in_transaction)
.map_err(SyscallError::InstructionError)?;
if account.borrow().executable() {
// Use the known account
if invoke_context
.feature_set
.is_active(&executables_incur_cpi_data_cost::id())
{
invoke_context
.get_compute_meter()
.consume((account.borrow().data().len() as u64).saturating_div(
invoke_context.get_compute_budget().cpi_bytes_per_unit,
))?;
}
accounts.push((instruction_account.index_in_transaction, None));
} else if let Some(caller_account_index) =
account_info_keys.iter().position(|key| *key == account_key)
for (index_in_instruction, instruction_account) in instruction_accounts.iter().enumerate() {
if index_in_instruction != instruction_account.index_in_callee {
continue; // Skip duplicate account
}
let account = invoke_context
.transaction_context
.get_account_at_index(instruction_account.index_in_transaction)
.map_err(SyscallError::InstructionError)?;
let account_key = invoke_context
.transaction_context
.get_key_of_account_at_index(instruction_account.index_in_transaction)
.map_err(SyscallError::InstructionError)?;
if account.borrow().executable() {
// Use the known account
if invoke_context
.feature_set
.is_active(&executables_incur_cpi_data_cost::id())
{
let mut caller_account = do_translate(
account_infos
.get(caller_account_index)
.ok_or(SyscallError::InvalidLength)?,
invoke_context,
invoke_context.get_compute_meter().consume(
(account.borrow().data().len() as u64)
.saturating_div(invoke_context.get_compute_budget().cpi_bytes_per_unit),
)?;
{
let mut account = account.borrow_mut();
account.copy_into_owner_from_slice(caller_account.owner.as_ref());
account.set_data_from_slice(caller_account.data);
account.set_lamports(*caller_account.lamports);
account.set_executable(caller_account.executable);
account.set_rent_epoch(caller_account.rent_epoch);
}
let caller_account = if instruction_account.is_writable {
let orig_data_lens = invoke_context
.get_orig_account_lengths()
.map_err(SyscallError::InstructionError)?;
caller_account.original_data_len = *orig_data_lens
.get(instruction_account.index_in_caller)
.ok_or_else(|| {
ic_msg!(
invoke_context,
"Internal error: index mismatch for account {}",
account_key
);
SyscallError::InstructionError(InstructionError::MissingAccount)
})?;
Some(caller_account)
} else {
None
};
accounts.push((instruction_account.index_in_transaction, caller_account));
} else {
ic_msg!(
invoke_context,
"Instruction references an unknown account {}",
account_key
);
return Err(
SyscallError::InstructionError(InstructionError::MissingAccount).into(),
);
}
Ok(())
},
SyscallError::InstructionError(InstructionError::NotEnoughAccountKeys).into(),
)?;
accounts.push((instruction_account.index_in_transaction, None));
} else if let Some(caller_account_index) =
account_info_keys.iter().position(|key| *key == account_key)
{
let mut caller_account = do_translate(
account_infos
.get(caller_account_index)
.ok_or(SyscallError::InvalidLength)?,
invoke_context,
)?;
{
let mut account = account.borrow_mut();
account.copy_into_owner_from_slice(caller_account.owner.as_ref());
account.set_data_from_slice(caller_account.data);
account.set_lamports(*caller_account.lamports);
account.set_executable(caller_account.executable);
account.set_rent_epoch(caller_account.rent_epoch);
}
let caller_account = if instruction_account.is_writable {
let orig_data_lens = invoke_context
.get_orig_account_lengths()
.map_err(SyscallError::InstructionError)?;
caller_account.original_data_len = *orig_data_lens
.get(instruction_account.index_in_caller)
.ok_or_else(|| {
ic_msg!(
invoke_context,
"Internal error: index mismatch for account {}",
account_key
);
SyscallError::InstructionError(InstructionError::MissingAccount)
})?;
Some(caller_account)
} else {
None
};
accounts.push((instruction_account.index_in_transaction, caller_account));
} else {
ic_msg!(
invoke_context,
"Instruction references an unknown account {}",
account_key
);
return Err(SyscallError::InstructionError(InstructionError::MissingAccount).into());
}
}
Ok(accounts)
}