From ab205b682a26bc073bfddbea5388d5bf2dcb5678 Mon Sep 17 00:00:00 2001 From: Jack May Date: Tue, 22 Dec 2020 09:26:55 -0800 Subject: [PATCH] Upgradeable programs called same as non-upgradeable (#14239) * Upgradeable programs called same as non-upgradeable * nudge --- .../bpf/rust/invoke_and_return/src/lib.rs | 11 +- programs/bpf/tests/programs.rs | 101 +++++++++++++----- programs/bpf_loader/src/lib.rs | 1 + programs/bpf_loader/src/syscalls.rs | 9 +- runtime/src/accounts.rs | 75 ++++++++++--- runtime/src/bank.rs | 27 +++-- runtime/src/message_processor.rs | 60 ++++++++--- sdk/src/process_instruction.rs | 4 +- 8 files changed, 212 insertions(+), 76 deletions(-) diff --git a/programs/bpf/rust/invoke_and_return/src/lib.rs b/programs/bpf/rust/invoke_and_return/src/lib.rs index 8855bfcead..5698103f11 100644 --- a/programs/bpf/rust/invoke_and_return/src/lib.rs +++ b/programs/bpf/rust/invoke_and_return/src/lib.rs @@ -2,8 +2,8 @@ //! uses the instruction data provided and all the accounts use solana_program::{ - account_info::AccountInfo, bpf_loader_upgradeable, entrypoint, entrypoint::ProgramResult, - instruction::AccountMeta, instruction::Instruction, program::invoke, pubkey::Pubkey, + account_info::AccountInfo, entrypoint, entrypoint::ProgramResult, instruction::AccountMeta, + instruction::Instruction, program::invoke, pubkey::Pubkey, }; entrypoint!(process_instruction); @@ -15,13 +15,8 @@ fn process_instruction( ) -> ProgramResult { let to_call = accounts[0].key; let infos = accounts; - let last = if bpf_loader_upgradeable::check_id(accounts[0].owner) { - accounts.len() - 1 - } else { - accounts.len() - }; let instruction = Instruction { - accounts: accounts[1..last] + accounts: accounts[1..] .iter() .map(|acc| AccountMeta { pubkey: *acc.key, diff --git a/programs/bpf/tests/programs.rs b/programs/bpf/tests/programs.rs index 67dab91925..8d0aee15da 100644 --- a/programs/bpf/tests/programs.rs +++ b/programs/bpf/tests/programs.rs @@ -21,9 +21,7 @@ use solana_runtime::{ }; use solana_sdk::{ account::Account, - account_utils::StateMut, bpf_loader, bpf_loader_deprecated, - bpf_loader_upgradeable::UpgradeableLoaderState, client::SyncClient, clock::{DEFAULT_SLOTS_PER_EPOCH, MAX_PROCESSING_AGE}, entrypoint::{MAX_PERMITTED_DATA_INCREASE, SUCCESS}, @@ -1479,11 +1477,11 @@ fn test_program_bpf_upgrade() { bank.add_builtin(&name, id, entrypoint); let bank_client = BankClient::new(bank); - // deploy upgrade program + // Deploy upgrade program let (program_id, authority_keypair) = load_upgradeable_bpf_program(&bank_client, &mint_keypair, "solana_bpf_rust_upgradeable"); - // call upgrade program + // Call upgrade program nonce += 1; let instruction = Instruction::new( program_id, @@ -1499,7 +1497,7 @@ fn test_program_bpf_upgrade() { TransactionError::InstructionError(0, InstructionError::Custom(42)) ); - // upgrade program + // Upgrade program upgrade_bpf_program( &bank_client, &mint_keypair, @@ -1508,7 +1506,7 @@ fn test_program_bpf_upgrade() { "solana_bpf_rust_upgraded", ); - // call upgraded program + // Call upgraded program nonce += 1; let instruction = Instruction::new( program_id, @@ -1524,7 +1522,7 @@ fn test_program_bpf_upgrade() { TransactionError::InstructionError(0, InstructionError::Custom(43)) ); - // upgrade back to the original program + // Set a new authority let new_authority_keypair = Keypair::new(); set_upgrade_authority( &bank_client, @@ -1534,7 +1532,7 @@ fn test_program_bpf_upgrade() { Some(&new_authority_keypair.pubkey()), ); - // upgrade back to the original program + // Upgrade back to the original program upgrade_bpf_program( &bank_client, &mint_keypair, @@ -1543,7 +1541,7 @@ fn test_program_bpf_upgrade() { "solana_bpf_rust_upgradeable", ); - // call original program + // Call original program nonce += 1; let instruction = Instruction::new( program_id, @@ -1562,9 +1560,10 @@ fn test_program_bpf_upgrade() { #[cfg(feature = "bpf_rust")] #[test] -fn test_program_bpf_invoke_upgradeable() { +fn test_program_bpf_invoke_upgradeable_via_cpi() { solana_logger::setup(); + let mut nonce = 0; let GenesisConfigInfo { genesis_config, mint_keypair, @@ -1583,29 +1582,81 @@ fn test_program_bpf_invoke_upgradeable() { "solana_bpf_rust_invoke_and_return", ); - // deploy upgrade program - let (program_id, _) = + // Deploy upgrade program + let (program_id, authority_keypair) = load_upgradeable_bpf_program(&bank_client, &mint_keypair, "solana_bpf_rust_upgradeable"); - let data = bank_client.get_account(&program_id).unwrap().unwrap(); - let programdata_address = if let UpgradeableLoaderState::Program { - programdata_address, - } = data.state().unwrap() - { - programdata_address - } else { - panic!("Not a program"); - }; - - // call invoker program to invoke the upgradeable program + // Call invoker program to invoke the upgradeable program + nonce += 1; let instruction = Instruction::new( invoke_and_return, - &[0], + &[nonce], + vec![ + AccountMeta::new(program_id, false), + AccountMeta::new(clock::id(), false), + AccountMeta::new(fees::id(), false), + ], + ); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + assert_eq!( + result.unwrap_err().unwrap(), + TransactionError::InstructionError(0, InstructionError::Custom(42)) + ); + + // Upgrade program + upgrade_bpf_program( + &bank_client, + &mint_keypair, + &program_id, + &authority_keypair, + "solana_bpf_rust_upgraded", + ); + + // Call the upgraded program + nonce += 1; + let instruction = Instruction::new( + invoke_and_return, + &[nonce], + vec![ + AccountMeta::new(program_id, false), + AccountMeta::new(clock::id(), false), + AccountMeta::new(fees::id(), false), + ], + ); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + assert_eq!( + result.unwrap_err().unwrap(), + TransactionError::InstructionError(0, InstructionError::Custom(43)) + ); + + // Set a new authority + let new_authority_keypair = Keypair::new(); + set_upgrade_authority( + &bank_client, + &mint_keypair, + &program_id, + &authority_keypair, + Some(&new_authority_keypair.pubkey()), + ); + + // Upgrade back to the original program + upgrade_bpf_program( + &bank_client, + &mint_keypair, + &program_id, + &new_authority_keypair, + "solana_bpf_rust_upgradeable", + ); + + // Call original program + nonce += 1; + let instruction = Instruction::new( + invoke_and_return, + &[nonce], vec![ AccountMeta::new(program_id, false), AccountMeta::new(clock::id(), false), AccountMeta::new(fees::id(), false), - AccountMeta::new(programdata_address, false), ], ); let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 10d54e1ef2..6f37cf96c3 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -993,6 +993,7 @@ mod tests { Rent::default(), vec![], &[], + &[], None, BpfComputeBudget { max_units: 1, diff --git a/programs/bpf_loader/src/syscalls.rs b/programs/bpf_loader/src/syscalls.rs index f034841828..b3ab065927 100644 --- a/programs/bpf_loader/src/syscalls.rs +++ b/programs/bpf_loader/src/syscalls.rs @@ -1389,18 +1389,19 @@ fn call<'a>( .ok_or(SyscallError::InstructionError( InstructionError::MissingAccount, ))?; - if !program_account.executable { + if !program_account.borrow().executable { return Err(SyscallError::InstructionError(InstructionError::AccountNotExecutable).into()); } - let programdata_executable = if program_account.owner == bpf_loader_upgradeable::id() { + let programdata_executable = if program_account.borrow().owner == bpf_loader_upgradeable::id() { if let UpgradeableLoaderState::Program { programdata_address, } = program_account + .borrow() .state() .map_err(SyscallError::InstructionError)? { if let Some(account) = invoke_context.get_account(&programdata_address) { - Some((programdata_address, RefCell::new(account))) + Some((programdata_address, account)) } else { return Err( SyscallError::InstructionError(InstructionError::MissingAccount).into(), @@ -1412,7 +1413,7 @@ fn call<'a>( } else { None }; - let mut executable_accounts = vec![(callee_program_id, RefCell::new(program_account))]; + let mut executable_accounts = vec![(callee_program_id, program_account)]; if let Some(programdata) = programdata_executable { executable_accounts.push(programdata); } diff --git a/runtime/src/accounts.rs b/runtime/src/accounts.rs index 76cd4ba482..75ed017f77 100644 --- a/runtime/src/accounts.rs +++ b/runtime/src/accounts.rs @@ -61,11 +61,17 @@ pub struct Accounts { // for the load instructions pub type TransactionAccounts = Vec; +pub type TransactionAccountDeps = Vec<(Pubkey, Account)>; pub type TransactionRent = u64; pub type TransactionLoaders = Vec>; pub type TransactionLoadResult = ( - Result<(TransactionAccounts, TransactionLoaders, TransactionRent)>, + Result<( + TransactionAccounts, + TransactionAccountDeps, + TransactionLoaders, + TransactionRent, + )>, Option, ); @@ -137,7 +143,7 @@ impl Accounts { error_counters: &mut ErrorCounters, rent_collector: &RentCollector, feature_set: &FeatureSet, - ) -> Result<(TransactionAccounts, TransactionRent)> { + ) -> Result<(TransactionAccounts, TransactionAccountDeps, TransactionRent)> { // Copy all the accounts let message = tx.message(); if tx.signatures.is_empty() && fee != 0 { @@ -148,6 +154,7 @@ impl Accounts { let mut payer_index = None; let mut tx_rent: TransactionRent = 0; let mut accounts = Vec::with_capacity(message.account_keys.len()); + let mut account_deps = Vec::with_capacity(message.account_keys.len()); let rent_fix_enabled = feature_set.cumulative_rent_related_fixes_enabled(); for (i, key) in message.account_keys.iter().enumerate() { @@ -181,6 +188,28 @@ impl Accounts { }) .unwrap_or_default(); + if account.executable && bpf_loader_upgradeable::check_id(&account.owner) { + // The upgradeable loader requires the derived ProgramData account + if let Ok(UpgradeableLoaderState::Program { + programdata_address, + }) = account.state() + { + if let Some(account) = self + .accounts_db + .load(ancestors, &programdata_address) + .map(|(account, _)| account) + { + account_deps.push((programdata_address, account)); + } else { + error_counters.account_not_found += 1; + return Err(TransactionError::ProgramAccountNotFound); + } + } else { + error_counters.invalid_program_for_execution += 1; + return Err(TransactionError::InvalidProgramForExecution); + } + } + tx_rent += rent; account } @@ -216,7 +245,7 @@ impl Accounts { Err(TransactionError::InsufficientFundsForFee) } else { accounts[payer_index].lamports -= fee; - Ok((accounts, tx_rent)) + Ok((accounts, account_deps, tx_rent)) } } } else { @@ -357,8 +386,8 @@ impl Accounts { rent_collector, feature_set, ); - let (accounts, rents) = match load_res { - Ok((a, r)) => (a, r), + let (accounts, account_deps, rents) = match load_res { + Ok((a, d, r)) => (a, d, r), Err(e) => return (Err(e), None), }; @@ -382,7 +411,7 @@ impl Accounts { None }; - (Ok((accounts, loaders, rents)), nonce_rollback) + (Ok((accounts, account_deps, loaders, rents)), nonce_rollback) } (_, (Err(e), _nonce_rollback)) => (Err(e), None), }) @@ -885,7 +914,7 @@ impl Accounts { } } if account.rent_epoch == 0 { - acc.2 += rent_collector.collect_from_created_account( + acc.3 += rent_collector.collect_from_created_account( &key, account, rent_fix_enabled, @@ -1226,7 +1255,7 @@ mod tests { ); assert_eq!(loaded_accounts.len(), 1); let (load_res, _nonce_rollback) = &loaded_accounts[0]; - let (tx_accounts, _loaders, _rents) = load_res.as_ref().unwrap(); + let (tx_accounts, _account_deps, _loaders, _rents) = load_res.as_ref().unwrap(); assert_eq!(tx_accounts[0].lamports, min_balance); // Fee leaves zero balance fails @@ -1288,7 +1317,12 @@ mod tests { assert_eq!(loaded_accounts.len(), 1); match &loaded_accounts[0] { ( - Ok((transaction_accounts, transaction_loaders, _transaction_rents)), + Ok(( + transaction_accounts, + _transaction_account_deps, + transaction_loaders, + _transaction_rents, + )), _nonce_rollback, ) => { assert_eq!(transaction_accounts.len(), 3); @@ -1514,7 +1548,12 @@ mod tests { assert_eq!(loaded_accounts.len(), 1); match &loaded_accounts[0] { ( - Ok((transaction_accounts, transaction_loaders, _transaction_rents)), + Ok(( + transaction_accounts, + _transaction_account_deps, + transaction_loaders, + _transaction_rents, + )), _nonce_rollback, ) => { assert_eq!(transaction_accounts.len(), 3); @@ -1811,6 +1850,7 @@ mod tests { let loaded0 = ( Ok(( transaction_accounts0, + vec![], transaction_loaders0, transaction_rent0, )), @@ -1823,6 +1863,7 @@ mod tests { let loaded1 = ( Ok(( transaction_accounts1, + vec![], transaction_loaders1, transaction_rent1, )), @@ -2193,7 +2234,12 @@ mod tests { let transaction_loaders = vec![]; let transaction_rent = 0; let loaded = ( - Ok((transaction_accounts, transaction_loaders, transaction_rent)), + Ok(( + transaction_accounts, + vec![], + transaction_loaders, + transaction_rent, + )), nonce_rollback, ); @@ -2298,7 +2344,12 @@ mod tests { let transaction_loaders = vec![]; let transaction_rent = 0; let loaded = ( - Ok((transaction_accounts, transaction_loaders, transaction_rent)), + Ok(( + transaction_accounts, + vec![], + transaction_loaders, + transaction_rent, + )), nonce_rollback, ); diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index fe786c6ae7..58b11e9ac8 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -4,8 +4,8 @@ //! already been signed and verified. use crate::{ accounts::{ - AccountAddressFilter, Accounts, TransactionAccounts, TransactionLoadResult, - TransactionLoaders, + AccountAddressFilter, Accounts, TransactionAccountDeps, TransactionAccounts, + TransactionLoadResult, TransactionLoaders, }, accounts_db::{ErrorCounters, SnapshotStorages}, accounts_index::Ancestors, @@ -117,6 +117,7 @@ type BankStatusCache = StatusCache>; #[frozen_abi(digest = "GSPuprru1pomsgvopKG7XRWiXdqdXJdLPkgJ2arPbkXM")] pub type BankSlotDelta = SlotDelta>; type TransactionAccountRefCells = Vec>>; +type TransactionAccountDepRefCells = Vec<(Pubkey, RefCell)>; type TransactionLoaderRefCells = Vec)>>; // Eager rent collection repeats in cyclic manner. @@ -2699,12 +2700,21 @@ impl Bank { /// ownership by draining the source fn accounts_to_refcells( accounts: &mut TransactionAccounts, + account_deps: &mut TransactionAccountDeps, loaders: &mut TransactionLoaders, - ) -> (TransactionAccountRefCells, TransactionLoaderRefCells) { + ) -> ( + TransactionAccountRefCells, + TransactionAccountDepRefCells, + TransactionLoaderRefCells, + ) { let account_refcells: Vec<_> = accounts .drain(..) .map(|account| Rc::new(RefCell::new(account))) .collect(); + let account_dep_refcells: Vec<_> = account_deps + .drain(..) + .map(|(pubkey, account_dep)| (pubkey, RefCell::new(account_dep))) + .collect(); let loader_refcells: Vec> = loaders .iter_mut() .map(|v| { @@ -2713,7 +2723,7 @@ impl Bank { .collect() }) .collect(); - (account_refcells, loader_refcells) + (account_refcells, account_dep_refcells, loader_refcells) } /// Converts back from RefCell to Account, this involves moving @@ -2865,13 +2875,13 @@ impl Bank { .zip(OrderedIterator::new(txs, batch.iteration_order())) .map(|(accs, (_, tx))| match accs { (Err(e), _nonce_rollback) => (Err(e.clone()), None), - (Ok((accounts, loaders, _rents)), nonce_rollback) => { + (Ok((accounts, account_deps, loaders, _rents)), nonce_rollback) => { signature_count += u64::from(tx.message().header.num_required_signatures); let executors = self.get_executors(&tx.message, &loaders); - let (account_refcells, loader_refcells) = - Self::accounts_to_refcells(accounts, loaders); + let (account_refcells, account_dep_refcells, loader_refcells) = + Self::accounts_to_refcells(accounts, account_deps, loaders); let instruction_recorders = if enable_cpi_recording { let ix_count = tx.message.instructions.len(); @@ -2892,6 +2902,7 @@ impl Bank { tx.message(), &loader_refcells, &account_refcells, + &account_dep_refcells, &self.rent_collector, log_collector.clone(), executors.clone(), @@ -3308,7 +3319,7 @@ impl Bank { let acc = raccs.as_ref().unwrap(); - collected_rent += acc.2; + collected_rent += acc.3; } self.collected_rent.fetch_add(collected_rent, Relaxed); diff --git a/runtime/src/message_processor.rs b/runtime/src/message_processor.rs index 1f0808c416..89facceeaa 100644 --- a/runtime/src/message_processor.rs +++ b/runtime/src/message_processor.rs @@ -207,6 +207,7 @@ pub struct ThisInvokeContext<'a> { program_ids: Vec, rent: Rent, pre_accounts: Vec, + account_deps: &'a [(Pubkey, RefCell)], programs: &'a [(Pubkey, ProcessInstructionWithContext)], logger: Rc>, bpf_compute_budget: BpfComputeBudget, @@ -216,10 +217,12 @@ pub struct ThisInvokeContext<'a> { feature_set: Arc, } impl<'a> ThisInvokeContext<'a> { + #[allow(clippy::too_many_arguments)] pub fn new( program_id: &Pubkey, rent: Rent, pre_accounts: Vec, + account_deps: &'a [(Pubkey, RefCell)], programs: &'a [(Pubkey, ProcessInstructionWithContext)], log_collector: Option>, bpf_compute_budget: BpfComputeBudget, @@ -233,6 +236,7 @@ impl<'a> ThisInvokeContext<'a> { program_ids, rent, pre_accounts, + account_deps, programs, logger: Rc::new(RefCell::new(ThisLogger { log_collector })), bpf_compute_budget, @@ -312,16 +316,25 @@ impl<'a> InvokeContext for ThisInvokeContext<'a> { fn is_feature_active(&self, feature_id: &Pubkey) -> bool { self.feature_set.is_active(feature_id) } - fn get_account(&self, pubkey: &Pubkey) -> Option { + fn get_account(&self, pubkey: &Pubkey) -> Option> { + if let Some(account) = self.account_deps.iter().find_map(|(key, account)| { + if key == pubkey { + Some(account.clone()) + } else { + None + } + }) { + return Some(account); + } self.pre_accounts.iter().find_map(|pre| { if pre.key == *pubkey { - Some(Account { + Some(RefCell::new(Account { lamports: pre.lamports, data: pre.data.clone(), owner: pre.owner, executable: pre.is_executable, rent_epoch: pre.rent_epoch, - }) + })) } else { None } @@ -641,26 +654,27 @@ impl MessageProcessor { let program_account = invoke_context .get_account(&callee_program_id) .ok_or(InstructionError::MissingAccount)?; - if !program_account.executable { + if !program_account.borrow().executable { return Err(InstructionError::AccountNotExecutable); } - let programdata_executable = if program_account.owner == bpf_loader_upgradeable::id() { - if let UpgradeableLoaderState::Program { - programdata_address, - } = program_account.state()? - { - if let Some(account) = invoke_context.get_account(&programdata_address) { - Some((programdata_address, RefCell::new(account))) + let programdata_executable = + if program_account.borrow().owner == bpf_loader_upgradeable::id() { + if let UpgradeableLoaderState::Program { + programdata_address, + } = program_account.borrow().state()? + { + if let Some(account) = invoke_context.get_account(&programdata_address) { + Some((programdata_address, account)) + } else { + return Err(InstructionError::MissingAccount); + } } else { return Err(InstructionError::MissingAccount); } } else { - return Err(InstructionError::MissingAccount); - } - } else { - None - }; - let mut executable_accounts = vec![(callee_program_id, RefCell::new(program_account))]; + None + }; + let mut executable_accounts = vec![(callee_program_id, program_account)]; if let Some(programdata) = programdata_executable { executable_accounts.push(programdata); } @@ -859,6 +873,7 @@ impl MessageProcessor { instruction: &CompiledInstruction, executable_accounts: &[(Pubkey, RefCell)], accounts: &[Rc>], + account_deps: &[(Pubkey, RefCell)], rent_collector: &RentCollector, log_collector: Option>, executors: Rc>, @@ -887,6 +902,7 @@ impl MessageProcessor { instruction.program_id(&message.account_keys), rent_collector.rent, pre_accounts, + account_deps, &self.programs, log_collector, bpf_compute_budget, @@ -917,6 +933,7 @@ impl MessageProcessor { message: &Message, loaders: &[Vec<(Pubkey, RefCell)>], accounts: &[Rc>], + account_deps: &[(Pubkey, RefCell)], rent_collector: &RentCollector, log_collector: Option>, executors: Rc>, @@ -933,6 +950,7 @@ impl MessageProcessor { instruction, &loaders[instruction_index], accounts, + account_deps, rent_collector, log_collector.clone(), executors.clone(), @@ -988,6 +1006,7 @@ mod tests { Rent::default(), pre_accounts, &[], + &[], None, BpfComputeBudget::default(), Rc::new(RefCell::new(Executors::default())), @@ -1540,6 +1559,7 @@ mod tests { &message, &loaders, &accounts, + &[], &rent_collector, None, executors.clone(), @@ -1564,6 +1584,7 @@ mod tests { &message, &loaders, &accounts, + &[], &rent_collector, None, executors.clone(), @@ -1592,6 +1613,7 @@ mod tests { &message, &loaders, &accounts, + &[], &rent_collector, None, executors, @@ -1704,6 +1726,7 @@ mod tests { &message, &loaders, &accounts, + &[], &rent_collector, None, executors.clone(), @@ -1732,6 +1755,7 @@ mod tests { &message, &loaders, &accounts, + &[], &rent_collector, None, executors.clone(), @@ -1757,6 +1781,7 @@ mod tests { &message, &loaders, &accounts, + &[], &rent_collector, None, executors, @@ -1842,6 +1867,7 @@ mod tests { not_owned_preaccount, executable_preaccount, ], + &[], programs.as_slice(), None, BpfComputeBudget::default(), diff --git a/sdk/src/process_instruction.rs b/sdk/src/process_instruction.rs index 5347019bed..4faac756d5 100644 --- a/sdk/src/process_instruction.rs +++ b/sdk/src/process_instruction.rs @@ -62,7 +62,7 @@ pub trait InvokeContext { /// Get the bank's active feature set fn is_feature_active(&self, feature_id: &Pubkey) -> bool; /// Get an account from a pre-account - fn get_account(&self, pubkey: &Pubkey) -> Option; + fn get_account(&self, pubkey: &Pubkey) -> Option>; } #[derive(Clone, Copy, Debug, AbiExample)] @@ -342,7 +342,7 @@ impl InvokeContext for MockInvokeContext { fn is_feature_active(&self, _feature_id: &Pubkey) -> bool { true } - fn get_account(&self, _pubkey: &Pubkey) -> Option { + fn get_account(&self, _pubkey: &Pubkey) -> Option> { None } }