Merge bitcoindevkit/bdk#657: Release 0.20.0

46c344feb02e2ecfdc065d85cba320d06811bedf Bump version to 0.20.1-dev (Steve Myers)
78d26f6eb3dcccd791a858cfc6c8303b409e9a6e Bump version to 0.20.0 (Steve Myers)
92b9597f8b8dc3694508062b5e7c5f23acbc3a4f Rename `set_current_height` to `current_height` (Alekos Filini)
b5a120c649a17302f3825c45efd9ea0c3b2dd266 Missing newlines (Alekos Filini)
af6bde3997ac2fe4cfdbb691f2af2e802f0ab7c6 Fix: Wallet sync may decrement address index (志宇)
45db468c9be0c8bb2dc5fac52fbd52b6b41b4d53 Deprecate `AddressValidator` (志宇)
01141bed5ab1f73feade4058d9488c7a12f025a8 Update CHANGELOG and lib.rs docs version (Steve Myers)
87e864674367172022feaa6d3cf5e746caa3124c Bump version to 0.20.0-rc.1 (Steve Myers)

Pull request description:

  Proposed tweet:

  📢 Release 0.20.0 is out! Highlights include bug fixes for the ElectrumBlockchain and descriptor templates, discourage fee sniping in tx building, and new tx signing options. A big thanks to our past and latest new contributors. For all changes see: https://github.com/bitcoindevkit/bdk/releases/tag/v0.20.0

ACKs for top commit:
  afilini:
    ACK 46c344feb02e2ecfdc065d85cba320d06811bedf

Tree-SHA512: 7c36a85611f715d76a37d5a285bc72f1a06297fc06b85cca7e38c3350fcbc0a3e35d38ce617a82d191538776aa49362e523beef70bbe3b93b21d8d28d774b75f

CHANGELOG.md

@@ -5,11 +5,18 @@ 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`
+- 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]
 
@@ -26,7 +33,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 +478,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

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "bdk"
-version = "0.19.1-dev"
+version = "0.20.1-dev"
 edition = "2018"
 authors = ["Alekos Filini <alekos.filini@gmail.com>", "Riccardo Casatta <riccardo@casatta.it>"]
 homepage = "https://bitcoindevkit.org"

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)?;

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:{:?}",

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<Txid> = 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::<Result<Vec<(KeychainKind, u32)>, _>>()?;
+
         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!(

src/lib.rs

@@ -44,7 +44,7 @@
 //! interact with the bitcoin P2P network.
 //!
 //! ```toml
-//! bdk = "0.18.0"
+//! bdk = "0.20.0"
 //! ```
 //!
 //! # Examples

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]

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);

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<D> {
     signers: Arc<SignersContainer>,
     change_signers: Arc<SignersContainer>,
 
+    #[allow(deprecated)]
     address_validators: Vec<Arc<dyn AddressValidator>>,
 
     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<dyn AddressValidator>) {
         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<dyn AddressValidator>] {
         &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)?;
         }
 
@@ -2058,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
@@ -2106,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();
 
@@ -4907,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 {
@@ -4920,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 {
@@ -4933,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();
     }
 }

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.

src/wallet/tx_builder.rs

@@ -556,7 +556,7 @@ impl<'a, D: BatchDatabase, Cs: CoinSelectionAlgorithm<D>, 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
     }