gaspersic/solana
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Refactor: Remove Rc from PreAccount and InvokeContext::get_account() (#21882)

Browse Source 
* Removes Rc and RefCell from PreAccount

* Splits get_account() into find_index_of_account() and get_account_at_index()
in order to remove Rc from return type.
This commit is contained in:
Alexander Meißner
2021-12-14 15:44:31 +01:00
committed by GitHub
parent c92c09a8be
commit 4adc8b133f
5 changed files with 74 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
program-runtime/benches/pre_account.rs
View File

@@ -17,7 +17,7 @@ fn bench_verify_account_changes_data(bencher: &mut Bencher) {
let non_owner = pubkey::new_rand(); let non_owner = pubkey::new_rand();
let pre = PreAccount::new( let pre = PreAccount::new(
&pubkey::new_rand(), &pubkey::new_rand(),
&AccountSharedData::new(0, BUFSIZE, &owner), AccountSharedData::new(0, BUFSIZE, &owner),
); );
let post = AccountSharedData::new(0, BUFSIZE, &owner); let post = AccountSharedData::new(0, BUFSIZE, &owner);
assert_eq!( assert_eq!(
@@ -57,7 +57,7 @@ fn bench_verify_account_changes_data(bencher: &mut Bencher) {
let pre = PreAccount::new( let pre = PreAccount::new(
&pubkey::new_rand(), &pubkey::new_rand(),
&AccountSharedData::new(0, BUFSIZE, &owner), AccountSharedData::new(0, BUFSIZE, &owner),
); );
bencher.iter(|| { bencher.iter(|| {
pre.verify( pre.verify(

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

@@ -257,9 +257,9 @@ impl<'a> InvokeContext<'a> {
self.pre_accounts = Vec::with_capacity(instruction.accounts.len()); self.pre_accounts = Vec::with_capacity(instruction.accounts.len());
let mut work = |_unique_index: usize, account_index: usize| { let mut work = |_unique_index: usize, account_index: usize| {
if account_index < self.accounts.len() { if account_index < self.accounts.len() {
let account = self.accounts[account_index].1.borrow(); let account = self.accounts[account_index].1.borrow().clone();
self.pre_accounts self.pre_accounts
.push(PreAccount::new(&self.accounts[account_index].0, &account)); .push(PreAccount::new(&self.accounts[account_index].0, account));
return Ok(()); return Ok(());
} }
Err(InstructionError::MissingAccount) Err(InstructionError::MissingAccount)
@@ -461,7 +461,7 @@ impl<'a> InvokeContext<'a> {
.checked_add(u128::from(account.lamports())) .checked_add(u128::from(account.lamports()))
.ok_or(InstructionError::UnbalancedInstruction)?; .ok_or(InstructionError::UnbalancedInstruction)?;
if is_writable && !pre_account.executable() { if is_writable && !pre_account.executable() {
pre_account.update(&account); pre_account.update(account.clone());
} }
return Ok(()); return Ok(());
} }
@@ -489,12 +489,12 @@ impl<'a> InvokeContext<'a> {
let mut account_indices = Vec::with_capacity(message.account_keys.len()); let mut account_indices = Vec::with_capacity(message.account_keys.len());
let mut prev_account_sizes = Vec::with_capacity(message.account_keys.len()); let mut prev_account_sizes = Vec::with_capacity(message.account_keys.len());
for account_key in message.account_keys.iter() { for account_key in message.account_keys.iter() {
let (account_index, account) = self let account_index = self
.get_account(account_key) .find_index_of_account(account_key)
.ok_or(InstructionError::MissingAccount)?; .ok_or(InstructionError::MissingAccount)?;
let account_length = account.borrow().data().len(); let account_length = self.accounts[account_index].1.borrow().data().len();
account_indices.push(account_index); account_indices.push(account_index);
prev_account_sizes.push((account, account_length)); prev_account_sizes.push((account_index, account_length));
} }
if let Some(instruction_recorder) = &self.instruction_recorder { if let Some(instruction_recorder) = &self.instruction_recorder {
@@ -510,8 +510,10 @@ impl<'a> InvokeContext<'a> {
// Verify the called program has not misbehaved // Verify the called program has not misbehaved
let do_support_realloc = self.feature_set.is_active(&do_support_realloc::id()); let do_support_realloc = self.feature_set.is_active(&do_support_realloc::id());
for (account, prev_size) in prev_account_sizes.iter() { for (account_index, prev_size) in prev_account_sizes.into_iter() {
if !do_support_realloc && *prev_size != account.borrow().data().len() && *prev_size != 0 if !do_support_realloc
&& prev_size != self.accounts[account_index].1.borrow().data().len()
&& prev_size != 0
{ {
// Only support for `CreateAccount` at this time. // Only support for `CreateAccount` at this time.
// Need a way to limit total realloc size across multiple CPI calls // Need a way to limit total realloc size across multiple CPI calls
@@ -595,26 +597,27 @@ impl<'a> InvokeContext<'a> {
// Find and validate executables / program accounts // Find and validate executables / program accounts
let callee_program_id = instruction.program_id; let callee_program_id = instruction.program_id;
let (program_account_index, program_account) = callee_keyed_accounts let program_account_index = callee_keyed_accounts
.iter() .iter()
.find(|keyed_account| &callee_program_id == keyed_account.unsigned_key()) .find(|keyed_account| &callee_program_id == keyed_account.unsigned_key())
.and_then(|_keyed_account| self.get_account(&callee_program_id)) .and_then(|_keyed_account| self.find_index_of_account(&callee_program_id))
.ok_or_else(|| { .ok_or_else(|| {
ic_msg!(self, "Unknown program {}", callee_program_id); ic_msg!(self, "Unknown program {}", callee_program_id);
InstructionError::MissingAccount InstructionError::MissingAccount
})?; })?;
if !program_account.borrow().executable() { let program_account = self.accounts[program_account_index].1.borrow();
if !program_account.executable() {
ic_msg!(self, "Account {} is not executable", callee_program_id); ic_msg!(self, "Account {} is not executable", callee_program_id);
return Err(InstructionError::AccountNotExecutable); return Err(InstructionError::AccountNotExecutable);
} }
let mut program_indices = vec![]; let mut program_indices = vec![];
if program_account.borrow().owner() == &bpf_loader_upgradeable::id() { if program_account.owner() == &bpf_loader_upgradeable::id() {
if let UpgradeableLoaderState::Program { if let UpgradeableLoaderState::Program {
programdata_address, programdata_address,
} = program_account.borrow().state()? } = program_account.state()?
{ {
if let Some((programdata_account_index, _programdata_account)) = if let Some(programdata_account_index) =
self.get_account(&programdata_address) self.find_index_of_account(&programdata_address)
{ {
program_indices.push(programdata_account_index); program_indices.push(programdata_account_index);
} else { } else {
@@ -797,16 +800,21 @@ impl<'a> InvokeContext<'a> {
self.executors.borrow().get(pubkey) self.executors.borrow().get(pubkey)
} }
/// Find an account_index and account by its key /// Finds an account_index by its key
pub fn get_account(&self, pubkey: &Pubkey) -> Option<(usize, Rc<RefCell<AccountSharedData>>)> { pub fn find_index_of_account(&self, pubkey: &Pubkey) -> Option<usize> {
for (index, (key, account)) in self.accounts.iter().enumerate().rev() { for (index, (key, _account)) in self.accounts.iter().enumerate().rev() {
if key == pubkey { if key == pubkey {
return Some((index, account.clone())); return Some(index);
} }
} }
None None
} }
/// Returns an account by its account_index
pub fn get_account_at_index(&self, account_index: usize) -> &RefCell<AccountSharedData> {
&self.accounts[account_index].1
}
/// Get this invocation's compute budget /// Get this invocation's compute budget
pub fn get_compute_budget(&self) -> &ComputeBudget { pub fn get_compute_budget(&self) -> &ComputeBudget {
&self.current_compute_budget &self.current_compute_budget

42
program-runtime/src/pre_account.rs
View File

@@ -8,11 +8,7 @@ use {
system_instruction::MAX_PERMITTED_DATA_LENGTH, system_instruction::MAX_PERMITTED_DATA_LENGTH,
system_program, system_program,
}, },
std::{ std::fmt::Debug,
cell::{Ref, RefCell},
fmt::Debug,
rc::Rc,
},
}; };
// The relevant state of an account before an Instruction executes, used // The relevant state of an account before an Instruction executes, used
@@ -20,14 +16,14 @@ use {
#[derive(Clone, Debug, Default)] #[derive(Clone, Debug, Default)]
pub struct PreAccount { pub struct PreAccount {
key: Pubkey, key: Pubkey,
account: Rc<RefCell<AccountSharedData>>, account: AccountSharedData,
changed: bool, changed: bool,
} }
impl PreAccount { impl PreAccount {
pub fn new(key: &Pubkey, account: &AccountSharedData) -> Self { pub fn new(key: &Pubkey, account: AccountSharedData) -> Self {
Self { Self {
key: *key, key: *key,
account: Rc::new(RefCell::new(account.clone())), account,
changed: false, changed: false,
} }
} }
@@ -42,7 +38,7 @@ impl PreAccount {
outermost_call: bool, outermost_call: bool,
do_support_realloc: bool, do_support_realloc: bool,
) -> Result<(), InstructionError> { ) -> Result<(), InstructionError> {
let pre = self.account.borrow(); let pre = &self.account;
// Only the owner of the account may change owner and // Only the owner of the account may change owner and
// only if the account is writable and // only if the account is writable and
@@ -157,11 +153,10 @@ impl PreAccount {
Ok(()) Ok(())
} }
pub fn update(&mut self, account: &AccountSharedData) { pub fn update(&mut self, account: AccountSharedData) {
let mut pre = self.account.borrow_mut(); let rent_epoch = self.account.rent_epoch();
let rent_epoch = pre.rent_epoch(); self.account = account;
*pre = account.clone(); self.account.set_rent_epoch(rent_epoch);
pre.set_rent_epoch(rent_epoch);
self.changed = true; self.changed = true;
} }
@@ -170,16 +165,16 @@ impl PreAccount {
&self.key &self.key
} }
pub fn data(&self) -> Ref<[u8]> { pub fn data(&self) -> &[u8] {
Ref::map(self.account.borrow(), |account| account.data()) self.account.data()
} }
pub fn lamports(&self) -> u64 { pub fn lamports(&self) -> u64 {
self.account.borrow().lamports() self.account.lamports()
} }
pub fn executable(&self) -> bool { pub fn executable(&self) -> bool {
self.account.borrow().executable() self.account.executable()
} }
pub fn is_zeroed(buf: &[u8]) -> bool { pub fn is_zeroed(buf: &[u8]) -> bool {
@@ -236,10 +231,9 @@ mod tests {
is_writable: true, is_writable: true,
pre: PreAccount::new( pre: PreAccount::new(
&solana_sdk::pubkey::new_rand(), &solana_sdk::pubkey::new_rand(),
&AccountSharedData::from(Account { AccountSharedData::from(Account {
owner: *owner, owner: *owner,
lamports: std::u64::MAX, lamports: std::u64::MAX,
data: vec![],
..Account::default() ..Account::default()
}), }),
), ),
@@ -255,12 +249,12 @@ mod tests {
self self
} }
pub fn executable(mut self, pre: bool, post: bool) -> Self { pub fn executable(mut self, pre: bool, post: bool) -> Self {
self.pre.account.borrow_mut().set_executable(pre); self.pre.account.set_executable(pre);
self.post.set_executable(post); self.post.set_executable(post);
self self
} }
pub fn lamports(mut self, pre: u64, post: u64) -> Self { pub fn lamports(mut self, pre: u64, post: u64) -> Self {
self.pre.account.borrow_mut().set_lamports(pre); self.pre.account.set_lamports(pre);
self.post.set_lamports(post); self.post.set_lamports(post);
self self
} }
@@ -269,12 +263,12 @@ mod tests {
self self
} }
pub fn data(mut self, pre: Vec<u8>, post: Vec<u8>) -> Self { pub fn data(mut self, pre: Vec<u8>, post: Vec<u8>) -> Self {
self.pre.account.borrow_mut().set_data(pre); self.pre.account.set_data(pre);
self.post.set_data(post); self.post.set_data(post);
self self
} }
pub fn rent_epoch(mut self, pre: u64, post: u64) -> Self { pub fn rent_epoch(mut self, pre: u64, post: u64) -> Self {
self.pre.account.borrow_mut().set_rent_epoch(pre); self.pre.account.set_rent_epoch(pre);
self.post.set_rent_epoch(post); self.post.set_rent_epoch(post);
self self
} }

24
program-test/src/lib.rs
View File

@@ -261,8 +261,8 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
let mut account_indices = Vec::with_capacity(message.account_keys.len()); let mut account_indices = Vec::with_capacity(message.account_keys.len());
let mut accounts = Vec::with_capacity(message.account_keys.len()); let mut accounts = Vec::with_capacity(message.account_keys.len());
for (i, account_key) in message.account_keys.iter().enumerate() { for (i, account_key) in message.account_keys.iter().enumerate() {
let ((account_index, account), account_info) = invoke_context let (account_index, account_info) = invoke_context
.get_account(account_key) .find_index_of_account(account_key)
.zip( .zip(
account_infos account_infos
.iter() .iter()
@@ -271,7 +271,9 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
.ok_or(InstructionError::MissingAccount) .ok_or(InstructionError::MissingAccount)
.unwrap(); .unwrap();
{ {
let mut account = account.borrow_mut(); let mut account = invoke_context
.get_account_at_index(account_index)
.borrow_mut();
account.copy_into_owner_from_slice(account_info.owner.as_ref()); account.copy_into_owner_from_slice(account_info.owner.as_ref());
account.set_data_from_slice(&account_info.try_borrow_data().unwrap()); account.set_data_from_slice(&account_info.try_borrow_data().unwrap());
account.set_lamports(account_info.lamports()); account.set_lamports(account_info.lamports());
@@ -284,10 +286,9 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
None None
}; };
account_indices.push(account_index); account_indices.push(account_index);
accounts.push((account, account_info)); accounts.push((account_index, account_info));
} }
let (program_account_index, _program_account) = let program_account_index = invoke_context.find_index_of_account(&program_id).unwrap();
invoke_context.get_account(&program_id).unwrap();
let program_indices = vec![program_account_index]; let program_indices = vec![program_account_index];
// Check Signers // Check Signers
@@ -329,19 +330,20 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs {
.map_err(|err| ProgramError::try_from(err).unwrap_or_else(|err| panic!("{}", err)))?; .map_err(|err| ProgramError::try_from(err).unwrap_or_else(|err| panic!("{}", err)))?;
// Copy writeable account modifications back into the caller's AccountInfos // Copy writeable account modifications back into the caller's AccountInfos
for (account, account_info) in accounts.iter() { for (account_index, account_info) in accounts.into_iter() {
if let Some(account_info) = account_info { if let Some(account_info) = account_info {
**account_info.try_borrow_mut_lamports().unwrap() = account.borrow().lamports(); let account = invoke_context.get_account_at_index(account_index);
let mut data = account_info.try_borrow_mut_data()?;
let account_borrow = account.borrow(); let account_borrow = account.borrow();
**account_info.try_borrow_mut_lamports().unwrap() = account_borrow.lamports();
let mut data = account_info.try_borrow_mut_data()?;
let new_data = account_borrow.data(); let new_data = account_borrow.data();
if account_info.owner != account.borrow().owner() { if account_info.owner != account_borrow.owner() {
// TODO Figure out a better way to allow the System Program to set the account owner // TODO Figure out a better way to allow the System Program to set the account owner
#[allow(clippy::transmute_ptr_to_ptr)] #[allow(clippy::transmute_ptr_to_ptr)]
#[allow(mutable_transmutes)] #[allow(mutable_transmutes)]
let account_info_mut = let account_info_mut =
unsafe { transmute::<&Pubkey, &mut Pubkey>(account_info.owner) }; unsafe { transmute::<&Pubkey, &mut Pubkey>(account_info.owner) };
*account_info_mut = *account.borrow().owner(); *account_info_mut = *account_borrow.owner();
} }
// TODO: Figure out how to allow the System Program to resize the account data // TODO: Figure out how to allow the System Program to resize the account data
assert!( assert!(

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

@@ -17,7 +17,7 @@ use {
vm::{EbpfVm, SyscallObject, SyscallRegistry}, vm::{EbpfVm, SyscallObject, SyscallRegistry},
}, },
solana_sdk::{ solana_sdk::{
account::{AccountSharedData, ReadableAccount, WritableAccount}, account::{ReadableAccount, WritableAccount},
account_info::AccountInfo, account_info::AccountInfo,
blake3, bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable, blake3, bpf_loader, bpf_loader_deprecated, bpf_loader_upgradeable,
clock::Clock, clock::Clock,
@@ -1667,10 +1667,7 @@ struct CallerAccount<'a> {
executable: bool, executable: bool,
rent_epoch: u64, rent_epoch: u64,
} }
type TranslatedAccounts<'a> = ( type TranslatedAccounts<'a> = (Vec<usize>, Vec<(usize, Option<CallerAccount<'a>>)>);
Vec<usize>,
Vec<(Rc<RefCell<AccountSharedData>>, Option<CallerAccount<'a>>)>,
);
/// Implemented by language specific data structure translators /// Implemented by language specific data structure translators
trait SyscallInvokeSigned<'a, 'b> { trait SyscallInvokeSigned<'a, 'b> {
@@ -2210,13 +2207,14 @@ where
let mut account_indices = Vec::with_capacity(message.account_keys.len()); let mut account_indices = Vec::with_capacity(message.account_keys.len());
let mut accounts = Vec::with_capacity(message.account_keys.len()); let mut accounts = Vec::with_capacity(message.account_keys.len());
for (i, account_key) in message.account_keys.iter().enumerate() { for (i, account_key) in message.account_keys.iter().enumerate() {
if let Some((account_index, account)) = invoke_context.get_account(account_key) { if let Some(account_index) = invoke_context.find_index_of_account(account_key) {
let account = invoke_context.get_account_at_index(account_index);
if i == message.instructions[0].program_id_index as usize if i == message.instructions[0].program_id_index as usize
|| account.borrow().executable() || account.borrow().executable()
{ {
// Use the known account // Use the known account
account_indices.push(account_index); account_indices.push(account_index);
accounts.push((account, None)); accounts.push((account_index, None));
continue; continue;
} else if let Some(caller_account_index) = } else if let Some(caller_account_index) =
account_info_keys.iter().position(|key| *key == account_key) account_info_keys.iter().position(|key| *key == account_key)
@@ -2265,7 +2263,7 @@ where
None None
}; };
account_indices.push(account_index); account_indices.push(account_index);
accounts.push((account, caller_account)); accounts.push((account_index, caller_account));
continue; continue;
} }
} }
@@ -2400,9 +2398,11 @@ fn call<'a, 'b: 'a>(
.map_err(SyscallError::InstructionError)?; .map_err(SyscallError::InstructionError)?;
// Copy results back to caller // Copy results back to caller
for (callee_account, caller_account) in accounts.iter_mut() { for (callee_account_index, caller_account) in accounts.iter_mut() {
if let Some(caller_account) = caller_account { if let Some(caller_account) = caller_account {
let callee_account = callee_account.borrow(); let callee_account = invoke_context
.get_account_at_index(*callee_account_index)
.borrow();
*caller_account.lamports = callee_account.lamports(); *caller_account.lamports = callee_account.lamports();
*caller_account.owner = *callee_account.owner(); *caller_account.owner = *callee_account.owner();
let new_len = callee_account.data().len(); let new_len = callee_account.data().len();
@@ -2672,7 +2672,9 @@ mod tests {
solana_rbpf::{ solana_rbpf::{
ebpf::HOST_ALIGN, memory_region::MemoryRegion, user_error::UserError, vm::Config, ebpf::HOST_ALIGN, memory_region::MemoryRegion, user_error::UserError, vm::Config,
}, },
solana_sdk::{bpf_loader, fee_calculator::FeeCalculator, hash::hashv}, solana_sdk::{
account::AccountSharedData, bpf_loader, fee_calculator::FeeCalculator, hash::hashv,
},
std::str::FromStr, std::str::FromStr,
}; };