Merge bitcoindevkit#1225: esplora: fix incorrect gap limit check in b…

…locking client

43aed38 esplora: also test the gap limit bounds in the async client (Antoine Poinsot)
cb713e5 esplora: also fix the gap limit check in the async client (Antoine Poinsot)
2c4e90a esplora: scan gap limit bounds testing (Antoine Poinsot)
18bd329 esplora: fix incorrect gap limit check in blocking client (Antoine Poinsot)

Pull request description:

  The gap limit was checked such as if the last_index was not None but the last_active_index was, we'd consider having reached it. But the last_index is never None for this check. This effectively made it so the gap limit was always 1: if the first address isn't used last_active_index will be None and we'd return immediately.

  Fix this by avoiding error-prone Option comparisons and correctly checking we've reached the gap limit before breaking out of the loop.

ACKs for top commit:
  danielabrozzoni:
    ACK 43aed38
  evanlinjin:
    ACK 43aed38

Tree-SHA512: c6a172e0627380add56aec79e7fe36c0358e092b59f0bea8885d3524e65c10336291943efe6aeb5cc83f90c4f2146ed63215e56e08583d703b6ab57284487f03

crates/esplora/src/async_ext.rs

@@ -259,7 +259,13 @@ impl EsploraAsyncExt for esplora_client::AsyncClient {
                     }
                 }
 
-                if last_index > last_active_index.map(|i| i.saturating_add(stop_gap as u32)) {
+                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)
+                } else {
+                    last_index >= stop_gap as u32
+                };
+                if past_gap_limit {
                     break;
                 }
             }

crates/esplora/src/blocking_ext.rs

@@ -250,7 +250,13 @@ impl EsploraExt for esplora_client::BlockingClient {
                     }
                 }
 
-                if last_index > last_active_index.map(|i| i.saturating_add(stop_gap as u32)) {
+                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)
+                } else {
+                    last_index >= stop_gap as u32
+                };
+                if past_gap_limit {
                     break;
                 }
             }

crates/esplora/tests/async_ext.rs

@@ -3,6 +3,7 @@ use electrsd::bitcoind::bitcoincore_rpc::RpcApi;
 use electrsd::bitcoind::{self, anyhow, BitcoinD};
 use electrsd::{Conf, ElectrsD};
 use esplora_client::{self, AsyncClient, Builder};
+use std::collections::{BTreeMap, HashSet};
 use std::str::FromStr;
 use std::thread::sleep;
 use std::time::Duration;
@@ -115,3 +116,121 @@ pub async fn test_update_tx_graph_without_keychain() -> anyhow::Result<()> {
     assert_eq!(graph_update_txids, expected_txids);
     Ok(())
 }
+
+/// Test the bounds of the address scan depending on the gap limit.
+#[tokio::test]
+pub async fn test_async_update_tx_graph_gap_limit() -> anyhow::Result<()> {
+    let env = TestEnv::new()?;
+    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 spks: Vec<_> = addresses
+        .iter()
+        .enumerate()
+        .map(|(i, addr)| (i as u32, addr.script_pubkey()))
+        .collect();
+    let mut keychains = BTreeMap::new();
+    keychains.insert(0, spks);
+
+    // Then receive coins on the 4th address.
+    let txid_4th_addr = env.bitcoind.client.send_to_address(
+        &addresses[3],
+        Amount::from_sat(10000),
+        None,
+        None,
+        None,
+        None,
+        Some(1),
+        None,
+    )?;
+    let _block_hashes = env.mine_blocks(1, None)?;
+    while env.client.get_height().await.unwrap() < 103 {
+        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
+    // will.
+    let (graph_update, active_indices) = env
+        .client
+        .scan_txs_with_keychains(
+            keychains.clone(),
+            vec![].into_iter(),
+            vec![].into_iter(),
+            2,
+            1,
+        )
+        .await?;
+    assert!(graph_update.full_txs().next().is_none());
+    assert!(active_indices.is_empty());
+    let (graph_update, active_indices) = env
+        .client
+        .scan_txs_with_keychains(
+            keychains.clone(),
+            vec![].into_iter(),
+            vec![].into_iter(),
+            3,
+            1,
+        )
+        .await?;
+    assert_eq!(graph_update.full_txs().next().unwrap().txid, txid_4th_addr);
+    assert_eq!(active_indices[&0], 3);
+
+    // Now receive a coin on the last address.
+    let txid_last_addr = env.bitcoind.client.send_to_address(
+        &addresses[addresses.len() - 1],
+        Amount::from_sat(10000),
+        None,
+        None,
+        None,
+        None,
+        Some(1),
+        None,
+    )?;
+    let _block_hashes = env.mine_blocks(1, None)?;
+    while env.client.get_height().await.unwrap() < 104 {
+        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.
+    // The last active indice won't be updated in the first case but will in the second one.
+    let (graph_update, active_indices) = env
+        .client
+        .scan_txs_with_keychains(
+            keychains.clone(),
+            vec![].into_iter(),
+            vec![].into_iter(),
+            4,
+            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) = env
+        .client
+        .scan_txs_with_keychains(keychains, vec![].into_iter(), vec![].into_iter(), 5, 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));
+    assert_eq!(active_indices[&0], 9);
+
+    Ok(())
+}

crates/esplora/tests/blocking_ext.rs

@@ -3,6 +3,7 @@ use electrsd::bitcoind::bitcoincore_rpc::RpcApi;
 use electrsd::bitcoind::{self, anyhow, BitcoinD};
 use electrsd::{Conf, ElectrsD};
 use esplora_client::{self, BlockingClient, Builder};
+use std::collections::{BTreeMap, HashSet};
 use std::str::FromStr;
 use std::thread::sleep;
 use std::time::Duration;
@@ -110,5 +111,118 @@ pub fn test_update_tx_graph_without_keychain() -> anyhow::Result<()> {
     let mut expected_txids = vec![txid1, txid2];
     expected_txids.sort();
     assert_eq!(graph_update_txids, expected_txids);
+
+    Ok(())
+}
+
+/// Test the bounds of the address scan depending on the gap limit.
+#[test]
+pub fn test_update_tx_graph_gap_limit() -> anyhow::Result<()> {
+    let env = TestEnv::new()?;
+    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 spks: Vec<_> = addresses
+        .iter()
+        .enumerate()
+        .map(|(i, addr)| (i as u32, addr.script_pubkey()))
+        .collect();
+    let mut keychains = BTreeMap::new();
+    keychains.insert(0, spks);
+
+    // Then receive coins on the 4th address.
+    let txid_4th_addr = env.bitcoind.client.send_to_address(
+        &addresses[3],
+        Amount::from_sat(10000),
+        None,
+        None,
+        None,
+        None,
+        Some(1),
+        None,
+    )?;
+    let _block_hashes = env.mine_blocks(1, None)?;
+    while env.client.get_height().unwrap() < 103 {
+        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
+    // will.
+    let (graph_update, active_indices) = env.client.scan_txs_with_keychains(
+        keychains.clone(),
+        vec![].into_iter(),
+        vec![].into_iter(),
+        2,
+        1,
+    )?;
+    assert!(graph_update.full_txs().next().is_none());
+    assert!(active_indices.is_empty());
+    let (graph_update, active_indices) = env.client.scan_txs_with_keychains(
+        keychains.clone(),
+        vec![].into_iter(),
+        vec![].into_iter(),
+        3,
+        1,
+    )?;
+    assert_eq!(graph_update.full_txs().next().unwrap().txid, txid_4th_addr);
+    assert_eq!(active_indices[&0], 3);
+
+    // Now receive a coin on the last address.
+    let txid_last_addr = env.bitcoind.client.send_to_address(
+        &addresses[addresses.len() - 1],
+        Amount::from_sat(10000),
+        None,
+        None,
+        None,
+        None,
+        Some(1),
+        None,
+    )?;
+    let _block_hashes = env.mine_blocks(1, None)?;
+    while env.client.get_height().unwrap() < 104 {
+        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.
+    // The last active indice won't be updated in the first case but will in the second one.
+    let (graph_update, active_indices) = env.client.scan_txs_with_keychains(
+        keychains.clone(),
+        vec![].into_iter(),
+        vec![].into_iter(),
+        4,
+        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) = env.client.scan_txs_with_keychains(
+        keychains,
+        vec![].into_iter(),
+        vec![].into_iter(),
+        5,
+        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));
+    assert_eq!(active_indices[&0], 9);
+
     Ok(())
 }