From b4306e3c8db6cbaedc8845c6d21c750b39f682bf Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 20 Nov 2023 18:04:57 -0300 Subject: [PATCH 1/5] refactor: rename FirstKeyTimeChanged to MaybeUpdateBirthTime In the following-up commit, the wallet birth time will also be modified by the transactions scanning process. When a tx older than all descriptor's timestamp is detected. --- src/wallet/wallet.cpp | 12 +++++++----- src/wallet/wallet.h | 4 ++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ecf18fbe783..625695faf7a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1747,11 +1747,11 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set 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 - FirstKeyTimeChanged(spkm.get(), spkm->GetTimeFirstKey()); + MaybeUpdateBirthTime(spkm->GetTimeFirstKey()); } void CWallet::SetupLegacyScriptPubKeyMan() @@ -3516,7 +3518,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::FirstKeyTimeChanged, this, std::placeholders::_1, std::placeholders::_2)); + 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 bc45010200d..a1bf3784c95 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -685,8 +685,8 @@ public: 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 'new_birth_time' is below it */ - void FirstKeyTimeChanged(const ScriptPubKeyMan* spkm, int64_t new_birth_time); + /** 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}; From 75fbf444c1e13c6ba0e79a34871534c845a13849 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 20 Nov 2023 12:41:50 -0300 Subject: [PATCH 2/5] wallet: birth time update during tx scanning As the user could have imported a descriptor with a newer timestamp (by blindly setting 'timestamp=now'), the wallet needs to update the birth time when it detects a transaction older than the oldest descriptor timestamp. --- src/wallet/wallet.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 625695faf7a..2dd659c4933 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1080,6 +1080,9 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx)); wtx.nTimeSmart = ComputeTimeSmart(wtx, rescanning_old_block); AddToSpends(wtx, &batch); + + // Update birth time when tx time is older than it. + MaybeUpdateBirthTime(wtx.GetTxTime()); } if (!fInsertedNew) @@ -1199,6 +1202,10 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx } } } + + // Update birth time when tx time is older than it. + MaybeUpdateBirthTime(wtx.GetTxTime()); + return true; } @@ -3087,7 +3094,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri int64_t time = spk_man->GetTimeFirstKey(); if (!time_first_key || time < *time_first_key) time_first_key = time; } - if (time_first_key) walletInstance->m_birth_time = *time_first_key; + if (time_first_key) walletInstance->MaybeUpdateBirthTime(*time_first_key); if (chain && !AttachChain(walletInstance, *chain, rescan_required, error, warnings)) { return nullptr; From 6f497377aa17cb8a590fd7717fa8ededf4249999 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 28 Nov 2023 17:31:45 -0300 Subject: [PATCH 3/5] wallet: fix legacy spkm default birth time To avoid scanning blocks, as assumed by a wallet with no generated keys or imported scripts, the default value for the birth time needs to be set to the maximum int64_t value. Once the first key is generated or the first script is imported, the legacy SPKM will update the birth time automatically. --- src/wallet/scriptpubkeyman.cpp | 2 +- src/wallet/scriptpubkeyman.h | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index d2b2801aa88..fcf87fd2a70 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -709,7 +709,7 @@ 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) { + } else if (nTimeFirstKey == UNKNOWN_TIME || nCreateTime < nTimeFirstKey) { nTimeFirstKey = nCreateTime; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 7c0eca1475e..7231486df09 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -51,6 +51,9 @@ public: virtual bool IsLocked() const = 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; @@ -286,7 +289,8 @@ private: WatchOnlySet setWatchOnly GUARDED_BY(cs_KeyStore); WatchKeyMap mapWatchKeys GUARDED_BY(cs_KeyStore); - int64_t nTimeFirstKey GUARDED_BY(cs_KeyStore) = 0; + // By default, do not scan any block until keys/scripts are generated/imported + int64_t nTimeFirstKey GUARDED_BY(cs_KeyStore) = UNKNOWN_TIME; //! Number of pre-generated keys/scripts (part of the look-ahead process, used to detect payments) int64_t m_keypool_size GUARDED_BY(cs_KeyStore){DEFAULT_KEYPOOL_SIZE}; From 83c66444d0604f0a9ec3bc3f89d4f1a810b7cda0 Mon Sep 17 00:00:00 2001 From: furszy Date: Mon, 20 Nov 2023 12:39:57 -0300 Subject: [PATCH 4/5] test: coverage for wallet birth time interaction with -reindex Verifying 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'. --- test/functional/test_runner.py | 2 + test/functional/wallet_reindex.py | 87 +++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100755 test/functional/wallet_reindex.py diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 35c662b0d98..287c917a075 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -206,6 +206,8 @@ BASE_SCRIPTS = [ 'wallet_createwallet.py --descriptors', 'wallet_watchonly.py --legacy-wallet', 'wallet_watchonly.py --usecli --legacy-wallet', + 'wallet_reindex.py --legacy-wallet', + 'wallet_reindex.py --descriptors', 'wallet_reorgsrestore.py', 'wallet_conflicts.py --legacy-wallet', 'wallet_conflicts.py --descriptors', diff --git a/test/functional/wallet_reindex.py b/test/functional/wallet_reindex.py new file mode 100755 index 00000000000..95d5fd85ad6 --- /dev/null +++ b/test/functional/wallet_reindex.py @@ -0,0 +1,87 @@ +#!/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, blank=True, load_on_startup=True) + wallet_watch_only = node.get_wallet_rpc('watch_only') + + # 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) + + # 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') + assert_equal(wallet_watch_only.gettransaction(tx_id)['confirmations'], 50) + + 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 1ce45baed7dd2da3f1cb85c9c25110e5537451ae Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 21 Nov 2023 21:48:57 -0300 Subject: [PATCH 5/5] rpc: getwalletinfo, return wallet 'birthtime' And add coverage for it --- src/wallet/rpc/wallet.cpp | 4 ++++ src/wallet/wallet.h | 3 +++ test/functional/wallet_reindex.py | 25 +++++++++++++++++++++++-- 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 164ce9afedb..c635093344e 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -69,6 +69,7 @@ static RPCHelpMan getwalletinfo() {RPCResult::Type::BOOL, "descriptors", "whether this wallet uses descriptors for scriptPubKey management"}, {RPCResult::Type::BOOL, "external_signer", "whether this wallet is configured to use an external signer such as a hardware wallet"}, {RPCResult::Type::BOOL, "blank", "Whether this wallet intentionally does not contain any keys, scripts, or descriptors"}, + {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."}, RESULT_LAST_PROCESSED_BLOCK, }}, }, @@ -132,6 +133,9 @@ static RPCHelpMan getwalletinfo() obj.pushKV("descriptors", pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); obj.pushKV("external_signer", pwallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)); obj.pushKV("blank", pwallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)); + if (int64_t birthtime = pwallet->GetBirthTime(); birthtime != UNKNOWN_TIME) { + obj.pushKV("birthtime", birthtime); + } AppendLastProcessedBlock(obj, *pwallet); return obj; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index a1bf3784c95..3fac98c898a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -884,6 +884,9 @@ public: /* 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; + /* 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; } + /** * Blocks until the wallet state is up-to-date to /at least/ the current * chain at the time this function is entered diff --git a/test/functional/wallet_reindex.py b/test/functional/wallet_reindex.py index 95d5fd85ad6..5388de4b717 100755 --- a/test/functional/wallet_reindex.py +++ b/test/functional/wallet_reindex.py @@ -44,8 +44,10 @@ class WalletReindexTest(BitcoinTestFramework): 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, blank=True, load_on_startup=True) + 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. @@ -53,6 +55,16 @@ class WalletReindexTest(BitcoinTestFramework): 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) @@ -65,7 +77,16 @@ class WalletReindexTest(BitcoinTestFramework): # Verify the transaction is still 'confirmed' after reindex wallet_watch_only = node.get_wallet_rpc('watch_only') - assert_equal(wallet_watch_only.gettransaction(tx_id)['confirmations'], 50) + 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()