Merge 6a5381a06b into merged_master (Bitcoin PR bitcoin/bitcoin#20591)

src/wallet/wallet.cpp

@@ -894,7 +894,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);
 
@@ -924,7 +924,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);
     }
 
@@ -1041,7 +1041,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;
     {
@@ -1079,7 +1079,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;
@@ -1208,9 +1208,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
@@ -1619,7 +1619,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;
@@ -2668,6 +2668,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
@@ -2682,38 +2684,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;
+        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;
+                // 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());
         }

src/wallet/wallet.h

@@ -277,8 +277,11 @@ private:
      * 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);
@@ -290,7 +293,7 @@ private:
 
     /* 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};
@@ -511,7 +514,7 @@ public:
     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
@@ -530,7 +533,7 @@ public:
     //! @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;

test/functional/test_runner.py

@@ -205,6 +205,7 @@ BASE_SCRIPTS = [
     # ELEMENTS: FIXME failing tests
     # 'wallet_groups.py --legacy-wallet',
     # 'wallet_groups.py --descriptors',
+    'wallet_transactiontime_rescan.py',
     'p2p_addrv2_relay.py',
     'p2p_compactblocks_hb.py',
     'p2p_disconnect_ban.py',

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()['bitcoin'], 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()['bitcoin'], 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()['bitcoin'], 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()