wallet: Minimal fix to restore conflicted transaction notifications

Summary:
> This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in
> bitcoin/bitcoin#18600.
>
> Unlike that PR, which implements some new behavior, this just restores previous
> wallet notification and status behavior for transactions removed from the
> mempool because they conflict with transactions in a block. The behavior was
> accidentally changed in two `CWallet::BlockConnected` updates:
> a31be09bfd77eed497a8e251d31358e16e2f2eb1 and
> 7e89994133725125dddbfa8d45484e3b9ed51c6e from
> bitcoin/bitcoin#16624, causing issue
> bitcoin/bitcoin#18325.
>
> The change here could be improved and replaced with a more comprehensive
> cleanup, so it includes a detailed comment explaining future considerations.
>
> Co-authored-by: Antoine Riard <ariard@student.42.fr>

This is a backport of [[bitcoin/bitcoin#18982 | core#18982]] [1/2]
bitcoin/bitcoin@b604c5c

Backport note: The test is already included in D9972, because it seemed to work without this fix. But now we have seen intermittent CI failures.

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9962

doc/release-notes.md

@@ -12,3 +12,10 @@ A new `bitcoin-cli -generate` command, equivalent to RPC `generatenewaddress`
 followed by `generatetoaddress`, can generate blocks for command line testing
 purposes. This is a client-side version of the former `generate` RPC. See
 the help for details.
+
+## Notification changes
+
+`-walletnotify` notifications are now sent for wallet transactions that are
+removed from the mempool because they conflict with a new block. These
+notifications were sent previously before the v0.21.13 release, but had been
+broken since that release.

src/interfaces/chain.cpp

@@ -75,8 +75,10 @@ namespace {
         void TransactionAddedToMempool(const CTransactionRef &tx) override {
             m_notifications->transactionAddedToMempool(tx);
         }
-        void TransactionRemovedFromMempool(const CTransactionRef &tx) override {
-            m_notifications->transactionRemovedFromMempool(tx);
+        void
+        TransactionRemovedFromMempool(const CTransactionRef &tx,
+                                      MemPoolRemovalReason reason) override {
+            m_notifications->transactionRemovedFromMempool(tx, reason);
         }
         void BlockConnected(const std::shared_ptr<const CBlock> &block,
                             const CBlockIndex *index) override {

src/interfaces/chain.h

@@ -25,6 +25,8 @@ class CRPCCommand;
 class CScheduler;
 class TxValidationState;
 
+enum class MemPoolRemovalReason;
+
 struct BlockHash;
 struct bilingual_str;
 struct CBlockLocator;
@@ -271,8 +273,9 @@ class Chain {
     public:
         virtual ~Notifications() {}
         virtual void transactionAddedToMempool(const CTransactionRef &tx) {}
-        virtual void transactionRemovedFromMempool(const CTransactionRef &ptx) {
-        }
+        virtual void
+        transactionRemovedFromMempool(const CTransactionRef &ptx,
+                                      MemPoolRemovalReason reason) {}
         virtual void blockConnected(const CBlock &block, int height) {}
         virtual void blockDisconnected(const CBlock &block, int height) {}
         virtual void updatedBlockTip() {}

src/txmempool.cpp

@@ -469,7 +469,8 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) {
         // for any reason except being included in a block. Clients interested
         // in transactions included in blocks can subscribe to the
         // BlockConnected notification.
-        GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx());
+        GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(),
+                                                       reason);
     }
 
     for (const CTxIn &txin : it->GetTx().vin) {

src/validationinterface.cpp

@@ -213,10 +213,11 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
                           ptx->GetHash().ToString());
 }
 
-void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx) {
-    auto event = [ptx, this] {
+void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx,
+                                                 MemPoolRemovalReason reason) {
+    auto event = [ptx, reason, this] {
         m_internals->Iterate([&](CValidationInterface &callbacks) {
-            callbacks.TransactionRemovedFromMempool(ptx);
+            callbacks.TransactionRemovedFromMempool(ptx, reason);
         });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s", __func__,

src/validationinterface.h

@@ -21,6 +21,7 @@ class CConnman;
 class CValidationInterface;
 class uint256;
 class CScheduler;
+enum class MemPoolRemovalReason;
 
 /** Register subscriber */
 void RegisterValidationInterface(CValidationInterface *callbacks);
@@ -137,7 +138,8 @@ class CValidationInterface {
      *
      * Called on a background thread.
      */
-    virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx) {}
+    virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx,
+                                               MemPoolRemovalReason reason) {}
     /**
      * Notifies listeners of a block being connected.
      * Provides a vector of transactions evicted from the mempool as a result.
@@ -218,7 +220,8 @@ class CMainSignals {
     void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *,
                          bool fInitialDownload);
     void TransactionAddedToMempool(const CTransactionRef &);
-    void TransactionRemovedFromMempool(const CTransactionRef &);
+    void TransactionRemovedFromMempool(const CTransactionRef &,
+                                       MemPoolRemovalReason);
     void BlockConnected(const std::shared_ptr<const CBlock> &,
                         const CBlockIndex *pindex);
     void BlockDisconnected(const std::shared_ptr<const CBlock> &,

src/wallet/wallet.cpp

@@ -23,6 +23,7 @@
 #include <script/sighashtype.h>
 #include <script/sign.h>
 #include <script/signingprovider.h>
+#include <txmempool.h>
 #include <util/bip32.h>
 #include <util/check.h>
 #include <util/error.h>
@@ -1225,12 +1226,45 @@ void CWallet::transactionAddedToMempool(const CTransactionRef &ptx) {
     }
 }
 
-void CWallet::transactionRemovedFromMempool(const CTransactionRef &ptx) {
+void CWallet::transactionRemovedFromMempool(const CTransactionRef &ptx,
+                                            MemPoolRemovalReason reason) {
     LOCK(cs_wallet);
     auto it = mapWallet.find(ptx->GetId());
     if (it != mapWallet.end()) {
         it->second.fInMempool = false;
     }
+    // Handle transactions that were removed from the mempool because they
+    // conflict with transactions in a newly connected block.
+    if (reason == MemPoolRemovalReason::CONFLICT) {
+        // Call SyncNotifications, so external -walletnotify notifications will
+        // be triggered for these transactions. Set Status::UNCONFIRMED instead
+        // of Status::CONFLICTED for a few reasons:
+        //
+        // 1. The transactionRemovedFromMempool callback does not currently
+        //    provide the conflicting block's hash and height, and for backwards
+        //    compatibility reasons it may not be not safe to store conflicted
+        //    wallet transactions with a null block hash. See
+        //    https://github.com/bitcoin/bitcoin/pull/18600#discussion_r420195993.
+        // 2. For most of these transactions, the wallet's internal conflict
+        //    detection in the blockConnected handler will subsequently call
+        //    MarkConflicted and update them with CONFLICTED status anyway. This
+        //    applies to any wallet transaction that has inputs spent in the
+        //    block, or that has ancestors in the wallet with inputs spent by
+        //    the block.
+        // 3. Longstanding behavior since the sync implementation in
+        //    https://github.com/bitcoin/bitcoin/pull/9371 and the prior sync
+        //    implementation before that was to mark these transactions
+        //    unconfirmed rather than conflicted.
+        //
+        // Nothing described above should be seen as an unchangeable requirement
+        // when improving this code in the future. The wallet's heuristics for
+        // distinguishing between conflicted and unconfirmed transactions are
+        // imperfect, and could be improved in general, see
+        // https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking
+        SyncTransaction(ptx,
+                        {CWalletTx::Status::UNCONFIRMED, /* block height  */ 0,
+                         BlockHash(), /* index */ 0});
+    }
 }
 
 void CWallet::blockConnected(const CBlock &block, int height) {
@@ -1243,7 +1277,8 @@ void CWallet::blockConnected(const CBlock &block, int height) {
         CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, height,
                                         block_hash, index);
         SyncTransaction(block.vtx[index], confirm);
-        transactionRemovedFromMempool(block.vtx[index]);
+        transactionRemovedFromMempool(block.vtx[index],
+                                      MemPoolRemovalReason::BLOCK);
     }
 }
 

src/wallet/wallet.h

@@ -1086,7 +1086,8 @@ class CWallet final : public WalletStorage,
                                          std::optional<int> max_height,
                                          const WalletRescanReserver &reserver,
                                          bool fUpdate);
-    void transactionRemovedFromMempool(const CTransactionRef &ptx) override;
+    void transactionRemovedFromMempool(const CTransactionRef &ptx,
+                                       MemPoolRemovalReason reason) override;
     void ReacceptWalletTransactions() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void ResendWalletTransactions();
     struct Balance {

test/functional/feature_notifications.py

@@ -13,6 +13,7 @@
     connect_nodes,
     disconnect_nodes,
     wait_until,
+    disconnect_nodes,
     hex_str_to_bytes,
 )