From d2732f7868a78a62056d71e791408230c12df1c7 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Tue, 11 Apr 2023 17:54:30 +0530 Subject: [PATCH 1/3] Retain usage counter on program upgrade --- program-runtime/src/executor_cache.rs | 13 ++- programs/bpf_loader/src/lib.rs | 122 +++++++++++++++++++++++++- 2 files changed, 127 insertions(+), 8 deletions(-) diff --git a/program-runtime/src/executor_cache.rs b/program-runtime/src/executor_cache.rs index 678d13b4c249d6..7cb16dfc33f0c6 100644 --- a/program-runtime/src/executor_cache.rs +++ b/program-runtime/src/executor_cache.rs @@ -63,13 +63,12 @@ impl TransactionExecutorCache { 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.visible.insert( - key, - Arc::new(LoadedProgram::new_tombstone( - current_slot, - LoadedProgramType::DelayVisibility, - )), - ); + let tombstone = + LoadedProgram::new_tombstone(current_slot, LoadedProgramType::DelayVisibility); + tombstone + .usage_counter + .store(executor.usage_counter.load(Relaxed), Relaxed); + self.visible.insert(key, Arc::new(tombstone)); } else { self.visible.insert(key, executor.clone()); } diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 6a259ee64106eb..786da498fe84ed 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -225,9 +225,15 @@ pub fn load_program_from_account( Ok((loaded_program, Some(load_program_metrics))) } +enum ProgramDeployReason { + Finalize, + DeployWithMaxDataLen, + Upgrade, +} + macro_rules! deploy_program { ($invoke_context:expr, $use_jit:expr, $program_id:expr, $loader_key:expr, - $account_size:expr, $slot:expr, $drop:expr, $new_programdata:expr $(,)?) => {{ + $account_size:expr, $slot:expr, $drop:expr, $reason:expr, $new_programdata:expr $(,)?) => {{ let delay_visibility_of_program_deployment = $invoke_context .feature_set .is_active(&delay_visibility_of_program_deployment::id()); @@ -244,6 +250,13 @@ macro_rules! deploy_program { $use_jit, true, )?; + let upgrade = matches!($reason, ProgramDeployReason::Upgrade); + if upgrade { + if let Some(old_entry) = $invoke_context.tx_executor_cache.borrow().get(&$program_id) { + let usage_counter = old_entry.usage_counter.load(Ordering::Relaxed); + executor.usage_counter.store(usage_counter, Ordering::Relaxed); + } + } $drop load_program_metrics.program_id = $program_id.to_string(); load_program_metrics.submit_datapoint(&mut $invoke_context.timings); @@ -793,6 +806,7 @@ fn process_loader_upgradeable_instruction( { drop(buffer); }, + ProgramDeployReason::DeployWithMaxDataLen, buffer .get_data() .get(buffer_data_offset..) @@ -982,6 +996,7 @@ fn process_loader_upgradeable_instruction( { drop(buffer); }, + ProgramDeployReason::Upgrade, buffer .get_data() .get(buffer_data_offset..) @@ -1503,6 +1518,7 @@ fn process_loader_instruction( program.get_data().len(), 0, {}, + ProgramDeployReason::Finalize, program.get_data(), ); program.set_executable(true)?; @@ -1641,6 +1657,8 @@ fn execute<'a, 'b: 'a>( #[cfg(test)] mod tests { + use solana_program_runtime::with_mock_invoke_context; + use std::sync::atomic::AtomicU64; use { super::*, rand::Rng, @@ -4046,4 +4064,106 @@ mod tests { ); } } + + fn deploy_test_program( + invoke_context: &mut InvokeContext, + program_id: Pubkey, + reason: ProgramDeployReason, + ) -> Result<(), InstructionError> { + let mut file = File::open("test_elfs/out/noop_unaligned.so").expect("file open failed"); + let mut elf = Vec::new(); + file.read_to_end(&mut elf).unwrap(); + deploy_program!( + invoke_context, + false, + program_id, + &bpf_loader_upgradeable::id(), + elf.len(), + 2, + {}, + reason, + &elf + ); + Ok(()) + } + + #[test] + fn test_program_usage_count_on_upgrade() { + let transaction_accounts = vec![]; + with_mock_invoke_context!(invoke_context, transaction_context, transaction_accounts); + let program_id = Pubkey::new_unique(); + let program = LoadedProgram { + program: LoadedProgramType::Unloaded, + account_size: 0, + deployment_slot: 0, + effective_slot: 0, + maybe_expiration_slot: None, + usage_counter: AtomicU64::new(100), + }; + invoke_context.tx_executor_cache.borrow_mut().set( + program_id, + Arc::new(program), + false, + false, + 0, + ); + + assert!(matches!( + deploy_test_program( + &mut invoke_context, + program_id, + ProgramDeployReason::Upgrade + ), + Ok(()) + )); + + let updated_program = invoke_context + .tx_executor_cache + .borrow() + .get(&program_id) + .expect("Didn't find upgraded program in the cache"); + + assert_eq!(updated_program.deployment_slot, 2); + assert_eq!(updated_program.usage_counter.load(Ordering::Relaxed), 100); + } + + #[test] + fn test_program_usage_count_on_non_upgrade() { + let transaction_accounts = vec![]; + with_mock_invoke_context!(invoke_context, transaction_context, transaction_accounts); + let program_id = Pubkey::new_unique(); + let program = LoadedProgram { + program: LoadedProgramType::Unloaded, + account_size: 0, + deployment_slot: 0, + effective_slot: 0, + maybe_expiration_slot: None, + usage_counter: AtomicU64::new(100), + }; + invoke_context.tx_executor_cache.borrow_mut().set( + program_id, + Arc::new(program), + false, + false, + 0, + ); + + assert!(matches!( + deploy_test_program( + &mut invoke_context, + program_id, + ProgramDeployReason::DeployWithMaxDataLen + ), + Ok(()) + )); + + let updated_program = invoke_context + .tx_executor_cache + .borrow() + .get(&program_id) + .expect("Didn't find upgraded program in the cache"); + + assert_eq!(updated_program.deployment_slot, 2); + assert_eq!(updated_program.usage_counter.load(Ordering::Relaxed), 0); + } } From 9faf1b20f02e547ae71866ed452392faeb3d9c51 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Tue, 11 Apr 2023 19:13:37 +0530 Subject: [PATCH 2/3] cleanup as per feedback --- programs/bpf_loader/src/lib.rs | 43 +++++++++------------------------- 1 file changed, 11 insertions(+), 32 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 786da498fe84ed..7ef6745a9dbf2a 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -225,15 +225,9 @@ pub fn load_program_from_account( Ok((loaded_program, Some(load_program_metrics))) } -enum ProgramDeployReason { - Finalize, - DeployWithMaxDataLen, - Upgrade, -} - macro_rules! deploy_program { ($invoke_context:expr, $use_jit:expr, $program_id:expr, $loader_key:expr, - $account_size:expr, $slot:expr, $drop:expr, $reason:expr, $new_programdata:expr $(,)?) => {{ + $account_size:expr, $slot:expr, $drop:expr, $new_programdata:expr $(,)?) => {{ let delay_visibility_of_program_deployment = $invoke_context .feature_set .is_active(&delay_visibility_of_program_deployment::id()); @@ -250,12 +244,9 @@ macro_rules! deploy_program { $use_jit, true, )?; - let upgrade = matches!($reason, ProgramDeployReason::Upgrade); - if upgrade { - if let Some(old_entry) = $invoke_context.tx_executor_cache.borrow().get(&$program_id) { - let usage_counter = old_entry.usage_counter.load(Ordering::Relaxed); - executor.usage_counter.store(usage_counter, Ordering::Relaxed); - } + if let Some(old_entry) = $invoke_context.tx_executor_cache.borrow().get(&$program_id) { + let usage_counter = old_entry.usage_counter.load(Ordering::Relaxed); + executor.usage_counter.store(usage_counter, Ordering::Relaxed); } $drop load_program_metrics.program_id = $program_id.to_string(); @@ -806,7 +797,6 @@ fn process_loader_upgradeable_instruction( { drop(buffer); }, - ProgramDeployReason::DeployWithMaxDataLen, buffer .get_data() .get(buffer_data_offset..) @@ -996,7 +986,6 @@ fn process_loader_upgradeable_instruction( { drop(buffer); }, - ProgramDeployReason::Upgrade, buffer .get_data() .get(buffer_data_offset..) @@ -1518,7 +1507,6 @@ fn process_loader_instruction( program.get_data().len(), 0, {}, - ProgramDeployReason::Finalize, program.get_data(), ); program.set_executable(true)?; @@ -4068,7 +4056,6 @@ mod tests { fn deploy_test_program( invoke_context: &mut InvokeContext, program_id: Pubkey, - reason: ProgramDeployReason, ) -> Result<(), InstructionError> { let mut file = File::open("test_elfs/out/noop_unaligned.so").expect("file open failed"); let mut elf = Vec::new(); @@ -4081,7 +4068,6 @@ mod tests { elf.len(), 2, {}, - reason, &elf ); Ok(()) @@ -4109,11 +4095,7 @@ mod tests { ); assert!(matches!( - deploy_test_program( - &mut invoke_context, - program_id, - ProgramDeployReason::Upgrade - ), + deploy_test_program(&mut invoke_context, program_id,), Ok(()) )); @@ -4148,22 +4130,19 @@ mod tests { 0, ); + let program_id2 = Pubkey::new_unique(); assert!(matches!( - deploy_test_program( - &mut invoke_context, - program_id, - ProgramDeployReason::DeployWithMaxDataLen - ), + deploy_test_program(&mut invoke_context, program_id2), Ok(()) )); - let updated_program = invoke_context + let program2 = invoke_context .tx_executor_cache .borrow() - .get(&program_id) + .get(&program_id2) .expect("Didn't find upgraded program in the cache"); - assert_eq!(updated_program.deployment_slot, 2); - assert_eq!(updated_program.usage_counter.load(Ordering::Relaxed), 0); + assert_eq!(program2.deployment_slot, 2); + assert_eq!(program2.usage_counter.load(Ordering::Relaxed), 0); } } From 3a66210b9fd2723760f1152254e48583c2388bff Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Tue, 11 Apr 2023 19:28:30 +0530 Subject: [PATCH 3/3] fix clippy --- programs/bpf_loader/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 7ef6745a9dbf2a..65546a1543b4e9 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -1645,12 +1645,12 @@ fn execute<'a, 'b: 'a>( #[cfg(test)] mod tests { - use solana_program_runtime::with_mock_invoke_context; - use std::sync::atomic::AtomicU64; use { super::*, rand::Rng, - solana_program_runtime::invoke_context::mock_process_instruction, + solana_program_runtime::{ + invoke_context::mock_process_instruction, with_mock_invoke_context, + }, solana_rbpf::{ ebpf::MM_INPUT_START, elf::Executable, @@ -1669,7 +1669,7 @@ mod tests { rent::Rent, system_program, sysvar, }, - std::{fs::File, io::Read, ops::Range}, + std::{fs::File, io::Read, ops::Range, sync::atomic::AtomicU64}, }; struct TestContextObject {