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.

diff --git a/CHANGELOG.md b/CHANGELOG.md
index d1cd281c08a1918bf5e3188f5a9b8c57db1fd310..c28ca2be546e5bf7f90c2b3998af04886866172d 100644 (file)
--- 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 9ba6f0f33221a7bb17f08443681a22460f6cf1ad..1d0d884c09b4d8558128c987b718cdb5c6f6aba5 100644 (file)
--- 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 ac78188e05651c60c9a3f56716a6acbd63a1bfd0..0575273608c0d78a99cf0e6375b0ce17ffa5be12 100644 (file)
--- 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<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!(

diff --git a/src/testutils/blockchain_tests.rs b/src/testutils/blockchain_tests.rs
index 2ae546324b9215205a4c9f2fed005cf37e3e7849..7c08699c67c4f2eafb54f0df8be740ec21d6249c 100644 (file)
--- 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]