From 18e9225a752e11d97e0d9faa7104e8a363ef5312 Mon Sep 17 00:00:00 2001 From: Lcchy Date: Mon, 18 Jul 2022 16:13:03 +0000 Subject: [PATCH] Use BPF serialization ABI for native target in program-test; Fix account data resizing and editing (#26507) * Use BPF serialization ABI for native target in program-test (PR22754) * Fixes for the program-test PR22754 rebase on v1.11.1 * Include bug test case in program-test unit tests * Fix segfault on duped accounts * Add CPI create_account test * Revert gitignore addition Co-authored-by: jozanza Co-authored-by: Jon Cinque --- program-test/src/lib.rs | 137 ++++++++++++++-------------------- program-test/tests/cpi.rs | 153 +++++++++++++++++++++++++++++++++++++- 2 files changed, 205 insertions(+), 85 deletions(-) diff --git a/program-test/src/lib.rs b/program-test/src/lib.rs index acfcf1f45..ddfca432d 100644 --- a/program-test/src/lib.rs +++ b/program-test/src/lib.rs @@ -9,6 +9,7 @@ use { log::*, solana_banks_client::start_client, solana_banks_server::banks_server::start_local_server, + solana_bpf_loader_program::serialization::serialize_parameters, solana_program_runtime::{ compute_budget::ComputeBudget, ic_msg, invoke_context::ProcessInstructionWithContext, stable_log, timings::ExecuteTimings, @@ -24,7 +25,7 @@ use { account::{Account, AccountSharedData, ReadableAccount}, account_info::AccountInfo, clock::Slot, - entrypoint::{ProgramResult, SUCCESS}, + entrypoint::{deserialize, ProgramResult, SUCCESS}, feature_set::FEATURE_NAMES, fee_calculator::{FeeCalculator, FeeRateGovernor, DEFAULT_TARGET_LAMPORTS_PER_SIGNATURE}, genesis_config::{ClusterType, GenesisConfig}, @@ -41,13 +42,12 @@ use { solana_vote_program::vote_state::{VoteState, VoteStateVersions}, std::{ cell::RefCell, - collections::HashSet, + collections::{HashMap, HashSet}, convert::TryFrom, fs::File, io::{self, Read}, mem::transmute, path::{Path, PathBuf}, - rc::Rc, sync::{ atomic::{AtomicBool, Ordering}, Arc, RwLock, @@ -112,58 +112,19 @@ pub fn builtin_process_instruction( ); // Copy indices_in_instruction into a HashSet to ensure there are no duplicates - let deduplicated_indices: HashSet = instruction_account_indices.clone().collect(); + let deduplicated_indices: HashSet = instruction_account_indices.collect(); - // Create copies of the accounts - let mut account_copies = deduplicated_indices - .iter() - .map(|instruction_account_index| { - let borrowed_account = instruction_context - .try_borrow_instruction_account(transaction_context, *instruction_account_index)?; - Ok(( - *borrowed_account.get_key(), - *borrowed_account.get_owner(), - borrowed_account.get_lamports(), - borrowed_account.get_data().to_vec(), - )) - }) - .collect::, InstructionError>>()?; + // Serialize entrypoint parameters with BPF ABI + let (mut parameter_bytes, _account_lengths) = serialize_parameters( + invoke_context.transaction_context, + invoke_context + .transaction_context + .get_current_instruction_context()?, + )?; - // Create shared references to account_copies - let account_refs: Vec<_> = account_copies - .iter_mut() - .map(|(key, owner, lamports, data)| { - ( - key, - owner, - Rc::new(RefCell::new(lamports)), - Rc::new(RefCell::new(data.as_mut())), - ) - }) - .collect(); - - // Create AccountInfos - let account_infos = instruction_account_indices - .map(|instruction_account_index| { - let account_copy_index = deduplicated_indices - .iter() - .position(|index| *index == instruction_account_index) - .unwrap(); - let (key, owner, lamports, data) = &account_refs[account_copy_index]; - let borrowed_account = instruction_context - .try_borrow_instruction_account(transaction_context, instruction_account_index)?; - Ok(AccountInfo { - key, - is_signer: borrowed_account.is_signer(), - is_writable: borrowed_account.is_writable(), - lamports: lamports.clone(), - data: data.clone(), - owner, - executable: borrowed_account.is_executable(), - rent_epoch: borrowed_account.get_rent_epoch(), - }) - }) - .collect::, InstructionError>>()?; + // Deserialize data back into instruction params + let (program_id, account_infos, _input) = + unsafe { deserialize(&mut parameter_bytes.as_slice_mut()[0] as *mut u8) }; // Execute the program process_instruction(program_id, &account_infos, instruction_data).map_err(|err| { @@ -173,27 +134,35 @@ pub fn builtin_process_instruction( })?; stable_log::program_success(&log_collector, program_id); + // Lookup table for AccountInfo + let account_info_map: HashMap<_, _> = account_infos.into_iter().map(|a| (a.key, a)).collect(); + + // Re-fetch the instruction context. The previous reference may have been + // invalidated due to the `set_invoke_context` in a CPI. + let transaction_context = &invoke_context.transaction_context; + let instruction_context = transaction_context.get_current_instruction_context()?; + // Commit AccountInfo changes back into KeyedAccounts - for (instruction_account_index, (_key, owner, lamports, data)) in deduplicated_indices - .into_iter() - .zip(account_copies.into_iter()) - { - let mut borrowed_account = instruction_context - .try_borrow_instruction_account(transaction_context, instruction_account_index)?; - if borrowed_account.get_lamports() != lamports { - borrowed_account.set_lamports(lamports)?; - } - // The redundant check helps to avoid the expensive data comparison if we can - match borrowed_account - .can_data_be_resized(data.len()) - .and_then(|_| borrowed_account.can_data_be_changed()) - { - Ok(()) => borrowed_account.set_data(&data)?, - Err(err) if borrowed_account.get_data() != data => return Err(err), - _ => {} - } - if borrowed_account.get_owner() != &owner { - borrowed_account.set_owner(owner.as_ref())?; + for i in deduplicated_indices.into_iter() { + let mut borrowed_account = + instruction_context.try_borrow_instruction_account(transaction_context, i)?; + if borrowed_account.is_writable() { + if let Some(account_info) = account_info_map.get(borrowed_account.get_key()) { + if borrowed_account.get_lamports() != account_info.lamports() { + borrowed_account.set_lamports(account_info.lamports())?; + } + + if borrowed_account + .can_data_be_resized(account_info.data_len()) + .is_ok() + && borrowed_account.can_data_be_changed().is_ok() + { + borrowed_account.set_data(&account_info.data.borrow())?; + } + if borrowed_account.get_owner() != account_info.owner { + borrowed_account.set_owner(account_info.owner.as_ref())?; + } + } } } @@ -276,6 +245,7 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { .iter() .map(|seeds| Pubkey::create_program_address(seeds, caller).unwrap()) .collect::>(); + let (instruction_accounts, program_indices) = invoke_context .prepare_instruction(instruction, &signers) .unwrap(); @@ -363,8 +333,6 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { .unwrap(); let account_info = &account_infos[account_info_index]; **account_info.try_borrow_mut_lamports().unwrap() = borrowed_account.get_lamports(); - let mut data = account_info.try_borrow_mut_data()?; - let new_data = borrowed_account.get_data(); if account_info.owner != borrowed_account.get_owner() { // TODO Figure out a better way to allow the System Program to set the account owner #[allow(clippy::transmute_ptr_to_ptr)] @@ -373,14 +341,17 @@ impl solana_sdk::program_stubs::SyscallStubs for SyscallStubs { unsafe { transmute::<&Pubkey, &mut Pubkey>(account_info.owner) }; *account_info_mut = *borrowed_account.get_owner(); } - // TODO: Figure out how to allow the System Program to resize the account data - assert!( - data.len() == new_data.len(), - "Account data resizing not supported yet: {} -> {}. \ - Consider making this test conditional on `#[cfg(feature = \"test-bpf\")]`", - data.len(), - new_data.len() - ); + + let new_data = borrowed_account.get_data(); + let new_len = new_data.len(); + + // Resize account_info data (grow-only) + if account_info.data_len() < new_len { + account_info.realloc(new_len, false)?; + } + + // Clone the data + let mut data = account_info.try_borrow_mut_data()?; data.clone_from_slice(new_data); } diff --git a/program-test/tests/cpi.rs b/program-test/tests/cpi.rs index 373d3adcd..b0833c95c 100644 --- a/program-test/tests/cpi.rs +++ b/program-test/tests/cpi.rs @@ -2,17 +2,22 @@ use { solana_program_test::{processor, ProgramTest}, solana_sdk::{ account_info::{next_account_info, AccountInfo}, - entrypoint::ProgramResult, + entrypoint::{ProgramResult, MAX_PERMITTED_DATA_INCREASE}, instruction::{AccountMeta, Instruction}, msg, program::invoke, pubkey::Pubkey, + rent::Rent, signature::Signer, + signer::keypair::Keypair, + system_instruction, system_program, + sysvar::Sysvar, transaction::Transaction, }, }; // Process instruction to invoke into another program +// We pass this specific number of accounts in order to test for a reported error. fn invoker_process_instruction( _program_id: &Pubkey, accounts: &[AccountInfo], @@ -23,13 +28,50 @@ fn invoker_process_instruction( let account_info_iter = &mut accounts.iter(); let invoked_program_info = next_account_info(account_info_iter)?; invoke( - &Instruction::new_with_bincode(*invoked_program_info.key, &[0], vec![]), + &Instruction::new_with_bincode( + *invoked_program_info.key, + &[0], + vec![AccountMeta::new_readonly(*invoked_program_info.key, false)], + ), &[invoked_program_info.clone()], )?; msg!("Processing invoker instruction after CPI"); Ok(()) } +// Process instruction to invoke into another program with duplicates +fn invoker_dupes_process_instruction( + _program_id: &Pubkey, + accounts: &[AccountInfo], + _input: &[u8], +) -> ProgramResult { + // if we can call `msg!` successfully, then InvokeContext exists as required + msg!("Processing invoker instruction before CPI"); + let account_info_iter = &mut accounts.iter(); + let invoked_program_info = next_account_info(account_info_iter)?; + invoke( + &Instruction::new_with_bincode( + *invoked_program_info.key, + &[0], + vec![ + AccountMeta::new_readonly(*invoked_program_info.key, false), + AccountMeta::new_readonly(*invoked_program_info.key, false), + AccountMeta::new_readonly(*invoked_program_info.key, false), + AccountMeta::new_readonly(*invoked_program_info.key, false), + ], + ), + &[ + invoked_program_info.clone(), + invoked_program_info.clone(), + invoked_program_info.clone(), + invoked_program_info.clone(), + invoked_program_info.clone(), + ], + )?; + msg!("Processing invoker instruction after CPI"); + Ok(()) +} + // Process instruction to be invoked by another program #[allow(clippy::unnecessary_wraps)] fn invoked_process_instruction( @@ -42,6 +84,37 @@ fn invoked_process_instruction( Ok(()) } +// Process instruction to invoke into system program to create an account +fn invoke_create_account( + program_id: &Pubkey, + accounts: &[AccountInfo], + _input: &[u8], +) -> ProgramResult { + msg!("Processing instruction before system program CPI instruction"); + let account_info_iter = &mut accounts.iter(); + let payer_info = next_account_info(account_info_iter)?; + let create_account_info = next_account_info(account_info_iter)?; + let system_program_info = next_account_info(account_info_iter)?; + let rent = Rent::get()?; + let minimum_balance = rent.minimum_balance(MAX_PERMITTED_DATA_INCREASE); + invoke( + &system_instruction::create_account( + payer_info.key, + create_account_info.key, + minimum_balance, + MAX_PERMITTED_DATA_INCREASE as u64, + program_id, + ), + &[ + payer_info.clone(), + create_account_info.clone(), + system_program_info.clone(), + ], + )?; + msg!("Processing instruction after system program CPI"); + Ok(()) +} + #[tokio::test] async fn cpi() { let invoker_program_id = Pubkey::new_unique(); @@ -77,3 +150,79 @@ async fn cpi() { .await .unwrap(); } + +#[tokio::test] +async fn cpi_dupes() { + let invoker_program_id = Pubkey::new_unique(); + let mut program_test = ProgramTest::new( + "invoker", + invoker_program_id, + processor!(invoker_dupes_process_instruction), + ); + let invoked_program_id = Pubkey::new_unique(); + program_test.add_program( + "invoked", + invoked_program_id, + processor!(invoked_process_instruction), + ); + + let mut context = program_test.start_with_context().await; + let instructions = vec![Instruction::new_with_bincode( + invoker_program_id, + &[0], + vec![ + AccountMeta::new_readonly(invoked_program_id, false), + AccountMeta::new_readonly(invoked_program_id, false), + AccountMeta::new_readonly(invoked_program_id, false), + AccountMeta::new_readonly(invoked_program_id, false), + ], + )]; + + let transaction = Transaction::new_signed_with_payer( + &instructions, + Some(&context.payer.pubkey()), + &[&context.payer], + context.last_blockhash, + ); + + context + .banks_client + .process_transaction(transaction) + .await + .unwrap(); +} + +#[tokio::test] +async fn cpi_create_account() { + let create_account_program_id = Pubkey::new_unique(); + let program_test = ProgramTest::new( + "create_account", + create_account_program_id, + processor!(invoke_create_account), + ); + + let create_account_keypair = Keypair::new(); + let mut context = program_test.start_with_context().await; + let instructions = vec![Instruction::new_with_bincode( + create_account_program_id, + &[0], + vec![ + AccountMeta::new(context.payer.pubkey(), true), + AccountMeta::new(create_account_keypair.pubkey(), true), + AccountMeta::new_readonly(system_program::id(), false), + ], + )]; + + let transaction = Transaction::new_signed_with_payer( + &instructions, + Some(&context.payer.pubkey()), + &[&context.payer, &create_account_keypair], + context.last_blockhash, + ); + + context + .banks_client + .process_transaction(transaction) + .await + .unwrap(); +}