Refactor: Rename BlockhashQueue fields and methods for clarity (#24426)

core/src/replay_stage.rs

@@ -1970,8 +1970,7 @@ impl ReplayStage {
         }
         if my_latest_landed_vote >= last_voted_slot
             || heaviest_bank_on_same_fork
-                .check_hash_age(&tower.last_vote_tx_blockhash(), MAX_PROCESSING_AGE)
+                .is_hash_valid_for_age(&tower.last_vote_tx_blockhash(), MAX_PROCESSING_AGE)
-                .unwrap_or(false)
             // In order to avoid voting on multiple forks all past MAX_PROCESSING_AGE that don't
             // include the last voted blockhash
             || last_vote_refresh_time.last_refresh_time.elapsed().as_millis() < MAX_VOTE_REFRESH_INTERVAL_MILLIS as u128

poh/src/poh_recorder.rs

@@ -1967,7 +1967,7 @@ mod tests {
                 poh_recorder.tick();
             }
             poh_recorder.set_bank(&bank);
-            assert_eq!(Some(false), bank.check_hash_age(&genesis_hash, 1));
+            assert!(!bank.is_hash_valid_for_age(&genesis_hash, 1));
         }
     }
 

runtime/benches/bank.rs

@@ -203,10 +203,7 @@ fn bench_bank_update_recent_blockhashes(bencher: &mut Bencher) {
         goto_end_of_slot(Arc::get_mut(&mut bank).unwrap());
     }
     // Verify blockhash_queue is full (genesis hash has been kicked out)
-    assert_eq!(
+    assert!(!bank.is_hash_valid_for_age(&genesis_hash, MAX_RECENT_BLOCKHASHES));
-        Some(false),
-        bank.check_hash_age(&genesis_hash, MAX_RECENT_BLOCKHASHES)
-    );
     bencher.iter(|| {
         bank.update_recent_blockhashes();
     });

runtime/src/bank.rs

@@ -3332,7 +3332,7 @@ impl Bank {
 
     pub fn is_blockhash_valid(&self, hash: &Hash) -> bool {
         let blockhash_queue = self.blockhash_queue.read().unwrap();
-        blockhash_queue.check_hash(hash)
+        blockhash_queue.is_hash_valid(hash)
     }
 
     pub fn get_minimum_balance_for_rent_exemption(&self, data_len: usize) -> u64 {
@@ -3680,14 +3680,10 @@ impl Bank {
             .map(|(tx, lock_res)| match lock_res {
                 Ok(()) => {
                     let recent_blockhash = tx.message().recent_blockhash();
-                    let hash_age = hash_queue.check_hash_age(recent_blockhash, max_age);
+                    if hash_queue.is_hash_valid_for_age(recent_blockhash, max_age) {
-                    if hash_age == Some(true) {
                         (Ok(()), None)
                     } else if let Some((address, account)) = self.check_transaction_for_nonce(tx) {
                         (Ok(()), Some(NoncePartial::new(address, account)))
-                    } else if hash_age == Some(false) {
-                        error_counters.blockhash_too_old += 1;
-                        (Err(TransactionError::BlockhashNotFound), None)
                     } else {
                         error_counters.blockhash_not_found += 1;
                         (Err(TransactionError::BlockhashNotFound), None)
@@ -3737,11 +3733,11 @@ impl Bank {
         self.blockhash_queue.read().unwrap().get_hash_age(hash)
     }
 
-    pub fn check_hash_age(&self, hash: &Hash, max_age: usize) -> Option<bool> {
+    pub fn is_hash_valid_for_age(&self, hash: &Hash, max_age: usize) -> bool {
         self.blockhash_queue
             .read()
             .unwrap()
-            .check_hash_age(hash, max_age)
+            .is_hash_valid_for_age(hash, max_age)
     }
 
     fn check_message_for_nonce(&self, message: &SanitizedMessage) -> Option<TransactionAccount> {
@@ -11592,7 +11588,7 @@ pub(crate) mod tests {
             assert_eq!(recent_blockhashes.len(), i);
             let most_recent_hash = recent_blockhashes.iter().next().unwrap().blockhash;
             // Check order
-            assert_eq!(Some(true), bank.check_hash_age(&most_recent_hash, 0));
+            assert!(bank.is_hash_valid_for_age(&most_recent_hash, 0));
             goto_end_of_slot(Arc::get_mut(&mut bank).unwrap());
             bank = Arc::new(new_from_parent(&bank));
         }

runtime/src/blockhash_queue.rs

@@ -11,16 +11,16 @@ use {
 #[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, AbiExample)]
 struct HashAge {
     fee_calculator: FeeCalculator,
-    hash_height: u64,
+    hash_index: u64,
     timestamp: u64,
 }
 
 /// Low memory overhead, so can be cloned for every checkpoint
-#[frozen_abi(digest = "J1fGiMHyiKEBcWE6mfm7grAEGJgYEaVLzcrNZvd37iA2")]
+#[frozen_abi(digest = "J66ssCYGtWdQu5oyJxFKFeZY86nUjThBdBeXQYuRPDvE")]
 #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, AbiExample)]
 pub struct BlockhashQueue {
-    /// updated whenever an hash is registered
+    /// index of last hash to be registered
-    hash_height: u64,
+    last_hash_index: u64,
 
     /// last hash to be registered
     last_hash: Option<Hash>,
@@ -41,17 +41,12 @@ impl BlockhashQueue {
     pub fn new(max_age: usize) -> Self {
         Self {
             ages: HashMap::new(),
-            hash_height: 0,
+            last_hash_index: 0,
             last_hash: None,
             max_age,
         }
     }
 
-    #[allow(dead_code)]
-    pub fn hash_height(&self) -> u64 {
-        self.hash_height
-    }
-
     pub fn last_hash(&self) -> Hash {
         self.last_hash.expect("no hash has been set")
     }
@@ -62,24 +57,23 @@ impl BlockhashQueue {
             .map(|hash_age| hash_age.fee_calculator.lamports_per_signature)
     }
 
-    /// Check if the age of the hash is within the max_age
+    /// Check if the age of the hash is within the queue's max age
-    /// return false for any hashes with an age above max_age
+    pub fn is_hash_valid(&self, hash: &Hash) -> bool {
-    /// return None for any hashes that were not found
+        self.ages.get(hash).is_some()
-    pub fn check_hash_age(&self, hash: &Hash, max_age: usize) -> Option<bool> {
+    }
+
+    /// Check if the age of the hash is within the specified age
+    pub fn is_hash_valid_for_age(&self, hash: &Hash, max_age: usize) -> bool {
         self.ages
             .get(hash)
-            .map(|age| self.hash_height - age.hash_height <= max_age as u64)
+            .map(|age| Self::is_hash_index_valid(self.last_hash_index, max_age, age.hash_index))
+            .unwrap_or(false)
     }
 
     pub fn get_hash_age(&self, hash: &Hash) -> Option<u64> {
         self.ages
             .get(hash)
-            .map(|age| self.hash_height - age.hash_height)
+            .map(|age| self.last_hash_index - age.hash_index)
-    }
-
-    /// check if hash is valid
-    pub fn check_hash(&self, hash: &Hash) -> bool {
-        self.ages.get(hash).is_some()
     }
 
     pub fn genesis_hash(&mut self, hash: &Hash, lamports_per_signature: u64) {
@@ -87,7 +81,7 @@ impl BlockhashQueue {
             *hash,
             HashAge {
                 fee_calculator: FeeCalculator::new(lamports_per_signature),
-                hash_height: 0,
+                hash_index: 0,
                 timestamp: timestamp(),
             },
         );
@@ -95,26 +89,23 @@ impl BlockhashQueue {
         self.last_hash = Some(*hash);
     }
 
-    fn check_age(hash_height: u64, max_age: usize, age: &HashAge) -> bool {
+    fn is_hash_index_valid(last_hash_index: u64, max_age: usize, hash_index: u64) -> bool {
-        hash_height - age.hash_height <= max_age as u64
+        last_hash_index - hash_index <= max_age as u64
     }
 
     pub fn register_hash(&mut self, hash: &Hash, lamports_per_signature: u64) {
-        self.hash_height += 1;
+        self.last_hash_index += 1;
-        let hash_height = self.hash_height;
+        if self.ages.len() >= self.max_age {
-
+            self.ages.retain(|_, age| {
-        // this clean up can be deferred until sigs gets larger
+                Self::is_hash_index_valid(self.last_hash_index, self.max_age, age.hash_index)
-        //  because we verify age.nth every place we check for validity
+            });
-        let max_age = self.max_age;
-        if self.ages.len() >= max_age {
-            self.ages
-                .retain(|_, age| Self::check_age(hash_height, max_age, age));
         }
+
         self.ages.insert(
             *hash,
             HashAge {
                 fee_calculator: FeeCalculator::new(lamports_per_signature),
-                hash_height,
+                hash_index: self.last_hash_index,
                 timestamp: timestamp(),
             },
         );
@@ -122,16 +113,6 @@ impl BlockhashQueue {
         self.last_hash = Some(*hash);
     }
 
-    /// Maps a hash height to a timestamp
-    pub fn hash_height_to_timestamp(&self, hash_height: u64) -> Option<u64> {
-        for age in self.ages.values() {
-            if age.hash_height == hash_height {
-                return Some(age.timestamp);
-            }
-        }
-        None
-    }
-
     #[deprecated(
         since = "1.9.0",
         note = "Please do not use, will no longer be available in the future"
@@ -139,7 +120,7 @@ impl BlockhashQueue {
     #[allow(deprecated)]
     pub fn get_recent_blockhashes(&self) -> impl Iterator<Item = recent_blockhashes::IterItem> {
         (self.ages).iter().map(|(k, v)| {
-            recent_blockhashes::IterItem(v.hash_height, k, v.fee_calculator.lamports_per_signature)
+            recent_blockhashes::IterItem(v.hash_index, k, v.fee_calculator.lamports_per_signature)
         })
     }
 
@@ -161,10 +142,10 @@ mod tests {
     fn test_register_hash() {
         let last_hash = Hash::default();
         let mut hash_queue = BlockhashQueue::new(100);
-        assert!(!hash_queue.check_hash(&last_hash));
+        assert!(!hash_queue.is_hash_valid(&last_hash));
         hash_queue.register_hash(&last_hash, 0);
-        assert!(hash_queue.check_hash(&last_hash));
+        assert!(hash_queue.is_hash_valid(&last_hash));
-        assert_eq!(hash_queue.hash_height(), 1);
+        assert_eq!(hash_queue.last_hash_index, 1);
     }
 
     #[test]
@@ -176,13 +157,13 @@ mod tests {
             hash_queue.register_hash(&last_hash, 0);
         }
         // Assert we're no longer able to use the oldest hash.
-        assert!(!hash_queue.check_hash(&last_hash));
+        assert!(!hash_queue.is_hash_valid(&last_hash));
-        assert_eq!(None, hash_queue.check_hash_age(&last_hash, 0));
+        assert!(!hash_queue.is_hash_valid_for_age(&last_hash, 0));
 
         // Assert we are not able to use the oldest remaining hash.
         let last_valid_hash = hash(&serialize(&1).unwrap());
-        assert!(hash_queue.check_hash(&last_valid_hash));
+        assert!(hash_queue.is_hash_valid(&last_valid_hash));
-        assert_eq!(Some(false), hash_queue.check_hash_age(&last_valid_hash, 0));
+        assert!(!hash_queue.is_hash_valid_for_age(&last_valid_hash, 0));
     }
 
     /// test that when max age is 0, that a valid last_hash still passes the age check
@@ -192,7 +173,7 @@ mod tests {
         let mut hash_queue = BlockhashQueue::new(100);
         hash_queue.register_hash(&last_hash, 0);
         assert_eq!(last_hash, hash_queue.last_hash());
-        assert_eq!(Some(true), hash_queue.check_hash_age(&last_hash, 0));
+        assert!(hash_queue.is_hash_valid_for_age(&last_hash, 0));
     }
 
     #[test]
@@ -211,10 +192,7 @@ mod tests {
         // Verify that the returned hashes are most recent
         #[allow(deprecated)]
         for IterItem(_slot, hash, _lamports_per_signature) in recent_blockhashes {
-            assert_eq!(
+            assert!(blockhash_queue.is_hash_valid_for_age(hash, MAX_RECENT_BLOCKHASHES));
-                Some(true),
-                blockhash_queue.check_hash_age(hash, MAX_RECENT_BLOCKHASHES)
-            );
         }
     }
 
@@ -268,14 +246,14 @@ mod tests {
     }
 
     #[test]
-    fn test_check_hash_age() {
+    fn test_is_hash_valid_for_age() {
         const MAX_AGE: usize = 10;
         let mut hash_list: Vec<Hash> = Vec::new();
         hash_list.resize_with(MAX_AGE + 1, Hash::new_unique);
 
         let mut hash_queue = BlockhashQueue::new(MAX_AGE);
         for hash in &hash_list {
-            assert!(hash_queue.check_hash_age(hash, MAX_AGE).is_none());
+            assert!(!hash_queue.is_hash_valid_for_age(hash, MAX_AGE));
         }
 
         for hash in &hash_list {
@@ -286,19 +264,11 @@ mod tests {
         // the age of a hash is within max age, the hash from 11 slots ago is considered
         // to be within the max age of 10.
         for hash in &hash_list {
-            assert_eq!(hash_queue.check_hash_age(hash, MAX_AGE), Some(true));
+            assert!(hash_queue.is_hash_valid_for_age(hash, MAX_AGE));
         }
 
-        // When max age is 0, the most recent blockhash is still considered valid
+        // When max age is 0, only the most recent blockhash is still considered valid
-        assert_eq!(
+        assert!(hash_queue.is_hash_valid_for_age(&hash_list[MAX_AGE], 0));
-            hash_queue.check_hash_age(&hash_list[MAX_AGE], 0),
+        assert!(!hash_queue.is_hash_valid_for_age(&hash_list[MAX_AGE - 1], 0));
-            Some(true)
-        );
-
-        // Just because a hash is in the queue, doesn't mean it's valid for max age
-        assert_eq!(
-            hash_queue.check_hash_age(&hash_list[0], MAX_AGE - 1),
-            Some(false)
-        );
     }
 }

runtime/src/serde_snapshot/tests.rs

@@ -367,7 +367,7 @@ mod test_bank_serialize {
 
     // This some what long test harness is required to freeze the ABI of
     // Bank's serialization due to versioned nature
-    #[frozen_abi(digest = "H2XtVdhokwLMTbjXh4Lh3Mw8m7PYQDMh4Ha5ojuxip9Z")]
+    #[frozen_abi(digest = "DnT7iwf29YvGxLTt2iLYPmixwP13LXvusVVKpXGgizhc")]
     #[derive(Serialize, AbiExample)]
     pub struct BankAbiTestWrapperNewer {
         #[serde(serialize_with = "wrapper_newer")]