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]