From 2a12c7b57b49475514810db171baf3ac25781b51 Mon Sep 17 00:00:00 2001
From: Andrew Chow <achow101-github@achow101.com>
Date: Tue, 25 Jan 2022 16:13:02 -0500
Subject: [PATCH] Merge bitcoin/bitcoin#24067: wallet: Actually treat
 (un)confirmed txs as (un)confirmed

fac816544317cee6553d60cb0f5f24f6f9ec98de Remove unused checkFinalTx (MarcoFalke)
fa272eab44553df9b0bed3ca20caf2a7bd193680 wallet: Avoid dropping confirmed coins (MarcoFalke)
888841ea8d38fc059ca06ef67af7f31f8d8418a4 interfaces: Remove unused is_final (MarcoFalke)
dddd05e7a3389fcbd90bb4acdfe1f59945d9f381 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 fac816544317cee6553d60cb0f5f24f6f9ec98de
  prayank23:
    reACK https://github.com/bitcoin/bitcoin/pull/24067/commits/fac816544317cee6553d60cb0f5f24f6f9ec98de
  glozow:
    code review ACK fac8165443, I understand now how this fixes both issues.

Tree-SHA512: 210afb855f4c6d903fee49eba6b1a9735d699cf0168b669eabb38178e53b3a522258b7cc669f52489c6cd3e38bf358afde12eef3ba2e2f2ffaeb06b8f652ccd0
---
 src/interfaces/chain.h             |  3 --
 src/interfaces/wallet.h            |  1 -
 src/node/interfaces.cpp            |  5 ---
 src/qt/transactiondesc.cpp         |  9 -----
 src/qt/transactionrecord.cpp       | 15 +------
 src/qt/transactionrecord.h         |  2 -
 src/qt/transactiontablemodel.cpp   |  9 -----
 src/wallet/interfaces.cpp          |  1 -
 src/wallet/rpcwallet.cpp           |  4 +-
 src/wallet/wallet.cpp              | 64 ++++++++++++++++++++++++++++++
 test/functional/test_runner.py     |  1 +
 test/functional/wallet_timelock.py | 50 +++++++++++++++++++++++
 12 files changed, 118 insertions(+), 46 deletions(-)
 create mode 100755 test/functional/wallet_timelock.py

diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index bdaa621a975f99..ecaffb1ef44fac 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -133,9 +133,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;
-
     //! Check if transaction is locked by InstantSendManager
     virtual bool isInstantSendLockedTx(const uint256& hash) = 0;
 
diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
index 57a01676a32d9e..774ee255162740 100644
--- a/src/interfaces/wallet.h
+++ b/src/interfaces/wallet.h
@@ -424,7 +424,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
index 1a25271431f0b5..678f295aa91feb 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -771,11 +771,6 @@ class ChainImpl : public Chain
         }
         return std::nullopt;
     }
-    bool checkFinalTx(const CTransaction& tx) override
-    {
-        LOCK(cs_main);
-        return CheckFinalTxAtTip(*Assert(m_node.chainman->ActiveChain().Tip()), tx);
-    }
     bool isInstantSendLockedTx(const uint256& hash) override
     {
         if (m_node.llmq_ctx == nullptr || m_node.llmq_ctx->isman == nullptr) return false;
diff --git a/src/qt/transactiondesc.cpp b/src/qt/transactiondesc.cpp
index 9587e0fac20eb7..98df0a08a7cb22 100644
--- a/src/qt/transactiondesc.cpp
+++ b/src/qt/transactiondesc.cpp
@@ -18,7 +18,6 @@
 #include <key_io.h>
 #include <interfaces/node.h>
 #include <interfaces/wallet.h>
-#include <script/script.h>
 #include <util/system.h>
 #include <validation.h>
 #include <wallet/ismine.h>
@@ -30,14 +29,6 @@
 
 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) return tr("conflicted");
diff --git a/src/qt/transactionrecord.cpp b/src/qt/transactionrecord.cpp
index 46a5ffb6e8153a..a2af0a61a89a39 100644
--- a/src/qt/transactionrecord.cpp
+++ b/src/qt/transactionrecord.cpp
@@ -279,21 +279,8 @@ void TransactionRecord::updateStatus(const interfaces::WalletTxStatus& wtx, cons
     status.lockedByChainLocks = wtx.is_chainlocked;
     status.lockedByInstantSend = wtx.is_islocked;
 
-    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
index fe2064907ea153..a279e415b39f71 100644
--- a/src/qt/transactionrecord.h
+++ b/src/qt/transactionrecord.h
@@ -33,8 +33,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
index ecb40062f9e3cc..49187fbb529b0c 100644
--- a/src/qt/transactiontablemodel.cpp
+++ b/src/qt/transactiontablemodel.cpp
@@ -350,12 +350,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;
@@ -564,9 +558,6 @@ QVariant TransactionTableModel::txStatusDecoration(const TransactionRecord *wtx)
 {
     switch(wtx->status.status)
     {
-    case TransactionStatus::OpenUntilBlock:
-    case TransactionStatus::OpenUntilDate:
-        return GUIUtil::getThemedQColor(GUIUtil::ThemedColor::TX_STATUS_OPENUNTILDATE);
     case TransactionStatus::Unconfirmed:
         return GUIUtil::getIcon("transaction_0");
     case TransactionStatus::Abandoned:
diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp
index a4fbb369bcc5c9..89cae839d6218b 100644
--- a/src/wallet/interfaces.cpp
+++ b/src/wallet/interfaces.cpp
@@ -100,7 +100,6 @@ WalletTxStatus MakeWalletTxStatus(const CWallet& wallet, const CWalletTx& wtx)
     result.depth_in_main_chain = wtx.GetDepthInMainChain();
     result.time_received = wtx.nTimeReceived;
     result.lock_time = wtx.tx->nLockTime;
-    result.is_final = wallet.chain().checkFinalTx(*wtx.tx);
     result.is_trusted = wtx.IsTrusted();
     result.is_abandoned = wtx.isAbandoned();
     result.is_coinbase = wtx.IsCoinBase();
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 172bb82fefd573..dd272241d22dde 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -569,7 +569,7 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b
     CAmount amount = 0;
     for (const std::pair<const uint256, CWalletTx>& wtx_pair : wallet.mapWallet) {
         const CWalletTx& wtx = wtx_pair.second;
-        if (wtx.IsCoinBase() || !wallet.chain().checkFinalTx(*wtx.tx)) {
+        if (wtx.IsCoinBase()) {
             continue;
         }
         if (wtx.GetDepthInMainChain() < min_depth && !(fAddLocked && wtx.IsLockedByInstantSend())) continue;
@@ -966,7 +966,7 @@ static UniValue ListReceived(const CWallet& wallet, const UniValue& params, bool
     for (const std::pair<const uint256, CWalletTx>& pairWtx : wallet.mapWallet) {
         const CWalletTx& wtx = pairWtx.second;
 
-        if (wtx.IsCoinBase() || !wallet.chain().checkFinalTx(*wtx.tx))
+        if (wtx.IsCoinBase())
             continue;
 
         int nDepth = wtx.GetDepthInMainChain();
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 0ad8540e146c77..51ee5964391cea 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2080,6 +2080,70 @@ CAmount CWalletTx::GetDenominatedCredit(bool unconfirmed, bool fUseCache) const
     return nCredit;
 }
 
+<<<<<<< HEAD
+=======
+CAmount CWalletTx::GetChange() const
+{
+    if (fChangeCached)
+        return nChangeCached;
+    nChangeCached = pwallet->GetChange(*tx);
+    fChangeCached = true;
+    return nChangeCached;
+}
+
+bool CWalletTx::InMempool() const
+{
+    return fInMempool;
+}
+
+bool CWalletTx::IsTrusted() const
+{
+    std::set<uint256> trusted_parents;
+    LOCK(pwallet->cs_wallet);
+    return pwallet->IsTrusted(*this, trusted_parents);
+}
+
+bool CWallet::IsTrusted(const CWalletTx& wtx, std::set<uint256>& trusted_parents) const
+{
+    AssertLockHeld(cs_wallet);
+    int nDepth = wtx.GetDepthInMainChain();
+    if (nDepth >= 1) return true;
+    if (nDepth < 0) return false;
+    if (wtx.IsLockedByInstantSend()) return true;
+    // using wtx's cached debit
+    if (!m_spend_zero_conf_change || !wtx.IsFromMe(ISMINE_ALL)) return false;
+
+    // Don't trust unconfirmed transactions from us unless they are in the mempool.
+    if (!wtx.InMempool()) return false;
+
+    // Trusted if all inputs are from us and are in the mempool:
+    for (const CTxIn& txin : wtx.tx->vin)
+    {
+        // Transactions not sent by us: not trusted
+        const CWalletTx* parent = GetWalletTx(txin.prevout.hash);
+        if (parent == nullptr) return false;
+        const CTxOut& parentOut = parent->tx->vout[txin.prevout.n];
+        // Check that this specific input being spent is trusted
+        if (IsMine(parentOut) != ISMINE_SPENDABLE) return false;
+        // If we've already trusted this parent, continue
+        if (trusted_parents.count(parent->GetHash())) continue;
+        // Recurse to check that the parent is also trusted
+        if (!IsTrusted(*parent, trusted_parents)) return false;
+        trusted_parents.insert(parent->GetHash());
+    }
+    return true;
+}
+
+bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const
+{
+        CMutableTransaction tx1 {*this->tx};
+        CMutableTransaction tx2 {*_tx.tx};
+        for (auto& txin : tx1.vin) txin.scriptSig = CScript();
+        for (auto& txin : tx2.vin) txin.scriptSig = CScript();
+        return CTransaction(tx1) == CTransaction(tx2);
+}
+
+>>>>>>> Merge bitcoin/bitcoin#24067: wallet: Actually treat (un)confirmed txs as (un)confirmed
 // Rebroadcast transactions from the wallet. We do this on a random timer
 // to slightly obfuscate which transactions come from our wallet.
 //
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 7c9a2dd1d37cb4..f034c0cc7b88e8 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -370,6 +370,7 @@
     'feature_coinstatsindex.py',
     'wallet_orphanedreward.py',
     'p2p_node_network_limited.py --v1transport',
+    'wallet_timelock.py',
     'p2p_node_network_limited.py --v2transport',
     'p2p_permissions.py',
     'feature_blocksdir.py',
diff --git a/test/functional/wallet_timelock.py b/test/functional/wallet_timelock.py
new file mode 100755
index 00000000000000..cf233a00effd00
--- /dev/null
+++ 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()