From 87e864674367172022feaa6d3cf5e746caa3124c Mon Sep 17 00:00:00 2001 From: Steve Myers Date: Wed, 6 Jul 2022 12:52:00 -0700 Subject: [PATCH 1/8] Bump version to 0.20.0-rc.1 --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 49d5359e..36a2cc09 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "bdk" -version = "0.19.1-dev" +version = "0.20.0-rc.1" edition = "2018" authors = ["Alekos Filini ", "Riccardo Casatta "] homepage = "https://bitcoindevkit.org" From 01141bed5ab1f73feade4058d9488c7a12f025a8 Mon Sep 17 00:00:00 2001 From: Steve Myers Date: Wed, 6 Jul 2022 13:19:01 -0700 Subject: [PATCH 2/8] Update CHANGELOG and lib.rs docs version --- CHANGELOG.md | 9 +++++++-- src/lib.rs | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4982ae2..4cff6b17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,11 +5,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] + + +## [v0.20.0] - [v0.19.0] + - 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. - Set coin type in BIP44, BIP49, and BIP84 templates - Get block hash given a block height - A `get_block_hash` method is now defined on the `GetBlockHash` trait and implemented on every blockchain backend. This method expects a block height and returns the corresponding block hash. +- Add `remove_partial_sigs` and `try_finalize` to `SignOptions` ## [v0.19.0] - [v0.18.0] @@ -26,7 +31,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Signing Taproot PSBTs (key spend and script spend) - Support for `tr()` descriptors in the `descriptor!()` macro - Add support for Bitcoin Core 23.0 when using the `rpc` blockchain -- Add `remove_partial_sigs` and `try_finalize` to `SignOptions` ## [v0.18.0] - [v0.17.0] @@ -472,4 +476,5 @@ final transaction is created by calling `finish` on the builder. [v0.17.0]: https://github.com/bitcoindevkit/bdk/compare/v0.16.1...v0.17.0 [v0.18.0]: https://github.com/bitcoindevkit/bdk/compare/v0.17.0...v0.18.0 [v0.19.0]: https://github.com/bitcoindevkit/bdk/compare/v0.18.0...v0.19.0 -[unreleased]: https://github.com/bitcoindevkit/bdk/compare/v0.19.0...HEAD +[v0.20.0]: https://github.com/bitcoindevkit/bdk/compare/v0.19.0...v0.20.0 +[unreleased]: https://github.com/bitcoindevkit/bdk/compare/v0.20.0...HEAD diff --git a/src/lib.rs b/src/lib.rs index 6db7103e..4c4bb3c2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -44,7 +44,7 @@ //! interact with the bitcoin P2P network. //! //! ```toml -//! bdk = "0.18.0" +//! bdk = "0.20.0" //! ``` //! //! # Examples From 45db468c9be0c8bb2dc5fac52fbd52b6b41b4d53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Mon, 11 Jul 2022 16:37:41 +0800 Subject: [PATCH 3/8] Deprecate `AddressValidator` --- CHANGELOG.md | 1 + examples/address_validator.rs | 3 +++ src/wallet/address_validator.rs | 4 ++++ src/wallet/mod.rs | 9 +++++++++ 4 files changed, 17 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cff6b17..d1cd281c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Set coin type in BIP44, BIP49, and BIP84 templates - Get block hash given a block height - A `get_block_hash` method is now defined on the `GetBlockHash` trait and implemented on every blockchain backend. This method expects a block height and returns the corresponding block hash. - Add `remove_partial_sigs` and `try_finalize` to `SignOptions` +- Deprecate `AddressValidator` ## [v0.19.0] - [v0.18.0] diff --git a/examples/address_validator.rs b/examples/address_validator.rs index 85a23560..26c36dfe 100644 --- a/examples/address_validator.rs +++ b/examples/address_validator.rs @@ -14,6 +14,7 @@ use std::sync::Arc; use bdk::bitcoin; use bdk::database::MemoryDatabase; use bdk::descriptor::HdKeyPaths; +#[allow(deprecated)] use bdk::wallet::address_validator::{AddressValidator, AddressValidatorError}; use bdk::KeychainKind; use bdk::Wallet; @@ -25,6 +26,7 @@ use bitcoin::{Network, Script}; #[derive(Debug)] struct DummyValidator; +#[allow(deprecated)] impl AddressValidator for DummyValidator { fn validate( &self, @@ -50,6 +52,7 @@ fn main() -> Result<(), bdk::Error> { let descriptor = "sh(and_v(v:pk(tpubDDpWvmUrPZrhSPmUzCMBHffvC3HyMAPnWDSAQNBTnj1iZeJa7BZQEttFiP4DS4GCcXQHezdXhn86Hj6LHX5EDstXPWrMaSneRWM8yUf6NFd/*),after(630000)))"; let mut wallet = Wallet::new(descriptor, None, Network::Regtest, MemoryDatabase::new())?; + #[allow(deprecated)] wallet.add_address_validator(Arc::new(DummyValidator)); wallet.get_address(New)?; diff --git a/src/wallet/address_validator.rs b/src/wallet/address_validator.rs index a0e418ab..eaac582c 100644 --- a/src/wallet/address_validator.rs +++ b/src/wallet/address_validator.rs @@ -100,6 +100,7 @@ impl std::error::Error for AddressValidatorError {} /// validator will be propagated up to the original caller that triggered the address generation. /// /// For a usage example see [this module](crate::address_validator)'s documentation. +#[deprecated = "AddressValidator was rarely used. Address validation can occur outside of BDK"] pub trait AddressValidator: Send + Sync + fmt::Debug { /// Validate or inspect an address fn validate( @@ -120,6 +121,7 @@ mod test { #[derive(Debug)] struct TestValidator; + #[allow(deprecated)] impl AddressValidator for TestValidator { fn validate( &self, @@ -135,6 +137,7 @@ mod test { #[should_panic(expected = "InvalidScript")] fn test_address_validator_external() { let (mut wallet, _, _) = get_funded_wallet(get_test_wpkh()); + #[allow(deprecated)] wallet.add_address_validator(Arc::new(TestValidator)); wallet.get_address(New).unwrap(); @@ -144,6 +147,7 @@ mod test { #[should_panic(expected = "InvalidScript")] fn test_address_validator_internal() { let (mut wallet, descriptors, _) = get_funded_wallet(get_test_wpkh()); + #[allow(deprecated)] wallet.add_address_validator(Arc::new(TestValidator)); let addr = crate::testutils!(@external descriptors, 10); diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index cab3b020..dad10657 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -50,6 +50,7 @@ pub mod verify; pub use utils::IsDust; +#[allow(deprecated)] use address_validator::AddressValidator; use coin_selection::DefaultCoinSelectionAlgorithm; use signer::{SignOptions, SignerOrdering, SignersContainer, TransactionSigner}; @@ -94,6 +95,7 @@ pub struct Wallet { signers: Arc, change_signers: Arc, + #[allow(deprecated)] address_validators: Vec>, network: Network, @@ -500,11 +502,17 @@ where /// Add an address validator /// /// See [the `address_validator` module](address_validator) for an example. + #[deprecated] + #[allow(deprecated)] pub fn add_address_validator(&mut self, validator: Arc) { self.address_validators.push(validator); } /// Get the address validators + /// + /// See [the `address_validator` module](address_validator). + #[deprecated] + #[allow(deprecated)] pub fn get_address_validators(&self) -> &[Arc] { &self.address_validators } @@ -1267,6 +1275,7 @@ where let script = derived_descriptor.script_pubkey(); for validator in &self.address_validators { + #[allow(deprecated)] validator.validate(keychain, &hd_keypaths, &script)?; } From af6bde3997ac2fe4cfdbb691f2af2e802f0ab7c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Sun, 3 Jul 2022 14:32:05 +0800 Subject: [PATCH 4/8] Fix: Wallet sync may decrement address index This bug seems to be Electrum-specific. The fix is to check the proposed changes against the current state of the database. Ensure newly suggested indexes are not smaller than indexes already in database. Changes: * Check index updates before they are applied to database during Electrum Blockchain sync (Thank you @rajarshimaitra for providing an elegant solution). Tests added: * bdk_blockchain_tests!::test_sync_address_index_should_not_decrement * bdk_blockchain_tests!::test_sync_address_index_should_increment These tests ensure there will be no unexpected address reuse when grabbing a new address via `Wallet::get_address` with `AddressIndex::New`. Other changes: * Tweak `rpc.rs` so that clippy is happy. --- CHANGELOG.md | 1 + src/blockchain/rpc.rs | 2 +- src/blockchain/script_sync.rs | 22 +++++++++++-- src/testutils/blockchain_tests.rs | 55 +++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1cd281c..c28ca2be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Get block hash given a block height - A `get_block_hash` method is now defined on the `GetBlockHash` trait and implemented on every blockchain backend. This method expects a block height and returns the corresponding block hash. - Add `remove_partial_sigs` and `try_finalize` to `SignOptions` - Deprecate `AddressValidator` +- Fix Electrum wallet sync potentially causing address index decrement - compare proposed index and current index before applying batch operations during sync. ## [v0.19.0] - [v0.18.0] diff --git a/src/blockchain/rpc.rs b/src/blockchain/rpc.rs index 9ba6f0f3..1d0d884c 100644 --- a/src/blockchain/rpc.rs +++ b/src/blockchain/rpc.rs @@ -340,7 +340,7 @@ impl WalletSync for RpcBlockchain { ), received, sent, - fee: tx_result.fee.map(|f| f.as_sat().abs() as u64), + fee: tx_result.fee.map(|f| f.as_sat().unsigned_abs()), }; debug!( "saving tx: {} tx_result.fee:{:?} td.fees:{:?}", diff --git a/src/blockchain/script_sync.rs b/src/blockchain/script_sync.rs index ac78188e..05752736 100644 --- a/src/blockchain/script_sync.rs +++ b/src/blockchain/script_sync.rs @@ -314,6 +314,22 @@ impl<'a, D: BatchDatabase> State<'a, D> { let finished_txs = make_txs_consistent(&self.finished_txs); let observed_txids: HashSet = finished_txs.iter().map(|tx| tx.txid).collect(); let txids_to_delete = existing_txids.difference(&observed_txids); + + // Ensure `last_active_index` does not decrement database's current state. + let index_updates = self + .last_active_index + .iter() + .map(|(keychain, sync_index)| { + let sync_index = *sync_index as u32; + let index_res = match self.db.get_last_index(*keychain) { + Ok(Some(db_index)) => Ok(std::cmp::max(db_index, sync_index)), + Ok(None) => Ok(sync_index), + Err(err) => Err(err), + }; + index_res.map(|index| (*keychain, index)) + }) + .collect::, _>>()?; + let mut batch = self.db.begin_batch(); // Delete old txs that no longer exist @@ -377,8 +393,10 @@ impl<'a, D: BatchDatabase> State<'a, D> { batch.set_tx(finished_tx)?; } - for (keychain, last_active_index) in self.last_active_index { - batch.set_last_index(keychain, last_active_index as u32)?; + // apply index updates + for (keychain, new_index) in index_updates { + debug!("updating index ({}, {})", keychain.as_byte(), new_index); + batch.set_last_index(keychain, new_index)?; } info!( diff --git a/src/testutils/blockchain_tests.rs b/src/testutils/blockchain_tests.rs index 2ae54632..7c08699c 100644 --- a/src/testutils/blockchain_tests.rs +++ b/src/testutils/blockchain_tests.rs @@ -372,6 +372,7 @@ macro_rules! bdk_blockchain_tests { use $crate::blockchain::Blockchain; use $crate::database::MemoryDatabase; use $crate::types::KeychainKind; + use $crate::wallet::AddressIndex; use $crate::{Wallet, FeeRate, SyncOptions}; use $crate::testutils; @@ -651,6 +652,60 @@ macro_rules! bdk_blockchain_tests { assert_eq!(wallet.list_unspent().unwrap().len(), 1, "incorrect number of unspents"); } + // Syncing wallet should not result in wallet address index to decrement. + // This is critical as we should always ensure to not reuse addresses. + #[test] + fn test_sync_address_index_should_not_decrement() { + let (wallet, blockchain, _descriptors, mut test_client) = init_single_sig(); + + const ADDRS_TO_FUND: u32 = 7; + const ADDRS_TO_IGNORE: u32 = 11; + + let mut first_addr_index: u32 = 0; + + (0..ADDRS_TO_FUND + ADDRS_TO_IGNORE).for_each(|i| { + let new_addr = wallet.get_address(AddressIndex::New).unwrap(); + + if i == 0 { + first_addr_index = new_addr.index; + } + assert_eq!(new_addr.index, i+first_addr_index, "unexpected new address index (before sync)"); + + if i < ADDRS_TO_FUND { + test_client.receive(testutils! { + @tx ((@addr new_addr.address) => 50_000) + }); + } + }); + + wallet.sync(&blockchain, SyncOptions::default()).unwrap(); + + let new_addr = wallet.get_address(AddressIndex::New).unwrap(); + assert_eq!(new_addr.index, ADDRS_TO_FUND+ADDRS_TO_IGNORE+first_addr_index, "unexpected new address index (after sync)"); + } + + // Even if user does not explicitly grab new addresses, the address index should + // increment after sync (if wallet has a balance). + #[test] + fn test_sync_address_index_should_increment() { + let (wallet, blockchain, descriptors, mut test_client) = init_single_sig(); + + const START_FUND: u32 = 4; + const END_FUND: u32 = 20; + + // "secretly" fund wallet via given range + (START_FUND..END_FUND).for_each(|addr_index| { + test_client.receive(testutils! { + @tx ((@external descriptors, addr_index) => 50_000) + }); + }); + + wallet.sync(&blockchain, SyncOptions::default()).unwrap(); + + let address = wallet.get_address(AddressIndex::New).unwrap(); + assert_eq!(address.index, END_FUND, "unexpected new address index (after sync)"); + } + /// Send two conflicting transactions to the same address twice in a row. /// The coins should only be received once! #[test] From 92b9597f8b8dc3694508062b5e7c5f23acbc3a4f Mon Sep 17 00:00:00 2001 From: Alekos Filini Date: Wed, 13 Jul 2022 10:27:38 +0200 Subject: [PATCH 5/8] Rename `set_current_height` to `current_height` Usually we don't have any prefix except for methods that can *add* to a list or replace the list entirely (e.g. `add_recipients` vs `set_recipients`) --- src/wallet/mod.rs | 10 +++++----- src/wallet/tx_builder.rs | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/wallet/mod.rs b/src/wallet/mod.rs index dad10657..9231c3b7 100644 --- a/src/wallet/mod.rs +++ b/src/wallet/mod.rs @@ -2067,7 +2067,7 @@ pub(crate) mod test { .set_sync_time(sync_time) .unwrap(); let current_height = 25; - builder.set_current_height(current_height); + builder.current_height(current_height); let (psbt, _) = builder.finish().unwrap(); // current_height will override the last sync height @@ -2115,7 +2115,7 @@ pub(crate) mod test { let mut builder = wallet.build_tx(); builder .add_recipient(addr.script_pubkey(), 25_000) - .set_current_height(630_001) + .current_height(630_001) .nlocktime(630_000); let (psbt, _) = builder.finish().unwrap(); @@ -4916,7 +4916,7 @@ pub(crate) mod test { let mut builder = wallet.build_tx(); builder .add_recipient(addr.script_pubkey(), balance / 2) - .set_current_height(confirmation_time); + .current_height(confirmation_time); assert!(matches!( builder.finish().unwrap_err(), Error::InsufficientFunds { @@ -4929,7 +4929,7 @@ pub(crate) mod test { let mut builder = wallet.build_tx(); builder .add_recipient(addr.script_pubkey(), balance / 2) - .set_current_height(not_yet_mature_time); + .current_height(not_yet_mature_time); assert!(matches!( builder.finish().unwrap_err(), Error::InsufficientFunds { @@ -4942,7 +4942,7 @@ pub(crate) mod test { let mut builder = wallet.build_tx(); builder .add_recipient(addr.script_pubkey(), balance / 2) - .set_current_height(maturity_time); + .current_height(maturity_time); builder.finish().unwrap(); } } diff --git a/src/wallet/tx_builder.rs b/src/wallet/tx_builder.rs index 315a299a..0fee8aa9 100644 --- a/src/wallet/tx_builder.rs +++ b/src/wallet/tx_builder.rs @@ -556,7 +556,7 @@ impl<'a, D: BatchDatabase, Cs: CoinSelectionAlgorithm, Ctx: TxBuilderContext> /// add them using [`TxBuilder::add_utxos`]. /// /// In both cases, if you don't provide a current height, we use the last sync height. - pub fn set_current_height(&mut self, height: u32) -> &mut Self { + pub fn current_height(&mut self, height: u32) -> &mut Self { self.params.current_height = Some(height); self } From b5a120c649a17302f3825c45efd9ea0c3b2dd266 Mon Sep 17 00:00:00 2001 From: Alekos Filini Date: Wed, 13 Jul 2022 11:13:05 +0200 Subject: [PATCH 6/8] Missing newlines --- src/wallet/signer.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/wallet/signer.rs b/src/wallet/signer.rs index e93618da..cb190bf9 100644 --- a/src/wallet/signer.rs +++ b/src/wallet/signer.rs @@ -667,10 +667,12 @@ pub struct SignOptions { /// /// Defaults to `false` which will only allow signing using `SIGHASH_ALL`. pub allow_all_sighashes: bool, + /// Whether to remove partial_sigs from psbt inputs while finalizing psbt. /// /// Defaults to `true` which will remove partial_sigs after finalizing. pub remove_partial_sigs: bool, + /// Whether to try finalizing psbt input after the inputs are signed. /// /// Defaults to `true` which will try fianlizing psbt after inputs are signed. From 78d26f6eb3dcccd791a858cfc6c8303b409e9a6e Mon Sep 17 00:00:00 2001 From: Steve Myers Date: Wed, 13 Jul 2022 10:55:57 -0700 Subject: [PATCH 7/8] Bump version to 0.20.0 --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 36a2cc09..00d0783d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "bdk" -version = "0.20.0-rc.1" +version = "0.20.0" edition = "2018" authors = ["Alekos Filini ", "Riccardo Casatta "] homepage = "https://bitcoindevkit.org" From 46c344feb02e2ecfdc065d85cba320d06811bedf Mon Sep 17 00:00:00 2001 From: Steve Myers Date: Wed, 13 Jul 2022 11:41:57 -0700 Subject: [PATCH 8/8] Bump version to 0.20.1-dev --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 00d0783d..84171ab5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "bdk" -version = "0.20.0" +version = "0.20.1-dev" edition = "2018" authors = ["Alekos Filini ", "Riccardo Casatta "] homepage = "https://bitcoindevkit.org"