fix(electrum): treat batch_size of 0 as 1

A `batch_size` of 0 made `full_scan` and `sync` skip every request and
return an empty `Ok`, because `populate_with_spks` builds each batch with
`(0..batch_size)` and returns when the batch is empty.

Clamp it to 1 in `populate_with_spks`, document this on `full_scan` and
`sync`, and add a test covering both.

Fixes #2291

diff --git a/crates/electrum/src/bdk_electrum_client.rs b/crates/electrum/src/bdk_electrum_client.rs
index 752d00c8dfbbe8a906c828fe3a268398bf68d0a5..8efc86cc42f3d01582aecc87cab460eea5aab674 100644 (file)
--- a/crates/electrum/src/bdk_electrum_client.rs
+++ b/crates/electrum/src/bdk_electrum_client.rs
@@ -109,7 +109,7 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
     /// - `stop_gap`: the full scan for each keychain stops after a gap of script pubkeys with no
     ///   associated transactions.
     /// - `batch_size`: specifies the max number of script pubkeys to request for in a single batch
-    ///   request.
+    ///   request. A `batch_size` of 0 is treated as 1.
     /// - `fetch_prev_txouts`: specifies whether we want previous `TxOut`s for fee calculation. Note
     ///   that this requires additional calls to the Electrum server, but is necessary for
     ///   calculating the fee on a transaction if your wallet does not own the inputs. Methods like
@@ -195,7 +195,7 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
     /// - `request`: struct with data required to perform a spk-based blockchain client sync, see
     ///   [`SyncRequest`]
     /// - `batch_size`: specifies the max number of script pubkeys to request for in a single batch
-    ///   request
+    ///   request. A `batch_size` of 0 is treated as 1.
     /// - `fetch_prev_txouts`: specifies whether we want previous `TxOut`s for fee calculation. Note
     ///   that this requires additional calls to the Electrum server, but is necessary for
     ///   calculating the fee on a transaction if your wallet does not own the inputs. Methods like
@@ -301,6 +301,7 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
     ) -> Result<Option<u32>, Error> {
         let mut unused_spk_count = 0_usize;
         let mut last_active_index = Option::<u32>::None;
+        let batch_size = batch_size.max(1);
 
         loop {
             let spks = (0..batch_size)

diff --git a/crates/electrum/tests/test_electrum.rs b/crates/electrum/tests/test_electrum.rs
index 29ef950012d461a8949d72c2da72d7c2876d0fc0..d405acd9cfc4b95852cf656354543f8cf58515ea 100644 (file)
--- a/crates/electrum/tests/test_electrum.rs
+++ b/crates/electrum/tests/test_electrum.rs
@@ -1029,3 +1029,31 @@ fn test_sync_outpoints_propagates_connection_error() -> anyhow::Result<()> {
 
     Ok(())
 }
+
+/// A `batch_size` of 0 must not make `sync` or `full_scan` skip their scripts.
+///
+/// The server drops every connection, so a scan that actually requests something returns an error.
+/// If no request is made, the scan returns an empty `Ok`.
+#[test]
+fn test_zero_batch_size() -> anyhow::Result<()> {
+    let listener = std::net::TcpListener::bind("127.0.0.1:0")?;
+    let url = format!("tcp://{}", listener.local_addr()?);
+    std::thread::spawn(move || listener.incoming().for_each(drop));
+
+    let config = electrum_client::ConfigBuilder::new().retry(0).build();
+    let client = BdkElectrumClient::new(electrum_client::Client::from_config(&url, config)?);
+
+    let response = client.sync(SyncRequest::builder().spks([get_test_spk()]), 0, false);
+    assert!(
+        response.is_err(),
+        "sync skipped its script and returned {response:?}"
+    );
+
+    let request = FullScanRequest::builder().spks_for_keychain(0u32, [(0, get_test_spk())]);
+    let response = client.full_scan(request, 3, 0, false);
+    assert!(
+        response.is_err(),
+        "full scan skipped its script and returned {response:?}"
+    );
+    Ok(())
+}