From f74c69233aa58cdbb5563e0f8510eaa5dd76ac94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Wed, 14 Feb 2024 15:32:32 +0000 Subject: [PATCH 1/3] Forbids all program replacements except for reloads and builtins. --- program-runtime/src/loaded_programs.rs | 81 +++++++++----------------- 1 file changed, 27 insertions(+), 54 deletions(-) diff --git a/program-runtime/src/loaded_programs.rs b/program-runtime/src/loaded_programs.rs index 6da84b0d1f0692..94c72d17fc6946 100644 --- a/program-runtime/src/loaded_programs.rs +++ b/program-runtime/src/loaded_programs.rs @@ -721,33 +721,39 @@ impl LoadedPrograms { }) { Ok(index) => { let existing = slot_versions.get_mut(index).unwrap(); - if std::mem::discriminant(&existing.program) - != std::mem::discriminant(&entry.program) - { - // Copy over the usage counter to the new entry - entry.tx_usage_counter.fetch_add( - existing.tx_usage_counter.load(Ordering::Relaxed), - Ordering::Relaxed, - ); - entry.ix_usage_counter.fetch_add( - existing.ix_usage_counter.load(Ordering::Relaxed), - Ordering::Relaxed, - ); - *existing = entry.clone(); - self.stats.reloads.fetch_add(1, Ordering::Relaxed); - false - } else { - // Something is wrong, I can feel it ... - self.stats.replacements.fetch_add(1, Ordering::Relaxed); - true + match (&existing.program, &entry.program) { + (LoadedProgramType::Builtin(_), LoadedProgramType::Builtin(_)) + | (LoadedProgramType::Unloaded(_), LoadedProgramType::LegacyV0(_)) + | (LoadedProgramType::Unloaded(_), LoadedProgramType::LegacyV1(_)) + | (LoadedProgramType::Unloaded(_), LoadedProgramType::Typed(_)) => {} + #[cfg(test)] + (LoadedProgramType::Unloaded(_), LoadedProgramType::TestLoaded(_)) => {} + _ => { + // Something is wrong, I can feel it ... + error!("LoadedPrograms::assign_program() failed key={:?} existing={:?} entry={:?}", key, slot_versions, entry); + debug_assert!(false, "Unexpected replacement of an entry"); + self.stats.replacements.fetch_add(1, Ordering::Relaxed); + return true; + } } + // Copy over the usage counter to the new entry + entry.tx_usage_counter.fetch_add( + existing.tx_usage_counter.load(Ordering::Relaxed), + Ordering::Relaxed, + ); + entry.ix_usage_counter.fetch_add( + existing.ix_usage_counter.load(Ordering::Relaxed), + Ordering::Relaxed, + ); + *existing = Arc::clone(&entry); + self.stats.reloads.fetch_add(1, Ordering::Relaxed); } Err(index) => { self.stats.insertions.fetch_add(1, Ordering::Relaxed); - slot_versions.insert(index, entry.clone()); - false + slot_versions.insert(index, Arc::clone(&entry)); } } + false } pub fn prune_by_deployment_slot(&mut self, slot: Slot) { @@ -1675,39 +1681,6 @@ mod tests { } } - #[test] - fn test_assign_program_tombstones() { - let mut cache = new_mock_cache::(); - let program1 = Pubkey::new_unique(); - let env = cache.environments.program_runtime_v1.clone(); - - set_tombstone( - &mut cache, - program1, - 10, - LoadedProgramType::FailedVerification(env.clone()), - ); - assert_eq!(cache.entries.get(&program1).unwrap().slot_versions.len(), 1); - set_tombstone(&mut cache, program1, 10, LoadedProgramType::Closed); - assert_eq!(cache.entries.get(&program1).unwrap().slot_versions.len(), 1); - set_tombstone( - &mut cache, - program1, - 10, - LoadedProgramType::FailedVerification(env.clone()), - ); - assert_eq!(cache.entries.get(&program1).unwrap().slot_versions.len(), 1); - - // Fail on exact replacement - assert!(cache.assign_program( - program1, - Arc::new(LoadedProgram::new_tombstone( - 10, - LoadedProgramType::FailedVerification(env) - )) - )); - } - #[test] fn test_tombstone() { let env = Arc::new(BuiltinProgram::new_mock()); From ffab72423771c561036b01a544e90028621cae6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 23 Feb 2024 10:54:41 +0000 Subject: [PATCH 2/3] Adds test_assign_program_failure() and test_assign_program_success(). --- Cargo.lock | 1 + program-runtime/Cargo.toml | 1 + program-runtime/src/loaded_programs.rs | 100 +++++++++++++++++++++++++ 3 files changed, 102 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index a727fae2c8b0ee..4ff1942a25c170 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6649,6 +6649,7 @@ dependencies = [ "solana-metrics", "solana-sdk", "solana_rbpf", + "test-case", "thiserror", ] diff --git a/program-runtime/Cargo.toml b/program-runtime/Cargo.toml index ed4b2a60aa3f0a..afec7352e1fb70 100644 --- a/program-runtime/Cargo.toml +++ b/program-runtime/Cargo.toml @@ -35,6 +35,7 @@ assert_matches = { workspace = true } libsecp256k1 = { workspace = true } solana-logger = { workspace = true } solana-sdk = { workspace = true, features = ["dev-context-only-utils"] } +test-case = { workspace = true } [lib] crate-type = ["lib"] diff --git a/program-runtime/src/loaded_programs.rs b/program-runtime/src/loaded_programs.rs index 94c72d17fc6946..a569730716a3ec 100644 --- a/program-runtime/src/loaded_programs.rs +++ b/program-runtime/src/loaded_programs.rs @@ -1159,6 +1159,7 @@ mod tests { Arc, RwLock, }, }, + test_case::{test_case, test_matrix}, }; static MOCK_ENVIRONMENT: std::sync::OnceLock = @@ -1681,6 +1682,105 @@ mod tests { } } + #[test_matrix( + ( + LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())), + LoadedProgramType::Closed, + LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock())), + ), + ( + LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())), + LoadedProgramType::Closed, + LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())), + LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock())), + LoadedProgramType::Builtin(BuiltinProgram::new_mock()), + ) + )] + #[test_matrix( + (LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())),), + ( + LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())), + LoadedProgramType::Closed, + LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())), + LoadedProgramType::Builtin(BuiltinProgram::new_mock()), + ) + )] + #[test_matrix( + (LoadedProgramType::Builtin(BuiltinProgram::new_mock()),), + ( + LoadedProgramType::FailedVerification(Arc::new(BuiltinProgram::new_mock())), + LoadedProgramType::Closed, + LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())), + LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock())), + ) + )] + #[should_panic(expected = "Unexpected replacement of an entry")] + fn test_assign_program_failure(old: LoadedProgramType, new: LoadedProgramType) { + let mut cache = new_mock_cache::(); + let program_id = Pubkey::new_unique(); + assert!(!cache.assign_program( + program_id, + Arc::new(LoadedProgram { + program: old, + account_size: 0, + deployment_slot: 10, + effective_slot: 11, + tx_usage_counter: AtomicU64::default(), + ix_usage_counter: AtomicU64::default(), + latest_access_slot: AtomicU64::default(), + }), + )); + cache.assign_program( + program_id, + Arc::new(LoadedProgram { + program: new, + account_size: 0, + deployment_slot: 10, + effective_slot: 11, + tx_usage_counter: AtomicU64::default(), + ix_usage_counter: AtomicU64::default(), + latest_access_slot: AtomicU64::default(), + }), + ); + } + + #[test_case( + LoadedProgramType::Unloaded(Arc::new(BuiltinProgram::new_mock())), + LoadedProgramType::TestLoaded(Arc::new(BuiltinProgram::new_mock())) + )] + #[test_case( + LoadedProgramType::Builtin(BuiltinProgram::new_mock()), + LoadedProgramType::Builtin(BuiltinProgram::new_mock()) + )] + fn test_assign_program_success(old: LoadedProgramType, new: LoadedProgramType) { + let mut cache = new_mock_cache::(); + let program_id = Pubkey::new_unique(); + assert!(!cache.assign_program( + program_id, + Arc::new(LoadedProgram { + program: old, + account_size: 0, + deployment_slot: 10, + effective_slot: 11, + tx_usage_counter: AtomicU64::default(), + ix_usage_counter: AtomicU64::default(), + latest_access_slot: AtomicU64::default(), + }), + )); + assert!(!cache.assign_program( + program_id, + Arc::new(LoadedProgram { + program: new, + account_size: 0, + deployment_slot: 10, + effective_slot: 11, + tx_usage_counter: AtomicU64::default(), + ix_usage_counter: AtomicU64::default(), + latest_access_slot: AtomicU64::default(), + }), + )); + } + #[test] fn test_tombstone() { let env = Arc::new(BuiltinProgram::new_mock()); From 89b434d19b062f0b02a1acb865b3719c0f631225 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 23 Feb 2024 11:13:46 +0000 Subject: [PATCH 3/3] Explicitly disallows LoadedProgramType::DelayVisibility to be inserted in the global cache. --- program-runtime/src/loaded_programs.rs | 35 ++++++++------------------ 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/program-runtime/src/loaded_programs.rs b/program-runtime/src/loaded_programs.rs index a569730716a3ec..75302284e344d0 100644 --- a/program-runtime/src/loaded_programs.rs +++ b/program-runtime/src/loaded_programs.rs @@ -713,6 +713,10 @@ impl LoadedPrograms { /// Insert a single entry. It's typically called during transaction loading, /// when the cache doesn't contain the entry corresponding to program `key`. pub fn assign_program(&mut self, key: Pubkey, entry: Arc) -> bool { + debug_assert!(!matches!( + &entry.program, + LoadedProgramType::DelayVisibility + )); let slot_versions = &mut self.entries.entry(key).or_default().slot_versions; match slot_versions.binary_search_by(|at| { at.effective_slot @@ -1349,15 +1353,6 @@ mod tests { programs.push((program2, *deployment_slot, usage_counter)); }); - for slot in 21..31 { - set_tombstone( - &mut cache, - program2, - slot, - LoadedProgramType::DelayVisibility, - ); - } - for slot in 31..41 { insert_unloaded_program(&mut cache, program2, slot); } @@ -1409,12 +1404,12 @@ mod tests { // Test that the cache is constructed with the expected number of entries. assert_eq!(num_loaded, 8); assert_eq!(num_unloaded, 30); - assert_eq!(num_tombstones, 30); + assert_eq!(num_tombstones, 20); // Evicting to 2% should update cache with // * 5 active entries // * 33 unloaded entries (3 active programs will get unloaded) - // * 30 tombstones (tombstones are not evicted) + // * 20 tombstones (tombstones are not evicted) cache.evict_using_2s_random_selection(Percentage::from(2), 21); let num_loaded = num_matching_entries(&cache, |program_type| { @@ -1435,7 +1430,7 @@ mod tests { // Test that expected number of loaded entries get evicted/unloaded. assert_eq!(num_loaded, 5); assert_eq!(num_unloaded, 33); - assert_eq!(num_tombstones, 30); + assert_eq!(num_tombstones, 20); } #[test] @@ -1496,15 +1491,6 @@ mod tests { programs.push((program2, *deployment_slot, usage_counter)); }); - for slot in 21..31 { - set_tombstone( - &mut cache, - program2, - slot, - LoadedProgramType::DelayVisibility, - ); - } - for slot in 31..41 { insert_unloaded_program(&mut cache, program2, slot); } @@ -1555,12 +1541,12 @@ mod tests { assert_eq!(num_loaded, 8); assert_eq!(num_unloaded, 30); - assert_eq!(num_tombstones, 30); + assert_eq!(num_tombstones, 20); // Evicting to 2% should update cache with // * 5 active entries // * 33 unloaded entries (3 active programs will get unloaded) - // * 30 tombstones (tombstones are not evicted) + // * 20 tombstones (tombstones are not evicted) cache.sort_and_unload(Percentage::from(2)); // Check that every program is still in the cache. programs.iter().for_each(|entry| { @@ -1600,7 +1586,7 @@ mod tests { assert_eq!(num_loaded, 5); assert_eq!(num_unloaded, 33); - assert_eq!(num_tombstones, 30); + assert_eq!(num_tombstones, 20); } #[test] @@ -2458,7 +2444,6 @@ mod tests { for loaded_program_type in [ LoadedProgramType::FailedVerification(cache.environments.program_runtime_v1.clone()), LoadedProgramType::Closed, - LoadedProgramType::DelayVisibility, // Never inserted in the global cache LoadedProgramType::Unloaded(cache.environments.program_runtime_v1.clone()), LoadedProgramType::Builtin(BuiltinProgram::new_mock()), ] {