From 38b3abcfacca8af42188660e9e075818b0fdf156 Mon Sep 17 00:00:00 2001 From: LLFourn Date: Wed, 19 Aug 2026 15:15:39 +1000 Subject: [PATCH] fix(chain): let the indexer decide how far to rescan `IndexedTxGraph::reindex` was a single pass over `TxGraph::full_txs`, which iterates a `HashMap`, so the same graph reindexed twice could produce two different `last_revealed` results. Each match inside `KeychainTxOutIndex::_index_txout` bumps `last_revealed` and replenishes the lookahead, widening the derived window the remaining outputs are judged against. An output far enough out to need that widening was skipped whenever it happened to be visited first, and no pass revisited it. This is not merely misuse of the API. The lookahead exists precisely to catch indices the persisted frontier does not know about -- a restored wallet, another signer on the same descriptor, an externally built PSBT paying one of our far indices -- so whenever it does its job the frontier moves mid-walk, and the order dependence is present in the intended use. Add `Indexer::rescan`, which is handed the whole graph and returns what indexing it produced. The default implementation offers every full transaction and floating output once, which is all an indexer needs when what it recognizes is fixed up front; `reindex` becomes a call to it. `KeychainTxOutIndex` overrides it and looks repeatedly, stopping when a pass leaves its revealed frontier unmoved. Putting the loop behind the trait rather than in `IndexedTxGraph` keeps the decision where the knowledge is. An indexer that does not widen what it matches is walked exactly once, as before, so the looping cannot leak into implementations that have no use for it. And `KeychainTxOutIndex` can key the loop on its own frontier instead of on whether a changeset came back empty: the changeset also carries staged spk cache entries, which move without the frontier moving, so an emptiness test spends an extra full walk on the ordinary restore path. The regression test uses a single transaction with both outputs rather than two transactions, because the graph's walk order is a `HashMap` order: a two-transaction test would pass a single-pass implementation about half the time. `index_tx` walks `tx.output` in vout order, so the far index at vout 0 is always judged against the initial window and always missed until the near index at vout 1 lifts the frontier. --- crates/chain/src/indexed_tx_graph.rs | 18 +++--- crates/chain/src/indexer.rs | 25 ++++++++ crates/chain/src/indexer/keychain_txout.rs | 23 ++++++++ crates/chain/tests/test_indexed_tx_graph.rs | 65 ++++++++++++++++++++- 4 files changed, 120 insertions(+), 11 deletions(-) diff --git a/crates/chain/src/indexed_tx_graph.rs b/crates/chain/src/indexed_tx_graph.rs index 496367ef26..4bc4230731 100644 --- a/crates/chain/src/indexed_tx_graph.rs +++ b/crates/chain/src/indexed_tx_graph.rs @@ -145,18 +145,16 @@ where /// Synchronizes the indexer to reflect every entry in the transaction graph. /// - /// Iterates over **all** full transactions and floating outputs in `self.graph`, passing each - /// into `self.index`. Any indexer-side changes produced (via `index_tx` or `index_txout`) are - /// merged into a fresh `ChangeSet`, which is then returned. + /// Hands the whole graph to [`Indexer::rescan`] and returns the indexer-side changes it + /// produces. How thoroughly the graph is walked is the indexer's decision: one whose set of + /// recognized outputs grows as it matches — `KeychainTxOutIndex` extends its lookahead past + /// each newly revealed index — has to look more than once to reach everything it can, and it is + /// the only party that knows when it is done. pub fn reindex(&mut self) -> ChangeSet { - let mut changeset = ChangeSet::::default(); - for tx in self.graph.full_txs() { - changeset.indexer.merge(self.index.index_tx(&tx)); - } - for (op, txout) in self.graph.floating_txouts() { - changeset.indexer.merge(self.index.index_txout(op, txout)); + ChangeSet { + tx_graph: Default::default(), + indexer: self.index.rescan(&self.graph), } - changeset } fn index_tx_graph_changeset( diff --git a/crates/chain/src/indexer.rs b/crates/chain/src/indexer.rs index 22e8398152..86b4dd4728 100644 --- a/crates/chain/src/indexer.rs +++ b/crates/chain/src/indexer.rs @@ -2,6 +2,8 @@ use bitcoin::{OutPoint, Transaction, TxOut}; +use crate::{tx_graph::TxGraph, Merge}; + #[cfg(feature = "miniscript")] pub mod keychain_txout; pub mod spk_txout; @@ -30,4 +32,27 @@ pub trait Indexer { /// Determines whether the transaction should be included in the index. fn is_tx_relevant(&self, tx: &Transaction) -> bool; + + /// Index everything in `graph` that this indexer has not already accounted for. + /// + /// The default implementation offers every full transaction and floating output to + /// [`index_tx`](Self::index_tx) and [`index_txout`](Self::index_txout) exactly once, which is + /// all an indexer needs when what it recognizes is fixed up front. + /// + /// Override this when a match can *widen* what the indexer recognizes, so that an output + /// offered earlier in the walk could match on a later look. Only the indexer knows when its + /// recognition set has stopped growing, so only the indexer can decide when to stop looking. + fn rescan(&mut self, graph: &TxGraph) -> Self::ChangeSet + where + Self::ChangeSet: Merge, + { + let mut changeset = Self::ChangeSet::default(); + for tx in graph.full_txs() { + changeset.merge(self.index_tx(&tx)); + } + for (op, txout) in graph.floating_txouts() { + changeset.merge(self.index_txout(op, txout)); + } + changeset + } } diff --git a/crates/chain/src/indexer/keychain_txout.rs b/crates/chain/src/indexer/keychain_txout.rs index 7973a0254e..33b5c97270 100644 --- a/crates/chain/src/indexer/keychain_txout.rs +++ b/crates/chain/src/indexer/keychain_txout.rs @@ -8,6 +8,7 @@ use crate::{ spk_client::{FullScanRequestBuilder, SyncRequestBuilder}, spk_iter::BIP32_MAX_INDEX, spk_txout::SpkTxOutIndex, + tx_graph::TxGraph, DescriptorExt, DescriptorId, Indexed, Indexer, KeychainIndexed, SpkIterator, }; use alloc::{borrow::ToOwned, vec::Vec}; @@ -198,6 +199,28 @@ impl Indexer for KeychainTxOutIndex { fn is_tx_relevant(&self, tx: &bitcoin::Transaction) -> bool { self.inner.is_relevant(tx) } + + /// Looks repeatedly, because a match here widens what the next look can find: revealing an + /// index replenishes the lookahead past it, bringing spks into the derived set that outputs + /// already walked past may pay to. Looking once would make the outcome depend on the order the + /// graph happens to yield its transactions. + fn rescan(&mut self, graph: &TxGraph) -> Self::ChangeSet { + let mut changeset = ChangeSet::default(); + loop { + // The frontier, not `changeset.is_empty()`: the changeset also carries staged spk cache + // entries, which move without the frontier moving and would buy a pointless extra look. + let frontier = self.last_revealed.clone(); + for tx in graph.full_txs() { + changeset.merge(self.index_tx(&tx)); + } + for (op, txout) in graph.floating_txouts() { + changeset.merge(self.index_txout(op, txout)); + } + if self.last_revealed == frontier { + return changeset; + } + } + } } impl KeychainTxOutIndex { diff --git a/crates/chain/tests/test_indexed_tx_graph.rs b/crates/chain/tests/test_indexed_tx_graph.rs index 96cafcb8ed..dd0c978930 100644 --- a/crates/chain/tests/test_indexed_tx_graph.rs +++ b/crates/chain/tests/test_indexed_tx_graph.rs @@ -10,7 +10,7 @@ use bdk_chain::{ indexer::keychain_txout::KeychainTxOutIndex, local_chain::LocalChain, spk_txout::SpkTxOutIndex, - tx_graph, Balance, ChainPosition, ConfirmationBlockTime, DescriptorExt, SpkIterator, + tx_graph, Balance, ChainPosition, ConfirmationBlockTime, DescriptorExt, Merge, SpkIterator, }; use bdk_testenv::{ anyhow::{self}, @@ -302,6 +302,69 @@ fn insert_relevant_txs() { assert_eq!(graph.initial_changeset(), initial_changeset); } +/// `reindex` must climb to the highest index the lookahead can reach, not stop at whichever match +/// it happened to see first. +/// +/// A match widens the derived window the *remaining* outputs are judged against, so one pass can +/// miss an output it already walked past. Two transactions would express that too, but the walk +/// order over the graph is a `HashMap` order — the very thing that is unreliable — so such a test +/// would pass a single-pass implementation about half the time. One transaction with both outputs +/// pins it down: `index_tx` walks `tx.output`, a `Vec`, in vout order, so `far` at vout 0 is +/// always judged against the initial window and always missed, and `near` at vout 1 only then +/// lifts the frontier that brings `far` into range. Nothing here depends on a hash seed. +#[test] +fn reindex_reaches_fixed_point() { + let (descriptor, _) = Descriptor::parse_descriptor(&Secp256k1::signing_only(), DESCRIPTORS[0]) + .expect("must be valid"); + let did = descriptor.descriptor_id(); + + let lookahead = 10; + let near = lookahead - 1; + let far = lookahead + 5; + assert!( + far >= lookahead && far < 2 * lookahead, + "`far` must be out of the initial window but within the one `near` widens it to", + ); + + let tx = Transaction { + output: [far, near] + .iter() + .map(|&index| TxOut { + value: Amount::from_sat(10_000), + script_pubkey: descriptor + .at_derivation_index(index) + .unwrap() + .script_pubkey(), + }) + .collect(), + ..new_tx(0) + }; + + let (mut graph, changeset) = + IndexedTxGraph::>::from_changeset( + indexed_tx_graph::ChangeSet { + tx_graph: tx_graph::ChangeSet { + txs: [Arc::new(tx)].into(), + ..Default::default() + }, + ..Default::default() + }, + |_| -> anyhow::Result<_> { + let mut indexer = KeychainTxOutIndex::new(lookahead, true); + assert!(indexer.insert_descriptor((), descriptor.clone()).unwrap()); + Ok(indexer) + }, + ) + .expect("must construct"); + + assert_eq!(graph.index.last_revealed_index(()), Some(far)); + assert_eq!(changeset.indexer.last_revealed.get(&did), Some(&far)); + assert!( + graph.reindex().is_empty(), + "reindexing an already-reindexed graph must find nothing new", + ); +} + /// Ensure consistency IndexedTxGraph list_* and balance methods. These methods lists /// relevant txouts and utxos from the information fetched from a LocalChain. ///