From 7c1861aab9a76f2ba4a2ae372a16c9669a33f5de Mon Sep 17 00:00:00 2001 From: Jose Storopoli Date: Tue, 26 Mar 2024 12:44:03 -0300 Subject: [PATCH] fix: define and document `stop_gap` - changes the code implementation to "the maximum number of consecutive unused addresses" in esplora async and blocking extjensions. - treat `stop_gap = 0` as `stop_gap = 1` for all purposes. - 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 https://github.com/bitcoindevkit/bdk/issues/1227#issuecomment-1859040463 - 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 --- crates/esplora/src/async_ext.rs | 22 ++++++++++++++++++---- crates/esplora/src/blocking_ext.rs | 22 ++++++++++++++++++---- crates/esplora/tests/async_ext.rs | 16 ++++++++-------- crates/esplora/tests/blocking_ext.rs | 16 ++++++++-------- 4 files changed, 52 insertions(+), 24 deletions(-) diff --git a/crates/esplora/src/async_ext.rs b/crates/esplora/src/async_ext.rs index 4d6e0dfa8..9e25eedfb 100644 --- a/crates/esplora/src/async_ext.rs +++ b/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( &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::::default(); let mut last_active_indexes = BTreeMap::::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; } } diff --git a/crates/esplora/src/blocking_ext.rs b/crates/esplora/src/blocking_ext.rs index 993e33ac0..9cd11e819 100644 --- a/crates/esplora/src/blocking_ext.rs +++ b/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( &self, keychain_spks: BTreeMap>, @@ -149,6 +162,7 @@ impl EsploraExt for esplora_client::BlockingClient { let parallel_requests = Ord::max(parallel_requests, 1); let mut graph = TxGraph::::default(); let mut last_active_indexes = BTreeMap::::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; } } diff --git a/crates/esplora/tests/async_ext.rs b/crates/esplora/tests/async_ext.rs index 6c3c9cf1f..e08d98a3c 100644 --- a/crates/esplora/tests/async_ext.rs +++ b/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)); diff --git a/crates/esplora/tests/blocking_ext.rs b/crates/esplora/tests/blocking_ext.rs index 6225a6a6b..d3795ed36 100644 --- a/crates/esplora/tests/blocking_ext.rs +++ b/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));