Ensure highest_confirmed_root only grows (#11596)

* Split out commitment-cache update for unit testing

* Add failing test

* Ensure highest_confirmed_root only grows

core/src/commitment_service.rs

@@ -8,6 +8,7 @@ use solana_runtime::{
 use solana_sdk::clock::Slot;
 use solana_vote_program::vote_state::VoteState;
 use std::{
+    cmp::max,
     collections::HashMap,
     sync::atomic::{AtomicBool, Ordering},
     sync::mpsc::{channel, Receiver, RecvTimeoutError, Sender},
@@ -103,28 +104,8 @@ impl AggregateCommitmentService {
             }
 
             let mut aggregate_commitment_time = Measure::start("aggregate-commitment-ms");
-            let (block_commitment, rooted_stake) =
-                Self::aggregate_commitment(&ancestors, &aggregation_data.bank);
-
-            let highest_confirmed_root =
-                get_highest_confirmed_root(rooted_stake, aggregation_data.total_stake);
-
-            let mut new_block_commitment = BlockCommitmentCache::new(
-                block_commitment,
-                aggregation_data.total_stake,
-                CommitmentSlots {
-                    slot: aggregation_data.bank.slot(),
-                    root: aggregation_data.root,
-                    highest_confirmed_slot: aggregation_data.root,
-                    highest_confirmed_root,
-                },
-            );
-            let highest_confirmed_slot = new_block_commitment.calculate_highest_confirmed_slot();
-            new_block_commitment.set_highest_confirmed_slot(highest_confirmed_slot);
-
-            let mut w_block_commitment_cache = block_commitment_cache.write().unwrap();
-
-            std::mem::swap(&mut *w_block_commitment_cache, &mut new_block_commitment);
+            let update_commitment_slots =
+                Self::update_commitment_cache(block_commitment_cache, aggregation_data, ancestors);
             aggregate_commitment_time.stop();
             datapoint_info!(
                 "block-commitment-cache",
@@ -138,10 +119,46 @@ impl AggregateCommitmentService {
             // Triggers rpc_subscription notifications as soon as new commitment data is available,
             // sending just the commitment cache slot information that the notifications thread
             // needs
-            subscriptions.notify_subscribers(w_block_commitment_cache.commitment_slots());
+            subscriptions.notify_subscribers(update_commitment_slots);
         }
     }
 
+    fn update_commitment_cache(
+        block_commitment_cache: &RwLock<BlockCommitmentCache>,
+        aggregation_data: CommitmentAggregationData,
+        ancestors: Vec<u64>,
+    ) -> CommitmentSlots {
+        let (block_commitment, rooted_stake) =
+            Self::aggregate_commitment(&ancestors, &aggregation_data.bank);
+
+        let highest_confirmed_root =
+            get_highest_confirmed_root(rooted_stake, aggregation_data.total_stake);
+
+        let mut new_block_commitment = BlockCommitmentCache::new(
+            block_commitment,
+            aggregation_data.total_stake,
+            CommitmentSlots {
+                slot: aggregation_data.bank.slot(),
+                root: aggregation_data.root,
+                highest_confirmed_slot: aggregation_data.root,
+                highest_confirmed_root,
+            },
+        );
+        let highest_confirmed_slot = new_block_commitment.calculate_highest_confirmed_slot();
+        new_block_commitment.set_highest_confirmed_slot(highest_confirmed_slot);
+
+        let mut w_block_commitment_cache = block_commitment_cache.write().unwrap();
+
+        let highest_confirmed_root = max(
+            new_block_commitment.highest_confirmed_root(),
+            w_block_commitment_cache.highest_confirmed_root(),
+        );
+        new_block_commitment.set_highest_confirmed_root(highest_confirmed_root);
+
+        std::mem::swap(&mut *w_block_commitment_cache, &mut new_block_commitment);
+        w_block_commitment_cache.commitment_slots()
+    }
+
     pub fn aggregate_commitment(
         ancestors: &[Slot],
         bank: &Bank,
@@ -225,9 +242,19 @@ impl AggregateCommitmentService {
 mod tests {
     use super::*;
     use solana_ledger::genesis_utils::{create_genesis_config, GenesisConfigInfo};
-    use solana_sdk::pubkey::Pubkey;
+    use solana_runtime::{
+        bank_forks::BankForks,
+        genesis_utils::{create_genesis_config_with_vote_accounts, ValidatorVoteKeypairs},
+    };
+    use solana_sdk::{
+        pubkey::Pubkey,
+        signature::{Keypair, Signer},
+    };
     use solana_stake_program::stake_state;
-    use solana_vote_program::vote_state::{self, VoteStateVersions};
+    use solana_vote_program::{
+        vote_state::{self, VoteStateVersions},
+        vote_transaction,
+    };
 
     #[test]
     fn test_get_highest_confirmed_root() {
@@ -450,4 +477,157 @@ mod tests {
         assert_eq!(rooted_stake.len(), 2);
         assert_eq!(get_highest_confirmed_root(rooted_stake, 100), 1)
     }
+
+    #[test]
+    fn test_highest_confirmed_root_advance() {
+        fn get_vote_account_root_slot(vote_pubkey: Pubkey, bank: &Arc<Bank>) -> Slot {
+            let account = &bank.vote_accounts()[&vote_pubkey].1;
+            let vote_state = VoteState::from(account).unwrap();
+            vote_state.root_slot.unwrap()
+        }
+
+        let block_commitment_cache = RwLock::new(BlockCommitmentCache::new_for_tests());
+
+        let node_keypair = Arc::new(Keypair::new());
+        let vote_keypair = Arc::new(Keypair::new());
+        let stake_keypair = Arc::new(Keypair::new());
+        let validator_keypairs = vec![ValidatorVoteKeypairs {
+            node_keypair: node_keypair.clone(),
+            vote_keypair: vote_keypair.clone(),
+            stake_keypair,
+        }];
+        let GenesisConfigInfo {
+            genesis_config,
+            mint_keypair: _,
+            voting_keypair: _,
+        } = create_genesis_config_with_vote_accounts(
+            1_000_000_000,
+            &validator_keypairs,
+            vec![100; 1],
+        );
+
+        let bank0 = Bank::new(&genesis_config);
+        let mut bank_forks = BankForks::new(bank0);
+
+        // Fill bank_forks with banks with votes landing in the next slot
+        // Create enough banks such that vote account will root slots 0 and 1
+        for x in 0..33 {
+            let previous_bank = bank_forks.get(x).unwrap();
+            let bank = Bank::new_from_parent(previous_bank, &Pubkey::default(), x + 1);
+            let vote = vote_transaction::new_vote_transaction(
+                vec![x],
+                previous_bank.hash(),
+                previous_bank.last_blockhash(),
+                &node_keypair,
+                &vote_keypair,
+                &vote_keypair,
+                None,
+            );
+            bank.process_transaction(&vote).unwrap();
+            bank_forks.insert(bank);
+        }
+
+        let working_bank = bank_forks.working_bank();
+        let root = get_vote_account_root_slot(vote_keypair.pubkey(), &working_bank);
+        for x in 0..root {
+            bank_forks.set_root(x, &None, None);
+        }
+
+        // Add an additional bank/vote that will root slot 2
+        let bank33 = bank_forks.get(33).unwrap();
+        let bank34 = Bank::new_from_parent(bank33, &Pubkey::default(), 34);
+        let vote33 = vote_transaction::new_vote_transaction(
+            vec![33],
+            bank33.hash(),
+            bank33.last_blockhash(),
+            &node_keypair,
+            &vote_keypair,
+            &vote_keypair,
+            None,
+        );
+        bank34.process_transaction(&vote33).unwrap();
+        bank_forks.insert(bank34);
+
+        let working_bank = bank_forks.working_bank();
+        let root = get_vote_account_root_slot(vote_keypair.pubkey(), &working_bank);
+        let ancestors = working_bank.status_cache_ancestors();
+        let _ = AggregateCommitmentService::update_commitment_cache(
+            &block_commitment_cache,
+            CommitmentAggregationData {
+                bank: working_bank,
+                root: 0,
+                total_stake: 100,
+            },
+            ancestors,
+        );
+        let highest_confirmed_root = block_commitment_cache
+            .read()
+            .unwrap()
+            .highest_confirmed_root();
+        bank_forks.set_root(root, &None, Some(highest_confirmed_root));
+        let highest_confirmed_root_bank = bank_forks.get(highest_confirmed_root);
+        assert!(highest_confirmed_root_bank.is_some());
+
+        // Add a forked bank. Because the vote for bank 33 landed in the non-ancestor, the vote
+        // account's root (and thus the highest_confirmed_root) rolls back to slot 1
+        let bank33 = bank_forks.get(33).unwrap();
+        let bank35 = Bank::new_from_parent(bank33, &Pubkey::default(), 35);
+        bank_forks.insert(bank35);
+
+        let working_bank = bank_forks.working_bank();
+        let ancestors = working_bank.status_cache_ancestors();
+        let _ = AggregateCommitmentService::update_commitment_cache(
+            &block_commitment_cache,
+            CommitmentAggregationData {
+                bank: working_bank,
+                root: 1,
+                total_stake: 100,
+            },
+            ancestors,
+        );
+        let highest_confirmed_root = block_commitment_cache
+            .read()
+            .unwrap()
+            .highest_confirmed_root();
+        let highest_confirmed_root_bank = bank_forks.get(highest_confirmed_root);
+        assert!(highest_confirmed_root_bank.is_some());
+
+        // Add additional banks beyond lockout built on the new fork to ensure that behavior
+        // continues normally
+        for x in 35..=37 {
+            let previous_bank = bank_forks.get(x).unwrap();
+            let bank = Bank::new_from_parent(previous_bank, &Pubkey::default(), x + 1);
+            let vote = vote_transaction::new_vote_transaction(
+                vec![x],
+                previous_bank.hash(),
+                previous_bank.last_blockhash(),
+                &node_keypair,
+                &vote_keypair,
+                &vote_keypair,
+                None,
+            );
+            bank.process_transaction(&vote).unwrap();
+            bank_forks.insert(bank);
+        }
+
+        let working_bank = bank_forks.working_bank();
+        let root = get_vote_account_root_slot(vote_keypair.pubkey(), &working_bank);
+        let ancestors = working_bank.status_cache_ancestors();
+        let _ = AggregateCommitmentService::update_commitment_cache(
+            &block_commitment_cache,
+            CommitmentAggregationData {
+                bank: working_bank,
+                root: 0,
+                total_stake: 100,
+            },
+            ancestors,
+        );
+        let highest_confirmed_root = block_commitment_cache
+            .read()
+            .unwrap()
+            .highest_confirmed_root();
+        bank_forks.set_root(root, &None, Some(highest_confirmed_root));
+        let highest_confirmed_root_bank = bank_forks.get(highest_confirmed_root);
+        assert!(highest_confirmed_root_bank.is_some());
+    }
 }