From 6b8545061fffab0c1412aef4f63bcaa3acfdc64e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Fri, 17 Nov 2023 09:54:21 +0100 Subject: [PATCH] Fix - `Bank::compute_active_feature_set()` and `Bank::apply_feature_activations()` (#34124) * Moves modification of feature accounts from Bank::compute_active_feature_set() into Bank::apply_feature_activations(). * Renames allow_new_activations and newly_activated to include_pending and pending. * Fix test_compute_active_feature_set. --- runtime/src/bank.rs | 50 ++++++++++++++++++++------------------- runtime/src/bank/tests.rs | 21 ++++++++-------- 2 files changed, 37 insertions(+), 34 deletions(-) diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs index e9cdc01a3..70e5d7c2f 100644 --- a/runtime/src/bank.rs +++ b/runtime/src/bank.rs @@ -7968,6 +7968,19 @@ impl Bank { self.compute_active_feature_set(allow_new_activations); self.feature_set = Arc::new(feature_set); + // Update activation slot of features in `new_feature_activations` + for feature_id in new_feature_activations.iter() { + if let Some(mut account) = self.get_account_with_fixed_root(feature_id) { + if let Some(mut feature) = feature::from_account(&account) { + feature.activated_at = Some(self.slot()); + if feature::to_account(&feature, &mut account).is_some() { + self.store_account(feature_id, &account); + } + info!("Feature {} activated at slot {}", feature_id, self.slot()); + } + } + } + if new_feature_activations.contains(&feature_set::pico_inflation::id()) { *self.inflation.write().unwrap() = Inflation::pico(); self.fee_rate_governor.burn_percent = 50; // 50% fee burn @@ -8036,38 +8049,27 @@ impl Bank { /// Compute the active feature set based on the current bank state, /// and return it together with the set of newly activated features. - fn compute_active_feature_set( - &mut self, - allow_new_activations: bool, - ) -> (FeatureSet, HashSet) { + fn compute_active_feature_set(&self, include_pending: bool) -> (FeatureSet, HashSet) { let mut active = self.feature_set.active.clone(); let mut inactive = HashSet::new(); - let mut newly_activated = HashSet::new(); + let mut pending = HashSet::new(); let slot = self.slot(); for feature_id in &self.feature_set.inactive { let mut activated = None; - if let Some(mut account) = self.get_account_with_fixed_root(feature_id) { - if let Some(mut feature) = feature::from_account(&account) { + if let Some(account) = self.get_account_with_fixed_root(feature_id) { + if let Some(feature) = feature::from_account(&account) { match feature.activated_at { - None => { - if allow_new_activations { - // Feature has been requested, activate it now - feature.activated_at = Some(slot); - if feature::to_account(&feature, &mut account).is_some() { - self.store_account(feature_id, &account); - } - newly_activated.insert(*feature_id); - activated = Some(slot); - info!("Feature {} activated at slot {}", feature_id, slot); - } + None if include_pending => { + // Feature activation is pending + pending.insert(*feature_id); + activated = Some(slot); } - Some(activation_slot) => { - if slot >= activation_slot { - // Feature is already active - activated = Some(activation_slot); - } + Some(activation_slot) if slot >= activation_slot => { + // Feature has been activated already + activated = Some(activation_slot); } + _ => {} } } } @@ -8078,7 +8080,7 @@ impl Bank { } } - (FeatureSet { active, inactive }, newly_activated) + (FeatureSet { active, inactive }, pending) } fn apply_builtin_program_feature_transitions( diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs index 23278f0da..4b8767454 100644 --- a/runtime/src/bank/tests.rs +++ b/runtime/src/bank/tests.rs @@ -7749,25 +7749,26 @@ fn test_compute_active_feature_set() { let feature = Feature::default(); assert_eq!(feature.activated_at, None); bank.store_account(&test_feature, &feature::create_account(&feature, 42)); - - // Run `compute_active_feature_set` disallowing new activations - let (feature_set, new_activations) = bank.compute_active_feature_set(false); - assert!(new_activations.is_empty()); - assert!(!feature_set.is_active(&test_feature)); let feature = feature::from_account(&bank.get_account(&test_feature).expect("get_account")) .expect("from_account"); assert_eq!(feature.activated_at, None); - // Run `compute_active_feature_set` allowing new activations - let (feature_set, new_activations) = bank.compute_active_feature_set(true); + // Run `compute_active_feature_set` excluding pending activation + let (feature_set, new_activations) = bank.compute_active_feature_set(false); + assert!(new_activations.is_empty()); + assert!(!feature_set.is_active(&test_feature)); + + // Run `compute_active_feature_set` including pending activation + let (_feature_set, new_activations) = bank.compute_active_feature_set(true); assert_eq!(new_activations.len(), 1); - assert!(feature_set.is_active(&test_feature)); + assert!(new_activations.contains(&test_feature)); + + // Actually activate the pending activation + bank.apply_feature_activations(ApplyFeatureActivationsCaller::NewFromParent, true); let feature = feature::from_account(&bank.get_account(&test_feature).expect("get_account")) .expect("from_account"); assert_eq!(feature.activated_at, Some(1)); - // Running `compute_active_feature_set` will not cause new activations, but - // `test_feature` is now be active let (feature_set, new_activations) = bank.compute_active_feature_set(true); assert!(new_activations.is_empty()); assert!(feature_set.is_active(&test_feature));