From 8a5f89e129d421a41af02ea85383d5b82f5ff665 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Sun, 3 Jul 2022 12:47:54 +0800 Subject: [PATCH 1/2] Fix hang when `ElectrumBlockchainConfig::stop_gap == 0` * Ensure chunk_size is > 0 during wallet sync. * Slight refactoring for better readability. * Add test: test_electrum_blockchain_factory_sync_with_stop_gaps --- CHANGELOG.md | 1 + src/blockchain/electrum.rs | 133 ++++++++++++++++++++++++++++++++++--- 2 files changed, 124 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a720dfb5..c41329c6 100644 --- a/CHANGELOG.md +++ b/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] diff --git a/src/blockchain/electrum.rs b/src/blockchain/electrum.rs index f8ac758c..784218e7 100644 --- a/src/blockchain/electrum.rs +++ b/src/blockchain/electrum.rs @@ -108,7 +108,10 @@ impl WalletSync for ElectrumBlockchain { let mut block_times = HashMap::::new(); let mut txid_to_height = HashMap::::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. @@ -145,18 +148,16 @@ impl WalletSync for ElectrumBlockchain { Request::Conftime(conftime_req) => { // collect up to chunk_size heights to fetch from electrum let needs_block_height = { - let mut needs_block_height_iter = conftime_req + let mut needs_block_height = HashSet::with_capacity(chunk_size); + conftime_req .request() .filter_map(|txid| txid_to_height.get(txid).cloned()) - .filter(|height| block_times.get(height).is_none()); - let mut needs_block_height = HashSet::new(); + .filter(|height| block_times.get(height).is_none()) + .take(chunk_size) + .for_each(|height| { + needs_block_height.insert(height); + }); - 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 }; @@ -385,4 +386,116 @@ mod test { assert_eq!(wallet.get_balance().unwrap(), 50_000); } + + #[test] + fn test_electrum_blockchain_factory_sync_with_stop_gaps() { + // Test whether Electrum blockchain 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). + + // 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], + ]; + + let mut test_client = TestClient::default(); + + 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 factory = Arc::new( + ElectrumBlockchain::from_config(&ElectrumBlockchainConfig { + url: test_client.electrsd.electrum_url.clone(), + socks5: None, + retry: 0, + timeout: None, + stop_gap: stop_gap as _, + }) + .unwrap(), + ); + let wallet = Wallet::new( + descriptor.as_str(), + None, + bitcoin::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(); + let tx = testutils! { + @tx ( (@addr address.address) => AMOUNT_PER_TX ) + }; + test_client.receive(tx); + sum + AMOUNT_PER_TX + }); + + // generate blocks to confirm new transactions + test_client.generate(3, None); + + // 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 + factory + .sync_wallet(&wallet, None, 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 + ); + } + } } From 612da165f8cfbc2b0aee0a37b2cdc44d6da017c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Mon, 4 Jul 2022 20:37:21 +0800 Subject: [PATCH 2/2] `Blockchain` stop_gap testing improvements This is a continuation of the #651 fix. We should also check whether the same bug affects esplora as noted by @afilini. To achieve this, I've introduced a `ConfigurableBlockchainTester` trait that can test multiple blockchain implementations. * Introduce `ConfigurableBlockchainTester` trait. * Use the aforementioned trait to also test esplora. * Change the electrum test to also use the new trait. * Fix some complaints by clippy in ureq.rs file (why is CI not seeing this?). * Refactor some code. --- src/blockchain/electrum.rs | 135 +++-------------- src/blockchain/esplora/mod.rs | 34 +++++ src/blockchain/esplora/ureq.rs | 2 +- .../configurable_blockchain_tests.rs | 140 ++++++++++++++++++ src/testutils/mod.rs | 4 + 5 files changed, 200 insertions(+), 115 deletions(-) create mode 100644 src/testutils/configurable_blockchain_tests.rs diff --git a/src/blockchain/electrum.rs b/src/blockchain/electrum.rs index 784218e7..b71390cb 100644 --- a/src/blockchain/electrum.rs +++ b/src/blockchain/electrum.rs @@ -147,19 +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 mut needs_block_height = HashSet::with_capacity(chunk_size); - conftime_req - .request() - .filter_map(|txid| txid_to_height.get(txid).cloned()) - .filter(|height| block_times.get(height).is_none()) - .take(chunk_size) - .for_each(|height| { - needs_block_height.insert(height); - }); - - needs_block_height - }; + let needs_block_height = conftime_req + .request() + .filter_map(|txid| txid_to_height.get(txid).cloned()) + .filter(|height| block_times.get(height).is_none()) + .take(chunk_size) + .collect::>(); let new_block_headers = self .client @@ -329,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! { @@ -388,114 +382,27 @@ mod test { } #[test] - fn test_electrum_blockchain_factory_sync_with_stop_gaps() { - // Test whether Electrum blockchain 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_electrum_with_variable_configs() { + struct ElectrumTester; - // Generates wallet descriptor - let descriptor_of_account = |account_index: usize| -> String { - format!("wpkh([c258d2e4/84h/1h/0h]tpubDDYkZojQFQjht8Tm4jsS3iuEmKjTiEGjG6KnuFNKKJb5A6ZUCUZKdvLdSDWofKi4ToRCwb9poe1XdqfUnP4jaJjCB2Zwv11ZLgSbnZSNecE/{account_index}/*)") - }; + impl ConfigurableBlockchainTester for ElectrumTester { + const BLOCKCHAIN_NAME: &'static str = "Electrum"; - // 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], - ]; - - let mut test_client = TestClient::default(); - - 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 factory = Arc::new( - ElectrumBlockchain::from_config(&ElectrumBlockchainConfig { + fn config_with_stop_gap( + &self, + test_client: &mut TestClient, + stop_gap: usize, + ) -> Option { + Some(ElectrumBlockchainConfig { url: test_client.electrsd.electrum_url.clone(), socks5: None, retry: 0, timeout: None, - stop_gap: stop_gap as _, + stop_gap: stop_gap, }) - .unwrap(), - ); - let wallet = Wallet::new( - descriptor.as_str(), - None, - bitcoin::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(); - let tx = testutils! { - @tx ( (@addr address.address) => AMOUNT_PER_TX ) - }; - test_client.receive(tx); - sum + AMOUNT_PER_TX - }); - - // generate blocks to confirm new transactions - test_client.generate(3, None); - - // 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 - factory - .sync_wallet(&wallet, None, 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 - ); + } } + + ElectrumTester.run(); } } diff --git a/src/blockchain/esplora/mod.rs b/src/blockchain/esplora/mod.rs index 83c5c3dd..30d29d64 100644 --- a/src/blockchain/esplora/mod.rs +++ b/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 for EsploraTester { + const BLOCKCHAIN_NAME: &'static str = "Esplora"; + + fn config_with_stop_gap( + &self, + test_client: &mut TestClient, + stop_gap: usize, + ) -> Option { + 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(); + } } diff --git a/src/blockchain/esplora/ureq.rs b/src/blockchain/esplora/ureq.rs index 50493f9c..7ce57a13 100644 --- a/src/blockchain/esplora/ureq.rs +++ b/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(()) } diff --git a/src/testutils/configurable_blockchain_tests.rs b/src/testutils/configurable_blockchain_tests.rs new file mode 100644 index 00000000..a39608bb --- /dev/null +++ b/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: 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 { + 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(test_client: &mut TestClient, tester: &T) +where + T: ConfigurableBlockchainTester, + 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); + } +} diff --git a/src/testutils/mod.rs b/src/testutils/mod.rs index 3af47849..82949ecc 100644 --- a/src/testutils/mod.rs +++ b/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)]