Merge bitcoin#28920: wallet: birth time update during tx scanning

1ce45ba rpc: getwalletinfo, return wallet 'birthtime' (furszy)
83c6644 test: coverage for wallet birth time interaction with -reindex (furszy)
6f49737 wallet: fix legacy spkm default birth time (furszy)
75fbf44 wallet: birth time update during tx scanning (furszy)
b4306e3 refactor: rename FirstKeyTimeChanged to MaybeUpdateBirthTime (furszy)

Pull request description:

  Fixing bitcoin#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 1ce45ba
  achow101:
    ACK 1ce45ba

Tree-SHA512: 10c2382f87356ae9ea3fcb637d7edc5ed0e51e13cc2729c314c9ffb57c684b9ac3c4b757b85810c0a674020b7287c43d3be8273bcf75e2aff0cc1c037f1159f9

Author: achow101

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;

src/wallet/scriptpubkeyman.cpp

@@ -710,7 +710,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;
     }
 

src/wallet/scriptpubkeyman.h

@@ -51,6 +51,9 @@ class WalletStorage
     virtual bool IsLocked() const = 0;
 };
 
+//! Constant representing an unknown spkm creation time
+static constexpr int64_t UNKNOWN_TIME = std::numeric_limits<int64_t>::max();
+
 //! Default for -keypool
 static const unsigned int DEFAULT_KEYPOOL_SIZE = 1000;
 
@@ -286,7 +289,8 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
     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};

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;
 }
 
@@ -1747,11 +1754,11 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
     return true;
 }
 
-void CWallet::FirstKeyTimeChanged(const ScriptPubKeyMan* spkm, int64_t new_birth_time)
+void CWallet::MaybeUpdateBirthTime(int64_t time)
 {
     int64_t birthtime = m_birth_time.load();
-    if (new_birth_time < birthtime) {
-        m_birth_time = new_birth_time;
+    if (time < birthtime) {
+        m_birth_time = time;
     }
 }
 
@@ -3089,7 +3096,7 @@ std::shared_ptr<CWallet> 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;
@@ -3482,10 +3489,12 @@ LegacyScriptPubKeyMan* CWallet::GetOrCreateLegacyScriptPubKeyMan()
 
 void CWallet::AddScriptPubKeyMan(const uint256& id, std::unique_ptr<ScriptPubKeyMan> 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()
@@ -3518,7 +3527,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));
     }
 }
 

src/wallet/wallet.h

@@ -688,8 +688,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     bool ImportPubKeys(const std::vector<CKeyID>& ordered_pubkeys, const std::map<CKeyID, CPubKey>& pubkey_map, const std::map<CKeyID, std::pair<CPubKey, KeyOriginInfo>>& 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<CScript>& 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};
@@ -887,6 +887,9 @@ 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;
 
+    /* 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

test/functional/test_runner.py

@@ -207,6 +207,8 @@
     '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',

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