Cherry-pick account set root fixes (#4076)

automerge

core/src/bank_forks.rs

@@ -35,7 +35,8 @@ impl BankForks {
     pub fn ancestors(&self) -> HashMap<u64, HashSet<u64>> {
         let mut ancestors = HashMap::new();
         for bank in self.banks.values() {
-            let set = bank.parents().into_iter().map(|b| b.slot()).collect();
+            let mut set: HashSet<u64> = bank.ancestors.keys().cloned().collect();
+            set.remove(&bank.slot());
             ancestors.insert(bank.slot(), set);
         }
         ancestors
@@ -46,9 +47,11 @@ impl BankForks {
         let mut descendants = HashMap::new();
         for bank in self.banks.values() {
             let _ = descendants.entry(bank.slot()).or_insert(HashSet::new());
-            for parent in bank.parents() {
+            let mut set: HashSet<u64> = bank.ancestors.keys().cloned().collect();
+            set.remove(&bank.slot());
+            for parent in set {
                 descendants
-                    .entry(parent.slot())
+                    .entry(parent)
                     .or_insert(HashSet::new())
                     .insert(bank.slot());
             }
@@ -116,8 +119,9 @@ impl BankForks {
     }
 
     fn prune_non_root(&mut self, root: u64) {
+        let descendants = self.descendants();
         self.banks
-            .retain(|slot, bank| *slot >= root || bank.is_in_subtree_of(root))
+            .retain(|slot, _| descendants[&root].contains(slot))
     }
 }
 

runtime/src/accounts_index.rs

@@ -66,9 +66,11 @@ impl<T: Clone> AccountsIndex<T> {
         self.roots.contains(&fork)
     }
     pub fn add_root(&mut self, fork: Fork) {
-        if fork > self.last_root {
+        assert!(
-            self.last_root = fork;
+            (self.last_root == 0 && fork == 0) || (fork > self.last_root),
-        }
+            "new roots must be increasing"
+        );
+        self.last_root = fork;
         self.roots.insert(fork);
     }
     /// Remove the fork when the storage for the fork is freed
@@ -156,15 +158,22 @@ mod tests {
     fn test_max_last_root() {
         let mut index = AccountsIndex::<bool>::default();
         index.add_root(1);
-        index.add_root(0);
         assert_eq!(index.last_root, 1);
     }
 
+    #[test]
+    #[should_panic]
+    fn test_max_last_root_old() {
+        let mut index = AccountsIndex::<bool>::default();
+        index.add_root(1);
+        index.add_root(0);
+    }
+
     #[test]
     fn test_cleanup_first() {
         let mut index = AccountsIndex::<bool>::default();
-        index.add_root(1);
         index.add_root(0);
+        index.add_root(1);
         index.cleanup_dead_fork(0);
         assert!(index.is_root(1));
         assert!(!index.is_root(0));
@@ -174,8 +183,8 @@ mod tests {
     fn test_cleanup_last() {
         //this behavior might be undefined, clean up should only occur on older forks
         let mut index = AccountsIndex::<bool>::default();
-        index.add_root(1);
         index.add_root(0);
+        index.add_root(1);
         index.cleanup_dead_fork(1);
         assert!(!index.is_root(1));
         assert!(index.is_root(0));

runtime/src/bank.rs

@@ -124,7 +124,7 @@ pub struct Bank {
     parent: RwLock<Option<Arc<Bank>>>,
 
     /// The set of parents including this bank
-    ancestors: HashMap<u64, usize>,
+    pub ancestors: HashMap<u64, usize>,
 
     /// Hash of this Bank's state. Only meaningful after freezing.
     hash: RwLock<Hash>,
@@ -293,7 +293,7 @@ impl Bank {
         *self.parent.write().unwrap() = None;
 
         let squash_accounts_start = Instant::now();
-        for p in &parents {
+        for p in parents.iter().rev() {
             // root forks cannot be purged
             self.accounts.add_root(p.slot());
         }
@@ -1026,24 +1026,6 @@ impl Bank {
         // Register a bogus executable account, which will be loaded and ignored.
         self.register_native_instruction_processor("", &program_id);
     }
-
-    pub fn is_in_subtree_of(&self, parent: u64) -> bool {
-        if self.slot() == parent {
-            return true;
-        }
-        let mut next_parent = self.parent();
-
-        while let Some(p) = next_parent {
-            if p.slot() == parent {
-                return true;
-            } else if p.slot() < parent {
-                return false;
-            }
-            next_parent = p.parent();
-        }
-
-        false
-    }
 }
 
 impl Drop for Bank {
@@ -1841,30 +1823,6 @@ mod tests {
         assert_eq!(bank.is_votable(), true);
     }
 
-    #[test]
-    fn test_is_in_subtree_of() {
-        let (genesis_block, _) = GenesisBlock::new(1);
-        let parent = Arc::new(Bank::new(&genesis_block));
-        // Bank 1
-        let bank = Arc::new(new_from_parent(&parent));
-        // Bank 2
-        let bank2 = new_from_parent(&bank);
-        // Bank 5
-        let bank5 = Bank::new_from_parent(&bank, &Pubkey::default(), 5);
-
-        // Parents of bank 2: 0 -> 1 -> 2
-        assert!(bank2.is_in_subtree_of(0));
-        assert!(bank2.is_in_subtree_of(1));
-        assert!(bank2.is_in_subtree_of(2));
-        assert!(!bank2.is_in_subtree_of(3));
-
-        // Parents of bank 5: 0 -> 1 -> 5
-        assert!(bank5.is_in_subtree_of(0));
-        assert!(bank5.is_in_subtree_of(1));
-        assert!(!bank5.is_in_subtree_of(2));
-        assert!(!bank5.is_in_subtree_of(4));
-    }
-
     #[test]
     fn test_bank_inherit_tx_count() {
         let (genesis_block, mint_keypair) = GenesisBlock::new(500);