Refactor - Bank::compute_active_feature_set() (solana-labs#32872)

Returns the feature set instead of overwriting it inside Bank::compute_active_feature_set().
wen-coding · Aug 21, 2023 · f6a624a · f6a624a
1 parent 561e0b6
commit f6a624a
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 27 deletions.
diff --git a/runtime/src/bank.rs b/runtime/src/bank.rs
@@ -7933,7 +7933,9 @@ impl Bank {
             NewFromParent => true,
             WarpFromParent => false,
         };
-        let new_feature_activations = self.compute_active_feature_set(allow_new_activations);
+        let (feature_set, new_feature_activations) =
+            self.compute_active_feature_set(allow_new_activations);
+        self.feature_set = Arc::new(feature_set);
 
         if new_feature_activations.contains(&feature_set::pico_inflation::id()) {
             *self.inflation.write().unwrap() = Inflation::pico();
@@ -7981,8 +7983,12 @@ impl Bank {
         );
     }
 
-    // Compute the active feature set based on the current bank state, and return the set of newly activated features
-    fn compute_active_feature_set(&mut self, allow_new_activations: bool) -> HashSet<Pubkey> {
+    /// 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<Pubkey>) {
         let mut active = self.feature_set.active.clone();
         let mut inactive = HashSet::new();
         let mut newly_activated = HashSet::new();
@@ -8021,8 +8027,7 @@ impl Bank {
             }
         }
 
-        self.feature_set = Arc::new(FeatureSet { active, inactive });
-        newly_activated
+        (FeatureSet { active, inactive }, newly_activated)
     }
 
     fn apply_builtin_program_feature_transitions(

diff --git a/runtime/src/bank/tests.rs b/runtime/src/bank/tests.rs
@@ -7971,46 +7971,42 @@ fn test_compute_active_feature_set() {
     feature_set.inactive.insert(test_feature);
     bank.feature_set = Arc::new(feature_set.clone());
 
-    let new_activations = bank.compute_active_feature_set(true);
+    let (feature_set, new_activations) = bank.compute_active_feature_set(true);
     assert!(new_activations.is_empty());
-    assert!(!bank.feature_set.is_active(&test_feature));
+    assert!(!feature_set.is_active(&test_feature));
 
     // Depositing into the `test_feature` account should do nothing
     bank.deposit(&test_feature, 42).unwrap();
-    let new_activations = bank.compute_active_feature_set(true);
+    let (feature_set, new_activations) = bank.compute_active_feature_set(true);
     assert!(new_activations.is_empty());
-    assert!(!bank.feature_set.is_active(&test_feature));
+    assert!(!feature_set.is_active(&test_feature));
 
     // Request `test_feature` activation
     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 new_activations = bank.compute_active_feature_set(false);
+    let (feature_set, new_activations) = bank.compute_active_feature_set(false);
     assert!(new_activations.is_empty());
-    assert!(!bank.feature_set.is_active(&test_feature));
+    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 new_activations = bank.compute_active_feature_set(true);
+    let (feature_set, new_activations) = bank.compute_active_feature_set(true);
     assert_eq!(new_activations.len(), 1);
-    assert!(bank.feature_set.is_active(&test_feature));
+    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, Some(1));
 
-    // Reset the bank's feature set
-    bank.feature_set = Arc::new(feature_set);
-    assert!(!bank.feature_set.is_active(&test_feature));
-
     // Running `compute_active_feature_set` will not cause new activations, but
     // `test_feature` is now be active
-    let new_activations = bank.compute_active_feature_set(true);
+    let (feature_set, new_activations) = bank.compute_active_feature_set(true);
     assert!(new_activations.is_empty());
-    assert!(bank.feature_set.is_active(&test_feature));
+    assert!(feature_set.is_active(&test_feature));
 }
 
 #[test]
@@ -8880,7 +8876,7 @@ fn test_get_inflation_start_slot_devnet_testnet() {
             42,
         ),
     );
-    bank.compute_active_feature_set(true);
+    bank.feature_set = Arc::new(bank.compute_active_feature_set(true).0);
     assert_eq!(bank.get_inflation_start_slot(), 1);
 
     // Advance slot
@@ -8898,7 +8894,7 @@ fn test_get_inflation_start_slot_devnet_testnet() {
             42,
         ),
     );
-    bank.compute_active_feature_set(true);
+    bank.feature_set = Arc::new(bank.compute_active_feature_set(true).0);
     assert_eq!(bank.get_inflation_start_slot(), 2);
 
     // Request `full_inflation::mainnet::certusone` activation,
@@ -8921,7 +8917,7 @@ fn test_get_inflation_start_slot_devnet_testnet() {
             42,
         ),
     );
-    bank.compute_active_feature_set(true);
+    bank.feature_set = Arc::new(bank.compute_active_feature_set(true).0);
     assert_eq!(bank.get_inflation_start_slot(), 2);
 }
 
@@ -8961,7 +8957,7 @@ fn test_get_inflation_start_slot_mainnet() {
             42,
         ),
     );
-    bank.compute_active_feature_set(true);
+    bank.feature_set = Arc::new(bank.compute_active_feature_set(true).0);
     assert_eq!(bank.get_inflation_start_slot(), 1);
 
     // Advance slot
@@ -8988,7 +8984,7 @@ fn test_get_inflation_start_slot_mainnet() {
             42,
         ),
     );
-    bank.compute_active_feature_set(true);
+    bank.feature_set = Arc::new(bank.compute_active_feature_set(true).0);
     assert_eq!(bank.get_inflation_start_slot(), 2);
 
     // Advance slot
@@ -9006,7 +9002,7 @@ fn test_get_inflation_start_slot_mainnet() {
             42,
         ),
     );
-    bank.compute_active_feature_set(true);
+    bank.feature_set = Arc::new(bank.compute_active_feature_set(true).0);
     assert_eq!(bank.get_inflation_start_slot(), 2);
 }
 
@@ -9048,7 +9044,7 @@ fn test_get_inflation_num_slots_with_activations() {
             42,
         ),
     );
-    bank.compute_active_feature_set(true);
+    bank.feature_set = Arc::new(bank.compute_active_feature_set(true).0);
     assert_eq!(bank.get_inflation_num_slots(), slots_per_epoch);
     for _ in 0..slots_per_epoch {
         bank = new_from_parent(&Arc::new(bank));
@@ -9066,7 +9062,7 @@ fn test_get_inflation_num_slots_with_activations() {
             42,
         ),
     );
-    bank.compute_active_feature_set(true);
+    bank.feature_set = Arc::new(bank.compute_active_feature_set(true).0);
     assert_eq!(bank.get_inflation_num_slots(), slots_per_epoch);
     for _ in 0..slots_per_epoch {
         bank = new_from_parent(&Arc::new(bank));