Merge bitcoin/bitcoin#24067: wallet: Actually treat (un)confirmed txs as (un)confirmed

achow101 · achow101 · commit e30b6ea194fe · 2022-01-25T16:17:51.000-05:00
fac8165 Remove unused checkFinalTx (MarcoFalke) fa272ea wallet: Avoid dropping confirmed coins (MarcoFalke) 888841e interfaces: Remove unused is_final (MarcoFalke) dddd05e qt: Treat unconfirmed txs as unconfirmed (MarcoFalke) Pull request description: The wallet has several issues: ## Unconfirmed txs in the GUI The GUI clumsily attempts to guess if unconfirmed txs are locked until a future time. This is currently based on the locktime only, not nSequence, thus wrong. Fix this by removing the clumsy code and treat all unconfirmed txs as unconfirmed. The GUI already prints whether a tx is in the mempool, in which case the user knows that the tx wasn't locked until a future time. If the tx is not in the mempool, it might be better to report the exact reject reason from the mempool instead of using incorrect heuristics. ## Confirmed txs in the wallet The wallet drops coins that it incorrectly assumes to be locked until a future time, even if they are already confirmed in the chain. This is because the wallet is using the wrong time (adjusted network time) instead of MTP, due to the `-1` default argument of `CheckFinalTx`. The issues are fixed in separate commits and there is even a test. ACKs for top commit: achow101: ACK fac8165 prayank23: reACK bitcoin/bitcoin@fac8165 glozow: code review ACK fac8165, I understand now how this fixes both issues. Tree-SHA512: 210afb855f4c6d903fee49eba6b1a9735d699cf0168b669eabb38178e53b3a522258b7cc669f52489c6cd3e38bf358afde12eef3ba2e2f2ffaeb06b8f652ccd0
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
@@ -116,9 +116,6 @@ class Chain
     //! or one of its ancestors.
     virtual std::optional<int> findLocatorFork(const CBlockLocator& locator) = 0;
 
-    //! Check if transaction will be final given chain height current time.
-    virtual bool checkFinalTx(const CTransaction& tx) = 0;
-
     //! Return whether node has the block and optionally return block metadata
     //! or contents.
     virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0;
diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
@@ -405,7 +405,6 @@ struct WalletTxStatus
     int depth_in_main_chain;
     unsigned int time_received;
     uint32_t lock_time;
-    bool is_final;
     bool is_trusted;
     bool is_abandoned;
     bool is_coinbase;
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
@@ -486,11 +486,6 @@ class ChainImpl : public Chain
         const CChain& active = Assert(m_node.chainman)->ActiveChain();
         return active.GetLocator();
     }
-    bool checkFinalTx(const CTransaction& tx) override
-    {
-        LOCK(cs_main);
-        return CheckFinalTx(chainman().ActiveChain().Tip(), tx);
-    }
     std::optional<int> findLocatorFork(const CBlockLocator& locator) override
     {
         LOCK(cs_main);
diff --git a/src/qt/guiconstants.h b/src/qt/guiconstants.h
@@ -33,8 +33,6 @@ static const bool DEFAULT_SPLASHSCREEN = true;
 #define COLOR_NEGATIVE QColor(255, 0, 0)
 /* Transaction list -- bare address (without label) */
 #define COLOR_BAREADDRESS QColor(140, 140, 140)
-/* Transaction list -- TX status decoration - open until date */
-#define COLOR_TX_STATUS_OPENUNTILDATE QColor(64, 64, 255)
 /* Transaction list -- TX status decoration - danger, tx needs attention */
 #define COLOR_TX_STATUS_DANGER QColor(200, 100, 100)
 /* Transaction list -- TX status decoration - default color */
diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp
@@ -18,7 +18,6 @@
 #include <interfaces/wallet.h>
 #include <key_io.h>
 #include <policy/policy.h>
-#include <script/script.h>
 #include <util/system.h>
 #include <validation.h>
 #include <wallet/ismine.h>
@@ -35,14 +34,6 @@ using wallet::isminetype;
 
 QString TransactionDesc::FormatTxStatus(const interfaces::WalletTx& wtx, const interfaces::WalletTxStatus& status, bool inMempool, int numBlocks)
 {
-    if (!status.is_final)
-    {
-        if (wtx.tx->nLockTime < LOCKTIME_THRESHOLD)
-            return tr("Open for %n more block(s)", "", wtx.tx->nLockTime - numBlocks);
-        else
-            return tr("Open until %1").arg(GUIUtil::dateTimeStr(wtx.tx->nLockTime));
-    }
-    else
     {
         int nDepth = status.depth_in_main_chain;
         if (nDepth < 0) {
diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp
@@ -179,21 +179,8 @@ void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, cons
     status.depth = wtx.depth_in_main_chain;
     status.m_cur_block_hash = block_hash;
 
-    const bool up_to_date = ((int64_t)QDateTime::currentMSecsSinceEpoch() / 1000 - block_time < MAX_BLOCK_TIME_GAP);
-    if (up_to_date && !wtx.is_final) {
-        if (wtx.lock_time < LOCKTIME_THRESHOLD) {
-            status.status = TransactionStatus::OpenUntilBlock;
-            status.open_for = wtx.lock_time - numBlocks;
-        }
-        else
-        {
-            status.status = TransactionStatus::OpenUntilDate;
-            status.open_for = wtx.lock_time;
-        }
-    }
     // For generated transactions, determine maturity
-    else if(type == TransactionRecord::Generated)
-    {
+    if (type == TransactionRecord::Generated) {
         if (wtx.blocks_to_maturity > 0)
         {
             status.status = TransactionStatus::Immature;
diff --git a/src/qt/transactionrecord.h b/src/qt/transactionrecord.h
@@ -30,8 +30,6 @@ class TransactionStatus
     enum Status {
         Confirmed,          /**< Have 6 or more confirmations (normal tx) or fully mature (mined tx) **/
         /// Normal (sent/received) transactions
-        OpenUntilDate,      /**< Transaction not yet final, waiting for date */
-        OpenUntilBlock,     /**< Transaction not yet final, waiting for block */
         Unconfirmed,        /**< Not yet mined into a block **/
         Confirming,         /**< Confirmed, but waiting for the recommended number of confirmations **/
         Conflicted,         /**< Conflicts with other transaction or mempool **/
diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp
@@ -316,12 +316,6 @@ QString TransactionTableModel::formatTxStatus(const TransactionRecord *wtx) cons
 
     switch(wtx->status.status)
     {
-    case TransactionStatus::OpenUntilBlock:
-        status = tr("Open for %n more block(s)","",wtx->status.open_for);
-        break;
-    case TransactionStatus::OpenUntilDate:
-        status = tr("Open until %1").arg(GUIUtil::dateTimeStr(wtx->status.open_for));
-        break;
     case TransactionStatus::Unconfirmed:
         status = tr("Unconfirmed");
         break;
@@ -475,9 +469,6 @@ QVariant TransactionTableModel::txStatusDecoration(const TransactionRecord *wtx)
 {
     switch(wtx->status.status)
     {
-    case TransactionStatus::OpenUntilBlock:
-    case TransactionStatus::OpenUntilDate:
-        return COLOR_TX_STATUS_OPENUNTILDATE;
     case TransactionStatus::Unconfirmed:
         return QIcon(":/icons/transaction_0");
     case TransactionStatus::Abandoned:
diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
@@ -90,7 +90,6 @@ WalletTxStatus MakeWalletTxStatus(const CWallet& wallet, const CWalletTx& wtx)
     result.depth_in_main_chain = wallet.GetTxDepthInMainChain(wtx);
     result.time_received = wtx.nTimeReceived;
     result.lock_time = wtx.tx->nLockTime;
-    result.is_final = wallet.chain().checkFinalTx(*wtx.tx);
     result.is_trusted = CachedTxIsTrusted(wallet, wtx);
     result.is_abandoned = wtx.isAbandoned();
     result.is_coinbase = wtx.IsCoinBase();
diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp
@@ -279,8 +279,6 @@ bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminef
 bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<uint256>& trusted_parents)
 {
     AssertLockHeld(wallet.cs_wallet);
-    // Quick answer in most cases
-    if (!wallet.chain().checkFinalTx(*wtx.tx)) return false;
     int nDepth = wallet.GetTxDepthInMainChain(wtx);
     if (nDepth >= 1) return true;
     if (nDepth < 0) return false;
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
@@ -60,8 +60,8 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b
         if (depth < min_depth
             // Coinbase with less than 1 confirmation is no longer in the main chain
             || (wtx.IsCoinBase() && (depth < 1 || !include_coinbase))
-            || (wallet.IsTxImmatureCoinBase(wtx) && !include_immature_coinbase)
-            || !wallet.chain().checkFinalTx(*wtx.tx)) {
+            || (wallet.IsTxImmatureCoinBase(wtx) && !include_immature_coinbase))
+        {
             continue;
         }
 
diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp
@@ -114,8 +114,8 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, cons
 
         // Coinbase with less than 1 confirmation is no longer in the main chain
         if ((wtx.IsCoinBase() && (nDepth < 1 || !include_coinbase))
-            || (wallet.IsTxImmatureCoinBase(wtx) && !include_immature_coinbase)
-            || !wallet.chain().checkFinalTx(*wtx.tx)) {
+            || (wallet.IsTxImmatureCoinBase(wtx) && !include_immature_coinbase))
+        {
             continue;
         }
 
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
@@ -105,10 +105,6 @@ void AvailableCoins(const CWallet& wallet, std::vector<COutput>& vCoins, const C
         const uint256& wtxid = entry.first;
         const CWalletTx& wtx = entry.second;
 
-        if (!wallet.chain().checkFinalTx(*wtx.tx)) {
-            continue;
-        }
-
         if (wallet.IsTxImmatureCoinBase(wtx))
             continue;
 
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
@@ -308,6 +308,7 @@
     'feature_coinstatsindex.py --legacy-wallet',
     'feature_coinstatsindex.py --descriptors',
     'wallet_orphanedreward.py',
+    'wallet_timelock.py',
     'p2p_node_network_limited.py',
     'p2p_permissions.py',
     'feature_blocksdir.py',
diff --git a/test/functional/wallet_timelock.py b/test/functional/wallet_timelock.py
@@ -0,0 +1,50 @@
+#!/usr/bin/env python3
+# Copyright (c) 2022 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
+
+
+class WalletLocktimeTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    def run_test(self):
+        node = self.nodes[0]
+
+        mtp_tip = node.getblockheader(node.getbestblockhash())["mediantime"]
+
+        self.log.info("Get new address with label")
+        label = "timelock⌛🔓"
+        address = node.getnewaddress(label=label)
+
+        self.log.info("Send to new address with locktime")
+        node.send(
+            outputs={address: 5},
+            options={"locktime": mtp_tip - 1},
+        )
+        self.generate(node, 1)
+
+        self.log.info("Check that clock can not change finality of confirmed txs")
+        amount_before_ad = node.getreceivedbyaddress(address)
+        amount_before_lb = node.getreceivedbylabel(label)
+        list_before_ad = node.listreceivedbyaddress(address_filter=address)
+        list_before_lb = node.listreceivedbylabel(include_empty=False)
+        balance_before = node.getbalances()["mine"]["trusted"]
+        coin_before = node.listunspent(maxconf=1)
+        node.setmocktime(mtp_tip - 1)
+        assert_equal(node.getreceivedbyaddress(address), amount_before_ad)
+        assert_equal(node.getreceivedbylabel(label), amount_before_lb)
+        assert_equal(node.listreceivedbyaddress(address_filter=address), list_before_ad)
+        assert_equal(node.listreceivedbylabel(include_empty=False), list_before_lb)
+        assert_equal(node.getbalances()["mine"]["trusted"], balance_before)
+        assert_equal(node.listunspent(maxconf=1), coin_before)
+
+
+if __name__ == "__main__":
+    WalletLocktimeTest().main()

Original file line number	Diff line number	Diff line change
`@@ -486,11 +486,6 @@ class ChainImpl : public Chain`
`486`	`486`	`const CChain& active = Assert(m_node.chainman)->ActiveChain();`
`487`	`487`	`return active.GetLocator();`
`488`	`488`	`}`
`489`		`- bool checkFinalTx(const CTransaction& tx) override`
`490`		`- {`
`491`		`- LOCK(cs_main);`
`492`		`- return CheckFinalTx(chainman().ActiveChain().Tip(), tx);`
`493`		`- }`
`494`	`489`	`std::optional<int> findLocatorFork(const CBlockLocator& locator) override`
`495`	`490`	`{`
`496`	`491`	`LOCK(cs_main);`
Original file line number	Diff line number	Diff line change
`@@ -279,8 +279,6 @@ bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminef`
`279`	`279`	`bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<uint256>& trusted_parents)`
`280`	`280`	`{`
`281`	`281`	`AssertLockHeld(wallet.cs_wallet);`
`282`		`- // Quick answer in most cases`
`283`		`- if (!wallet.chain().checkFinalTx(*wtx.tx)) return false;`
`284`	`282`	`int nDepth = wallet.GetTxDepthInMainChain(wtx);`
`285`	`283`	`if (nDepth >= 1) return true;`
`286`	`284`	`if (nDepth < 0) return false;`