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

src/wallet/rpc/wallet.cpp

@@ -191,6 +191,7 @@ 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."},
                         },
                 },
                 RPCExamples{
@@ -268,6 +269,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;
 },
     };

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);
     }
 }
 

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<int64_t>::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<void ()> NotifyCanGetAddressesChanged;
+
+    /** Birth time changed */
+    boost::signals2::signal<void (const ScriptPubKeyMan* spkm, int64_t new_birth_time)> 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); ///<! memory map of HD extended pubkeys
 
-    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;
 
     bool HaveKeyInner(const CKeyID &address) const;
     bool AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey);

src/wallet/wallet.cpp

@@ -941,6 +941,9 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
         wtx.nTimeSmart = ComputeTimeSmart(wtx, rescanning_old_block);
         AddToSpends(wtx, &batch);
         candidates = AddWalletUTXOs(wtx.tx, /*ret_dups=*/true);
+
+        // Update birth time when tx time is older than it.
+        MaybeUpdateBirthTime(wtx.GetTxTime());
     }
 
     if (!fInsertedNew)
@@ -1047,6 +1050,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;
 }
 
@@ -1646,6 +1653,13 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
     return true;
 }
 
+void CWallet::MaybeUpdateBirthTime(int64_t time)
+{
+    int64_t birthtime = m_birth_time.load();
+    if (time < birthtime) {
+        m_birth_time = time;
+    }
+}
 /**
  * Scan active chain for relevant transactions after importing keys. This should
  * be called whenever new keys are added to the wallet, with the oldest key
@@ -3063,6 +3077,14 @@ std::shared_ptr<CWallet> 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<int64_t> 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<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
+    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));
     }
 }
 

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<int64_t> m_best_block_time {0};
 
+    // Wallet birth time. Uses std::numeric_limits<int64_t>::max() for not known.
+    std::atomic<int64_t> m_birth_time{std::numeric_limits<int64_t>::max()};
+
     mutable bool fAnonymizableTallyCached = false;
     mutable std::vector<CompactTallyItem> vecAnonymizableTallyCached;
     mutable bool fAnonymizableTallyCachedNonDenom = false;
@@ -725,6 +728,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 '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<const instantsend::InstantSendLock>& islock) override;
     void notifyChainLock(const CBlockIndex* pindexChainLock, const std::shared_ptr<const chainlock::ChainLockSig>& 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<const Governance::Object*> 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

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',

test/functional/wallet_reindex.py

@@ -0,0 +1,106 @@
+#!/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 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()