diff --git a/program-runtime/src/executor_cache.rs b/program-runtime/src/executor_cache.rs index fced218b0..22b68cb90 100644 --- a/program-runtime/src/executor_cache.rs +++ b/program-runtime/src/executor_cache.rs @@ -9,87 +9,77 @@ use { ops::Div, sync::{ atomic::{AtomicU64, Ordering::Relaxed}, - Arc, RwLock, + Arc, }, }, }; -/// Relation between a TransactionExecutorCacheEntry and its matching BankExecutorCacheEntry -#[repr(u8)] -#[derive(Clone, Copy, PartialEq, Eq, Debug)] -pub enum TxBankExecutorCacheDiff { - /// The TransactionExecutorCacheEntry did not change and is the same as the BankExecutorCacheEntry. - None, - /// The TransactionExecutorCacheEntry was inserted, no matching BankExecutorCacheEntry exists, so it needs to be inserted. - Inserted, - /// The TransactionExecutorCacheEntry was replaced, the matching BankExecutorCacheEntry needs to be updated. - Updated, -} - -/// An entry of the TransactionExecutorCache -#[derive(Debug)] -pub struct TransactionExecutorCacheEntry { - executor: Arc, - difference: TxBankExecutorCacheDiff, -} - /// A subset of the BankExecutorCache containing only the executors relevant to one transaction /// /// The BankExecutorCache can not be updated directly as transaction batches are /// processed in parallel, which would cause a race condition. #[derive(Default, Debug)] pub struct TransactionExecutorCache { - pub executors: HashMap, + /// Executors or tombstones which are visible during the transaction + pub visible: HashMap>>, + /// Executors of programs which were re-/deploymed during the transaction + pub deployments: HashMap>, + /// Executors which were missing in the cache and not re-/deploymed during the transaction + pub add_to_cache: HashMap>, } impl TransactionExecutorCache { pub fn new(executable_keys: impl Iterator)>) -> Self { Self { - executors: executable_keys - .map(|(key, executor)| { - let entry = TransactionExecutorCacheEntry { - executor, - difference: TxBankExecutorCacheDiff::None, - }; - (key, entry) - }) + visible: executable_keys + .map(|(id, executor)| (id, Some(executor))) .collect(), + deployments: HashMap::new(), + add_to_cache: HashMap::new(), } } - pub fn get(&self, key: &Pubkey) -> Option> { - self.executors.get(key).map(|entry| entry.executor.clone()) + pub fn get(&self, key: &Pubkey) -> Option>> { + self.visible.get(key).cloned() } - pub fn set(&mut self, key: Pubkey, executor: Arc, replacement: bool) { - let difference = if replacement { - TxBankExecutorCacheDiff::Updated - } else { - TxBankExecutorCacheDiff::Inserted - }; - let entry = TransactionExecutorCacheEntry { - executor, - difference, - }; - let _was_replaced = self.executors.insert(key, entry).is_some(); + pub fn set_tombstone(&mut self, key: Pubkey) { + self.visible.insert(key, None); } - pub fn update_global_cache( - &self, - global_cache: &RwLock, - selector: impl Fn(TxBankExecutorCacheDiff) -> bool, + pub fn set( + &mut self, + key: Pubkey, + executor: Arc, + upgrade: bool, + delay_visibility_of_program_deployment: bool, ) { - let executors_delta: Vec<_> = self - .executors - .iter() - .filter_map(|(key, entry)| { - selector(entry.difference).then(|| (key, entry.executor.clone())) - }) - .collect(); - if !executors_delta.is_empty() { - global_cache.write().unwrap().put(&executors_delta); + if upgrade { + if delay_visibility_of_program_deployment { + // Place a tombstone in the cache so that + // we don't load the new version from the database as it should remain invisible + self.set_tombstone(key); + } else { + self.visible.insert(key, Some(executor.clone())); + } + self.deployments.insert(key, executor); + } else { + self.visible.insert(key, Some(executor.clone())); + self.add_to_cache.insert(key, executor); } } + + pub fn get_executors_added_to_the_cache(&mut self) -> HashMap> { + let mut executors = HashMap::new(); + std::mem::swap(&mut executors, &mut self.add_to_cache); + executors + } + + pub fn get_executors_which_were_deployed(&mut self) -> HashMap> { + let mut executors = HashMap::new(); + std::mem::swap(&mut executors, &mut self.deployments); + executors + } } /// Capacity of `BankExecutorCache` @@ -197,18 +187,17 @@ impl BankExecutorCache { } } - fn put(&mut self, executors: &[(&Pubkey, Arc)]) { + pub fn put(&mut self, executors: impl Iterator)>) { let mut new_executors: Vec<_> = executors - .iter() .filter_map(|(key, executor)| { - if let Some(mut entry) = self.remove(key) { + if let Some(mut entry) = self.remove(&key) { self.stats.replacements.fetch_add(1, Relaxed); entry.executor = executor.clone(); - let _ = self.executors.insert(**key, entry); + let _ = self.executors.insert(key, entry); None } else { self.stats.insertions.fetch_add(1, Relaxed); - Some((*key, executor)) + Some((key, executor)) } }) .collect(); @@ -251,7 +240,7 @@ impl BankExecutorCache { executor: executor.clone(), hit_count: AtomicU64::new(1), }; - let _ = self.executors.insert(*key, entry); + let _ = self.executors.insert(key, entry); } } } @@ -388,9 +377,9 @@ mod tests { let executor: Arc = Arc::new(TestExecutor {}); let mut cache = BankExecutorCache::new(3, 0); - cache.put(&[(&key1, executor.clone())]); - cache.put(&[(&key2, executor.clone())]); - cache.put(&[(&key3, executor.clone())]); + cache.put([(key1, executor.clone())].into_iter()); + cache.put([(key2, executor.clone())].into_iter()); + cache.put([(key3, executor.clone())].into_iter()); assert!(cache.get(&key1).is_some()); assert!(cache.get(&key2).is_some()); assert!(cache.get(&key3).is_some()); @@ -398,7 +387,7 @@ mod tests { assert!(cache.get(&key1).is_some()); assert!(cache.get(&key1).is_some()); assert!(cache.get(&key2).is_some()); - cache.put(&[(&key4, executor.clone())]); + cache.put([(key4, executor.clone())].into_iter()); assert!(cache.get(&key4).is_some()); let num_retained = [&key1, &key2, &key3] .iter() @@ -409,7 +398,7 @@ mod tests { assert!(cache.get(&key4).is_some()); assert!(cache.get(&key4).is_some()); assert!(cache.get(&key4).is_some()); - cache.put(&[(&key3, executor.clone())]); + cache.put([(key3, executor.clone())].into_iter()); assert!(cache.get(&key3).is_some()); let num_retained = [&key1, &key2, &key4] .iter() @@ -428,9 +417,9 @@ mod tests { let mut cache = BankExecutorCache::new(3, 0); assert!(cache.current_epoch == 0); - cache.put(&[(&key1, executor.clone())]); - cache.put(&[(&key2, executor.clone())]); - cache.put(&[(&key3, executor.clone())]); + cache.put([(key1, executor.clone())].into_iter()); + cache.put([(key2, executor.clone())].into_iter()); + cache.put([(key3, executor.clone())].into_iter()); assert!(cache.get(&key1).is_some()); assert!(cache.get(&key1).is_some()); assert!(cache.get(&key1).is_some()); @@ -441,7 +430,7 @@ mod tests { assert!(cache.get(&key2).is_some()); assert!(cache.get(&key2).is_some()); assert!(cache.get(&key3).is_some()); - cache.put(&[(&key4, executor.clone())]); + cache.put([(key4, executor.clone())].into_iter()); assert!(cache.get(&key4).is_some()); let num_retained = [&key1, &key2, &key3] @@ -450,8 +439,8 @@ mod tests { .count(); assert_eq!(num_retained, 2); - cache.put(&[(&key1, executor.clone())]); - cache.put(&[(&key3, executor.clone())]); + cache.put([(key1, executor.clone())].into_iter()); + cache.put([(key3, executor.clone())].into_iter()); assert!(cache.get(&key1).is_some()); assert!(cache.get(&key3).is_some()); let num_retained = [&key2, &key4] @@ -463,7 +452,7 @@ mod tests { cache = BankExecutorCache::new_from_parent_bank_executors(&cache, 2); assert!(cache.current_epoch == 2); - cache.put(&[(&key3, executor.clone())]); + cache.put([(key3, executor.clone())].into_iter()); assert!(cache.get(&key3).is_some()); } @@ -475,11 +464,11 @@ mod tests { let executor: Arc = Arc::new(TestExecutor {}); let mut cache = BankExecutorCache::new(2, 0); - cache.put(&[(&key1, executor.clone())]); + cache.put([(key1, executor.clone())].into_iter()); for _ in 0..5 { let _ = cache.get(&key1); } - cache.put(&[(&key2, executor.clone())]); + cache.put([(key2, executor.clone())].into_iter()); // make key1's use-count for sure greater than key2's let _ = cache.get(&key1); @@ -491,7 +480,7 @@ mod tests { entries.sort_by_key(|(_, v)| *v); assert!(entries[0].1 < entries[1].1); - cache.put(&[(&key3, executor.clone())]); + cache.put([(key3, executor.clone())].into_iter()); assert!(cache.get(&entries[0].0).is_none()); assert!(cache.get(&entries[1].0).is_some()); } @@ -508,10 +497,10 @@ mod tests { assert_eq!(cache.stats.one_hit_wonders.load(Relaxed), 0); // add our one-hit-wonder - cache.put(&[(&one_hit_wonder, executor.clone())]); + cache.put([(one_hit_wonder, executor.clone())].into_iter()); assert_eq!(cache.executors[&one_hit_wonder].hit_count.load(Relaxed), 1); // displace the one-hit-wonder with "popular program" - cache.put(&[(&popular, executor.clone())]); + cache.put([(popular, executor.clone())].into_iter()); assert_eq!(cache.executors[&popular].hit_count.load(Relaxed), 1); // one-hit-wonder counter incremented @@ -522,7 +511,7 @@ mod tests { assert_eq!(cache.executors[&popular].hit_count.load(Relaxed), 2); // evict "popular program" - cache.put(&[(&one_hit_wonder, executor.clone())]); + cache.put([(one_hit_wonder, executor.clone())].into_iter()); assert_eq!(cache.executors[&one_hit_wonder].hit_count.load(Relaxed), 1); // one-hit-wonder counter not incremented @@ -593,13 +582,13 @@ mod tests { assert_eq!(ComparableStats::from(&cache.stats), expected_stats,); // insert some executors - cache.put(&[(&program_id1, executor.clone())]); - cache.put(&[(&program_id2, executor.clone())]); + cache.put([(program_id1, executor.clone())].into_iter()); + cache.put([(program_id2, executor.clone())].into_iter()); expected_stats.insertions += 2; assert_eq!(ComparableStats::from(&cache.stats), expected_stats); // replace a one-hit-wonder executor - cache.put(&[(&program_id1, executor.clone())]); + cache.put([(program_id1, executor.clone())].into_iter()); expected_stats.replacements += 1; expected_stats.one_hit_wonders += 1; assert_eq!(ComparableStats::from(&cache.stats), expected_stats); @@ -617,7 +606,7 @@ mod tests { assert_eq!(ComparableStats::from(&cache.stats), expected_stats); // evict an executor - cache.put(&[(&Pubkey::new_unique(), executor.clone())]); + cache.put([(Pubkey::new_unique(), executor.clone())].into_iter()); expected_stats.insertions += 1; expected_stats.evictions.insert(program_id2, 1); assert_eq!(ComparableStats::from(&cache.stats), expected_stats); diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 86d3cd959..462961c53 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -41,9 +41,10 @@ use { entrypoint::{HEAP_LENGTH, SUCCESS}, feature_set::{ cap_accounts_data_allocations_per_transaction, cap_bpf_program_instruction_accounts, - check_slice_translation_size, disable_deploy_of_alloc_free_syscall, - enable_bpf_loader_extend_program_ix, enable_bpf_loader_set_authority_checked_ix, - enable_program_redeployment_cooldown, limit_max_instruction_trace_length, FeatureSet, + check_slice_translation_size, delay_visibility_of_program_deployment, + disable_deploy_of_alloc_free_syscall, enable_bpf_loader_extend_program_ix, + enable_bpf_loader_set_authority_checked_ix, enable_program_redeployment_cooldown, + limit_max_instruction_trace_length, FeatureSet, }, instruction::{AccountMeta, InstructionError}, loader_instruction::LoaderInstruction, @@ -228,8 +229,14 @@ pub fn create_executor_from_account( }; if let Some(ref tx_executor_cache) = tx_executor_cache { - if let Some(executor) = tx_executor_cache.get(program.get_key()) { - return Ok((executor, None)); + match tx_executor_cache.get(program.get_key()) { + // Executor exists and is cached, use it + Some(Some(executor)) => return Ok((executor, None)), + // We cached that the Executor does not exist, abort + // This case can only happen once delay_visibility_of_program_deployment is active. + Some(None) => return Err(InstructionError::InvalidAccountData), + // Nothing cached, try to load from account instead + None => {} } } @@ -250,11 +257,44 @@ pub fn create_executor_from_account( false, /* reject_deployment_of_broken_elfs */ )?; if let Some(mut tx_executor_cache) = tx_executor_cache { - tx_executor_cache.set(*program.get_key(), executor.clone(), false); + tx_executor_cache.set( + *program.get_key(), + executor.clone(), + false, + feature_set.is_active(&delay_visibility_of_program_deployment::id()), + ); } Ok((executor, Some(create_executor_metrics))) } +macro_rules! deploy_program { + ($invoke_context:expr, $use_jit:expr, $program_id:expr, + $drop:expr, $new_programdata:expr $(,)?) => {{ + let delay_visibility_of_program_deployment = $invoke_context + .feature_set + .is_active(&delay_visibility_of_program_deployment::id()); + let mut create_executor_metrics = CreateMetrics::default(); + let executor = create_executor_from_bytes( + &$invoke_context.feature_set, + $invoke_context.get_compute_budget(), + $invoke_context.get_log_collector(), + &mut create_executor_metrics, + $new_programdata, + $use_jit, + true, + )?; + $drop + create_executor_metrics.program_id = $program_id.to_string(); + create_executor_metrics.submit_datapoint(&mut $invoke_context.timings); + $invoke_context.tx_executor_cache.borrow_mut().set( + $program_id, + executor, + true, + delay_visibility_of_program_deployment, + ); + }}; +} + fn write_program_data( program_account_index: IndexOfAccount, program_data_offset: usize, @@ -642,26 +682,18 @@ fn process_loader_upgradeable_instruction( let instruction_context = transaction_context.get_current_instruction_context()?; let buffer = instruction_context.try_borrow_instruction_account(transaction_context, 3)?; - let mut create_executor_metrics = CreateMetrics::default(); - let executor = create_executor_from_bytes( - &invoke_context.feature_set, - invoke_context.get_compute_budget(), - invoke_context.get_log_collector(), - &mut create_executor_metrics, + deploy_program!( + invoke_context, + use_jit, + new_program_id, + { + drop(buffer); + }, buffer .get_data() .get(buffer_data_offset..) .ok_or(InstructionError::AccountDataTooSmall)?, - use_jit, - true, - )?; - drop(buffer); - create_executor_metrics.program_id = new_program_id.to_string(); - create_executor_metrics.submit_datapoint(&mut invoke_context.timings); - invoke_context - .tx_executor_cache - .borrow_mut() - .set(new_program_id, executor, true); + ); let transaction_context = &invoke_context.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; @@ -835,26 +867,18 @@ fn process_loader_upgradeable_instruction( // Load and verify the program bits let buffer = instruction_context.try_borrow_instruction_account(transaction_context, 2)?; - let mut create_executor_metrics = CreateMetrics::default(); - let executor = create_executor_from_bytes( - &invoke_context.feature_set, - invoke_context.get_compute_budget(), - invoke_context.get_log_collector(), - &mut create_executor_metrics, + deploy_program!( + invoke_context, + use_jit, + new_program_id, + { + drop(buffer); + }, buffer .get_data() .get(buffer_data_offset..) .ok_or(InstructionError::AccountDataTooSmall)?, - use_jit, - true, - )?; - drop(buffer); - create_executor_metrics.program_id = new_program_id.to_string(); - create_executor_metrics.submit_datapoint(&mut invoke_context.timings); - invoke_context - .tx_executor_cache - .borrow_mut() - .set(new_program_id, executor, true); + ); let transaction_context = &invoke_context.transaction_context; let instruction_context = transaction_context.get_current_instruction_context()?; @@ -1142,6 +1166,15 @@ fn process_loader_upgradeable_instruction( instruction_context, &log_collector, )?; + if invoke_context + .feature_set + .is_active(&delay_visibility_of_program_deployment::id()) + { + invoke_context + .tx_executor_cache + .borrow_mut() + .set_tombstone(program_key); + } } _ => { ic_logger_msg!(log_collector, "Invalid Program account"); @@ -1358,22 +1391,13 @@ fn process_loader_instruction( ic_msg!(invoke_context, "key[0] did not sign the transaction"); return Err(InstructionError::MissingRequiredSignature); } - let mut create_executor_metrics = CreateMetrics::default(); - let executor = create_executor_from_bytes( - &invoke_context.feature_set, - invoke_context.get_compute_budget(), - invoke_context.get_log_collector(), - &mut create_executor_metrics, - program.get_data(), + deploy_program!( + invoke_context, use_jit, - true, - )?; - create_executor_metrics.program_id = program.get_key().to_string(); - create_executor_metrics.submit_datapoint(&mut invoke_context.timings); - invoke_context - .tx_executor_cache - .borrow_mut() - .set(*program.get_key(), executor, true); + *program.get_key(), + {}, + program.get_data(), + ); program.set_executable(true)?; ic_msg!(invoke_context, "Finalized account {:?}", program.get_key()); } diff --git a/programs/sbf/tests/programs.rs b/programs/sbf/tests/programs.rs index 7e6383af8..335e54a96 100644 --- a/programs/sbf/tests/programs.rs +++ b/programs/sbf/tests/programs.rs @@ -1824,14 +1824,11 @@ fn test_program_sbf_invoke_in_same_tx_as_deployment() { ) .unwrap(); - // Deployment is invisible to top-level-instructions but visible to CPI instructions - for (invoke_instruction, expected) in [ - ( - invoke_instruction, - Err(TransactionError::ProgramAccountNotFound), - ), - (indirect_invoke_instruction, Ok(())), - ] { + // Deployment is invisible to both top-level-instructions and CPI instructions + for (index, invoke_instruction) in [invoke_instruction, indirect_invoke_instruction] + .into_iter() + .enumerate() + { let mut instructions = deployment_instructions.clone(); instructions.push(invoke_instruction); let tx = Transaction::new( @@ -1839,11 +1836,18 @@ fn test_program_sbf_invoke_in_same_tx_as_deployment() { Message::new(&instructions, Some(&mint_keypair.pubkey())), bank.last_blockhash(), ); - let results = execute_transactions(&bank, vec![tx]); - if let Err(err) = expected { - assert_eq!(results[0].as_ref().unwrap_err(), &err); + if index == 0 { + let results = execute_transactions(&bank, vec![tx]); + assert_eq!( + results[0].as_ref().unwrap_err(), + &TransactionError::ProgramAccountNotFound, + ); } else { - assert!(results[0].is_ok()); + let (result, _, _) = process_transaction_and_record_inner(&bank, tx); + assert_eq!( + result.unwrap_err(), + TransactionError::InstructionError(2, InstructionError::InvalidAccountData), + ); } } } @@ -1889,98 +1893,6 @@ fn test_program_sbf_invoke_in_same_tx_as_redeployment() { "solana_sbf_rust_invoke_and_return", ); - let invoke_instruction = - Instruction::new_with_bytes(program_id, &[0], vec![AccountMeta::new(clock::id(), false)]); - let indirect_invoke_instruction = Instruction::new_with_bytes( - indirect_program_keypair.pubkey(), - &[0], - vec![ - AccountMeta::new_readonly(program_id, false), - AccountMeta::new_readonly(clock::id(), false), - ], - ); - - // Prepare redeployment - load_upgradeable_buffer( - &bank_client, - &mint_keypair, - &buffer_keypair, - &authority_keypair, - "solana_sbf_rust_panic", - ); - let redeployment_instruction = bpf_loader_upgradeable::upgrade( - &program_id, - &buffer_keypair.pubkey(), - &authority_keypair.pubkey(), - &mint_keypair.pubkey(), - ); - - // Redeployment is visible to both top-level-instructions and CPI instructions - for invoke_instruction in [invoke_instruction, indirect_invoke_instruction] { - // Call upgradeable program - let result = - bank_client.send_and_confirm_instruction(&mint_keypair, invoke_instruction.clone()); - assert!(result.is_ok()); - - // Upgrade the program and invoke in same tx - let message = Message::new( - &[redeployment_instruction.clone(), invoke_instruction], - Some(&mint_keypair.pubkey()), - ); - let tx = Transaction::new( - &[&mint_keypair, &authority_keypair], - message.clone(), - bank.last_blockhash(), - ); - let (result, _, _) = process_transaction_and_record_inner(&bank, tx); - assert_eq!( - result.unwrap_err(), - TransactionError::InstructionError(1, InstructionError::ProgramFailedToComplete), - ); - } -} - -#[test] -#[cfg(feature = "sbf_rust")] -fn test_program_sbf_invoke_in_same_tx_as_undeployment() { - solana_logger::setup(); - - let GenesisConfigInfo { - genesis_config, - mint_keypair, - .. - } = create_genesis_config(50); - let mut bank = Bank::new_for_tests(&genesis_config); - let (name, id, entrypoint) = solana_bpf_loader_upgradeable_program!(); - bank.add_builtin(&name, &id, entrypoint); - let bank = Arc::new(bank); - let bank_client = BankClient::new_shared(&bank); - - // Deploy upgradeable program - let buffer_keypair = Keypair::new(); - let program_keypair = Keypair::new(); - let program_id = program_keypair.pubkey(); - let authority_keypair = Keypair::new(); - load_upgradeable_program( - &bank_client, - &mint_keypair, - &buffer_keypair, - &program_keypair, - &authority_keypair, - "solana_sbf_rust_noop", - ); - - // Deploy indirect invocation program - let indirect_program_keypair = Keypair::new(); - load_upgradeable_program( - &bank_client, - &mint_keypair, - &buffer_keypair, - &indirect_program_keypair, - &authority_keypair, - "solana_sbf_rust_invoke_and_return", - ); - // Deploy panic program let panic_program_keypair = Keypair::new(); load_upgradeable_program( @@ -2002,8 +1914,99 @@ fn test_program_sbf_invoke_in_same_tx_as_undeployment() { AccountMeta::new_readonly(clock::id(), false), ], ); - let panic_instruction = - Instruction::new_with_bytes(panic_program_keypair.pubkey(), &[], vec![]); + + // Prepare redeployment + let buffer_keypair = Keypair::new(); + load_upgradeable_buffer( + &bank_client, + &mint_keypair, + &buffer_keypair, + &authority_keypair, + "solana_sbf_rust_panic", + ); + let redeployment_instruction = bpf_loader_upgradeable::upgrade( + &program_id, + &buffer_keypair.pubkey(), + &authority_keypair.pubkey(), + &mint_keypair.pubkey(), + ); + + // Redeployment causes programs to be unavailable to both top-level-instructions and CPI instructions + for invoke_instruction in [invoke_instruction, indirect_invoke_instruction] { + // Call upgradeable program + let result = + bank_client.send_and_confirm_instruction(&mint_keypair, invoke_instruction.clone()); + assert!(result.is_ok()); + + // Upgrade the program and invoke in same tx + let message = Message::new( + &[redeployment_instruction.clone(), invoke_instruction], + Some(&mint_keypair.pubkey()), + ); + let tx = Transaction::new( + &[&mint_keypair, &authority_keypair], + message.clone(), + bank.last_blockhash(), + ); + let (result, _, _) = process_transaction_and_record_inner(&bank, tx); + assert_eq!( + result.unwrap_err(), + TransactionError::InstructionError(1, InstructionError::InvalidAccountData), + ); + } +} + +#[test] +#[cfg(feature = "sbf_rust")] +fn test_program_sbf_invoke_in_same_tx_as_undeployment() { + solana_logger::setup(); + + let GenesisConfigInfo { + genesis_config, + mint_keypair, + .. + } = create_genesis_config(50); + let mut bank = Bank::new_for_tests(&genesis_config); + let (name, id, entrypoint) = solana_bpf_loader_upgradeable_program!(); + bank.add_builtin(&name, &id, entrypoint); + let bank = Arc::new(bank); + let bank_client = BankClient::new_shared(&bank); + + // Deploy upgradeable program + let buffer_keypair = Keypair::new(); + let program_keypair = Keypair::new(); + let program_id = program_keypair.pubkey(); + let authority_keypair = Keypair::new(); + load_upgradeable_program( + &bank_client, + &mint_keypair, + &buffer_keypair, + &program_keypair, + &authority_keypair, + "solana_sbf_rust_noop", + ); + + // Deploy indirect invocation program + let indirect_program_keypair = Keypair::new(); + load_upgradeable_program( + &bank_client, + &mint_keypair, + &buffer_keypair, + &indirect_program_keypair, + &authority_keypair, + "solana_sbf_rust_invoke_and_return", + ); + + let invoke_instruction = + Instruction::new_with_bytes(program_id, &[0], vec![AccountMeta::new(clock::id(), false)]); + let indirect_invoke_instruction = Instruction::new_with_bytes( + indirect_program_keypair.pubkey(), + &[0], + vec![ + AccountMeta::new_readonly(program_id, false), + AccountMeta::new_readonly(clock::id(), false), + ], + ); // Prepare undeployment let (programdata_address, _) = Pubkey::find_program_address( @@ -2017,7 +2020,7 @@ fn test_program_sbf_invoke_in_same_tx_as_undeployment() { Some(&program_id), ); - // Undeployment is invisible to both top-level-instructions and CPI instructions + // Undeployment is visible to both top-level-instructions and CPI instructions for invoke_instruction in [invoke_instruction, indirect_invoke_instruction] { // Call upgradeable program let result = @@ -2026,11 +2029,7 @@ fn test_program_sbf_invoke_in_same_tx_as_undeployment() { // Upgrade the program and invoke in same tx let message = Message::new( - &[ - undeployment_instruction.clone(), - invoke_instruction, - panic_instruction.clone(), // Make sure the TX fails, so we don't have to deploy another program - ], + &[undeployment_instruction.clone(), invoke_instruction], Some(&mint_keypair.pubkey()), ); let tx = Transaction::new( @@ -2041,7 +2040,7 @@ fn test_program_sbf_invoke_in_same_tx_as_undeployment() { let (result, _, _) = process_transaction_and_record_inner(&bank, tx); assert_eq!( result.unwrap_err(), - TransactionError::InstructionError(2, InstructionError::ProgramFailedToComplete), + TransactionError::InstructionError(1, InstructionError::InvalidAccountData), ); } } diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index f4991c962..ca4da8252 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -93,10 +93,7 @@ use { accounts_data_meter::MAX_ACCOUNTS_DATA_LEN, compute_budget::{self, ComputeBudget}, executor::Executor, - executor_cache::{ - BankExecutorCache, TransactionExecutorCache, TxBankExecutorCacheDiff, - MAX_CACHED_EXECUTORS, - }, + executor_cache::{BankExecutorCache, TransactionExecutorCache, MAX_CACHED_EXECUTORS}, invoke_context::{BuiltinProgram, ProcessInstructionWithContext}, loaded_programs::{LoadedPrograms, WorkingSlot}, log_collector::LogCollector, @@ -4027,22 +4024,36 @@ impl Bank { Rc::new(RefCell::new(tx_executor_cache)) } - /// Add executors back to the bank's cache if they were missing and not updated - fn store_missing_executors(&self, tx_executor_cache: &RefCell) { - tx_executor_cache - .borrow() - .update_global_cache(&self.executor_cache, |difference| { - difference == TxBankExecutorCacheDiff::Inserted - }); + /// Add executors back to the bank's cache if they were missing and not re-/deployed + fn store_executors_which_added_to_the_cache( + &self, + tx_executor_cache: &RefCell, + ) { + let executors = tx_executor_cache + .borrow_mut() + .get_executors_added_to_the_cache(); + if !executors.is_empty() { + self.executor_cache + .write() + .unwrap() + .put(executors.into_iter()); + } } - /// Add updated executors back to the bank's cache - fn store_updated_executors(&self, tx_executor_cache: &RefCell) { - tx_executor_cache - .borrow() - .update_global_cache(&self.executor_cache, |difference| { - difference == TxBankExecutorCacheDiff::Updated - }); + /// Add re-/deployed executors to the bank's cache + fn store_executors_which_were_deployed( + &self, + tx_executor_cache: &RefCell, + ) { + let executors = tx_executor_cache + .borrow_mut() + .get_executors_which_were_deployed(); + if !executors.is_empty() { + self.executor_cache + .write() + .unwrap() + .put(executors.into_iter()); + } } #[allow(dead_code)] // Preparation for BankExecutorCache rework @@ -4219,12 +4230,13 @@ impl Bank { process_message_time.as_us() ); - let mut store_missing_executors_time = Measure::start("store_missing_executors_time"); - self.store_missing_executors(&tx_executor_cache); - store_missing_executors_time.stop(); + let mut store_executors_which_added_to_the_cache_time = + Measure::start("store_executors_which_added_to_the_cache_time"); + self.store_executors_which_added_to_the_cache(&tx_executor_cache); + store_executors_which_added_to_the_cache_time.stop(); saturating_add_assign!( timings.execute_accessories.update_executors_us, - store_missing_executors_time.as_us() + store_executors_which_added_to_the_cache_time.as_us() ); let status = process_result @@ -4903,7 +4915,8 @@ impl Bank { sanitized_txs.len() ); - let mut store_updated_executors_time = Measure::start("store_updated_executors_time"); + let mut store_executors_which_were_deployed_time = + Measure::start("store_executors_which_were_deployed_time"); for execution_result in &execution_results { if let TransactionExecutionResult::Executed { details, @@ -4911,14 +4924,14 @@ impl Bank { } = execution_result { if details.status.is_ok() { - self.store_updated_executors(tx_executor_cache); + self.store_executors_which_were_deployed(tx_executor_cache); } } } - store_updated_executors_time.stop(); + store_executors_which_were_deployed_time.stop(); saturating_add_assign!( timings.execute_accessories.update_executors_us, - store_updated_executors_time.as_us() + store_executors_which_were_deployed_time.as_us() ); let accounts_data_len_delta = execution_results diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 1eaf1533d..46758a39b 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -7708,42 +7708,42 @@ fn test_bank_executor_cache() { let executors = TransactionExecutorCache::new((0..4).map(|i| (accounts[i].0, executor.clone()))); let executors = Rc::new(RefCell::new(executors)); - bank.store_missing_executors(&executors); - bank.store_updated_executors(&executors); + bank.store_executors_which_added_to_the_cache(&executors); + bank.store_executors_which_were_deployed(&executors); let stored_executors = bank.get_tx_executor_cache(accounts); - assert_eq!(stored_executors.borrow().executors.len(), 0); + assert_eq!(stored_executors.borrow().visible.len(), 0); // do work let mut executors = TransactionExecutorCache::new((2..3).map(|i| (accounts[i].0, executor.clone()))); - executors.set(key1, executor.clone(), false); - executors.set(key2, executor.clone(), false); - executors.set(key3, executor.clone(), true); - executors.set(key4, executor.clone(), false); + executors.set(key1, executor.clone(), false, true); + executors.set(key2, executor.clone(), false, true); + executors.set(key3, executor.clone(), true, true); + executors.set(key4, executor.clone(), false, true); let executors = Rc::new(RefCell::new(executors)); // store Missing - bank.store_missing_executors(&executors); + bank.store_executors_which_added_to_the_cache(&executors); let stored_executors = bank.get_tx_executor_cache(accounts); - assert_eq!(stored_executors.borrow().executors.len(), 2); - assert!(stored_executors.borrow().executors.contains_key(&key1)); - assert!(stored_executors.borrow().executors.contains_key(&key2)); + assert_eq!(stored_executors.borrow().visible.len(), 2); + assert!(stored_executors.borrow().visible.contains_key(&key1)); + assert!(stored_executors.borrow().visible.contains_key(&key2)); // store Updated - bank.store_updated_executors(&executors); + bank.store_executors_which_were_deployed(&executors); let stored_executors = bank.get_tx_executor_cache(accounts); - assert_eq!(stored_executors.borrow().executors.len(), 3); - assert!(stored_executors.borrow().executors.contains_key(&key1)); - assert!(stored_executors.borrow().executors.contains_key(&key2)); - assert!(stored_executors.borrow().executors.contains_key(&key3)); + assert_eq!(stored_executors.borrow().visible.len(), 3); + assert!(stored_executors.borrow().visible.contains_key(&key1)); + assert!(stored_executors.borrow().visible.contains_key(&key2)); + assert!(stored_executors.borrow().visible.contains_key(&key3)); // Check inheritance let bank = Bank::new_from_parent(&Arc::new(bank), &solana_sdk::pubkey::new_rand(), 1); let stored_executors = bank.get_tx_executor_cache(accounts); - assert_eq!(stored_executors.borrow().executors.len(), 3); - assert!(stored_executors.borrow().executors.contains_key(&key1)); - assert!(stored_executors.borrow().executors.contains_key(&key2)); - assert!(stored_executors.borrow().executors.contains_key(&key3)); + assert_eq!(stored_executors.borrow().visible.len(), 3); + assert!(stored_executors.borrow().visible.contains_key(&key1)); + assert!(stored_executors.borrow().visible.contains_key(&key2)); + assert!(stored_executors.borrow().visible.contains_key(&key3)); // Force compilation of an executor let mut file = File::open("../programs/bpf_loader/test_elfs/out/noop_aligned.so").unwrap(); @@ -7784,7 +7784,7 @@ fn test_bank_executor_cache() { bank.remove_executor(&key3); bank.remove_executor(&key4); let stored_executors = bank.get_tx_executor_cache(accounts); - assert_eq!(stored_executors.borrow().executors.len(), 0); + assert_eq!(stored_executors.borrow().visible.len(), 0); } #[test] @@ -7810,36 +7810,36 @@ fn test_bank_executor_cow() { // add one to root bank let mut executors = TransactionExecutorCache::default(); - executors.set(key1, executor.clone(), false); + executors.set(key1, executor.clone(), false, true); let executors = Rc::new(RefCell::new(executors)); - root.store_missing_executors(&executors); + root.store_executors_which_added_to_the_cache(&executors); let executors = root.get_tx_executor_cache(accounts); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().visible.len(), 1); let fork1 = Bank::new_from_parent(&root, &Pubkey::default(), 1); let fork2 = Bank::new_from_parent(&root, &Pubkey::default(), 2); let executors = fork1.get_tx_executor_cache(accounts); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().visible.len(), 1); let executors = fork2.get_tx_executor_cache(accounts); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().visible.len(), 1); let mut executors = TransactionExecutorCache::default(); - executors.set(key2, executor.clone(), false); + executors.set(key2, executor.clone(), false, true); let executors = Rc::new(RefCell::new(executors)); - fork1.store_missing_executors(&executors); + fork1.store_executors_which_added_to_the_cache(&executors); let executors = fork1.get_tx_executor_cache(accounts); - assert_eq!(executors.borrow().executors.len(), 2); + assert_eq!(executors.borrow().visible.len(), 2); let executors = fork2.get_tx_executor_cache(accounts); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().visible.len(), 1); fork1.remove_executor(&key1); let executors = fork1.get_tx_executor_cache(accounts); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().visible.len(), 1); let executors = fork2.get_tx_executor_cache(accounts); - assert_eq!(executors.borrow().executors.len(), 1); + assert_eq!(executors.borrow().visible.len(), 1); } #[test] diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index fc0c8dc76..81d614831 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -610,6 +610,10 @@ pub mod prevent_rent_paying_rent_recipients { solana_sdk::declare_id!("Fab5oP3DmsLYCiQZXdjyqT3ukFFPrsmqhXU4WU1AWVVF"); } +pub mod delay_visibility_of_program_deployment { + solana_sdk::declare_id!("GmuBvtFb2aHfSfMXpuFeWZGHyDeCLPS79s48fmCWCfM5"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -757,6 +761,7 @@ lazy_static! { (remove_congestion_multiplier_from_fee_calculation::id(), "Remove congestion multiplier from transaction fee calculation #29881"), (enable_request_heap_frame_ix::id(), "Enable transaction to request heap frame using compute budget instruction #30076"), (prevent_rent_paying_rent_recipients::id(), "prevent recipients of rent rewards from ending in rent-paying state #30151"), + (delay_visibility_of_program_deployment::id(), "delay visibility of program upgrades #30085"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()