Only add hashes for completed blocks to recent blockhashes (#24389)

* Only add hashes for completed blocks to recent blockhashes

* feedback

core/src/replay_stage.rs

@@ -5879,7 +5879,7 @@ pub mod tests {
 
         // Simulate landing a vote for slot 0 landing in slot 1
         let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1));
-        bank1.fill_bank_with_ticks();
+        bank1.fill_bank_with_ticks_for_tests();
         tower.record_bank_vote(&bank0, &my_vote_pubkey);
         ReplayStage::push_vote(
             &bank0,
@@ -5919,7 +5919,7 @@ pub mod tests {
         // Trying to refresh the vote for bank 0 in bank 1 or bank 2 won't succeed because
         // the last vote has landed already
         let bank2 = Arc::new(Bank::new_from_parent(&bank1, &Pubkey::default(), 2));
-        bank2.fill_bank_with_ticks();
+        bank2.fill_bank_with_ticks_for_tests();
         bank2.freeze();
         for refresh_bank in &[&bank1, &bank2] {
             ReplayStage::refresh_last_vote(
@@ -6001,13 +6001,19 @@ pub mod tests {
         assert_eq!(tower.last_voted_slot().unwrap(), 1);
 
         // Create a bank where the last vote transaction will have expired
-        let expired_bank = Arc::new(Bank::new_from_parent(
-            &bank2,
+        let expired_bank = {
+            let mut parent_bank = bank2.clone();
+            for _ in 0..MAX_PROCESSING_AGE {
+                parent_bank = Arc::new(Bank::new_from_parent(
+                    &parent_bank,
                     &Pubkey::default(),
-            bank2.slot() + MAX_PROCESSING_AGE as Slot,
+                    parent_bank.slot() + 1,
                 ));
-        expired_bank.fill_bank_with_ticks();
-        expired_bank.freeze();
+                parent_bank.fill_bank_with_ticks_for_tests();
+                parent_bank.freeze();
+            }
+            parent_bank
+        };
 
         // Now trying to refresh the vote for slot 1 will succeed because the recent blockhash
         // of the last vote transaction has expired
@@ -6070,7 +6076,7 @@ pub mod tests {
             vote_account.vote_state().as_ref().unwrap().tower(),
             vec![0, 1]
         );
-        expired_bank_child.fill_bank_with_ticks();
+        expired_bank_child.fill_bank_with_ticks_for_tests();
         expired_bank_child.freeze();
 
         // Trying to refresh the vote on a sibling bank where:
@@ -6082,7 +6088,7 @@ pub mod tests {
             &Pubkey::default(),
             expired_bank_child.slot() + 1,
         ));
-        expired_bank_sibling.fill_bank_with_ticks();
+        expired_bank_sibling.fill_bank_with_ticks_for_tests();
         expired_bank_sibling.freeze();
         // Set the last refresh to now, shouldn't refresh because the last refresh just happened.
         last_vote_refresh_time.last_refresh_time = Instant::now();

ledger/src/blockstore_processor.rs

@@ -3951,7 +3951,7 @@ pub mod tests {
     }
 
     #[test]
-    fn test_confirm_slot_entries() {
+    fn test_confirm_slot_entries_without_fix() {
         const HASHES_PER_TICK: u64 = 10;
         const TICKS_PER_SLOT: u64 = 2;
 
@@ -3966,7 +3966,9 @@ pub mod tests {
         genesis_config.ticks_per_slot = TICKS_PER_SLOT;
         let genesis_hash = genesis_config.hash();
 
-        let slot_0_bank = Arc::new(Bank::new_for_tests(&genesis_config));
+        let mut slot_0_bank = Bank::new_for_tests(&genesis_config);
+        slot_0_bank.deactivate_feature(&feature_set::fix_recent_blockhashes::id());
+        let slot_0_bank = Arc::new(slot_0_bank);
         assert_eq!(slot_0_bank.slot(), 0);
         assert_eq!(slot_0_bank.tick_height(), 0);
         assert_eq!(slot_0_bank.max_tick_height(), 2);
@@ -4033,4 +4035,124 @@ pub mod tests {
         assert_eq!(slot_2_bank.get_hash_age(&slot_1_hash), Some(1));
         assert_eq!(slot_2_bank.get_hash_age(&slot_2_hash), Some(0));
     }
+
+    #[test]
+    fn test_confirm_slot_entries_with_fix() {
+        const HASHES_PER_TICK: u64 = 10;
+        const TICKS_PER_SLOT: u64 = 2;
+
+        let collector_id = Pubkey::new_unique();
+
+        let GenesisConfigInfo {
+            mut genesis_config,
+            mint_keypair,
+            ..
+        } = create_genesis_config(10_000);
+        genesis_config.poh_config.hashes_per_tick = Some(HASHES_PER_TICK);
+        genesis_config.ticks_per_slot = TICKS_PER_SLOT;
+        let genesis_hash = genesis_config.hash();
+
+        let slot_0_bank = Arc::new(Bank::new_for_tests(&genesis_config));
+        assert_eq!(slot_0_bank.slot(), 0);
+        assert_eq!(slot_0_bank.tick_height(), 0);
+        assert_eq!(slot_0_bank.max_tick_height(), 2);
+        assert_eq!(slot_0_bank.last_blockhash(), genesis_hash);
+        assert_eq!(slot_0_bank.get_hash_age(&genesis_hash), Some(0));
+
+        let slot_0_entries = entry::create_ticks(TICKS_PER_SLOT, HASHES_PER_TICK, genesis_hash);
+        let slot_0_hash = slot_0_entries.last().unwrap().hash;
+        confirm_slot_entries_for_tests(&slot_0_bank, slot_0_entries, true, genesis_hash).unwrap();
+        assert_eq!(slot_0_bank.tick_height(), slot_0_bank.max_tick_height());
+        assert_eq!(slot_0_bank.last_blockhash(), slot_0_hash);
+        assert_eq!(slot_0_bank.get_hash_age(&genesis_hash), Some(1));
+        assert_eq!(slot_0_bank.get_hash_age(&slot_0_hash), Some(0));
+
+        let slot_2_bank = Arc::new(Bank::new_from_parent(&slot_0_bank, &collector_id, 2));
+        assert_eq!(slot_2_bank.slot(), 2);
+        assert_eq!(slot_2_bank.tick_height(), 2);
+        assert_eq!(slot_2_bank.max_tick_height(), 6);
+        assert_eq!(slot_2_bank.last_blockhash(), slot_0_hash);
+
+        let slot_1_entries = entry::create_ticks(TICKS_PER_SLOT, HASHES_PER_TICK, slot_0_hash);
+        let slot_1_hash = slot_1_entries.last().unwrap().hash;
+        confirm_slot_entries_for_tests(&slot_2_bank, slot_1_entries, false, slot_0_hash).unwrap();
+        assert_eq!(slot_2_bank.tick_height(), 4);
+        assert_eq!(slot_2_bank.last_blockhash(), slot_0_hash);
+        assert_eq!(slot_2_bank.get_hash_age(&genesis_hash), Some(1));
+        assert_eq!(slot_2_bank.get_hash_age(&slot_0_hash), Some(0));
+
+        struct TestCase {
+            recent_blockhash: Hash,
+            expected_result: result::Result<(), BlockstoreProcessorError>,
+        }
+
+        let test_cases = [
+            TestCase {
+                recent_blockhash: slot_1_hash,
+                expected_result: Err(BlockstoreProcessorError::InvalidTransaction(
+                    TransactionError::BlockhashNotFound,
+                )),
+            },
+            TestCase {
+                recent_blockhash: slot_0_hash,
+                expected_result: Ok(()),
+            },
+        ];
+
+        // Check that slot 2 transactions can only use hashes for completed blocks.
+        for TestCase {
+            recent_blockhash,
+            expected_result,
+        } in test_cases
+        {
+            let slot_2_entries = {
+                let to_pubkey = Pubkey::new_unique();
+                let mut prev_entry_hash = slot_1_hash;
+                let mut remaining_entry_hashes = HASHES_PER_TICK;
+
+                let tx =
+                    system_transaction::transfer(&mint_keypair, &to_pubkey, 1, recent_blockhash);
+                remaining_entry_hashes = remaining_entry_hashes.checked_sub(1).unwrap();
+                let mut entries = vec![next_entry_mut(&mut prev_entry_hash, 1, vec![tx])];
+
+                entries.push(next_entry_mut(
+                    &mut prev_entry_hash,
+                    remaining_entry_hashes,
+                    vec![],
+                ));
+                entries.push(next_entry_mut(
+                    &mut prev_entry_hash,
+                    HASHES_PER_TICK,
+                    vec![],
+                ));
+
+                entries
+            };
+
+            let slot_2_hash = slot_2_entries.last().unwrap().hash;
+            let result =
+                confirm_slot_entries_for_tests(&slot_2_bank, slot_2_entries, true, slot_1_hash);
+            match (result, expected_result) {
+                (Ok(()), Ok(())) => {
+                    assert_eq!(slot_2_bank.tick_height(), slot_2_bank.max_tick_height());
+                    assert_eq!(slot_2_bank.last_blockhash(), slot_2_hash);
+                    assert_eq!(slot_2_bank.get_hash_age(&genesis_hash), Some(2));
+                    assert_eq!(slot_2_bank.get_hash_age(&slot_0_hash), Some(1));
+                    assert_eq!(slot_2_bank.get_hash_age(&slot_2_hash), Some(0));
+                }
+                (
+                    Err(BlockstoreProcessorError::InvalidTransaction(err)),
+                    Err(BlockstoreProcessorError::InvalidTransaction(expected_err)),
+                ) => {
+                    assert_eq!(err, expected_err);
+                }
+                (result, expected_result) => {
+                    panic!(
+                        "actual result {:?} != expected result {:?}",
+                        result, expected_result
+                    );
+                }
+            }
+        }
+    }
 }

poh/src/poh_recorder.rs

@@ -1127,7 +1127,7 @@ mod tests {
                 Arc::new(AtomicBool::default()),
             );
 
-            bank0.fill_bank_with_ticks();
+            bank0.fill_bank_with_ticks_for_tests();
             let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1));
 
             // Set a working bank
@@ -1239,7 +1239,7 @@ mod tests {
                 Arc::new(AtomicBool::default()),
             );
 
-            bank0.fill_bank_with_ticks();
+            bank0.fill_bank_with_ticks_for_tests();
             let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1));
             poh_recorder.set_bank(&bank1);
             // Let poh_recorder tick up to bank1.tick_height() - 1
@@ -1324,7 +1324,7 @@ mod tests {
                 Arc::new(AtomicBool::default()),
             );
 
-            bank0.fill_bank_with_ticks();
+            bank0.fill_bank_with_ticks_for_tests();
             let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1));
             poh_recorder.set_bank(&bank1);
 
@@ -1420,7 +1420,7 @@ mod tests {
                 Arc::new(AtomicBool::default()),
             );
 
-            bank0.fill_bank_with_ticks();
+            bank0.fill_bank_with_ticks_for_tests();
             let bank1 = Arc::new(Bank::new_from_parent(&bank0, &Pubkey::default(), 1));
             poh_recorder.set_bank(&bank1);
 
@@ -1962,12 +1962,13 @@ mod tests {
             );
             //create a new bank
             let bank = Arc::new(Bank::new_from_parent(&bank, &Pubkey::default(), 2));
-            //put 2 slots worth of virtual ticks into poh
-            for _ in 0..(bank.ticks_per_slot() * 2) {
+            // add virtual ticks into poh for slots 0, 1, and 2
+            for _ in 0..(bank.ticks_per_slot() * 3) {
                 poh_recorder.tick();
             }
             poh_recorder.set_bank(&bank);
-            assert!(!bank.is_hash_valid_for_age(&genesis_hash, 1));
+            assert!(!bank.is_hash_valid_for_age(&genesis_hash, 0));
+            assert!(bank.is_hash_valid_for_age(&genesis_hash, 1));
         }
     }
 

program-test/src/lib.rs

@@ -26,7 +26,7 @@ use {
         clock::Slot,
         entrypoint::{ProgramResult, SUCCESS},
         feature_set::FEATURE_NAMES,
-        fee_calculator::{FeeCalculator, FeeRateGovernor},
+        fee_calculator::{FeeCalculator, FeeRateGovernor, DEFAULT_TARGET_LAMPORTS_PER_SIGNATURE},
         genesis_config::{ClusterType, GenesisConfig},
         hash::Hash,
         instruction::{Instruction, InstructionError},
@@ -421,41 +421,6 @@ pub fn read_file<P: AsRef<Path>>(path: P) -> Vec<u8> {
     file_data
 }
 
-fn setup_fees(bank: Bank) -> Bank {
-    // Realistic fees part 1: Fake a single signature by calling
-    // `bank.commit_transactions()` so that the fee in the child bank will be
-    // initialized with a non-zero fee.
-    assert_eq!(bank.signature_count(), 0);
-    bank.commit_transactions(
-        &[],     // transactions
-        &mut [], // loaded accounts
-        vec![],  // transaction execution results
-        0,       // executed tx count
-        0,       // executed with failure output tx count
-        1,       // signature count
-        &mut ExecuteTimings::default(),
-    );
-    assert_eq!(bank.signature_count(), 1);
-
-    // Advance beyond slot 0 for a slightly more realistic test environment
-    let bank = Arc::new(bank);
-    let bank = Bank::new_from_parent(&bank, bank.collector_id(), bank.slot() + 1);
-    debug!("Bank slot: {}", bank.slot());
-
-    // Realistic fees part 2: Tick until a new blockhash is produced to pick up the
-    // non-zero fees
-    let last_blockhash = bank.last_blockhash();
-    while last_blockhash == bank.last_blockhash() {
-        bank.register_tick(&Hash::new_unique());
-    }
-
-    // Make sure a fee is now required
-    let lamports_per_signature = bank.get_lamports_per_signature();
-    assert_ne!(lamports_per_signature, 0);
-
-    bank
-}
-
 pub struct ProgramTest {
     accounts: Vec<(Pubkey, AccountSharedData)>,
     builtins: Vec<Builtin>,
@@ -748,7 +713,11 @@ impl ProgramTest {
         }
 
         let rent = Rent::default();
-        let fee_rate_governor = FeeRateGovernor::default();
+        let fee_rate_governor = FeeRateGovernor {
+            // Initialize with a non-zero fee
+            lamports_per_signature: DEFAULT_TARGET_LAMPORTS_PER_SIGNATURE / 2,
+            ..FeeRateGovernor::default()
+        };
         let bootstrap_validator_pubkey = Pubkey::new_unique();
         let bootstrap_validator_stake_lamports =
             rent.minimum_balance(VoteState::size_of()) + sol_to_lamports(1_000_000.0);
@@ -837,7 +806,14 @@ impl ProgramTest {
                 ..ComputeBudget::default()
             }));
         }
-        let bank = setup_fees(bank);
+        // Advance beyond slot 0 for a slightly more realistic test environment
+        let bank = {
+            let bank = Arc::new(bank);
+            bank.fill_bank_with_ticks_for_tests();
+            let bank = Bank::new_from_parent(&bank, bank.collector_id(), bank.slot() + 1);
+            debug!("Bank slot: {}", bank.slot());
+            bank
+        };
         let slot = bank.slot();
         let last_blockhash = bank.last_blockhash();
         let bank_forks = Arc::new(RwLock::new(BankForks::new(bank)));
@@ -861,6 +837,7 @@ impl ProgramTest {
     pub async fn start(self) -> (BanksClient, Keypair, Hash) {
         let (bank_forks, block_commitment_cache, last_blockhash, gci) = self.setup_bank();
         let target_tick_duration = gci.genesis_config.poh_config.target_tick_duration;
+        let target_slot_duration = target_tick_duration * gci.genesis_config.ticks_per_slot as u32;
         let transport = start_local_server(
             bank_forks.clone(),
             block_commitment_cache.clone(),
@@ -876,12 +853,12 @@ impl ProgramTest {
         // test
         tokio::spawn(async move {
             loop {
+                tokio::time::sleep(target_slot_duration).await;
                 bank_forks
                     .read()
                     .unwrap()
                     .working_bank()
-                    .register_tick(&Hash::new_unique());
-                tokio::time::sleep(target_tick_duration).await;
+                    .register_recent_blockhash(&Hash::new_unique());
             }
         });
 
@@ -1018,6 +995,8 @@ impl ProgramTestContext {
             .genesis_config
             .poh_config
             .target_tick_duration;
+        let target_slot_duration =
+            target_tick_duration * genesis_config_info.genesis_config.ticks_per_slot as u32;
         let exit = Arc::new(AtomicBool::new(false));
         let bank_task = DroppableTask(
             exit.clone(),
@@ -1026,12 +1005,12 @@ impl ProgramTestContext {
                     if exit.load(Ordering::Relaxed) {
                         break;
                     }
+                    tokio::time::sleep(target_slot_duration).await;
                     running_bank_forks
                         .read()
                         .unwrap()
                         .working_bank()
-                        .register_tick(&Hash::new_unique());
-                    tokio::time::sleep(target_tick_duration).await;
+                        .register_recent_blockhash(&Hash::new_unique());
                 }
             }),
         );
@@ -1102,12 +1081,9 @@ impl ProgramTestContext {
         let mut bank_forks = self.bank_forks.write().unwrap();
         let bank = bank_forks.working_bank();
 
-        // Force ticks until a new blockhash, otherwise retried transactions will have
+        // Fill ticks until a new blockhash is recorded, otherwise retried transactions will have
         // the same signature
-        let last_blockhash = bank.last_blockhash();
-        while last_blockhash == bank.last_blockhash() {
-            bank.register_tick(&Hash::new_unique());
-        }
+        bank.fill_bank_with_ticks_for_tests();
 
         // Ensure that we are actually progressing forward
         let working_slot = bank.slot();

runtime/src/bank.rs

@@ -3463,6 +3463,18 @@ impl Bank {
         }
     }
 
+    /// Register a new recent blockhash in the bank's recent blockhash queue. Called when a bank
+    /// reaches its max tick height. Can be called by tests to get new blockhashes for transaction
+    /// processing without advancing to a new bank slot.
+    pub fn register_recent_blockhash(&self, blockhash: &Hash) {
+        // Only acquire the write lock for the blockhash queue on block boundaries because
+        // readers can starve this write lock acquisition and ticks would be slowed down too
+        // much if the write lock is acquired for each tick.
+        let mut w_blockhash_queue = self.blockhash_queue.write().unwrap();
+        w_blockhash_queue.register_hash(blockhash, self.fee_rate_governor.lamports_per_signature);
+        self.update_recent_blockhashes_locked(&w_blockhash_queue);
+    }
+
     /// Tell the bank which Entry IDs exist on the ledger. This function assumes subsequent calls
     /// correspond to later entries, and will boot the oldest ones once its internal cache is full.
     /// Once boot, the bank will reject transactions using that `hash`.
@@ -3477,12 +3489,7 @@ impl Bank {
 
         inc_new_counter_debug!("bank-register_tick-registered", 1);
         if self.is_block_boundary(self.tick_height.load(Relaxed) + 1) {
-            // Only acquire the write lock for the blockhash queue on block boundaries because
-            // readers can starve this write lock acquisition and ticks would be slowed down too
-            // much if the write lock is acquired for each tick.
-            let mut w_blockhash_queue = self.blockhash_queue.write().unwrap();
-            w_blockhash_queue.register_hash(hash, self.fee_rate_governor.lamports_per_signature);
-            self.update_recent_blockhashes_locked(&w_blockhash_queue);
+            self.register_recent_blockhash(hash);
         }
 
         // ReplayStage will start computing the accounts delta hash when it
@@ -3498,8 +3505,15 @@ impl Bank {
     }
 
     pub fn is_block_boundary(&self, tick_height: u64) -> bool {
+        if self
+            .feature_set
+            .is_active(&feature_set::fix_recent_blockhashes::id())
+        {
+            tick_height == self.max_tick_height
+        } else {
             tick_height % self.ticks_per_slot == 0
         }
+    }
 
     /// Prepare a transaction batch from a list of legacy transactions. Used for tests only.
     pub fn prepare_batch_for_tests(&self, txs: Vec<Transaction>) -> TransactionBatch {
@@ -6567,17 +6581,14 @@ impl Bank {
         self.feature_set = Arc::new(feature_set);
     }
 
-    pub fn fill_bank_with_ticks(&self) {
-        let parent_distance = if self.slot() == 0 {
-            1
-        } else {
-            self.slot() - self.parent_slot()
-        };
-        for _ in 0..parent_distance {
+    pub fn fill_bank_with_ticks_for_tests(&self) {
+        if self.tick_height.load(Relaxed) < self.max_tick_height {
             let last_blockhash = self.last_blockhash();
             while self.last_blockhash() == last_blockhash {
                 self.register_tick(&Hash::new_unique())
             }
+        } else {
+            warn!("Bank already reached max tick height, cannot fill it with more ticks");
         }
     }
 

sdk/program/src/clock.rs

@@ -57,7 +57,7 @@ pub const MAX_HASH_AGE_IN_SECONDS: usize = 120;
 
 #[cfg(test)]
 static_assertions::const_assert_eq!(MAX_RECENT_BLOCKHASHES, 300);
-// Number of maximum recent blockhashes (one blockhash per slot)
+// Number of maximum recent blockhashes (one blockhash per non-skipped slot)
 pub const MAX_RECENT_BLOCKHASHES: usize =
     MAX_HASH_AGE_IN_SECONDS * DEFAULT_TICKS_PER_SECOND as usize / DEFAULT_TICKS_PER_SLOT as usize;
 

sdk/src/feature_set.rs

@@ -355,6 +355,10 @@ pub mod executables_incur_cpi_data_cost {
     solana_sdk::declare_id!("7GUcYgq4tVtaqNCKT3dho9r4665Qp5TxCZ27Qgjx3829");
 }
 
+pub mod fix_recent_blockhashes {
+    solana_sdk::declare_id!("6iyggb5MTcsvdcugX7bEKbHV8c6jdLbpHwkncrgLMhfo");
+}
+
 lazy_static! {
     /// Map of feature identifiers to user-visible description
     pub static ref FEATURE_NAMES: HashMap<Pubkey, &'static str> = [
@@ -438,6 +442,7 @@ lazy_static! {
         (reject_callx_r10::id(), "Reject bpf callx r10 instructions"),
         (drop_redundant_turbine_path::id(), "drop redundant turbine path"),
         (executables_incur_cpi_data_cost::id(), "Executables incure CPI data costs"),
+        (fix_recent_blockhashes::id(), "stop adding hashes for skipped slots to recent blockhashes"),
         /*************** ADD NEW FEATURES HERE ***************/
     ]
     .iter()