diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f1c9add..cfcaa4f6 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..b71390cb 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. @@ -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 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()); - let mut needs_block_height = HashSet::new(); - - 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 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 @@ -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 for ElectrumTester { + const BLOCKCHAIN_NAME: &'static str = "Electrum"; + + 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, + }) + } + } + + 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)]