From 740a23bf295ca7af2aacb859a76d93ba2d583632 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 14 Dec 2023 16:13:34 -0500 Subject: [PATCH 1/2] Merge bitcoin/bitcoin#28920: wallet: birth time update during tx scanning 1ce45baed7dd2da3f1cb85c9c25110e5537451ae rpc: getwalletinfo, return wallet 'birthtime' (furszy) 83c66444d0604f0a9ec3bc3f89d4f1a810b7cda0 test: coverage for wallet birth time interaction with -reindex (furszy) 6f497377aa17cb8a590fd7717fa8ededf4249999 wallet: fix legacy spkm default birth time (furszy) 75fbf444c1e13c6ba0e79a34871534c845a13849 wallet: birth time update during tx scanning (furszy) b4306e3c8db6cbaedc8845c6d21c750b39f682bf refactor: rename FirstKeyTimeChanged to MaybeUpdateBirthTime (furszy) Pull request description: Fixing #28897. As the user may have imported a descriptor with a timestamp newer than the actual birth time of the first key (by setting 'timestamp=now'), the wallet needs to update the birth time when it detects a transaction older than the oldest descriptor timestamp. Testing Notes: Can cherry-pick the test commit on top of master. It will fail there. ACKs for top commit: Sjors: re-utACK 1ce45baed7dd2da3f1cb85c9c25110e5537451ae achow101: ACK 1ce45baed7dd2da3f1cb85c9c25110e5537451ae Tree-SHA512: 10c2382f87356ae9ea3fcb637d7edc5ed0e51e13cc2729c314c9ffb57c684b9ac3c4b757b85810c0a674020b7287c43d3be8273bcf75e2aff0cc1c037f1159f9 (cherry picked from commit 08e6aaabef6c569ba634c86d396f0a840002a8d2) --- src/wallet/rpc/wallet.cpp | 20 ++++++ src/wallet/scriptpubkeyman.cpp | 4 +- src/wallet/scriptpubkeyman.h | 9 ++- src/wallet/wallet.cpp | 32 +++++++++ src/wallet/wallet.h | 10 +++ test/functional/test_runner.py | 2 + test/functional/wallet_reindex.py | 108 ++++++++++++++++++++++++++++++ 7 files changed, 183 insertions(+), 2 deletions(-) create mode 100755 test/functional/wallet_reindex.py diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 62c800c27a37..e4f56b40683f 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -191,7 +191,24 @@ static RPCHelpMan getwalletinfo() {RPCResult::Type::NUM, "progress", "scanning progress percentage [0.0, 1.0]"}, }}, {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"}, + {RPCResult::Type::NUM_TIME, "birthtime", /*optional=*/true, "The start time for blocks scanning. It could be modified by (re)importing any descriptor with an earlier timestamp."}, }, + {RPCResult::Type::STR_AMOUNT, "immature_balance", "DEPRECATED. Identical to getbalances().mine.immature"}, + {RPCResult::Type::NUM, "txcount", "the total number of transactions in the wallet"}, + {RPCResult::Type::NUM_TIME, "keypoololdest", /*optional=*/true, "the " + UNIX_EPOCH_TIME + " of the oldest pre-generated key in the key pool. Legacy wallets only."}, + {RPCResult::Type::NUM, "keypoolsize", "how many new keys are pre-generated (only counts external keys)"}, + {RPCResult::Type::NUM, "keypoolsize_hd_internal", /*optional=*/true, "how many new keys are pre-generated for internal use (used for change outputs, only appears if the wallet is using this feature, otherwise external keys are used)"}, + {RPCResult::Type::NUM_TIME, "unlocked_until", /*optional=*/true, "the " + UNIX_EPOCH_TIME + " until which the wallet is unlocked for transfers, or 0 if the wallet is locked (only present for passphrase-encrypted wallets)"}, + {RPCResult::Type::STR_AMOUNT, "paytxfee", "the transaction fee configuration, set in " + CURRENCY_UNIT + "/kvB"}, + {RPCResult::Type::STR_HEX, "hdseedid", /*optional=*/true, "the Hash160 of the HD seed (only present when HD is enabled)"}, + {RPCResult::Type::BOOL, "private_keys_enabled", "false if privatekeys are disabled for this wallet (enforced watch-only wallet)"}, + {RPCResult::Type::BOOL, "avoid_reuse", "whether this wallet tracks clean/dirty coins in terms of reuse"}, + {RPCResult::Type::OBJ, "scanning", "current scanning details, or false if no scan is in progress", + { + {RPCResult::Type::NUM, "duration", "elapsed seconds since scan start"}, + {RPCResult::Type::NUM, "progress", "scanning progress percentage [0.0, 1.0]"}, + }, /*skip_type_check=*/true}, + {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"}, }, RPCExamples{ HelpExampleCli("getwalletinfo", "") @@ -268,6 +285,9 @@ static RPCHelpMan getwalletinfo() obj.pushKV("scanning", false); } obj.pushKV("descriptors", pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); + if (int64_t birthtime = pwallet->GetBirthTime(); birthtime != UNKNOWN_TIME) { + obj.pushKV("birthtime", birthtime); + } return obj; }, }; diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index d8ef16cab882..bcbcdf782b59 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -818,8 +818,10 @@ void LegacyScriptPubKeyMan::UpdateTimeFirstKey(int64_t nCreateTime) // Cannot determine birthday information, so set the wallet birthday to // the beginning of time. nTimeFirstKey = 1; - } else if (!nTimeFirstKey || nCreateTime < nTimeFirstKey) { + NotifyFirstKeyTimeChanged(this, nTimeFirstKey); + } else if (nTimeFirstKey == UNKNOWN_TIME || nCreateTime < nTimeFirstKey) { nTimeFirstKey = nCreateTime; + NotifyFirstKeyTimeChanged(this, nTimeFirstKey); } } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 6b936e833e2c..b09793205f88 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -53,6 +53,9 @@ class WalletStorage virtual void KeepDestinationCallback(bool erased) = 0; }; +//! Constant representing an unknown spkm creation time +static constexpr int64_t UNKNOWN_TIME = std::numeric_limits::max(); + //! Default for -keypool static const unsigned int DEFAULT_KEYPOOL_SIZE = 1000; @@ -230,6 +233,9 @@ class ScriptPubKeyMan /** Keypool has new keys */ boost::signals2::signal NotifyCanGetAddressesChanged; + + /** Birth time changed */ + boost::signals2::signal NotifyFirstKeyTimeChanged; }; class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider @@ -252,7 +258,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv WatchKeyMap mapWatchKeys GUARDED_BY(cs_KeyStore); HDPubKeyMap mapHdPubKeys GUARDED_BY(cs_KeyStore); /// CWallet::Create(WalletContext& context, const std::stri // Try to top up keypool. No-op if the wallet is locked. walletInstance->TopUpKeyPool(); + // Cache the first key time + std::optional time_first_key; + for (auto spk_man : walletInstance->GetAllScriptPubKeyMans()) { + int64_t time = spk_man->GetTimeFirstKey(); + if (!time_first_key || time < *time_first_key) time_first_key = time; + } + if (time_first_key) walletInstance->MaybeUpdateBirthTime(*time_first_key); + if (chain && !AttachChain(walletInstance, *chain, error, warnings)) { return nullptr; } @@ -3722,6 +3744,15 @@ LegacyScriptPubKeyMan* CWallet::GetOrCreateLegacyScriptPubKeyMan() return GetLegacyScriptPubKeyMan(); } +void CWallet::AddScriptPubKeyMan(const uint256& id, std::unique_ptr spkm_man) +{ + // Add spkm_man to m_spk_managers before calling any method + // that might access it. + const auto& spkm = m_spk_managers[id] = std::move(spkm_man); + + // Update birth time if needed + MaybeUpdateBirthTime(spkm->GetTimeFirstKey()); +} void CWallet::SetupLegacyScriptPubKeyMan() { if (m_internal_spk_managers || m_external_spk_managers || !m_spk_managers.empty() || IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { @@ -3750,6 +3781,7 @@ void CWallet::ConnectScriptPubKeyManNotifiers() for (const auto& spk_man : GetActiveScriptPubKeyMans()) { spk_man->NotifyWatchonlyChanged.connect(NotifyWatchonlyChanged); spk_man->NotifyCanGetAddressesChanged.connect(NotifyCanGetAddressesChanged); + spk_man->NotifyFirstKeyTimeChanged.connect(std::bind(&CWallet::MaybeUpdateBirthTime, this, std::placeholders::_2)); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 00fc23ee1446..379406c1f5d9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -279,6 +279,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati // Local time that the tip block was received. Used to schedule wallet rebroadcasts. std::atomic m_best_block_time {0}; + // Wallet birth time. Uses std::numeric_limits::max() for not known. + std::atomic m_birth_time{std::numeric_limits::max()}; + mutable bool fAnonymizableTallyCached = false; mutable std::vector vecAnonymizableTallyCached; mutable bool fAnonymizableTallyCachedNonDenom = false; @@ -725,6 +728,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool ImportPubKeys(const std::vector& ordered_pubkeys, const std::map& pubkey_map, const std::map>& key_origins, const bool add_keypool, const bool internal, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool ImportScriptPubKeys(const std::string& label, const std::set& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Updates wallet birth time if 'time' is below it */ + void MaybeUpdateBirthTime(int64_t time); CFeeRate m_pay_tx_fee{DEFAULT_PAY_TX_FEE}; unsigned int m_confirm_target{DEFAULT_TX_CONFIRM_TARGET}; /** Allow Coin Selection to pick unconfirmed UTXOs that were sent from our own wallet if it @@ -922,6 +927,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati /* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */ bool CanGetAddresses(bool internal = false) const; +<<<<<<< HEAD void notifyTransactionLock(const CTransactionRef &tx, const std::shared_ptr& islock) override; void notifyChainLock(const CBlockIndex* pindexChainLock, const std::shared_ptr& clsig) override; @@ -931,6 +937,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool WriteGovernanceObject(const Governance::Object& obj) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** Returns a vector containing pointers to the governance objects in m_gobjects */ std::vector GetGovernanceObjects() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); +======= + /* Returns the time of the first created key or, in case of an import, it could be the time of the first received transaction */ + int64_t GetBirthTime() const { return m_birth_time; } +>>>>>>> 08e6aaabef (Merge bitcoin/bitcoin#28920: wallet: birth time update during tx scanning) /** * Blocks until the wallet state is up-to-date to /at least/ the current diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 780d2721fd67..a2a91602dbaa 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -193,6 +193,8 @@ 'wallet_listtransactions.py --descriptors', 'wallet_watchonly.py --legacy-wallet', 'wallet_watchonly.py --usecli --legacy-wallet', + 'wallet_reindex.py --legacy-wallet', + 'wallet_reindex.py --descriptors', 'interface_http.py', 'interface_rpc.py', 'interface_usdt_coinselection.py', diff --git a/test/functional/wallet_reindex.py b/test/functional/wallet_reindex.py new file mode 100755 index 000000000000..5388de4b7171 --- /dev/null +++ b/test/functional/wallet_reindex.py @@ -0,0 +1,108 @@ +#!/usr/bin/env python3 +# Copyright (c) 2023-present The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or https://www.opensource.org/licenses/mit-license.php. + +"""Test wallet-reindex interaction""" + +import time + +from test_framework.blocktools import COINBASE_MATURITY +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, +) +BLOCK_TIME = 60 * 10 + +class WalletReindexTest(BitcoinTestFramework): + def add_options(self, parser): + self.add_wallet_options(parser) + + def set_test_params(self): + self.num_nodes = 1 + self.setup_clean_chain = True + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + def advance_time(self, node, secs): + self.node_time += secs + node.setmocktime(self.node_time) + + # Verify the wallet updates the birth time accordingly when it detects a transaction + # with a time older than the oldest descriptor timestamp. + # This could happen when the user blindly imports a descriptor with 'timestamp=now'. + def birthtime_test(self, node, miner_wallet): + self.log.info("Test birth time update during tx scanning") + # Fund address to test + wallet_addr = miner_wallet.getnewaddress() + tx_id = miner_wallet.sendtoaddress(wallet_addr, 2) + + # Generate 50 blocks, one every 10 min to surpass the 2 hours rescan window the wallet has + for _ in range(50): + self.generate(node, 1) + self.advance_time(node, BLOCK_TIME) + + # Now create a new wallet, and import the descriptor + node.createwallet(wallet_name='watch_only', disable_private_keys=True, load_on_startup=True) + wallet_watch_only = node.get_wallet_rpc('watch_only') + # Blank wallets don't have a birth time + assert 'birthtime' not in wallet_watch_only.getwalletinfo() + + # For a descriptors wallet: Import address with timestamp=now. + # For legacy wallet: There is no way of importing a script/address with a custom time. The wallet always imports it with birthtime=1. + # In both cases, disable rescan to not detect the transaction. + wallet_watch_only.importaddress(wallet_addr, rescan=False) + assert_equal(len(wallet_watch_only.listtransactions()), 0) + + # Depending on the wallet type, the birth time changes. + wallet_birthtime = wallet_watch_only.getwalletinfo()['birthtime'] + if self.options.descriptors: + # As blocks were generated every 10 min, the chain MTP timestamp is node_time - 60 min. + assert_equal(self.node_time - BLOCK_TIME * 6, wallet_birthtime) + else: + # No way of importing scripts/addresses with a custom time on a legacy wallet. + # It's always set to the beginning of time. + assert_equal(wallet_birthtime, 1) + + # Rescan the wallet to detect the missing transaction + wallet_watch_only.rescanblockchain() + assert_equal(wallet_watch_only.gettransaction(tx_id)['confirmations'], 50) + assert_equal(wallet_watch_only.getbalances()['mine' if self.options.descriptors else 'watchonly']['trusted'], 2) + + # Reindex and wait for it to finish + with node.assert_debug_log(expected_msgs=["initload thread exit"]): + self.restart_node(0, extra_args=['-reindex=1', f'-mocktime={self.node_time}']) + node.syncwithvalidationinterfacequeue() + + # Verify the transaction is still 'confirmed' after reindex + wallet_watch_only = node.get_wallet_rpc('watch_only') + tx_info = wallet_watch_only.gettransaction(tx_id) + assert_equal(tx_info['confirmations'], 50) + + # Depending on the wallet type, the birth time changes. + if self.options.descriptors: + # For descriptors, verify the wallet updated the birth time to the transaction time + assert_equal(tx_info['time'], wallet_watch_only.getwalletinfo()['birthtime']) + else: + # For legacy, as the birth time was set to the beginning of time, verify it did not change + assert_equal(wallet_birthtime, 1) + + wallet_watch_only.unloadwallet() + + def run_test(self): + node = self.nodes[0] + self.node_time = int(time.time()) + node.setmocktime(self.node_time) + + # Fund miner + node.createwallet(wallet_name='miner', load_on_startup=True) + miner_wallet = node.get_wallet_rpc('miner') + self.generatetoaddress(node, COINBASE_MATURITY + 10, miner_wallet.getnewaddress()) + + # Tests + self.birthtime_test(node, miner_wallet) + + +if __name__ == '__main__': + WalletReindexTest().main() From f0fc2c00732d49b16dcb10b4444caeb2f0ed83c9 Mon Sep 17 00:00:00 2001 From: pasta Date: Thu, 7 Aug 2025 03:12:48 -0500 Subject: [PATCH 2/2] fix: resolve merge conflicts and compatibility issues - Remove duplicated RPC field definitions in src/wallet/rpc/wallet.cpp - Remove incompatible add_wallet_options call in test/functional/wallet_reindex.py Addresses review feedback from CodeRabbit bot while preserving Bitcoin intent. --- src/wallet/rpc/wallet.cpp | 16 ---------------- test/functional/wallet_reindex.py | 2 -- 2 files changed, 18 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index e4f56b40683f..cf00e0408fbf 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -193,22 +193,6 @@ static RPCHelpMan getwalletinfo() {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"}, {RPCResult::Type::NUM_TIME, "birthtime", /*optional=*/true, "The start time for blocks scanning. It could be modified by (re)importing any descriptor with an earlier timestamp."}, }, - {RPCResult::Type::STR_AMOUNT, "immature_balance", "DEPRECATED. Identical to getbalances().mine.immature"}, - {RPCResult::Type::NUM, "txcount", "the total number of transactions in the wallet"}, - {RPCResult::Type::NUM_TIME, "keypoololdest", /*optional=*/true, "the " + UNIX_EPOCH_TIME + " of the oldest pre-generated key in the key pool. Legacy wallets only."}, - {RPCResult::Type::NUM, "keypoolsize", "how many new keys are pre-generated (only counts external keys)"}, - {RPCResult::Type::NUM, "keypoolsize_hd_internal", /*optional=*/true, "how many new keys are pre-generated for internal use (used for change outputs, only appears if the wallet is using this feature, otherwise external keys are used)"}, - {RPCResult::Type::NUM_TIME, "unlocked_until", /*optional=*/true, "the " + UNIX_EPOCH_TIME + " until which the wallet is unlocked for transfers, or 0 if the wallet is locked (only present for passphrase-encrypted wallets)"}, - {RPCResult::Type::STR_AMOUNT, "paytxfee", "the transaction fee configuration, set in " + CURRENCY_UNIT + "/kvB"}, - {RPCResult::Type::STR_HEX, "hdseedid", /*optional=*/true, "the Hash160 of the HD seed (only present when HD is enabled)"}, - {RPCResult::Type::BOOL, "private_keys_enabled", "false if privatekeys are disabled for this wallet (enforced watch-only wallet)"}, - {RPCResult::Type::BOOL, "avoid_reuse", "whether this wallet tracks clean/dirty coins in terms of reuse"}, - {RPCResult::Type::OBJ, "scanning", "current scanning details, or false if no scan is in progress", - { - {RPCResult::Type::NUM, "duration", "elapsed seconds since scan start"}, - {RPCResult::Type::NUM, "progress", "scanning progress percentage [0.0, 1.0]"}, - }, /*skip_type_check=*/true}, - {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"}, }, RPCExamples{ HelpExampleCli("getwalletinfo", "") diff --git a/test/functional/wallet_reindex.py b/test/functional/wallet_reindex.py index 5388de4b7171..e1585e543409 100755 --- a/test/functional/wallet_reindex.py +++ b/test/functional/wallet_reindex.py @@ -15,8 +15,6 @@ BLOCK_TIME = 60 * 10 class WalletReindexTest(BitcoinTestFramework): - def add_options(self, parser): - self.add_wallet_options(parser) def set_test_params(self): self.num_nodes = 1