Merge #1351: fix: define and document stop_gap

7c1861a fix: define and document `stop_gap` (Jose Storopoli)

Pull request description:

  ### Description

  - changes the code implementation to "the maximum number of consecutive unused addresses" in esplora async and blocking extensions.
  - for all purposes treats `stop_gap = 0` as `stop_gap = 1`.
  - renames `past_gap_limit` to `gap_limit_reached` to indicate we want to break once the gap limit is reached and not go further in `full_scan`, as suggested in #1227 (comment)
  - change the tests according to the new implementation.
  - add notes on what `stop_gap` means and links to convergent definition in other Bitcoin-related software.

  Closes #1227.

  ### Notes to the reviewers

  We can iterate over the wording and presentation of the `stop_gap` definition
  and details.

  ### Changelog notice

  - BREAKING: change `stop_gap` definition and effects in `full_scan`
    to reflect the common definitions in most Bitcoin-related software.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

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

  #### Bugfixes:

  * [x] 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:
  notmandatory:
    ACK 7c1861a

Tree-SHA512: d9bc5f8ebace47fa33f023ceddb3df5629ad3e7fa130a407311a0303ac59810e6527254efb9075f9e87bf37bec8655c4726eb9cb99f6b038fbeb742f79e995c0

Author: notmandatory

crates/esplora/src/async_ext.rs

@@ -52,6 +52,19 @@ pub trait EsploraAsyncExt {
     /// The full scan for each keychain stops after a gap of `stop_gap` script pubkeys with no associated
     /// transactions. `parallel_requests` specifies the max number of HTTP requests to make in
     /// parallel.
+    ///
+    /// ## Note
+    ///
+    /// `stop_gap` is defined as "the maximum number of consecutive unused addresses".
+    /// For example, with a `stop_gap` of  3, `full_scan` will keep scanning
+    /// until it encounters 3 consecutive script pubkeys with no associated transactions.
+    ///
+    /// This follows the same approach as other Bitcoin-related software,
+    /// such as [Electrum](https://electrum.readthedocs.io/en/latest/faq.html#what-is-the-gap-limit),
+    /// [BTCPay Server](https://docs.btcpayserver.org/FAQ/Wallet/#the-gap-limit-problem),
+    /// and [Sparrow](https://www.sparrowwallet.com/docs/faq.html#ive-restored-my-wallet-but-some-of-my-funds-are-missing).
+    ///
+    /// A `stop_gap` of 0 will be treated as a `stop_gap` of 1.
     async fn full_scan<K: Ord + Clone + Send>(
         &self,
         keychain_spks: BTreeMap<
@@ -162,6 +175,7 @@ impl EsploraAsyncExt for esplora_client::AsyncClient {
         let parallel_requests = Ord::max(parallel_requests, 1);
         let mut graph = TxGraph::<ConfirmationTimeHeightAnchor>::default();
         let mut last_active_indexes = BTreeMap::<K, u32>::new();
+        let stop_gap = Ord::max(stop_gap, 1);
 
         for (keychain, spks) in keychain_spks {
             let mut spks = spks.into_iter();
@@ -226,12 +240,12 @@ impl EsploraAsyncExt for esplora_client::AsyncClient {
                 }
 
                 let last_index = last_index.expect("Must be set since handles wasn't empty.");
-                let past_gap_limit = if let Some(i) = last_active_index {
-                    last_index > i.saturating_add(stop_gap as u32)
+                let gap_limit_reached = if let Some(i) = last_active_index {
+                    last_index >= i.saturating_add(stop_gap as u32)
                 } else {
-                    last_index >= stop_gap as u32
+                    last_index + 1 >= stop_gap as u32
                 };
-                if past_gap_limit {
+                if gap_limit_reached {
                     break;
                 }
             }

crates/esplora/src/blocking_ext.rs

@@ -50,6 +50,19 @@ pub trait EsploraExt {
     /// The full scan for each keychain stops after a gap of `stop_gap` script pubkeys with no associated
     /// transactions. `parallel_requests` specifies the max number of HTTP requests to make in
     /// parallel.
+    ///
+    /// ## Note
+    ///
+    /// `stop_gap` is defined as "the maximum number of consecutive unused addresses".
+    /// For example, with a `stop_gap` of  3, `full_scan` will keep scanning
+    /// until it encounters 3 consecutive script pubkeys with no associated transactions.
+    ///
+    /// This follows the same approach as other Bitcoin-related software,
+    /// such as [Electrum](https://electrum.readthedocs.io/en/latest/faq.html#what-is-the-gap-limit),
+    /// [BTCPay Server](https://docs.btcpayserver.org/FAQ/Wallet/#the-gap-limit-problem),
+    /// and [Sparrow](https://www.sparrowwallet.com/docs/faq.html#ive-restored-my-wallet-but-some-of-my-funds-are-missing).
+    ///
+    /// A `stop_gap` of 0 will be treated as a `stop_gap` of 1.
     fn full_scan<K: Ord + Clone>(
         &self,
         keychain_spks: BTreeMap<K, impl IntoIterator<Item = (u32, ScriptBuf)>>,
@@ -149,6 +162,7 @@ impl EsploraExt for esplora_client::BlockingClient {
         let parallel_requests = Ord::max(parallel_requests, 1);
         let mut graph = TxGraph::<ConfirmationTimeHeightAnchor>::default();
         let mut last_active_indexes = BTreeMap::<K, u32>::new();
+        let stop_gap = Ord::max(stop_gap, 1);
 
         for (keychain, spks) in keychain_spks {
             let mut spks = spks.into_iter();
@@ -216,12 +230,12 @@ impl EsploraExt for esplora_client::BlockingClient {
                 }
 
                 let last_index = last_index.expect("Must be set since handles wasn't empty.");
-                let past_gap_limit = if let Some(i) = last_active_index {
-                    last_index > i.saturating_add(stop_gap as u32)
+                let gap_limit_reached = if let Some(i) = last_active_index {
+                    last_index >= i.saturating_add(stop_gap as u32)
                 } else {
-                    last_index >= stop_gap as u32
+                    last_index + 1 >= stop_gap as u32
                 };
-                if past_gap_limit {
+                if gap_limit_reached {
                     break;
                 }
             }

crates/esplora/tests/async_ext.rs

@@ -91,9 +91,9 @@ pub async fn test_update_tx_graph_without_keychain() -> anyhow::Result<()> {
     Ok(())
 }
 
-/// Test the bounds of the address scan depending on the gap limit.
+/// Test the bounds of the address scan depending on the `stop_gap`.
 #[tokio::test]
-pub async fn test_async_update_tx_graph_gap_limit() -> anyhow::Result<()> {
+pub async fn test_async_update_tx_graph_stop_gap() -> anyhow::Result<()> {
     let env = TestEnv::new()?;
     let base_url = format!("http://{}", &env.electrsd.esplora_url.clone().unwrap());
     let client = Builder::new(base_url.as_str()).build_async()?;
@@ -140,12 +140,12 @@ pub async fn test_async_update_tx_graph_gap_limit() -> anyhow::Result<()> {
         sleep(Duration::from_millis(10))
     }
 
-    // A scan with a gap limit of 2 won't find the transaction, but a scan with a gap limit of 3
+    // A scan with a gap limit of 3 won't find the transaction, but a scan with a gap limit of 4
     // will.
-    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 2, 1).await?;
+    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 3, 1).await?;
     assert!(graph_update.full_txs().next().is_none());
     assert!(active_indices.is_empty());
-    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 3, 1).await?;
+    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 4, 1).await?;
     assert_eq!(graph_update.full_txs().next().unwrap().txid, txid_4th_addr);
     assert_eq!(active_indices[&0], 3);
 
@@ -165,14 +165,14 @@ pub async fn test_async_update_tx_graph_gap_limit() -> anyhow::Result<()> {
         sleep(Duration::from_millis(10))
     }
 
-    // A scan with gap limit 4 won't find the second transaction, but a scan with gap limit 5 will.
+    // A scan with gap limit 5 won't find the second transaction, but a scan with gap limit 6 will.
     // The last active indice won't be updated in the first case but will in the second one.
-    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 4, 1).await?;
+    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 5, 1).await?;
     let txs: HashSet<_> = graph_update.full_txs().map(|tx| tx.txid).collect();
     assert_eq!(txs.len(), 1);
     assert!(txs.contains(&txid_4th_addr));
     assert_eq!(active_indices[&0], 3);
-    let (graph_update, active_indices) = client.full_scan(keychains, 5, 1).await?;
+    let (graph_update, active_indices) = client.full_scan(keychains, 6, 1).await?;
     let txs: HashSet<_> = graph_update.full_txs().map(|tx| tx.txid).collect();
     assert_eq!(txs.len(), 2);
     assert!(txs.contains(&txid_4th_addr) && txs.contains(&txid_last_addr));

crates/esplora/tests/blocking_ext.rs

@@ -106,9 +106,9 @@ pub fn test_update_tx_graph_without_keychain() -> anyhow::Result<()> {
     Ok(())
 }
 
-/// Test the bounds of the address scan depending on the gap limit.
+/// Test the bounds of the address scan depending on the `stop_gap`.
 #[test]
-pub fn test_update_tx_graph_gap_limit() -> anyhow::Result<()> {
+pub fn test_update_tx_graph_stop_gap() -> anyhow::Result<()> {
     let env = TestEnv::new()?;
     let base_url = format!("http://{}", &env.electrsd.esplora_url.clone().unwrap());
     let client = Builder::new(base_url.as_str()).build_blocking()?;
@@ -155,12 +155,12 @@ pub fn test_update_tx_graph_gap_limit() -> anyhow::Result<()> {
         sleep(Duration::from_millis(10))
     }
 
-    // A scan with a gap limit of 2 won't find the transaction, but a scan with a gap limit of 3
+    // A scan with a stop_gap of 3 won't find the transaction, but a scan with a gap limit of 4
     // will.
-    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 2, 1)?;
+    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 3, 1)?;
     assert!(graph_update.full_txs().next().is_none());
     assert!(active_indices.is_empty());
-    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 3, 1)?;
+    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 4, 1)?;
     assert_eq!(graph_update.full_txs().next().unwrap().txid, txid_4th_addr);
     assert_eq!(active_indices[&0], 3);
 
@@ -180,14 +180,14 @@ pub fn test_update_tx_graph_gap_limit() -> anyhow::Result<()> {
         sleep(Duration::from_millis(10))
     }
 
-    // A scan with gap limit 4 won't find the second transaction, but a scan with gap limit 5 will.
+    // A scan with gap limit 5 won't find the second transaction, but a scan with gap limit 6 will.
     // The last active indice won't be updated in the first case but will in the second one.
-    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 4, 1)?;
+    let (graph_update, active_indices) = client.full_scan(keychains.clone(), 5, 1)?;
     let txs: HashSet<_> = graph_update.full_txs().map(|tx| tx.txid).collect();
     assert_eq!(txs.len(), 1);
     assert!(txs.contains(&txid_4th_addr));
     assert_eq!(active_indices[&0], 3);
-    let (graph_update, active_indices) = client.full_scan(keychains, 5, 1)?;
+    let (graph_update, active_indices) = client.full_scan(keychains, 6, 1)?;
     let txs: HashSet<_> = graph_update.full_txs().map(|tx| tx.txid).collect();
     assert_eq!(txs.len(), 2);
     assert!(txs.contains(&txid_4th_addr) && txs.contains(&txid_last_addr));