Merge bitcoin/bitcoin#20591: wallet, bugfix: fix ComputeTimeSmart function during rescanning process.

meshcollider · meshcollider · commit 6a5381a06b0b · 2021-09-29T11:18:23.000+13:00
240ea29 doc: update doxygen documention of ComputeTimeSmart() and AddToWalletIfInvolvingMe() regarding rescanning_old_block parameter (BitcoinTsunami) d6eb39a test: add functional test to check transaction time determination during block rescanning (BitcoinTsunami) 07b44f1 wallet: fix ComputeTimeSmart algorithm to use blocktime during old block rescanning (BitcoinTsunami) Pull request description: The function ComputeTimeSmart in wallet.cpp assume that transaction are discovered in the right order. Moreover the 'smarttime' determination algorithm is coded with realtime scenario in mind and not rescanning of old block. The functional test demonstrate that if the user import a wallet, then rescan only recent history, and then rescan the entire history, the older transaction discovered would have an incorrect time determination. In the context of rescanning old block, the only time value that as a meaning is the blocktime. That's why I've fixed the problem with a simple separation between rescanning of old block and realtime time determination. The fix is written to have no impact on every realtime scenario and only impact the behaviour during a rescanning process. This PR Fixes #20181. To be fair, I don't think that this bug could be triggered with the wallet GUI, because it always proceed with a proper rescan. But RPC API provide the possibility to trigger it. I've discovered it, because Specter desktop v0.10.0 was impacted. (cryptoadvance/specter-desktop#680). ACKs for top commit: jonatack: ACK 240ea29 per `git diff b92d552 240ea29`, re-verified rebase to latest master + debug build clean + new test passes on the branch and fails on master, only change since my review a few hours ago is incorporation of latest review suggestions meshcollider: re-utACK 240ea29 Tree-SHA512: 514b02e41d011ddfa325f5e8080b93800e1ea4ed5853fa420670a6ac700e8b463000dbea65f8ced8565cfb950c7f51b69154034dcb111e67aca3b964a0061494
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -884,7 +884,7 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
     return false;
 }
 
-CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx, bool fFlushOnClose)
+CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx, bool fFlushOnClose, bool rescanning_old_block)
 {
     LOCK(cs_wallet);
 
@@ -914,7 +914,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const CWalletTx::Confirmatio
         wtx.nTimeReceived = chain().getAdjustedTime();
         wtx.nOrderPos = IncOrderPosNext(&batch);
         wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
-        wtx.nTimeSmart = ComputeTimeSmart(wtx);
+        wtx.nTimeSmart = ComputeTimeSmart(wtx, rescanning_old_block);
         AddToSpends(hash, &batch);
     }
 
@@ -1031,7 +1031,7 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx
     return true;
 }
 
-bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Confirmation confirm, bool fUpdate)
+bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Confirmation confirm, bool fUpdate, bool rescanning_old_block)
 {
     const CTransaction& tx = *ptx;
     {
@@ -1069,7 +1069,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Co
 
             // Block disconnection override an abandoned tx as unconfirmed
             // which means user may have to call abandontransaction again
-            return AddToWallet(MakeTransactionRef(tx), confirm, /* update_wtx= */ nullptr, /* fFlushOnClose= */ false);
+            return AddToWallet(MakeTransactionRef(tx), confirm, /* update_wtx= */ nullptr, /* fFlushOnClose= */ false, rescanning_old_block);
         }
     }
     return false;
@@ -1198,9 +1198,9 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c
     }
 }
 
-void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Confirmation confirm, bool update_tx)
+void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Confirmation confirm, bool update_tx, bool rescanning_old_block)
 {
-    if (!AddToWalletIfInvolvingMe(ptx, confirm, update_tx))
+    if (!AddToWalletIfInvolvingMe(ptx, confirm, update_tx, rescanning_old_block))
         return; // Not one of ours
 
     // If a transaction changes 'conflicted' state, that changes the balance
@@ -1643,7 +1643,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
                 break;
             }
             for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
-                SyncTransaction(block.vtx[posInBlock], {CWalletTx::Status::CONFIRMED, block_height, block_hash, (int)posInBlock}, fUpdate);
+                SyncTransaction(block.vtx[posInBlock], {CWalletTx::Status::CONFIRMED, block_height, block_hash, (int)posInBlock}, fUpdate, /* rescanning_old_block */ true);
             }
             // scan succeeded, record block as most recent successfully scanned
             result.last_scanned_block = block_hash;
@@ -2384,6 +2384,8 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t>& mapKeyBirth) const {
  * - If sending a transaction, assign its timestamp to the current time.
  * - If receiving a transaction outside a block, assign its timestamp to the
  *   current time.
+ * - If receiving a transaction during a rescanning process, assign all its
+ *   (not already known) transactions' timestamps to the block time.
  * - If receiving a block with a future timestamp, assign all its (not already
  *   known) transactions' timestamps to the current time.
  * - If receiving a block with a past timestamp, before the most recent known
@@ -2398,38 +2400,43 @@ void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t>& mapKeyBirth) const {
  * https://bitcointalk.org/?topic=54527, or
  * https://github.com/bitcoin/bitcoin/pull/1393.
  */
-unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
+unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old_block) const
 {
     unsigned int nTimeSmart = wtx.nTimeReceived;
     if (!wtx.isUnconfirmed() && !wtx.isAbandoned()) {
         int64_t blocktime;
-        if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(blocktime))) {
-            int64_t latestNow = wtx.nTimeReceived;
-            int64_t latestEntry = 0;
-
-            // Tolerate times up to the last timestamp in the wallet not more than 5 minutes into the future
-            int64_t latestTolerated = latestNow + 300;
-            const TxItems& txOrdered = wtxOrdered;
-            for (auto it = txOrdered.rbegin(); it != txOrdered.rend(); ++it) {
-                CWalletTx* const pwtx = it->second;
-                if (pwtx == &wtx) {
-                    continue;
-                }
-                int64_t nSmartTime;
-                nSmartTime = pwtx->nTimeSmart;
-                if (!nSmartTime) {
-                    nSmartTime = pwtx->nTimeReceived;
-                }
-                if (nSmartTime <= latestTolerated) {
-                    latestEntry = nSmartTime;
-                    if (nSmartTime > latestNow) {
-                        latestNow = nSmartTime;
+        int64_t block_max_time;
+        if (chain().findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(blocktime).maxTime(block_max_time))) {
+            if (rescanning_old_block) {
+                nTimeSmart = block_max_time;
+            } else {
+                int64_t latestNow = wtx.nTimeReceived;
+                int64_t latestEntry = 0;
+
+                // Tolerate times up to the last timestamp in the wallet not more than 5 minutes into the future
+                int64_t latestTolerated = latestNow + 300;
+                const TxItems& txOrdered = wtxOrdered;
+                for (auto it = txOrdered.rbegin(); it != txOrdered.rend(); ++it) {
+                    CWalletTx* const pwtx = it->second;
+                    if (pwtx == &wtx) {
+                        continue;
+                    }
+                    int64_t nSmartTime;
+                    nSmartTime = pwtx->nTimeSmart;
+                    if (!nSmartTime) {
+                        nSmartTime = pwtx->nTimeReceived;
+                    }
+                    if (nSmartTime <= latestTolerated) {
+                        latestEntry = nSmartTime;
+                        if (nSmartTime > latestNow) {
+                            latestNow = nSmartTime;
+                        }
+                        break;
                     }
-                    break;
                 }
-            }
 
-            nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
+                nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));
+            }
         } else {
             WalletLogPrintf("%s: found %s in block %s not in index\n", __func__, wtx.GetHash().ToString(), wtx.m_confirm.hashBlock.ToString());
         }
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
@@ -271,8 +271,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
      * abandoned is an indication that it is not safe to be considered abandoned.
      * Abandoned state should probably be more carefully tracked via different
      * posInBlock signals or by checking mempool presence when necessary.
+     *
+     * Should be called with rescanning_old_block set to true, if the transaction is
+     * not discovered in real time, but during a rescan of old blocks.
      */
-    bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, CWalletTx::Confirmation confirm, bool fUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, CWalletTx::Confirmation confirm, bool fUpdate, bool rescanning_old_block) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     /** Mark a transaction (and its in-wallet descendants) as conflicting with a particular block. */
     void MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx);
@@ -284,7 +287,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
 
     /* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected/ScanForWalletTransactions.
      * Should be called with non-zero block_hash and posInBlock if this is for a transaction that is included in a block. */
-    void SyncTransaction(const CTransactionRef& tx, CWalletTx::Confirmation confirm, bool update_tx = true) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    void SyncTransaction(const CTransactionRef& tx, CWalletTx::Confirmation confirm, bool update_tx = true, bool rescanning_old_block = false) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     /** WalletFlags set on this wallet. */
     std::atomic<uint64_t> m_wallet_flags{0};
@@ -484,7 +487,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     bool EncryptWallet(const SecureString& strWalletPassphrase);
 
     void GetKeyBirthTimes(std::map<CKeyID, int64_t> &mapKeyBirth) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
-    unsigned int ComputeTimeSmart(const CWalletTx& wtx) const;
+    unsigned int ComputeTimeSmart(const CWalletTx& wtx, bool rescanning_old_block) const;
 
     /**
      * Increment the next transaction order id
@@ -503,7 +506,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     //! @return true if wtx is changed and needs to be saved to disk, otherwise false
     using UpdateWalletTxFn = std::function<bool(CWalletTx& wtx, bool new_tx)>;
 
-    CWalletTx* AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx=nullptr, bool fFlushOnClose=true);
+    CWalletTx* AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx=nullptr, bool fFlushOnClose=true, bool rescanning_old_block = false);
     bool LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void transactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override;
     void blockConnected(const CBlock& block, int height) override;
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
@@ -175,6 +175,7 @@
     'rpc_rawtransaction.py --legacy-wallet',
     'rpc_rawtransaction.py --descriptors',
     'wallet_groups.py --legacy-wallet',
+    'wallet_transactiontime_rescan.py',
     'p2p_addrv2_relay.py',
     'wallet_groups.py --descriptors',
     'p2p_compactblocks_hb.py',
diff --git a/test/functional/wallet_transactiontime_rescan.py b/test/functional/wallet_transactiontime_rescan.py
@@ -0,0 +1,161 @@
+#!/usr/bin/env python3
+# Copyright (c) 2018-2019 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test transaction time during old block rescanning
+"""
+
+import time
+
+from test_framework.blocktools import COINBASE_MATURITY
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import (
+    assert_equal
+)
+
+
+class TransactionTimeRescanTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = False
+        self.num_nodes = 3
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    def run_test(self):
+        self.log.info('Prepare nodes and wallet')
+
+        minernode = self.nodes[0]  # node used to mine BTC and create transactions
+        usernode = self.nodes[1]  # user node with correct time
+        restorenode = self.nodes[2]  # node used to restore user wallet and check time determination in ComputeSmartTime (wallet.cpp)
+
+        # time constant
+        cur_time = int(time.time())
+        ten_days = 10 * 24 * 60 * 60
+
+        # synchronize nodes and time
+        self.sync_all()
+        minernode.setmocktime(cur_time)
+        usernode.setmocktime(cur_time)
+        restorenode.setmocktime(cur_time)
+
+        # prepare miner wallet
+        minernode.createwallet(wallet_name='default')
+        miner_wallet = minernode.get_wallet_rpc('default')
+        m1 = miner_wallet.getnewaddress()
+
+        # prepare the user wallet with 3 watch only addresses
+        wo1 = usernode.getnewaddress()
+        wo2 = usernode.getnewaddress()
+        wo3 = usernode.getnewaddress()
+
+        usernode.createwallet(wallet_name='wo', disable_private_keys=True)
+        wo_wallet = usernode.get_wallet_rpc('wo')
+
+        wo_wallet.importaddress(wo1)
+        wo_wallet.importaddress(wo2)
+        wo_wallet.importaddress(wo3)
+
+        self.log.info('Start transactions')
+
+        # check blockcount
+        assert_equal(minernode.getblockcount(), 200)
+
+        # generate some btc to create transactions and check blockcount
+        initial_mine = COINBASE_MATURITY + 1
+        minernode.generatetoaddress(initial_mine, m1)
+        assert_equal(minernode.getblockcount(), initial_mine + 200)
+
+        # synchronize nodes and time
+        self.sync_all()
+        minernode.setmocktime(cur_time + ten_days)
+        usernode.setmocktime(cur_time + ten_days)
+        restorenode.setmocktime(cur_time + ten_days)
+        # send 10 btc to user's first watch-only address
+        self.log.info('Send 10 btc to user')
+        miner_wallet.sendtoaddress(wo1, 10)
+
+        # generate blocks and check blockcount
+        minernode.generatetoaddress(COINBASE_MATURITY, m1)
+        assert_equal(minernode.getblockcount(), initial_mine + 300)
+
+        # synchronize nodes and time
+        self.sync_all()
+        minernode.setmocktime(cur_time + ten_days + ten_days)
+        usernode.setmocktime(cur_time + ten_days + ten_days)
+        restorenode.setmocktime(cur_time + ten_days + ten_days)
+        # send 5 btc to our second watch-only address
+        self.log.info('Send 5 btc to user')
+        miner_wallet.sendtoaddress(wo2, 5)
+
+        # generate blocks and check blockcount
+        minernode.generatetoaddress(COINBASE_MATURITY, m1)
+        assert_equal(minernode.getblockcount(), initial_mine + 400)
+
+        # synchronize nodes and time
+        self.sync_all()
+        minernode.setmocktime(cur_time + ten_days + ten_days + ten_days)
+        usernode.setmocktime(cur_time + ten_days + ten_days + ten_days)
+        restorenode.setmocktime(cur_time + ten_days + ten_days + ten_days)
+        # send 1 btc to our third watch-only address
+        self.log.info('Send 1 btc to user')
+        miner_wallet.sendtoaddress(wo3, 1)
+
+        # generate more blocks and check blockcount
+        minernode.generatetoaddress(COINBASE_MATURITY, m1)
+        assert_equal(minernode.getblockcount(), initial_mine + 500)
+
+        self.log.info('Check user\'s final balance and transaction count')
+        assert_equal(wo_wallet.getbalance(), 16)
+        assert_equal(len(wo_wallet.listtransactions()), 3)
+
+        self.log.info('Check transaction times')
+        for tx in wo_wallet.listtransactions():
+            if tx['address'] == wo1:
+                assert_equal(tx['blocktime'], cur_time + ten_days)
+                assert_equal(tx['time'], cur_time + ten_days)
+            elif tx['address'] == wo2:
+                assert_equal(tx['blocktime'], cur_time + ten_days + ten_days)
+                assert_equal(tx['time'], cur_time + ten_days + ten_days)
+            elif tx['address'] == wo3:
+                assert_equal(tx['blocktime'], cur_time + ten_days + ten_days + ten_days)
+                assert_equal(tx['time'], cur_time + ten_days + ten_days + ten_days)
+
+        # restore user wallet without rescan
+        self.log.info('Restore user wallet on another node without rescan')
+        restorenode.createwallet(wallet_name='wo', disable_private_keys=True)
+        restorewo_wallet = restorenode.get_wallet_rpc('wo')
+
+        restorewo_wallet.importaddress(wo1, rescan=False)
+        restorewo_wallet.importaddress(wo2, rescan=False)
+        restorewo_wallet.importaddress(wo3, rescan=False)
+
+        # check user has 0 balance and no transactions
+        assert_equal(restorewo_wallet.getbalance(), 0)
+        assert_equal(len(restorewo_wallet.listtransactions()), 0)
+
+        # proceed to rescan, first with an incomplete one, then with a full rescan
+        self.log.info('Rescan last history part')
+        restorewo_wallet.rescanblockchain(initial_mine + 350)
+        self.log.info('Rescan all history')
+        restorewo_wallet.rescanblockchain()
+
+        self.log.info('Check user\'s final balance and transaction count after restoration')
+        assert_equal(restorewo_wallet.getbalance(), 16)
+        assert_equal(len(restorewo_wallet.listtransactions()), 3)
+
+        self.log.info('Check transaction times after restoration')
+        for tx in restorewo_wallet.listtransactions():
+            if tx['address'] == wo1:
+                assert_equal(tx['blocktime'], cur_time + ten_days)
+                assert_equal(tx['time'], cur_time + ten_days)
+            elif tx['address'] == wo2:
+                assert_equal(tx['blocktime'], cur_time + ten_days + ten_days)
+                assert_equal(tx['time'], cur_time + ten_days + ten_days)
+            elif tx['address'] == wo3:
+                assert_equal(tx['blocktime'], cur_time + ten_days + ten_days + ten_days)
+                assert_equal(tx['time'], cur_time + ten_days + ten_days + ten_days)
+
+
+if __name__ == '__main__':
+    TransactionTimeRescanTest().main()
