Fix lookup_confirmations() to account for stale entries

Also adds HeaderList::best_height() to help avoid off-by-one errors for
the chain length vs tip height (like I initially made when implementing
this >.<), and to make getting the tip height of an empty HeaderList an
explicit error (previously it over overflow and return usize::MAX).

Author: shesek

src/bin/db-migrate-v1-to-v2.rs

@@ -38,6 +38,7 @@ fn main() {
     let history_db = store.history_db();
     let cache_db = store.cache_db();
     let headers = store.headers();
+    let tip_height = headers.best_height() as u32;
 
     // Check the DB version under `V` matches the expected version
     for db in [txstore_db, history_db, cache_db] {
@@ -133,7 +134,7 @@ fn main() {
         }
 
         // Lookup the confirmation status for the entire chunk using a MultiGet operation
-        let confirmations = lookup_confirmations(history_db, spending_txids);
+        let confirmations = lookup_confirmations(history_db, tip_height, spending_txids);
 
         let mut batch = WriteBatch::default();
         for (v1_edge, v1_db_key) in v1_edges {
@@ -186,7 +187,7 @@ fn main() {
             }
 
             // Lookup the confirmation status for the entire chunk using a MultiGet operation
-            let confirmations = lookup_confirmations(history_db, history_txids);
+            let confirmations = lookup_confirmations(history_db, tip_height, history_txids);
 
             let mut batch = WriteBatch::default();
             for (hist, db_key) in history_entries {

src/new_index/schema.rs

@@ -394,7 +394,7 @@ impl Indexer {
             self.from = FetchFrom::Bitcoind;
         }
 
-        self.tip_metric.set(headers.len() as i64 - 1);
+        self.tip_metric.set(headers.best_height() as i64);
 
         Ok(tip)
     }
@@ -959,8 +959,9 @@ impl ChainQuery {
             .map(BlockId::from)
     }
 
+    /// Get the chain tip height. Panics if called on an empty HeaderList.
     pub fn best_height(&self) -> usize {
-        self.store.indexed_headers.read().unwrap().len() - 1
+        self.store.indexed_headers.read().unwrap().best_height()
     }
 
     pub fn best_hash(&self) -> BlockHash {
@@ -1087,7 +1088,9 @@ impl ChainQuery {
     }
 
     pub fn lookup_confirmations(&self, txids: BTreeSet<Txid>) -> HashMap<Txid, u32> {
-        lookup_confirmations(&self.store.history_db, txids)
+        let _timer = self.start_timer("lookup_confirmations");
+        let headers = self.store.indexed_headers.read().unwrap();
+        lookup_confirmations(&self.store.history_db, headers.best_height() as u32, txids)
     }
 
     pub fn get_block_status(&self, hash: &BlockHash) -> BlockStatus {
@@ -1242,14 +1245,19 @@ fn lookup_txo(txstore_db: &DB, outpoint: &OutPoint) -> Option<TxOut> {
         .map(|val| deserialize(&val).expect("failed to parse TxOut"))
 }
 
-pub fn lookup_confirmations(history_db: &DB, txids: BTreeSet<Txid>) -> HashMap<Txid, u32> {
+pub fn lookup_confirmations(
+    history_db: &DB,
+    tip_height: u32,
+    txids: BTreeSet<Txid>,
+) -> HashMap<Txid, u32> {
     history_db
         .multi_get(txids.iter().map(TxConfRow::key))
         .into_iter()
         .zip(txids)
         .filter_map(|(res, txid)| {
             let confirmation_height = u32::from_le_bytes(res.unwrap()?.try_into().unwrap());
-            Some((txid, confirmation_height))
+            // skip over entries that point to non-existing heights (may happen during reorg handling)
+            (confirmation_height <= tip_height).then_some((txid, confirmation_height))
         })
         .collect()
 }

src/util/block.rs

@@ -270,6 +270,13 @@ impl HeaderList {
         self.headers.len()
     }
 
+    /// Get the chain tip height. Panics if called on an empty HeaderList.
+    pub fn best_height(&self) -> usize {
+        self.len()
+            .checked_sub(1)
+            .expect("best_height() on empty HeaderList")
+    }
+
     pub fn is_empty(&self) -> bool {
         self.headers.is_empty()
     }
@@ -284,7 +291,7 @@ impl HeaderList {
         // Matches bitcoind's behaviour: bitcoin-cli getblock `bitcoin-cli getblockhash 0` | jq '.time == .mediantime'
         if height == 0 {
             self.headers.get(0).unwrap().header.time
-        } else if height > self.len() - 1 {
+        } else if height > self.best_height() {
             0
         } else {
             let mut timestamps = (height.saturating_sub(MTP_SPAN - 1)..=height)