Merge bitcoindevkit/bdk#652: Fix: Hang when ElectrumBlockchainConfig::stop_gap == 0

612da165f8cfbc2b0aee0a37b2cdc44d6da017c8 `Blockchain` stop_gap testing improvements (志宇)
8a5f89e129d421a41af02ea85383d5b82f5ff665 Fix hang when `ElectrumBlockchainConfig::stop_gap == 0` (志宇)

Pull request description:

  * Ensure `chunk_size` is > 0 during wallet sync.

  * Slight refactoring for better readability.

  * Add test: `test_electrum_blockchain_factory_sync_with_stop_gaps`

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### Description

  `Wallet::sync` hangs indefinitely when syncing with Electrum with `stop_gap` set as 0.

  The culprit is having `chunk_size` set as `stop_gap`. A zero value results in syncing not being able to progress.

  Fixes #651

  ### 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

  #### 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:
  afilini:
    ACK 612da165f8cfbc2b0aee0a37b2cdc44d6da017c8

Tree-SHA512: 56f1bff788855facc21856209922594cff9f639c5c58ecd180a0493322a75a564b72ded330ab0b6d6c90007ce859d2b8a5d2870d619bae5ddf9a3d64837f3753

CHANGELOG.md

@@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 - New MSRV set to `1.56.1`
 - Fee sniping discouraging through nLockTime - if the user specifies a `current_height`, we use that as a nlocktime, otherwise we use the last sync height (or 0 if we never synced)
+- Fix hang when `ElectrumBlockchainConfig::stop_gap` is zero.
 
 ## [v0.19.0] - [v0.18.0]
 

src/blockchain/electrum.rs

@@ -108,7 +108,10 @@ impl WalletSync for ElectrumBlockchain {
         let mut block_times = HashMap::<u32, u32>::new();
         let mut txid_to_height = HashMap::<Txid, u32>::new();
         let mut tx_cache = TxCache::new(database, &self.client);
-        let chunk_size = self.stop_gap;
+
+        // Set chunk_size to the smallest value capable of finding a gap greater than stop_gap.
+        let chunk_size = self.stop_gap + 1;
+
         // The electrum server has been inconsistent somehow in its responses during sync. For
         // example, we do a batch request of transactions and the response contains less
         // tranascations than in the request. This should never happen but we don't want to panic.
@@ -144,21 +147,12 @@ impl WalletSync for ElectrumBlockchain {
 
                 Request::Conftime(conftime_req) => {
                     // collect up to chunk_size heights to fetch from electrum
-                    let needs_block_height = {
+                    let needs_block_height = conftime_req
-                        let mut needs_block_height_iter = conftime_req
                         .request()
                         .filter_map(|txid| txid_to_height.get(txid).cloned())
-                            .filter(|height| block_times.get(height).is_none());
+                        .filter(|height| block_times.get(height).is_none())
-                        let mut needs_block_height = HashSet::new();
+                        .take(chunk_size)
-
+                        .collect::<HashSet<u32>>();
-                        while needs_block_height.len() < chunk_size {
-                            match needs_block_height_iter.next() {
-                                Some(height) => needs_block_height.insert(height),
-                                None => break,
-                            };
-                        }
-                        needs_block_height
-                    };
 
                     let new_block_headers = self
                         .client
@@ -328,6 +322,7 @@ mod test {
     use super::*;
     use crate::database::MemoryDatabase;
     use crate::testutils::blockchain_tests::TestClient;
+    use crate::testutils::configurable_blockchain_tests::ConfigurableBlockchainTester;
     use crate::wallet::{AddressIndex, Wallet};
 
     crate::bdk_blockchain_tests! {
@@ -385,4 +380,29 @@ mod test {
 
         assert_eq!(wallet.get_balance().unwrap(), 50_000);
     }
+
+    #[test]
+    fn test_electrum_with_variable_configs() {
+        struct ElectrumTester;
+
+        impl ConfigurableBlockchainTester<ElectrumBlockchain> for ElectrumTester {
+            const BLOCKCHAIN_NAME: &'static str = "Electrum";
+
+            fn config_with_stop_gap(
+                &self,
+                test_client: &mut TestClient,
+                stop_gap: usize,
+            ) -> Option<ElectrumBlockchainConfig> {
+                Some(ElectrumBlockchainConfig {
+                    url: test_client.electrsd.electrum_url.clone(),
+                    socks5: None,
+                    retry: 0,
+                    timeout: None,
+                    stop_gap: stop_gap,
+                })
+            }
+        }
+
+        ElectrumTester.run();
+    }
 }

src/blockchain/esplora/mod.rs

@@ -209,4 +209,38 @@ mod test {
             "should inherit from value for 25"
         );
     }
+
+    #[test]
+    #[cfg(feature = "test-esplora")]
+    fn test_esplora_with_variable_configs() {
+        use crate::testutils::{
+            blockchain_tests::TestClient,
+            configurable_blockchain_tests::ConfigurableBlockchainTester,
+        };
+
+        struct EsploraTester;
+
+        impl ConfigurableBlockchainTester<EsploraBlockchain> for EsploraTester {
+            const BLOCKCHAIN_NAME: &'static str = "Esplora";
+
+            fn config_with_stop_gap(
+                &self,
+                test_client: &mut TestClient,
+                stop_gap: usize,
+            ) -> Option<EsploraBlockchainConfig> {
+                Some(EsploraBlockchainConfig {
+                    base_url: format!(
+                        "http://{}",
+                        test_client.electrsd.esplora_url.as_ref().unwrap()
+                    ),
+                    proxy: None,
+                    concurrency: None,
+                    stop_gap: stop_gap,
+                    timeout: None,
+                })
+            }
+        }
+
+        EsploraTester.run();
+    }
 }

src/blockchain/esplora/ureq.rs

@@ -88,7 +88,7 @@ impl Blockchain for EsploraBlockchain {
     }
 
     fn broadcast(&self, tx: &Transaction) -> Result<(), Error> {
-        let _txid = self.url_client._broadcast(tx)?;
+        self.url_client._broadcast(tx)?;
         Ok(())
     }
 

src/testutils/configurable_blockchain_tests.rs

@@ -0,0 +1,140 @@
+use bitcoin::Network;
+
+use crate::{
+    blockchain::ConfigurableBlockchain, database::MemoryDatabase, testutils, wallet::AddressIndex,
+    Wallet,
+};
+
+use super::blockchain_tests::TestClient;
+
+/// Trait for testing [`ConfigurableBlockchain`] implementations.
+pub trait ConfigurableBlockchainTester<B: ConfigurableBlockchain>: Sized {
+    /// Blockchain name for logging.
+    const BLOCKCHAIN_NAME: &'static str;
+
+    /// Generates a blockchain config with a given stop_gap.
+    ///
+    /// If this returns [`Option::None`], then the associated tests will not run.
+    fn config_with_stop_gap(
+        &self,
+        _test_client: &mut TestClient,
+        _stop_gap: usize,
+    ) -> Option<B::Config> {
+        None
+    }
+
+    /// Runs all avaliable tests.
+    fn run(&self) {
+        let test_client = &mut TestClient::default();
+
+        if self.config_with_stop_gap(test_client, 0).is_some() {
+            test_wallet_sync_with_stop_gaps(test_client, self);
+        } else {
+            println!(
+                "{}: Skipped tests requiring config_with_stop_gap.",
+                Self::BLOCKCHAIN_NAME
+            );
+        }
+    }
+}
+
+/// Test whether blockchain implementation syncs with expected behaviour given different `stop_gap`
+/// parameters.
+///
+/// For each test vector:
+/// * Fill wallet's derived addresses with balances (as specified by test vector).
+///    * [0..addrs_before]          => 1000sats for each address
+///    * [addrs_before..actual_gap] => empty addresses
+///    * [actual_gap..addrs_after]  => 1000sats for each address
+/// * Then, perform wallet sync and obtain wallet balance
+/// * Check balance is within expected range (we can compare `stop_gap` and `actual_gap` to
+///    determine this).
+fn test_wallet_sync_with_stop_gaps<T, B>(test_client: &mut TestClient, tester: &T)
+where
+    T: ConfigurableBlockchainTester<B>,
+    B: ConfigurableBlockchain,
+{
+    // Generates wallet descriptor
+    let descriptor_of_account = |account_index: usize| -> String {
+        format!("wpkh([c258d2e4/84h/1h/0h]tpubDDYkZojQFQjht8Tm4jsS3iuEmKjTiEGjG6KnuFNKKJb5A6ZUCUZKdvLdSDWofKi4ToRCwb9poe1XdqfUnP4jaJjCB2Zwv11ZLgSbnZSNecE/{account_index}/*)")
+    };
+
+    // Amount (in satoshis) provided to a single address (which expects to have a balance)
+    const AMOUNT_PER_TX: u64 = 1000;
+
+    // [stop_gap, actual_gap, addrs_before, addrs_after]
+    //
+    // [0]     stop_gap: Passed to [`ElectrumBlockchainConfig`]
+    // [1]   actual_gap: Range size of address indexes without a balance
+    // [2] addrs_before: Range size of address indexes (before gap) which contains a balance
+    // [3]  addrs_after: Range size of address indexes (after gap) which contains a balance
+    let test_vectors: Vec<[u64; 4]> = vec![
+        [0, 0, 0, 5],
+        [0, 0, 5, 5],
+        [0, 1, 5, 5],
+        [0, 2, 5, 5],
+        [1, 0, 5, 5],
+        [1, 1, 5, 5],
+        [1, 2, 5, 5],
+        [2, 1, 5, 5],
+        [2, 2, 5, 5],
+        [2, 3, 5, 5],
+    ];
+
+    for (account_index, vector) in test_vectors.into_iter().enumerate() {
+        let [stop_gap, actual_gap, addrs_before, addrs_after] = vector;
+        let descriptor = descriptor_of_account(account_index);
+
+        let blockchain = B::from_config(
+            &tester
+                .config_with_stop_gap(test_client, stop_gap as _)
+                .unwrap(),
+        )
+        .unwrap();
+
+        let wallet =
+            Wallet::new(&descriptor, None, Network::Regtest, MemoryDatabase::new()).unwrap();
+
+        // fill server-side with txs to specified address indexes
+        // return the max balance of the wallet (also the actual balance)
+        let max_balance = (0..addrs_before)
+            .chain(addrs_before + actual_gap..addrs_before + actual_gap + addrs_after)
+            .fold(0_u64, |sum, i| {
+                let address = wallet.get_address(AddressIndex::Peek(i as _)).unwrap();
+                test_client.receive(testutils! {
+                    @tx ( (@addr address.address) => AMOUNT_PER_TX )
+                });
+                sum + AMOUNT_PER_TX
+            });
+
+        // minimum allowed balance of wallet (based on stop gap)
+        let min_balance = if actual_gap > stop_gap {
+            addrs_before * AMOUNT_PER_TX
+        } else {
+            max_balance
+        };
+
+        // perform wallet sync
+        wallet.sync(&blockchain, Default::default()).unwrap();
+
+        let wallet_balance = wallet.get_balance().unwrap();
+
+        let details = format!(
+            "test_vector: [stop_gap: {}, actual_gap: {}, addrs_before: {}, addrs_after: {}]",
+            stop_gap, actual_gap, addrs_before, addrs_after,
+        );
+        assert!(
+            wallet_balance <= max_balance,
+            "wallet balance is greater than received amount: {}",
+            details
+        );
+        assert!(
+            wallet_balance >= min_balance,
+            "wallet balance is smaller than expected: {}",
+            details
+        );
+
+        // generate block to confirm new transactions
+        test_client.generate(1, None);
+    }
+}

src/testutils/mod.rs

@@ -14,6 +14,10 @@
 #[cfg(feature = "test-blockchains")]
 pub mod blockchain_tests;
 
+#[cfg(test)]
+#[cfg(feature = "test-blockchains")]
+pub mod configurable_blockchain_tests;
+
 use bitcoin::{Address, Txid};
 
 #[derive(Clone, Debug)]