From a2d6f6efc5d71271d8d622fee83d8809cd650a30 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Tue, 21 Feb 2023 10:21:30 -0800 Subject: [PATCH] address review comments --- program-runtime/src/executor_cache.rs | 7 +-- program-runtime/src/loaded_programs.rs | 69 +++++++++++++------------- programs/bpf_loader/src/lib.rs | 5 +- runtime/src/bank.rs | 4 +- 4 files changed, 45 insertions(+), 40 deletions(-) diff --git a/program-runtime/src/executor_cache.rs b/program-runtime/src/executor_cache.rs index b01778ff311f5a..9c3982c54dba8f 100644 --- a/program-runtime/src/executor_cache.rs +++ b/program-runtime/src/executor_cache.rs @@ -41,9 +41,9 @@ impl TransactionExecutorCache { self.visible.get(key).cloned() } - pub fn set_tombstone(&mut self, key: Pubkey) { + pub fn set_tombstone(&mut self, key: Pubkey, slot: Slot) { self.visible - .insert(key, Arc::new(LoadedProgram::new_tombstone(0))); + .insert(key, Arc::new(LoadedProgram::new_tombstone(slot))); } pub fn set( @@ -52,12 +52,13 @@ impl TransactionExecutorCache { executor: Arc, upgrade: bool, delay_visibility_of_program_deployment: bool, + slot: Slot, ) { 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); + self.set_tombstone(key, slot); } else { self.visible.insert(key, executor.clone()); } diff --git a/program-runtime/src/loaded_programs.rs b/program-runtime/src/loaded_programs.rs index d678518df17557..7bb955c6519a06 100644 --- a/program-runtime/src/loaded_programs.rs +++ b/program-runtime/src/loaded_programs.rs @@ -180,12 +180,12 @@ impl LoadedProgram { } } - pub fn new_tombstone(deployment_slot: Slot) -> Self { + pub fn new_tombstone(slot: Slot) -> Self { Self { program: LoadedProgramType::Invalid, account_size: 0, - deployment_slot, - effective_slot: deployment_slot, + deployment_slot: slot, + effective_slot: slot, usage_counter: AtomicU64::default(), } } @@ -219,8 +219,10 @@ pub enum LoadedProgramEntry { } impl LoadedPrograms { - /// Inserts a single entry - pub fn insert_entry(&mut self, key: Pubkey, entry: Arc) -> LoadedProgramEntry { + /// Refill the cache with a single entry. It's typically called during transaction processing, + /// when the cache doesn't contain the entry corresponding to program `key`. + /// The function dedupes the cache, in case some other thread replenished the the entry in parallel. + pub fn replenish(&mut self, key: Pubkey, entry: Arc) -> LoadedProgramEntry { let second_level = self.entries.entry(key).or_insert_with(Vec::new); let index = second_level .iter() @@ -239,29 +241,28 @@ impl LoadedPrograms { LoadedProgramEntry::WasVacant(entry) } - /// Insert or replace the current entry with a tombstone. - /// This behaves differently than `insert_entry()` in case there's currently a program at the - /// `deploytment_slot`. It'll replace the current entry with a tombstone, whereas `insert_entry()` - /// would retain and return the current entry. - pub fn set_tombstone(&mut self, key: Pubkey, deployment_slot: Slot) -> Arc { - let tombstone = Arc::new(LoadedProgram::new_tombstone(deployment_slot)); + /// Assign the program `entry` to the given `key` in the cache. + /// This is typically called when a deployed program is managed (upgraded/un/reddeployed) via + /// bpf loader instructions. + /// The program management is not expected to overlap with initial program deployment slot. + /// Note: Do not call this function to replenish cache with a missing entry. As that use-case can + /// cause the cache to have duplicates. Use `replenish()` API for that use-case. + pub fn assign_program(&mut self, key: Pubkey, entry: Arc) -> Arc { let second_level = self.entries.entry(key).or_insert_with(Vec::new); let index = second_level .iter() - .position(|at| at.effective_slot >= tombstone.effective_slot); + .position(|at| at.effective_slot >= entry.effective_slot); if let Some(index) = index { let existing = second_level .get(index) .expect("Missing entry, even though position was found"); - if existing.deployment_slot == tombstone.deployment_slot - && existing.effective_slot >= tombstone.effective_slot - { - second_level.splice(index..=index, [tombstone.clone()]); - return tombstone; - } + assert!( + existing.deployment_slot != entry.deployment_slot + || existing.effective_slot != entry.effective_slot + ); } - second_level.insert(index.unwrap_or(second_level.len()), tombstone.clone()); - tombstone + second_level.insert(index.unwrap_or(second_level.len()), entry.clone()); + entry } /// Before rerooting the blockstore this removes all programs of orphan forks @@ -369,7 +370,7 @@ mod tests { let mut cache = LoadedPrograms::default(); let program1 = Pubkey::new_unique(); - let tombstone = cache.set_tombstone(program1, 10); + let tombstone = cache.assign_program(program1, 10); let second_level = &cache .entries .get(&program1) @@ -382,7 +383,7 @@ mod tests { // Add a program at slot 50, and a tombstone for the program at slot 60 let program2 = Pubkey::new_unique(); assert!(matches!( - cache.insert_entry(program2, new_test_builtin_program(50, 51)), + cache.replenish(program2, new_test_builtin_program(50, 51)), LoadedProgramEntry::WasVacant(_) )); let second_level = &cache @@ -392,7 +393,7 @@ mod tests { assert_eq!(second_level.len(), 1); assert!(!second_level.get(0).unwrap().is_tombstone()); - let tombstone = cache.set_tombstone(program2, 60); + let tombstone = cache.assign_program(program2, 60); let second_level = &cache .entries .get(&program2) @@ -405,7 +406,7 @@ mod tests { assert_eq!(tombstone.effective_slot, 60); // Replace the program at slot 50 with a tombstone - let tombstone = cache.set_tombstone(program2, 50); + let tombstone = cache.assign_program(program2, 50); let second_level = &cache .entries .get(&program2) @@ -604,52 +605,52 @@ mod tests { let program1 = Pubkey::new_unique(); assert!(matches!( - cache.insert_entry(program1, new_test_loaded_program(0, 1)), + cache.replenish(program1, new_test_loaded_program(0, 1)), LoadedProgramEntry::WasVacant(_) )); assert!(matches!( - cache.insert_entry(program1, new_test_loaded_program(10, 11)), + cache.replenish(program1, new_test_loaded_program(10, 11)), LoadedProgramEntry::WasVacant(_) )); assert!(matches!( - cache.insert_entry(program1, new_test_loaded_program(20, 21)), + cache.replenish(program1, new_test_loaded_program(20, 21)), LoadedProgramEntry::WasVacant(_) )); // Test: inserting duplicate entry return pre existing entry from the cache assert!(matches!( - cache.insert_entry(program1, new_test_loaded_program(20, 21)), + cache.replenish(program1, new_test_loaded_program(20, 21)), LoadedProgramEntry::WasOccupied(_) )); let program2 = Pubkey::new_unique(); assert!(matches!( - cache.insert_entry(program2, new_test_loaded_program(5, 6)), + cache.replenish(program2, new_test_loaded_program(5, 6)), LoadedProgramEntry::WasVacant(_) )); assert!(matches!( - cache.insert_entry(program2, new_test_loaded_program(11, 12)), + cache.replenish(program2, new_test_loaded_program(11, 12)), LoadedProgramEntry::WasVacant(_) )); let program3 = Pubkey::new_unique(); assert!(matches!( - cache.insert_entry(program3, new_test_loaded_program(25, 26)), + cache.replenish(program3, new_test_loaded_program(25, 26)), LoadedProgramEntry::WasVacant(_) )); let program4 = Pubkey::new_unique(); assert!(matches!( - cache.insert_entry(program4, new_test_loaded_program(0, 1)), + cache.replenish(program4, new_test_loaded_program(0, 1)), LoadedProgramEntry::WasVacant(_) )); assert!(matches!( - cache.insert_entry(program4, new_test_loaded_program(5, 6)), + cache.replenish(program4, new_test_loaded_program(5, 6)), LoadedProgramEntry::WasVacant(_) )); // The following is a special case, where effective slot is 4 slots in the future assert!(matches!( - cache.insert_entry(program4, new_test_loaded_program(15, 19)), + cache.replenish(program4, new_test_loaded_program(15, 19)), LoadedProgramEntry::WasVacant(_) )); diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 0a6762b4d8f8ec..16a083fb345ab7 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -258,6 +258,7 @@ pub fn load_program_from_account( loaded_program.clone(), false, feature_set.is_active(&delay_visibility_of_program_deployment::id()), + deployment_slot, ); } @@ -291,6 +292,7 @@ macro_rules! deploy_program { Arc::new(executor), true, delay_visibility_of_program_deployment, + $slot, ); }}; } @@ -1183,10 +1185,11 @@ fn process_loader_upgradeable_instruction( .feature_set .is_active(&delay_visibility_of_program_deployment::id()) { + let clock = invoke_context.get_sysvar_cache().get_clock()?; invoke_context .tx_executor_cache .borrow_mut() - .set_tombstone(program_key); + .set_tombstone(program_key, clock.slot); } } _ => { diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index dac55cf8cf34b2..6f96d0fcab5e70 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -4375,7 +4375,7 @@ impl Bank { .loaded_programs_cache .write() .unwrap() - .insert_entry(*pubkey, program) + .replenish(*pubkey, program) { LoadedProgramEntry::WasOccupied(entry) => { loaded_programs_for_txs.insert(*pubkey, entry); @@ -4393,7 +4393,7 @@ impl Bank { .loaded_programs_cache .write() .unwrap() - .set_tombstone(*pubkey, self.slot); + .assign_program(*pubkey, Arc::new(LoadedProgram::new_tombstone(self.slot))); loaded_programs_for_txs.insert(*pubkey, tombstone); } });