wallet: Minimal fix to restore conflicted transaction notifications #18982

This fix is a based on the fix by Antoine Riard (ariard) in #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 #16624, causing issue #18325.

The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations.

Fixes #18325

Co-authored-by: Antoine Riard (ariard)

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #19116 ([WIP] wallet: use BlockFilterIndex in ScanForWalletTransactions by pstratem)
• #18600 ([wallet] Track conflicted transactions removed from mempool and fix UI notifications by ariard)
• #18354 (Use shared pointers only in validation interface by bvbfan)
• #17443 (Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard)
• #15719 (Wallet passive startup by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Concept ACK and almost Code-Review ACK 90118ec. Thanks for taking this forward Russ, and taking (1) out of #18600

We may discuss/implement improvement in future works.

Updated 90118ec54f6921ed102ef45bb4f4d3a0ac8782b8 -> 8242b093d3db52b7e443d650bafd307a03527639 (pr/cblock.3 -> pr/cblock.4, compare) making suggested changes and also adding release notes

(requested @jonatack as reviewer to help with documentation, since a lot is added here, and it'd be good to know if it makes sense to someone not in the weeds of transaction state tracking)

Code review ACK 8242b093d3db52b7e443d650bafd307a03527639 Spent some time looking at the context and history of the issue. The changes look straightforward and the documentation is helpful and very welcome, as are the recent test additions in #18878 to feature_notifications.py. Built with both gcc and clang with the works; all clean. Feel free to ignore the nits below.

Verified that the test change at line 136 raises if it is reverted...

<details><summary>reverted test, info log output on PR branch</summary><p>

(pr/18982) $ test/functional/feature_notifications.py 
2020-05-22T15:38:19.760000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_2_pvzpci
2020-05-22T15:38:20.900000Z TestFramework (INFO): test -blocknotify
2020-05-22T15:38:21.062000Z TestFramework (INFO): test -walletnotify
2020-05-22T15:38:21.419000Z TestFramework (INFO): test -walletnotify after rescan
2020-05-22T15:38:22.009000Z TestFramework (INFO): test -walletnotify with conflicting transactions
2020-05-22T15:38:36.142000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 114, in main
    self.run_test()
  File "test/functional/feature_notifications.py", line 136, in run_test
    self.expect_wallet_notify([bump2])
  File "test/functional/feature_notifications.py", line 143, in expect_wallet_notify
    assert_equal(sorted(notify_outputname(self.wallet, tx_id) for tx_id in tx_ids), sorted(os.listdir(self.walletnotify_dir)))
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 46, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(['\x01\x02\x03\x04\x05\x06\x07\x08\t\n\x0b\x0c\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !"#$%&\'()*+,-.0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\x7f_63b821933781f3f5fe4fd35403579224391d753e74da0e1a517d3d1953a6d29b'] == ['\x01\x02\x03\x04\x05\x06\x07\x08\t\n\x0b\x0c\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !"#$%&\'()*+,-.0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\x7f_0084df402ed3ce44fdfb4e039b747cb6e4910f5d75d29fe0e3630dd4eded49be', '\x01\x02\x03\x04\x05\x06\x07\x08\t\n\x0b\x0c\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f !"#$%&\'()*+,-.0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\x7f_63b821933781f3f5fe4fd35403579224391d753e74da0e1a517d3d1953a6d29b'])
2020-05-22T15:38:36.194000Z TestFramework (INFO): Stopping nodes
2020-05-22T15:38:36.501000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_2_pvzpci
2020-05-22T15:38:36.501000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_2_pvzpci/test_framework.log
2020-05-22T15:38:36.501000Z TestFramework (ERROR): Hint: Call /home/jon/projects/bitcoin/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_2_pvzpci' to consolidate all logs

</p></details>

...and that the updated test also raises on current master...

<details><summary>updated test, info log output on master</summary><p>

(master) $ test/functional/feature_notifications.py 
2020-05-22T15:54:15.370000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_ygdr_dfe
2020-05-22T15:54:16.223000Z TestFramework (INFO): test -blocknotify
2020-05-22T15:54:16.420000Z TestFramework (INFO): test -walletnotify
2020-05-22T15:54:16.678000Z TestFramework (INFO): test -walletnotify after rescan
2020-05-22T15:54:17.321000Z TestFramework (INFO): test -walletnotify with conflicting transactions
2020-05-22T15:54:43.669000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
        wait_until(lambda: len(os.listdir(self.walletnotify_dir)) >= len(tx_ids), timeout=10)
'''
2020-05-22T15:54:43.669000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 116, in main
    self.run_test()
  File "test/functional/feature_notifications.py", line 141, in run_test
    self.expect_wallet_notify([bump2, tx2])
  File "test/functional/feature_notifications.py", line 147, in expect_wallet_notify
    wait_until(lambda: len(os.listdir(self.walletnotify_dir)) >= len(tx_ids), timeout=10)
  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 235, in wait_until
    raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
AssertionError: Predicate ''''
        wait_until(lambda: len(os.listdir(self.walletnotify_dir)) >= len(tx_ids), timeout=10)
''' not true after 10.0 seconds
2020-05-22T15:54:43.720000Z TestFramework (INFO): Stopping nodes
2020-05-22T15:54:44.125000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_ygdr_dfe
2020-05-22T15:54:44.125000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_ygdr_dfe/test_framework.log
2020-05-22T15:54:44.126000Z TestFramework (ERROR): Hint: Call /home/jon/projects/bitcoin/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_ygdr_dfe' to consolidate all logs

</p></details>

My main question is, should the CWalletTx::Status enum documentation in src/wallet/wallet.h::L367-373 be updated in this PR?

Also, AFAICT the added documentation in this PR coherently describes what the code is doing.

Thanks for reviewing and testing!

Updated 8242b093d3db52b7e443d650bafd307a03527639 -> 7eaf86d3bfc83f2beb3ef449707d5156853126fb (pr/cblock.4 -> pr/cblock.5, compare) with suggested cleanups

re: #18982#pullrequestreview-416974841

My main question is, should the CWalletTx::Status enum documentation in src/wallet/wallet.h::L367-373 be updated in this PR?

Forgot about that! Everything there still looks up to date, I think. I could expand the comment to mention behavior here, but it seems like more detail than would actually be helpful, since it's describing things at a pretty high level. I think there will be more cleanups later to state tracking so we should be able to revisit this.

Re-ACK 7eaf86d3bfc83f

<details><summary>changes since last review</summary><p>

((HEAD detached at origin/pr/18982))$ git diff 8242b09 7eaf86d
diff --git a/doc/release-notes-18982.md b/doc/release-notes-18982.md
index ee15a1070b..d1b8528d13 100644
--- a/doc/release-notes-18982.md
+++ b/doc/release-notes-18982.md
@@ -3,6 +3,6 @@ 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.19 release, but have been
+notifications were sent previously before the v0.19 release, but had been
 broken since that release (bug
 [#18325](https://github.com/bitcoin/bitcoin/issues/18325)).
diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
index 22d13704fb..3dfbcc581c 100644
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -199,22 +199,22 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd
                           fInitialDownload);
 }
 
-void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
-    auto event = [ptx, this] {
-        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(ptx); });
+void CMainSignals::TransactionAddedToMempool(const CTransactionRef& tx) {
+    auto event = [tx, this] {
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(tx); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s", __func__,
-                          ptx->GetHash().ToString(),
-                          ptx->GetWitnessHash().ToString());
+                          tx->GetHash().ToString(),
+                          tx->GetWitnessHash().ToString());
 }
 
-void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {
-    auto event = [ptx, reason, this] {
-        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(ptx, reason); });
+void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {
+    auto event = [tx, reason, this] {
+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(tx, reason); });
     };
     ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s", __func__,
-                          ptx->GetHash().ToString(),
-                          ptx->GetWitnessHash().ToString());
+                          tx->GetHash().ToString(),
+                          tx->GetWitnessHash().ToString());
 }
 
 void CMainSignals::BlockConnected(const std::shared_ptr<const CBlock> &pblock, const CBlockIndex *pindex) {
diff --git a/src/validationinterface.h b/src/validationinterface.h
index b35f01c4bb..e96f2883fc 100644
--- a/src/validationinterface.h
+++ b/src/validationinterface.h
@@ -97,7 +97,7 @@ protected:
      *
      * Called on a background thread.
      */
-    virtual void TransactionAddedToMempool(const CTransactionRef &ptxn) {}
+    virtual void TransactionAddedToMempool(const CTransactionRef& tx) {}
     /**
      * Notifies listeners of a transaction leaving mempool.
      *
@@ -130,7 +130,7 @@ protected:
      *
      * Called on a background thread.
      */
-    virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {}
+    virtual void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {}
     /**
      * Notifies listeners of a block being connected.
      * Provides a vector of transactions evicted from the mempool as a result.
@@ -197,8 +197,8 @@ public:
 
 
     void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload);
-    void TransactionAddedToMempool(const CTransactionRef &);
-    void TransactionRemovedFromMempool(const CTransactionRef &, MemPoolRemovalReason);
+    void TransactionAddedToMempool(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> &, const CBlockIndex* pindex);
     void ChainStateFlushed(const CBlockLocator &);
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 88a9e62259..862eb9b77f 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1101,20 +1101,19 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Confirmatio
     MarkInputsDirty(ptx);
 }
 
-void CWallet::transactionAddedToMempool(const CTransactionRef& ptx) {
+void CWallet::transactionAddedToMempool(const CTransactionRef& tx) {
     LOCK(cs_wallet);
-    CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0);
-    SyncTransaction(ptx, confirm);
+    SyncTransaction(tx, {CWalletTx::Status::UNCONFIRMED, /* block height */ 0, /* block hash */ {}, /* index */ 0});
 
-    auto it = mapWallet.find(ptx->GetHash());
+    auto it = mapWallet.find(tx->GetHash());
     if (it != mapWallet.end()) {
         it->second.fInMempool = true;
     }
 }
 
-void CWallet::transactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {
+void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {
     LOCK(cs_wallet);
-    auto it = mapWallet.find(ptx->GetHash());
+    auto it = mapWallet.find(tx->GetHash());
     if (it != mapWallet.end()) {
         it->second.fInMempool = false;
     }
@@ -1145,8 +1144,8 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolR
         // 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, /* hashBlock= */ {}, /* nIndex= */ 0});
+        // https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking
+        SyncTransaction(tx, {CWalletTx::Status::UNCONFIRMED, /* block height */ 0, /* block hash */ {}, /* index */ 0});
     }
 }
 
@@ -1158,8 +1157,7 @@ void CWallet::blockConnected(const CBlock& block, int height)
     m_last_block_processed_height = height;
     m_last_block_processed = block_hash;
     for (size_t index = 0; index < block.vtx.size(); index++) {
-        CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, height, block_hash, index);
-        SyncTransaction(block.vtx[index], confirm);
+        SyncTransaction(block.vtx[index], {CWalletTx::Status::CONFIRMED, height, block_hash, (int)index});
         transactionRemovedFromMempool(block.vtx[index], MemPoolRemovalReason::BLOCK);
     }
 }
@@ -1175,8 +1173,7 @@ void CWallet::blockDisconnected(const CBlock& block, int height)
     m_last_block_processed_height = height - 1;
     m_last_block_processed = block.hashPrevBlock;
     for (const CTransactionRef& ptx : block.vtx) {
-        CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0);
-        SyncTransaction(ptx, confirm);
+        SyncTransaction(ptx, {CWalletTx::Status::UNCONFIRMED, /* block height */ 0, /* block hash */ {}, /* index */ 0});
     }
 }
 
@@ -1716,8 +1713,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
                 break;
             }
             for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
-                CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, block_height, block_hash, posInBlock);
-                SyncTransaction(block.vtx[posInBlock], confirm, fUpdate);
+                SyncTransaction(block.vtx[posInBlock], {CWalletTx::Status::CONFIRMED, block_height, block_hash, (int)posInBlock}, fUpdate);
             }
             // scan succeeded, record block as most recent successfully scanned
             result.last_scanned_block = block_hash;
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 7d605beb78..350d731b83 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -923,7 +923,7 @@ public:
         uint256 last_failed_block;
     };
     ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, Optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate);
-    void transactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) override;
+    void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) override;
     void ReacceptWalletTransactions() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

</p></details>

Reviewed changes, built, ran tests and bitcoind. Thanks, Russ.

ACK 7eaf86d, reviewed, built and ran tests.

Thanks for stretching my fix to the minimal required !

Side-note: For backport only the first commit is required

ACK 7eaf86d3bfc83f2beb3ef449707d5156853126fb 🍡

<details><summary>Show signature and timestamp</summary>

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 7eaf86d3bfc83f2beb3ef449707d5156853126fb 🍡
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgdEgwAmst0UkNcIHGyE+2chyXer0u8xtpS5iFBAbP0JmCdo/ey/SOl7fKQZwkR
CuUvaVT5MK7gCpjTDOgBAME7auWgNIMdckCAvCktHwAzfklsDKfdl9MTyq6RSgWH
unE6zZ6PN7gY/DwgHyOtFh+UtYyfs3TGvuQE2IWLz9gTqUXzvAKnFvmjLLbfjWpT
NmLvh5m2e8LAw4iT/L7Qchjc4ZSkXWx1Eo8RejGcVF+gItKozUG5JL6FFXKEm70A
bpQpqze2gt9qvk+rvEHyeCU43kF5ON7afuh4FfP1tumJjGxuJSJUm7+rWgq3oM5G
ru6niN3NK57OEcRevDumkraG7EIMrjVgG4L6r9SsPDzTd3jEYnCjrPwGrm/zokS9
WgRizfSxXoqkGt+4qiuJkkC0iXtvzhcbIk1N/MAQwnRMj+NJ/I7BXG/L9MYf6G82
sJpNFq0i5Ww+W0V0MBIxgWm/ONkmp9GIeZ9+zo+ggHnB1LDIZTCtWJjtiVqbRrC1
B4KFoTs1
=CPHq
-----END PGP SIGNATURE-----

Timestamp of file with hash 7bc81c048dd3a3d462e02deb3c82e3c23ceb049acdcd359e6b7b9b2440608e3b -

</details>