From 9a3f713204c75d7a1519526035fa3e5019e48212 Mon Sep 17 00:00:00 2001 From: Sean Young Date: Tue, 21 Nov 2023 16:52:17 +0000 Subject: [PATCH 1/2] New api for creating a message with program ids not in instructions --- sdk/program/src/message/compiled_keys.rs | 17 +++++++++++++- sdk/program/src/message/legacy.rs | 26 +++++++++++++++++++++- sdk/program/src/message/versions/v0/mod.rs | 2 +- sdk/src/account.rs | 3 ++- 4 files changed, 44 insertions(+), 4 deletions(-) diff --git a/sdk/program/src/message/compiled_keys.rs b/sdk/program/src/message/compiled_keys.rs index 7e9b19a10591e1..f864f23e988603 100644 --- a/sdk/program/src/message/compiled_keys.rs +++ b/sdk/program/src/message/compiled_keys.rs @@ -37,7 +37,11 @@ struct CompiledKeyMeta { impl CompiledKeys { /// Compiles the pubkeys referenced by a list of instructions and organizes by /// signer/non-signer and writable/readonly. - pub(crate) fn compile(instructions: &[Instruction], payer: Option) -> Self { + pub(crate) fn compile( + instructions: &[Instruction], + programs: &[Pubkey], + payer: Option, + ) -> Self { let mut key_meta_map = BTreeMap::::new(); for ix in instructions { let meta = key_meta_map.entry(ix.program_id).or_default(); @@ -53,6 +57,12 @@ impl CompiledKeys { meta.is_signer = true; meta.is_writable = true; } + for program in programs { + let meta = key_meta_map.entry(*program).or_default(); + meta.is_signer = false; + meta.is_writable = false; + meta.is_invoked = true; + } Self { payer, key_meta_map, @@ -241,6 +251,7 @@ mod tests { Instruction::new_with_bincode(program_id2, &0, vec![]), Instruction::new_with_bincode(program_id3, &0, vec![]), ], + &[], None, ); @@ -272,6 +283,7 @@ mod tests { &0, vec![AccountMeta::new_readonly(payer, false)], )], + &[], Some(payer), ); assert_eq!( @@ -296,6 +308,7 @@ mod tests { &0, vec![AccountMeta::new(id0, false), AccountMeta::new(id0, true)], )], + &[], None, ); @@ -325,6 +338,7 @@ mod tests { AccountMeta::new(id0, true), ], )], + &[], None, ); @@ -357,6 +371,7 @@ mod tests { ), Instruction::new_with_bincode(program_id, &0, vec![AccountMeta::new(id0, false)]), ], + &[], None, ); diff --git a/sdk/program/src/message/legacy.rs b/sdk/program/src/message/legacy.rs index e81c7c485ff5f6..6f075dc40c94fa 100644 --- a/sdk/program/src/message/legacy.rs +++ b/sdk/program/src/message/legacy.rs @@ -311,7 +311,7 @@ impl Message { payer: Option<&Pubkey>, blockhash: &Hash, ) -> Self { - let compiled_keys = CompiledKeys::compile(instructions, payer.cloned()); + let compiled_keys = CompiledKeys::compile(instructions, &[], payer.cloned()); let (header, account_keys) = compiled_keys .try_into_message_components() .expect("overflow when compiling message keys"); @@ -326,6 +326,30 @@ impl Message { ) } + /// Create a message with extra program ids + /// + /// Program ids for CPI are not always required in the instruction, so construct a message with program + /// ids in the message accounts. + pub fn new_with_programs( + instructions: &[Instruction], + programs: &[Pubkey], + payer: Option<&Pubkey>, + ) -> Self { + let compiled_keys = CompiledKeys::compile(instructions, programs, payer.cloned()); + let (header, account_keys) = compiled_keys + .try_into_message_components() + .expect("overflow when compiling message keys"); + let instructions = compile_instructions(instructions, &account_keys); + Self::new_with_compiled_instructions( + header.num_required_signatures, + header.num_readonly_signed_accounts, + header.num_readonly_unsigned_accounts, + account_keys, + Hash::default(), + instructions, + ) + } + /// Create a new message for a [nonced transaction]. /// /// [nonced transaction]: https://docs.solana.com/implemented-proposals/durable-tx-nonces diff --git a/sdk/program/src/message/versions/v0/mod.rs b/sdk/program/src/message/versions/v0/mod.rs index eb4b4590b5be22..d8fb42894149f5 100644 --- a/sdk/program/src/message/versions/v0/mod.rs +++ b/sdk/program/src/message/versions/v0/mod.rs @@ -257,7 +257,7 @@ impl Message { address_lookup_table_accounts: &[AddressLookupTableAccount], recent_blockhash: Hash, ) -> Result { - let mut compiled_keys = CompiledKeys::compile(instructions, Some(*payer)); + let mut compiled_keys = CompiledKeys::compile(instructions, &[], Some(*payer)); let mut address_table_lookups = Vec::with_capacity(address_lookup_table_accounts.len()); let mut loaded_addresses_list = Vec::with_capacity(address_lookup_table_accounts.len()); diff --git a/sdk/src/account.rs b/sdk/src/account.rs index 75c68c301849e1..d94d7dc2c6b6e5 100644 --- a/sdk/src/account.rs +++ b/sdk/src/account.rs @@ -755,7 +755,8 @@ pub fn create_is_signer_account_infos<'a>( }) .collect() } -+ /// Replacement for the executable flag: An account being owned by one of these contains a program. + +/// Replacement for the executable flag: An account being owned by one of these contains a program. pub const PROGRAM_OWNERS: &[Pubkey] = &[ bpf_loader_upgradeable::id(), bpf_loader::id(), From 90a7fc4e535b5117ddfdfe3a859a61cf133d6028 Mon Sep 17 00:00:00 2001 From: Sean Young Date: Tue, 21 Nov 2023 16:52:57 +0000 Subject: [PATCH 2/2] Don't require program_id to passed down cpi call chain --- program-runtime/src/invoke_context.rs | 63 ++++++++++++++------ programs/sbf/tests/programs.rs | 86 ++++++++++++++++++++++++++- runtime/src/bank/tests.rs | 67 ++++++++++++++++++++- sdk/src/feature_set.rs | 5 ++ 4 files changed, 200 insertions(+), 21 deletions(-) diff --git a/program-runtime/src/invoke_context.rs b/program-runtime/src/invoke_context.rs index bdb870a02c1dda..f62355263bdd48 100644 --- a/program-runtime/src/invoke_context.rs +++ b/program-runtime/src/invoke_context.rs @@ -18,9 +18,11 @@ use { vm::{Config, ContextObject, EbpfVm}, }, solana_sdk::{ - account::AccountSharedData, + account::{AccountSharedData, ReadableAccount}, bpf_loader_deprecated, - feature_set::{native_programs_consume_cu, FeatureSet}, + feature_set::{ + native_programs_consume_cu, no_need_for_program_key_in_parent_instruction, FeatureSet, + }, hash::Hash, instruction::{AccountMeta, InstructionError}, native_loader, @@ -402,23 +404,50 @@ impl<'a> InvokeContext<'a> { // Find and validate executables / program accounts let callee_program_id = instruction.program_id; - let program_account_index = instruction_context - .find_index_of_instruction_account(self.transaction_context, &callee_program_id) - .ok_or_else(|| { + + if self + .feature_set + .is_active(&no_need_for_program_key_in_parent_instruction::id()) + { + let Some(program_account_index) = self + .transaction_context + .find_index_of_program_account(&callee_program_id) + else { ic_msg!(self, "Unknown program {}", callee_program_id); - InstructionError::MissingAccount - })?; - let borrowed_program_account = instruction_context - .try_borrow_instruction_account(self.transaction_context, program_account_index)?; - if !borrowed_program_account.is_executable() { - ic_msg!(self, "Account {} is not executable", callee_program_id); - return Err(InstructionError::AccountNotExecutable); - } + return Err(InstructionError::MissingAccount); + }; + + let borrowed_program_account = self + .transaction_context + .accounts() + .try_borrow(program_account_index)?; - Ok(( - instruction_accounts, - vec![borrowed_program_account.get_index_in_transaction()], - )) + if !borrowed_program_account.executable() { + ic_msg!(self, "Account {} is not executable", callee_program_id); + return Err(InstructionError::AccountNotExecutable); + } + + Ok((instruction_accounts, vec![program_account_index])) + } else { + let Some(program_account_index) = instruction_context + .find_index_of_instruction_account(self.transaction_context, &callee_program_id) + else { + ic_msg!(self, "Unknown program {}", callee_program_id); + return Err(InstructionError::MissingAccount); + }; + + let borrowed_program_account = instruction_context + .try_borrow_instruction_account(self.transaction_context, program_account_index)?; + if !borrowed_program_account.is_executable() { + ic_msg!(self, "Account {} is not executable", callee_program_id); + return Err(InstructionError::AccountNotExecutable); + } + + Ok(( + instruction_accounts, + vec![borrowed_program_account.get_index_in_transaction()], + )) + } } /// Processes an instruction and returns how many compute units were used diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 5dedca0f5bec1a..2b3411c2758c20 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -43,7 +43,10 @@ use { clock::MAX_PROCESSING_AGE, compute_budget::ComputeBudgetInstruction, entrypoint::MAX_PERMITTED_DATA_INCREASE, - feature_set::{self, remove_deprecated_request_unit_ix, FeatureSet}, + feature_set::{ + self, no_need_for_program_key_in_parent_instruction, remove_deprecated_request_unit_ix, + FeatureSet, + }, fee::FeeStructure, loader_instruction, message::{v0::LoadedAddresses, SanitizedMessage}, @@ -1232,6 +1235,85 @@ fn test_program_sbf_caller_has_access_to_cpi_program() { .. } = create_genesis_config(50); let bank = Bank::new_for_tests(&genesis_config); + + let bank = Arc::new(bank); + let mut bank_client = BankClient::new_shared(bank.clone()); + + let caller_pubkey = load_program( + &bank_client, + &bpf_loader::id(), + &mint_keypair, + "solana_sbf_rust_caller_access", + ); + let (_, caller2_pubkey) = load_program_and_advance_slot( + &mut bank_client, + &bpf_loader::id(), + &mint_keypair, + "solana_sbf_rust_caller_access", + ); + + // First try it with all the accounts + let account_metas = vec![ + AccountMeta::new_readonly(caller_pubkey, false), + AccountMeta::new_readonly(caller2_pubkey, false), + ]; + let instruction = Instruction::new_with_bytes(caller_pubkey, &[1], account_metas.clone()); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + assert!(result.is_ok(), "{result:?}"); + + // Trying cpi into account that does not exist + let random_pubkey = Pubkey::new_unique(); + let account_metas = vec![ + AccountMeta::new_readonly(caller_pubkey, false), + AccountMeta::new_readonly(random_pubkey, false), + ]; + let instruction = Instruction::new_with_bytes(caller_pubkey, &[1], account_metas); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + assert_eq!( + result.unwrap_err().unwrap(), + TransactionError::InstructionError(0, InstructionError::AccountNotExecutable) + ); + + // Trying cpi into account that is not part of the message at all + let account_metas = vec![AccountMeta::new_readonly(caller_pubkey, false)]; + + let instruction = Instruction::new_with_bytes( + caller_pubkey, + caller2_pubkey.to_bytes().as_ref(), + account_metas.clone(), + ); + let result = bank_client.send_and_confirm_instruction(&mint_keypair, instruction); + assert_eq!( + result.unwrap_err().unwrap(), + TransactionError::InstructionError(0, InstructionError::MissingAccount) + ); + + // Now add it to the message accounts - CPI without caller2 in the meta + let instruction = Instruction::new_with_bytes( + caller_pubkey, + caller2_pubkey.to_bytes().as_ref(), + account_metas, + ); + let message = Message::new_with_programs( + &[instruction], + &[caller2_pubkey], + Some(&mint_keypair.pubkey()), + ); + let result = bank_client.send_and_confirm_message(&[&mint_keypair], message); + assert!(result.is_ok(), "{result:?}"); +} + +#[test] +#[cfg(feature = "sbf_rust")] +fn test_program_sbf_caller_has_access_to_cpi_program_before() { + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_genesis_config(50); + let mut bank = Bank::new_for_tests(&genesis_config); + bank.deactivate_feature(&no_need_for_program_key_in_parent_instruction::id()); + let bank = Arc::new(bank); let mut bank_client = BankClient::new_shared(bank.clone()); @@ -1247,6 +1329,8 @@ fn test_program_sbf_caller_has_access_to_cpi_program() { &mint_keypair, "solana_sbf_rust_caller_access", ); + // caller_access calls itself without passing caller2_pubkey as meta account, not allowed + // without no_need_for_program_key_in_parent_instruction feature let account_metas = vec![ AccountMeta::new_readonly(caller_pubkey, false), AccountMeta::new_readonly(caller2_pubkey, false), diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index c280bb5f25d4f6..eddf120d823bd0 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -7523,7 +7523,7 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() { .unwrap() ); - // Test not the system account + // Test not the rent account bank.clear_signatures(); bank.store_account(&buffer_address, &buffer_account); bank.store_account(&program_keypair.pubkey(), &AccountSharedData::default()); @@ -7541,11 +7541,43 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() { .get_mut(1) .unwrap() .accounts - .get_mut(6) + .get_mut(4) + .unwrap() = AccountMeta::new_readonly(Pubkey::new_unique(), false); + let message = Message::new(&instructions, Some(&mint_keypair.pubkey())); + assert_eq!( + TransactionError::InstructionError(1, InstructionError::InvalidArgument), + bank_client + .send_and_confirm_message( + &[&mint_keypair, &program_keypair, &upgrade_authority_keypair], + message + ) + .unwrap_err() + .unwrap() + ); + + // Test not the clock account + bank.clear_signatures(); + bank.store_account(&buffer_address, &buffer_account); + bank.store_account(&program_keypair.pubkey(), &AccountSharedData::default()); + bank.store_account(&programdata_address, &AccountSharedData::default()); + let mut instructions = bpf_loader_upgradeable::deploy_with_max_program_len( + &mint_keypair.pubkey(), + &program_keypair.pubkey(), + &buffer_address, + &upgrade_authority_keypair.pubkey(), + min_program_balance, + elf.len(), + ) + .unwrap(); + *instructions + .get_mut(1) + .unwrap() + .accounts + .get_mut(5) .unwrap() = AccountMeta::new_readonly(Pubkey::new_unique(), false); let message = Message::new(&instructions, Some(&mint_keypair.pubkey())); assert_eq!( - TransactionError::InstructionError(1, InstructionError::MissingAccount), + TransactionError::InstructionError(1, InstructionError::InvalidArgument), bank_client .send_and_confirm_message( &[&mint_keypair, &program_keypair, &upgrade_authority_keypair], @@ -7555,6 +7587,35 @@ fn test_bpf_loader_upgradeable_deploy_with_max_len() { .unwrap() ); + // Test not the system account + // NOTE: the system account is not required any more in the instruction accounts + bank.clear_signatures(); + bank.store_account(&buffer_address, &buffer_account); + bank.store_account(&program_keypair.pubkey(), &AccountSharedData::default()); + bank.store_account(&programdata_address, &AccountSharedData::default()); + let mut instructions = bpf_loader_upgradeable::deploy_with_max_program_len( + &mint_keypair.pubkey(), + &program_keypair.pubkey(), + &buffer_address, + &upgrade_authority_keypair.pubkey(), + min_program_balance, + elf.len(), + ) + .unwrap(); + *instructions + .get_mut(1) + .unwrap() + .accounts + .get_mut(6) + .unwrap() = AccountMeta::new_readonly(Pubkey::new_unique(), false); + let message = Message::new(&instructions, Some(&mint_keypair.pubkey())); + assert!(bank_client + .send_and_confirm_message( + &[&mint_keypair, &program_keypair, &upgrade_authority_keypair], + message + ) + .is_ok()); + fn truncate_data(account: &mut AccountSharedData, len: usize) { let mut data = account.data().to_vec(); data.truncate(len); diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 806c3e0139575f..35e206c40045d3 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -732,6 +732,10 @@ pub mod enable_zk_transfer_with_fee { solana_sdk::declare_id!("zkNLP7EQALfC1TYeB3biDU7akDckj8iPkvh9y2Mt2K3"); } +pub mod no_need_for_program_key_in_parent_instruction { + solana_sdk::declare_id!("Sean2BxUmn4RLbqcoiJux3Ymij8SX2SsyjHWDCwJN5o"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -910,6 +914,7 @@ lazy_static! { (validate_fee_collector_account::id(), "validate fee collector account #33888"), (disable_rent_fees_collection::id(), "Disable rent fees collection #33945"), (enable_zk_transfer_with_fee::id(), "enable Zk Token proof program transfer with fee"), + (no_need_for_program_key_in_parent_instruction::id(), "Relax requirement that parent should have passed program id for CPI"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()