Merge #2182: fix: full_scan covers revealed range before applying stop_gap

187b007 test(esplora): allow(dead_code) at file level in common (Noah Joeris)
79d1f45 fix: full_scan covers revealed range before applying stop_gap (Noah Joeris)

Pull request description:

  Fixes #2057. `full_scan` now always covers the revealed range before applying the stop_gap

  ### Notes to the reviewers

  Replaces #2181. That PR unified full_scan and sync into one path; per @evanlinjin's feedback we're keeping them separate and doing the minimal fix here instead

  ### Changelog notice

  Fixed: full_scan now always scans the full revealed range before applying stop_gap past last_revealed.

  ### Checklists

  #### All Submissions:

  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)

  #### New Features:

  * [x] I've added tests for the new feature
  * [x] I've added docs for the new feature

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  evanlinjin:
    ACK 187b007

Tree-SHA512: 9899e5d857d3cc3e86d35fd0e6a49204d4029053893b98fe1d8eb2bbdb9ea85b204997d87a72c9d485544286a0107936f048e54a7c152ab66b2106c897a1d900

Author: evanlinjin

crates/chain/src/indexer/keychain_txout.rs

@@ -1128,7 +1128,11 @@ pub trait FullScanRequestBuilderExt<K> {
 impl<K: Clone + Ord + core::fmt::Debug> FullScanRequestBuilderExt<K> for FullScanRequestBuilder<K> {
     fn spks_from_indexer(mut self, indexer: &KeychainTxOutIndex<K>) -> Self {
         for (keychain, spks) in indexer.all_unbounded_spk_iters() {
-            self = self.spks_for_keychain(keychain, spks);
+            let last_revealed = indexer.last_revealed_index(keychain.clone());
+            self = self.spks_for_keychain(keychain.clone(), spks);
+            if let Some(index) = last_revealed {
+                self = self.last_revealed_for_keychain(keychain, index);
+            }
         }
         self
     }

crates/core/src/spk_client.rs

@@ -456,6 +456,18 @@ impl<K: Ord, D> FullScanRequestBuilder<K, D> {
         self
     }
 
+    /// Record the last revealed script pubkey `index` for a given `keychain`.
+    ///
+    /// `full_scan` covers `0..=index` for this keychain; `stop_gap`
+    /// applies only to indices past `index`. Keychains without a recorded last revealed
+    /// index fall back to applying `stop_gap` from index 0.
+    /// Users working with a `KeychainTxOutIndex` usually don't call this directly,
+    /// `spks_from_indexer` (from `bdk_chain`) populates it automatically.
+    pub fn last_revealed_for_keychain(mut self, keychain: K, index: u32) -> Self {
+        self.inner.last_revealed.insert(keychain, index);
+        self
+    }
+
     /// Set the closure that will inspect every sync item visited.
     pub fn inspect<F>(mut self, inspect: F) -> Self
     where
@@ -474,15 +486,17 @@ impl<K: Ord, D> FullScanRequestBuilder<K, D> {
 /// Data required to perform a spk-based blockchain client full scan.
 ///
 /// A client full scan iterates through all the scripts for the given keychains, fetching relevant
-/// data until some stop gap number of scripts is found that have no data. This operation is
-/// generally only used when importing or restoring previously used keychains in which the list of
-/// used scripts is not known. The full scan process also updates the chain from the given
+/// data. It always scans the revealed range (up to the last-revealed index), then keeps going until
+/// a run of `stop_gap` consecutive scripts with no data is found. This operation is generally only
+/// used when importing or restoring previously used keychains in which the list of used scripts is
+/// not known. The full scan process also updates the chain from the given
 /// [`chain_tip`](FullScanRequestBuilder::chain_tip) (if provided).
 #[must_use]
 pub struct FullScanRequest<K, D = BlockHash> {
     start_time: u64,
     chain_tip: Option<CheckPoint<D>>,
     spks_by_keychain: BTreeMap<K, Box<dyn Iterator<Item = Indexed<ScriptBuf>> + Send>>,
+    last_revealed: BTreeMap<K, u32>,
     inspect: Box<InspectFullScan<K>>,
 }
 
@@ -507,6 +521,7 @@ impl<K: Ord + Clone, D> FullScanRequest<K, D> {
                 start_time,
                 chain_tip: None,
                 spks_by_keychain: BTreeMap::new(),
+                last_revealed: BTreeMap::new(),
                 inspect: Box::new(|_, _, _| ()),
             },
         }
@@ -541,6 +556,14 @@ impl<K: Ord + Clone, D> FullScanRequest<K, D> {
         self.spks_by_keychain.keys().cloned().collect()
     }
 
+    /// Get the last revealed script pubkey index for `keychain` (if set).
+    ///
+    /// Chain sources use this to scan `0..=last_revealed` before applying
+    /// `stop_gap` to further discovery.
+    pub fn last_revealed(&self, keychain: &K) -> Option<u32> {
+        self.last_revealed.get(keychain).copied()
+    }
+
     /// Advances the full scan request and returns the next indexed [`ScriptBuf`] of the given
     /// `keychain`.
     pub fn next_spk(&mut self, keychain: K) -> Option<Indexed<ScriptBuf>> {

crates/electrum/src/bdk_electrum_client.rs

@@ -136,6 +136,7 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
         let mut last_active_indices = BTreeMap::<K, u32>::default();
         let mut pending_anchors = Vec::new();
         for keychain in request.keychains() {
+            let last_revealed = request.last_revealed(&keychain);
             let spks = request
                 .iter_spks(keychain.clone())
                 .map(|(spk_i, spk)| (spk_i, SpkWithExpectedTxids::from(spk)));
@@ -144,6 +145,7 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
                 &mut tx_update,
                 spks,
                 stop_gap,
+                last_revealed,
                 batch_size,
                 &mut pending_anchors,
             )? {
@@ -225,6 +227,7 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
                 .enumerate()
                 .map(|(i, spk)| (i as u32, spk)),
             usize::MAX,
+            None,
             batch_size,
             &mut pending_anchors,
         )?;
@@ -273,12 +276,14 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
     /// Transactions that contains an output with requested spk, or spends form an output with
     /// requested spk will be added to `tx_update`. Anchors of the aforementioned transactions are
     /// also included.
+    #[allow(clippy::too_many_arguments)]
     fn populate_with_spks(
         &self,
         start_time: u64,
         tx_update: &mut TxUpdate<ConfirmationBlockTime>,
         mut spks_with_expected_txids: impl Iterator<Item = (u32, SpkWithExpectedTxids)>,
         stop_gap: usize,
+        last_revealed: Option<u32>,
         batch_size: usize,
         pending_anchors: &mut Vec<(Txid, usize)>,
     ) -> Result<Option<u32>, Error> {
@@ -298,14 +303,16 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
                 .batch_script_get_history(spks.iter().map(|(_, s)| s.spk.as_script()))?;
 
             for ((spk_index, spk), spk_history) in spks.into_iter().zip(spk_histories) {
-                if spk_history.is_empty() {
-                    match unused_spk_count.checked_add(1) {
-                        Some(i) if i < stop_gap => unused_spk_count = i,
-                        _ => return Ok(last_active_index),
-                    };
-                } else {
+                let beyond_revealed = last_revealed.is_none_or(|lr| spk_index > lr);
+
+                if !spk_history.is_empty() {
                     last_active_index = Some(spk_index);
                     unused_spk_count = 0;
+                } else if beyond_revealed {
+                    unused_spk_count = unused_spk_count.saturating_add(1);
+                    if unused_spk_count >= stop_gap {
+                        return Ok(last_active_index);
+                    }
                 }
 
                 let spk_history_set = spk_history

crates/electrum/tests/test_electrum.rs

@@ -34,6 +34,24 @@ pub fn get_test_spk() -> ScriptBuf {
     ScriptBuf::new_p2tr(&secp, pk, None)
 }
 
+pub fn test_addresses() -> Vec<Address> {
+    [
+        "bcrt1qj9f7r8r3p2y0sqf4r3r62qysmkuh0fzep473d2ar7rcz64wqvhssjgf0z4",
+        "bcrt1qmm5t0ch7vh2hryx9ctq3mswexcugqe4atkpkl2tetm8merqkthas3w7q30",
+        "bcrt1qut9p7ej7l7lhyvekj28xknn8gnugtym4d5qvnp5shrsr4nksmfqsmyn87g",
+        "bcrt1qqz0xtn3m235p2k96f5wa2dqukg6shxn9n3txe8arlrhjh5p744hsd957ww",
+        "bcrt1q9c0t62a8l6wfytmf2t9lfj35avadk3mm8g4p3l84tp6rl66m48sqrme7wu",
+        "bcrt1qkmh8yrk2v47cklt8dytk8f3ammcwa4q7dzattedzfhqzvfwwgyzsg59zrh",
+        "bcrt1qvgrsrzy07gjkkfr5luplt0azxtfwmwq5t62gum5jr7zwcvep2acs8hhnp2",
+        "bcrt1qw57edarcg50ansq8mk3guyrk78rk0fwvrds5xvqeupteu848zayq549av8",
+        "bcrt1qvtve5ekf6e5kzs68knvnt2phfw6a0yjqrlgat392m6zt9jsvyxhqfx67ef",
+        "bcrt1qw03ddumfs9z0kcu76ln7jrjfdwam20qtffmkcral3qtza90sp9kqm787uk",
+    ]
+    .into_iter()
+    .map(|s| Address::from_str(s).unwrap().assume_checked())
+    .collect()
+}
+
 fn get_balance(
     recv_chain: &LocalChain,
     recv_graph: &IndexedTxGraph<ConfirmationBlockTime, SpkTxOutIndex<()>>,
@@ -383,23 +401,7 @@ pub fn test_update_tx_graph_stop_gap() -> anyhow::Result<()> {
     let client = BdkElectrumClient::new(electrum_client);
     let _block_hashes = env.mine_blocks(101, None)?;
 
-    // Now let's test the gap limit. First of all get a chain of 10 addresses.
-    let addresses = [
-        "bcrt1qj9f7r8r3p2y0sqf4r3r62qysmkuh0fzep473d2ar7rcz64wqvhssjgf0z4",
-        "bcrt1qmm5t0ch7vh2hryx9ctq3mswexcugqe4atkpkl2tetm8merqkthas3w7q30",
-        "bcrt1qut9p7ej7l7lhyvekj28xknn8gnugtym4d5qvnp5shrsr4nksmfqsmyn87g",
-        "bcrt1qqz0xtn3m235p2k96f5wa2dqukg6shxn9n3txe8arlrhjh5p744hsd957ww",
-        "bcrt1q9c0t62a8l6wfytmf2t9lfj35avadk3mm8g4p3l84tp6rl66m48sqrme7wu",
-        "bcrt1qkmh8yrk2v47cklt8dytk8f3ammcwa4q7dzattedzfhqzvfwwgyzsg59zrh",
-        "bcrt1qvgrsrzy07gjkkfr5luplt0azxtfwmwq5t62gum5jr7zwcvep2acs8hhnp2",
-        "bcrt1qw57edarcg50ansq8mk3guyrk78rk0fwvrds5xvqeupteu848zayq549av8",
-        "bcrt1qvtve5ekf6e5kzs68knvnt2phfw6a0yjqrlgat392m6zt9jsvyxhqfx67ef",
-        "bcrt1qw03ddumfs9z0kcu76ln7jrjfdwam20qtffmkcral3qtza90sp9kqm787uk",
-    ];
-    let addresses: Vec<_> = addresses
-        .into_iter()
-        .map(|s| Address::from_str(s).unwrap().assume_checked())
-        .collect();
+    let addresses = test_addresses();
     let spks: Vec<_> = addresses
         .iter()
         .enumerate()
@@ -490,6 +492,63 @@ pub fn test_update_tx_graph_stop_gap() -> anyhow::Result<()> {
     Ok(())
 }
 
+/// Test that `full_scan` scans the revealed range before applying `stop_gap`, and that `stop_gap`
+/// is still enforced beyond `last_revealed`.
+///
+/// With a tx at index 9, a scan with `stop_gap=3` and `last_revealed=9` must find the tx.
+/// A scan with `stop_gap=3` and `last_revealed=3` must not return it
+/// as 9 is beyond `last_revealed + stop_gap`, so the scan stops first.
+#[test]
+pub fn test_stop_gap_past_last_revealed() -> anyhow::Result<()> {
+    let env = TestEnv::new()?;
+    let electrum_client = electrum_client::Client::new(env.electrsd.electrum_url.as_str())?;
+    let client = BdkElectrumClient::new(electrum_client);
+    let _block_hashes = env.mine_blocks(101, None)?;
+
+    let addresses = test_addresses();
+    let spks: Vec<_> = addresses
+        .iter()
+        .enumerate()
+        .map(|(i, addr)| (i as u32, addr.script_pubkey()))
+        .collect();
+
+    // Receive coins at index 9.
+    let txid_last_addr = env
+        .bitcoind
+        .client
+        .send_to_address(&addresses[9], Amount::from_sat(10000))?
+        .txid()?;
+    env.mine_blocks(1, None)?;
+    env.wait_until_electrum_sees_block(Duration::from_secs(6))?;
+
+    let cp_tip = env.make_checkpoint_tip();
+
+    // Revealed range covers the tx: it must be found.
+    let request = FullScanRequest::builder()
+        .chain_tip(cp_tip.clone())
+        .spks_for_keychain(0, spks.clone())
+        .last_revealed_for_keychain(0, 9);
+    let response = client.full_scan(request, 3, 1, false)?;
+
+    assert_eq!(
+        response.tx_update.txs.first().unwrap().compute_txid(),
+        txid_last_addr
+    );
+    assert_eq!(response.last_active_indices[&0], 9);
+
+    // Tx sits beyond `last_revealed + stop_gap`. So tx should not be found.
+    let request = FullScanRequest::builder()
+        .chain_tip(cp_tip)
+        .spks_for_keychain(0, spks)
+        .last_revealed_for_keychain(0, 3);
+    let response = client.full_scan(request, 3, 1, false)?;
+
+    assert!(response.tx_update.txs.is_empty());
+    assert!(response.last_active_indices.is_empty());
+
+    Ok(())
+}
+
 /// Ensure that [`BdkElectrumClient::sync`] can confirm previously unconfirmed transactions in both
 /// reorg and no-reorg situations. After the transaction is confirmed after reorg, check if floating
 /// txouts for previous outputs were inserted for transaction fee calculation.

crates/esplora/src/async_ext.rs

@@ -79,6 +79,7 @@ where
         let mut inserted_txs = HashSet::<Txid>::new();
         let mut last_active_indices = BTreeMap::<K, u32>::new();
         for keychain in keychains {
+            let last_revealed = request.last_revealed(&keychain);
             let keychain_spks = request
                 .iter_spks(keychain.clone())
                 .map(|(spk_i, spk)| (spk_i, spk.into()));
@@ -88,6 +89,7 @@ where
                 &mut inserted_txs,
                 keychain_spks,
                 stop_gap,
+                last_revealed,
                 parallel_requests,
             )
             .await?;
@@ -305,6 +307,7 @@ async fn fetch_txs_with_keychain_spks<I, S>(
     inserted_txs: &mut HashSet<Txid>,
     mut keychain_spks: I,
     stop_gap: usize,
+    last_revealed: Option<u32>,
     parallel_requests: usize,
 ) -> Result<(TxUpdate<ConfirmationBlockTime>, Option<u32>), Error>
 where
@@ -355,12 +358,13 @@ where
         }
 
         for (index, txs, evicted) in handles.try_collect::<Vec<TxsOfSpkIndex>>().await? {
-            if txs.is_empty() {
-                consecutive_unused = consecutive_unused.saturating_add(1);
-            } else {
+            if !txs.is_empty() {
                 consecutive_unused = 0;
                 last_active_index = Some(index);
+            } else if last_revealed.is_none_or(|lr| index > lr) {
+                consecutive_unused = consecutive_unused.saturating_add(1);
             }
+
             for tx in txs {
                 if inserted_txs.insert(tx.txid) {
                     update.txs.push(tx.to_tx().into());
@@ -407,6 +411,7 @@ where
         inserted_txs,
         spks.into_iter().enumerate().map(|(i, spk)| (i as u32, spk)),
         usize::MAX,
+        None,
         parallel_requests,
     )
     .await

crates/esplora/src/blocking_ext.rs

@@ -69,6 +69,7 @@ impl EsploraExt for esplora_client::BlockingClient {
         let mut inserted_txs = HashSet::<Txid>::new();
         let mut last_active_indices = BTreeMap::<K, u32>::new();
         for keychain in request.keychains() {
+            let last_revealed = request.last_revealed(&keychain);
             let keychain_spks = request
                 .iter_spks(keychain.clone())
                 .map(|(spk_i, spk)| (spk_i, spk.into()));
@@ -78,6 +79,7 @@ impl EsploraExt for esplora_client::BlockingClient {
                 &mut inserted_txs,
                 keychain_spks,
                 stop_gap,
+                last_revealed,
                 parallel_requests,
             )?;
             tx_update.extend(update);
@@ -277,6 +279,7 @@ fn fetch_txs_with_keychain_spks<I: Iterator<Item = Indexed<SpkWithExpectedTxids>
     inserted_txs: &mut HashSet<Txid>,
     mut keychain_spks: I,
     stop_gap: usize,
+    last_revealed: Option<u32>,
     parallel_requests: usize,
 ) -> Result<(TxUpdate<ConfirmationBlockTime>, Option<u32>), Error> {
     type TxsOfSpkIndex = (u32, Vec<esplora_client::Tx>, HashSet<Txid>);
@@ -324,12 +327,13 @@ fn fetch_txs_with_keychain_spks<I: Iterator<Item = Indexed<SpkWithExpectedTxids>
 
         for handle in handles {
             let (index, txs, evicted) = handle.join().expect("thread must not panic")?;
-            if txs.is_empty() {
-                consecutive_unused = consecutive_unused.saturating_add(1);
-            } else {
+            if !txs.is_empty() {
                 consecutive_unused = 0;
                 last_active_index = Some(index);
+            } else if last_revealed.is_none_or(|lr| index > lr) {
+                consecutive_unused = consecutive_unused.saturating_add(1);
             }
+
             for tx in txs {
                 if inserted_txs.insert(tx.txid) {
                     update.txs.push(tx.to_tx().into());
@@ -371,6 +375,7 @@ fn fetch_txs_with_spks<I: IntoIterator<Item = SpkWithExpectedTxids>>(
         inserted_txs,
         spks.into_iter().enumerate().map(|(i, spk)| (i as u32, spk)),
         usize::MAX,
+        None,
         parallel_requests,
     )
     .map(|(update, _)| update)