wallet: Minimal fix to restore conflicted transaction notifications #18982

pull ryanofsky wants to merge 2 commits into bitcoin:master from ryanofsky:pr/cblock changing 9 files +71 −39
  1. ryanofsky commented at 1:41 pm on May 15, 2020: member

    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)

  2. wallet: Minimal fix to restore conflicted transaction notifications 
    This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in
https://github.com/bitcoin/bitcoin/pull/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
https://github.com/bitcoin/bitcoin/pull/16624, causing issue
https://github.com/bitcoin/bitcoin/issues/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@student.42.fr>
    b604c5c8b5
  3. fanquake added the label Wallet on May 15, 2020
  4. DrahtBot commented at 1:58 pm on May 15, 2020: member

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

    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.

  5. ryanofsky force-pushed on May 15, 2020
  6. MarcoFalke added the label Needs backport (0.20) on May 15, 2020
  7. ryanofsky force-pushed on May 15, 2020
  8. in src/validationinterface.h:24 in 90118ec54f outdated 
    20@@ -21,6 +21,7 @@ class CConnman;
21 class CValidationInterface;
22 class uint256;
23 class CScheduler;
24+enum class MemPoolRemovalReason;
    ariard commented at 7:29 am on May 20, 2020:
    I think we can dry-up MemPoolRemovalReason completely in the future. The only control flow pending on it I can see is in removeUnchecked and it’s only consumed by TransactionRemovedFromMempool
    ryanofsky commented at 11:16 pm on May 20, 2020:

    re: #18982 (review)

    I think we can dry-up MemPoolRemovalReason completely in the future. The only control flow pending on it I can see is in removeUnchecked and it’s only consumed by TransactionRemovedFromMempool

    I’m open to removing MemPoolRemovalReason in the future if it simplifies code, hopefully without losing debug or logging benefits or code clarity from using custom enum values instead of generic true/false values

  9. in src/wallet/wallet.cpp:1133 in 90118ec54f outdated 
    1129+        // 1. The transactionRemovedFromMempool callback does not currently
1130+        //    provide the conflicting block's hash and height, and for backwards
1131+        //    compatibility reasons it may not be not safe to store conflicted
1132+        //    wallet transactions with a null block hash. See
1133+        //    https://github.com/bitcoin/bitcoin/pull/18600#discussion_r420195993.
1134+        // 2. For most of these transactions, wallet internal conflict detection
    ariard commented at 7:33 am on May 20, 2020:
    Can you explain which one are going to updated with CONFLICTED status and which not, i.e transactions not directly connected to a previous wallet transaction ?
    ryanofsky commented at 4:28 am on May 21, 2020:

    re: #18982 (review)

    Can you explain which one are going to updated with CONFLICTED status and which not, i.e transactions not directly connected to a previous wallet transaction ?

    Sure, added more details

  10. in src/wallet/wallet.cpp:1143 in 90118ec54f outdated 
    1139+        //    implementation before that was to mark these transactions
1140+        //    unconfirmed rather than conflicted.
1141+        //
1142+        // The wallet's heuristics for distinguishing between conflicted and
1143+        // unconfirmed transactions are imperfect, so everything described above
1144+        // could be rethought and changed in the future.
    ariard commented at 7:34 am on May 20, 2020:
    Also, document that conflict transaction tracking will always be a best-effort, an unconfirmed transaction may be removed while wallet is shutdown an therefore never getting the notification.
    ryanofsky commented at 11:17 pm on May 20, 2020:

    re: #18982 (review)

    Also, document that conflict transaction tracking will always be a best-effort, an unconfirmed transaction may be removed while wallet is shutdown an therefore never getting the notification.

    The shutdown thing seems like a good thing to document, but this doesn’t seem like a great place to document it. This comment, which is already pretty long, is just trying to say why this piece of code is written this way, and say that it isn’t unchangeable without getting into how future improvements need to work.

    But I made a new devwiki page https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking, and added a link here that I think would be a good place to document complications from shutdown. By the way, I’m not sure if shutdowns would be the main cause of problems in practice, since startup block and mempool rescanning is pretty good and there’s a whole other set of problems that comes from the wallet having incomplete information about ancestors and whether wallet transactions are related to block and mempool transactions, which is even worse for detecting conflicted -> unconfimed transitions than for detecting unconfirmed -> conflicted transitions.

  11. ariard commented at 7:36 am on May 20, 2020: member

    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.

  12. ryanofsky force-pushed on May 21, 2020
  13. ryanofsky commented at 11:47 am on May 21, 2020: member
    Updated 90118ec54f6921ed102ef45bb4f4d3a0ac8782b8 -> 8242b093d3db52b7e443d650bafd307a03527639 (pr/cblock.3 -> pr/cblock.4, compare) making suggested changes and also adding release notes
  14. ryanofsky requested review from jonatack on May 21, 2020
  15. ryanofsky commented at 11:53 am on May 21, 2020: member
    (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)
  16. in src/wallet/wallet.cpp:1148 in 8242b093d3 outdated 
    1144+        //
1145+        // Nothing described above should be seen as an unchangeable requirement
1146+        // when improving this code in the future. The wallet's heuristics for
1147+        // distinguishing between conflicted and unconfirmed transactions are
1148+        // imperfect, and could be improved in general, see
1149+        // https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking,
    jonatack commented at 2:54 pm on May 22, 2020:

    nit: remove EOL comma, otherwise some people following the link may end up here :)

    Screenshot from 2020-05-22 16-50-24

    ryanofsky commented at 7:54 pm on May 22, 2020:

    re: #18982 (review)

    nit: remove EOL comma, otherwise some people following the link may end up here :)

    Thanks! Removed

  17. in src/validationinterface.cpp:211 in 8242b093d3 outdated 
    207@@ -208,9 +208,9 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
208                           ptx->GetWitnessHash().ToString());
209 }
210 
211-void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx) {
212-    auto event = [ptx, this] {
213-        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(ptx); });
214+void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {
    jonatack commented at 3:19 pm on May 22, 2020:

    nit: is ptx a pointer?

    0-const CTransactionRef &ptx
1+const CTransactionRef& tx
    ryanofsky commented at 7:53 pm on May 22, 2020:

    re: #18982 (review)

    nit: is ptx a pointer?

    0-const CTransactionRef &ptx
1+const CTransactionRef& tx

    Thanks, changed. It is a pointer (shared_ptr), as well: https://github.com/bitcoin/bitcoin/blob/b5c423c48e094bd098e11c3d1f57acae7502a4da/src/primitives/transaction.h#L387

  18. in src/wallet/wallet.cpp:1161 in 8242b093d3 outdated 
    1159@@ -1129,7 +1160,7 @@ void CWallet::blockConnected(const CBlock& block, int height)
1160     for (size_t index = 0; index < block.vtx.size(); index++) {
1161         CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, height, block_hash, index);
    jonatack commented at 3:36 pm on May 22, 2020:

    nit: suggested nicety while touching this code, at lines 1106 and 1178:

    0- CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0);
1+ CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, /* hashBlock= */ {}, /* nIndex */ 0);
    ryanofsky commented at 7:54 pm on May 22, 2020:

    re: #18982 (review)

    nit: suggested nicety while touching this code, at lines 1106 and 1178:

    0- CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0);
1+ CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, /* hashBlock= */ {}, /* nIndex */ 0);

    Thanks, changed

  19. jonatack commented at 4:19 pm on May 22, 2020: member

    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…

     0(pr/18982) $ test/functional/feature_notifications.py 
 12020-05-22T15:38:19.760000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_2_pvzpci
 22020-05-22T15:38:20.900000Z TestFramework (INFO): test -blocknotify
 32020-05-22T15:38:21.062000Z TestFramework (INFO): test -walletnotify
 42020-05-22T15:38:21.419000Z TestFramework (INFO): test -walletnotify after rescan
 52020-05-22T15:38:22.009000Z TestFramework (INFO): test -walletnotify with conflicting transactions
 62020-05-22T15:38:36.142000Z TestFramework (ERROR): Assertion failed
 7Traceback (most recent call last):
 8  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 114, in main
 9    self.run_test()
10  File "test/functional/feature_notifications.py", line 136, in run_test
11    self.expect_wallet_notify([bump2])
12  File "test/functional/feature_notifications.py", line 143, in expect_wallet_notify
13    assert_equal(sorted(notify_outputname(self.wallet, tx_id) for tx_id in tx_ids), sorted(os.listdir(self.walletnotify_dir)))
14  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 46, in assert_equal
15    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
16AssertionError: 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'])
172020-05-22T15:38:36.194000Z TestFramework (INFO): Stopping nodes
182020-05-22T15:38:36.501000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_2_pvzpci
192020-05-22T15:38:36.501000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_2_pvzpci/test_framework.log
202020-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

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

     0(master) $ test/functional/feature_notifications.py 
 12020-05-22T15:54:15.370000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_ygdr_dfe
 22020-05-22T15:54:16.223000Z TestFramework (INFO): test -blocknotify
 32020-05-22T15:54:16.420000Z TestFramework (INFO): test -walletnotify
 42020-05-22T15:54:16.678000Z TestFramework (INFO): test -walletnotify after rescan
 52020-05-22T15:54:17.321000Z TestFramework (INFO): test -walletnotify with conflicting transactions
 62020-05-22T15:54:43.669000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
 7        wait_until(lambda: len(os.listdir(self.walletnotify_dir)) >= len(tx_ids), timeout=10)
 8'''
 92020-05-22T15:54:43.669000Z TestFramework (ERROR): Assertion failed
10Traceback (most recent call last):
11  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 116, in main
12    self.run_test()
13  File "test/functional/feature_notifications.py", line 141, in run_test
14    self.expect_wallet_notify([bump2, tx2])
15  File "test/functional/feature_notifications.py", line 147, in expect_wallet_notify
16    wait_until(lambda: len(os.listdir(self.walletnotify_dir)) >= len(tx_ids), timeout=10)
17  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 235, in wait_until
18    raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
19AssertionError: Predicate ''''
20        wait_until(lambda: len(os.listdir(self.walletnotify_dir)) >= len(tx_ids), timeout=10)
21''' not true after 10.0 seconds
222020-05-22T15:54:43.720000Z TestFramework (INFO): Stopping nodes
232020-05-22T15:54:44.125000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_ygdr_dfe
242020-05-22T15:54:44.125000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_ygdr_dfe/test_framework.log
252020-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

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

  20. jonatack commented at 4:25 pm on May 22, 2020: member
    Also, AFAICT the added documentation in this PR coherently describes what the code is doing.
  21. trivial: Suggested cleanups to surrounding code 
    https://github.com/bitcoin/bitcoin/pull/18982#pullrequestreview-416974841
    7eaf86d3bf
  22. ryanofsky force-pushed on May 22, 2020
  23. ryanofsky commented at 8:48 pm on May 22, 2020: member

    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.

  24. jonatack commented at 12:13 pm on May 24, 2020: member

    Re-ACK 7eaf86d3bfc83f

      0((HEAD detached at origin/pr/18982))$ git diff 8242b09 7eaf86d
  1diff --git a/doc/release-notes-18982.md b/doc/release-notes-18982.md
  2index ee15a1070b..d1b8528d13 100644
  3--- a/doc/release-notes-18982.md
  4+++ b/doc/release-notes-18982.md
  5@@ -3,6 +3,6 @@ Notification changes
  6 
  7 `-walletnotify` notifications are now sent for wallet transactions that are
  8 removed from the mempool because they conflict with a new block. These
  9-notifications were sent previously before the v0.19 release, but have been
 10+notifications were sent previously before the v0.19 release, but had been
 11 broken since that release (bug
 12 [#18325](https://github.com/bitcoin/bitcoin/issues/18325)).
 13diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
 14index 22d13704fb..3dfbcc581c 100644
 15--- a/src/validationinterface.cpp
 16+++ b/src/validationinterface.cpp
 17@@ -199,22 +199,22 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd
 18                           fInitialDownload);
 19 }
 20 
 21-void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
 22-    auto event = [ptx, this] {
 23-        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(ptx); });
 24+void CMainSignals::TransactionAddedToMempool(const CTransactionRef& tx) {
 25+    auto event = [tx, this] {
 26+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionAddedToMempool(tx); });
 27     };
 28     ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s", __func__,
 29-                          ptx->GetHash().ToString(),
 30-                          ptx->GetWitnessHash().ToString());
 31+                          tx->GetHash().ToString(),
 32+                          tx->GetWitnessHash().ToString());
 33 }
 34 
 35-void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {
 36-    auto event = [ptx, reason, this] {
 37-        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(ptx, reason); });
 38+void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {
 39+    auto event = [tx, reason, this] {
 40+        m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(tx, reason); });
 41     };
 42     ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s", __func__,
 43-                          ptx->GetHash().ToString(),
 44-                          ptx->GetWitnessHash().ToString());
 45+                          tx->GetHash().ToString(),
 46+                          tx->GetWitnessHash().ToString());
 47 }
 48 
 49 void CMainSignals::BlockConnected(const std::shared_ptr<const CBlock> &pblock, const CBlockIndex *pindex) {
 50diff --git a/src/validationinterface.h b/src/validationinterface.h
 51index b35f01c4bb..e96f2883fc 100644
 52--- a/src/validationinterface.h
 53+++ b/src/validationinterface.h
 54@@ -97,7 +97,7 @@ protected:
 55      *
 56      * Called on a background thread.
 57      */
 58-    virtual void TransactionAddedToMempool(const CTransactionRef &ptxn) {}
 59+    virtual void TransactionAddedToMempool(const CTransactionRef& tx) {}
 60     /**
 61      * Notifies listeners of a transaction leaving mempool.
 62      *
 63@@ -130,7 +130,7 @@ protected:
 64      *
 65      * Called on a background thread.
 66      */
 67-    virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {}
 68+    virtual void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {}
 69     /**
 70      * Notifies listeners of a block being connected.
 71      * Provides a vector of transactions evicted from the mempool as a result.
 72@@ -197,8 +197,8 @@ public:
 73 
 74 
 75     void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload);
 76-    void TransactionAddedToMempool(const CTransactionRef &);
 77-    void TransactionRemovedFromMempool(const CTransactionRef &, MemPoolRemovalReason);
 78+    void TransactionAddedToMempool(const CTransactionRef&);
 79+    void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason);
 80     void BlockConnected(const std::shared_ptr<const CBlock> &, const CBlockIndex *pindex);
 81     void BlockDisconnected(const std::shared_ptr<const CBlock> &, const CBlockIndex* pindex);
 82     void ChainStateFlushed(const CBlockLocator &);
 83diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 84index 88a9e62259..862eb9b77f 100644
 85--- a/src/wallet/wallet.cpp
 86+++ b/src/wallet/wallet.cpp
 87@@ -1101,20 +1101,19 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Confirmatio
 88     MarkInputsDirty(ptx);
 89 }
 90 
 91-void CWallet::transactionAddedToMempool(const CTransactionRef& ptx) {
 92+void CWallet::transactionAddedToMempool(const CTransactionRef& tx) {
 93     LOCK(cs_wallet);
 94-    CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0);
 95-    SyncTransaction(ptx, confirm);
 96+    SyncTransaction(tx, {CWalletTx::Status::UNCONFIRMED, /* block height */ 0, /* block hash */ {}, /* index */ 0});
 97 
 98-    auto it = mapWallet.find(ptx->GetHash());
 99+    auto it = mapWallet.find(tx->GetHash());
100     if (it != mapWallet.end()) {
101         it->second.fInMempool = true;
102     }
103 }
104 
105-void CWallet::transactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {
106+void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {
107     LOCK(cs_wallet);
108-    auto it = mapWallet.find(ptx->GetHash());
109+    auto it = mapWallet.find(tx->GetHash());
110     if (it != mapWallet.end()) {
111         it->second.fInMempool = false;
112     }
113@@ -1145,8 +1144,8 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolR
114         // when improving this code in the future. The wallet's heuristics for
115         // distinguishing between conflicted and unconfirmed transactions are
116         // imperfect, and could be improved in general, see
117-        // https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking,
118-        SyncTransaction(ptx, {CWalletTx::Status::UNCONFIRMED, /* block_height= */ 0, /* hashBlock= */ {}, /* nIndex= */ 0});
119+        // https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking
120+        SyncTransaction(tx, {CWalletTx::Status::UNCONFIRMED, /* block height */ 0, /* block hash */ {}, /* index */ 0});
121     }
122 }
123 
124@@ -1158,8 +1157,7 @@ void CWallet::blockConnected(const CBlock& block, int height)
125     m_last_block_processed_height = height;
126     m_last_block_processed = block_hash;
127     for (size_t index = 0; index < block.vtx.size(); index++) {
128-        CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, height, block_hash, index);
129-        SyncTransaction(block.vtx[index], confirm);
130+        SyncTransaction(block.vtx[index], {CWalletTx::Status::CONFIRMED, height, block_hash, (int)index});
131         transactionRemovedFromMempool(block.vtx[index], MemPoolRemovalReason::BLOCK);
132     }
133 }
134@@ -1175,8 +1173,7 @@ void CWallet::blockDisconnected(const CBlock& block, int height)
135     m_last_block_processed_height = height - 1;
136     m_last_block_processed = block.hashPrevBlock;
137     for (const CTransactionRef& ptx : block.vtx) {
138-        CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0);
139-        SyncTransaction(ptx, confirm);
140+        SyncTransaction(ptx, {CWalletTx::Status::UNCONFIRMED, /* block height */ 0, /* block hash */ {}, /* index */ 0});
141     }
142 }
143 
144@@ -1716,8 +1713,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
145                 break;
146             }
147             for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
148-                CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, block_height, block_hash, posInBlock);
149-                SyncTransaction(block.vtx[posInBlock], confirm, fUpdate);
150+                SyncTransaction(block.vtx[posInBlock], {CWalletTx::Status::CONFIRMED, block_height, block_hash, (int)posInBlock}, fUpdate);
151             }
152             // scan succeeded, record block as most recent successfully scanned
153             result.last_scanned_block = block_hash;
154diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
155index 7d605beb78..350d731b83 100644
156--- a/src/wallet/wallet.h
157+++ b/src/wallet/wallet.h
158@@ -923,7 +923,7 @@ public:
159         uint256 last_failed_block;
160     };
161     ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, Optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate);
162-    void transactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) override;
163+    void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) override;
164     void ReacceptWalletTransactions() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

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

  25. ariard commented at 11:49 pm on May 24, 2020: member

    ACK 7eaf86d, reviewed, built and ran tests.

    Thanks for stretching my fix to the minimal required !

  26. MarcoFalke commented at 10:11 pm on June 2, 2020: member

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

    ACK 7eaf86d3bfc83f2beb3ef449707d5156853126fb 🍡

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 7eaf86d3bfc83f2beb3ef449707d5156853126fb 🍡
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgdEgwAmst0UkNcIHGyE+2chyXer0u8xtpS5iFBAbP0JmCdo/ey/SOl7fKQZwkR
 8CuUvaVT5MK7gCpjTDOgBAME7auWgNIMdckCAvCktHwAzfklsDKfdl9MTyq6RSgWH
 9unE6zZ6PN7gY/DwgHyOtFh+UtYyfs3TGvuQE2IWLz9gTqUXzvAKnFvmjLLbfjWpT
10NmLvh5m2e8LAw4iT/L7Qchjc4ZSkXWx1Eo8RejGcVF+gItKozUG5JL6FFXKEm70A
11bpQpqze2gt9qvk+rvEHyeCU43kF5ON7afuh4FfP1tumJjGxuJSJUm7+rWgq3oM5G
12ru6niN3NK57OEcRevDumkraG7EIMrjVgG4L6r9SsPDzTd3jEYnCjrPwGrm/zokS9
13WgRizfSxXoqkGt+4qiuJkkC0iXtvzhcbIk1N/MAQwnRMj+NJ/I7BXG/L9MYf6G82
14sJpNFq0i5Ww+W0V0MBIxgWm/ONkmp9GIeZ9+zo+ggHnB1LDIZTCtWJjtiVqbRrC1
15B4KFoTs1
16=CPHq
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 7bc81c048dd3a3d462e02deb3c82e3c23ceb049acdcd359e6b7b9b2440608e3b -

  27. MarcoFalke merged this on Jun 2, 2020
  28. MarcoFalke closed this on Jun 2, 2020
  29. sidhujag referenced this in commit 6007f5d865 on Jun 3, 2020
  30. fanquake referenced this in commit 654420d6df on Jun 9, 2020
  31. fanquake referenced this in commit 27786d072d on Jun 9, 2020
  32. fanquake removed the label Needs backport (0.20) on Jun 9, 2020
  33. luke-jr referenced this in commit 32d773bfc1 on Jun 9, 2020
  34. ComputerCraftr referenced this in commit 48245eb566 on Jun 10, 2020
  35. ComputerCraftr referenced this in commit 077eb62f7f on Jun 10, 2020
  36. stackman27 referenced this in commit 8bfa7061af on Jun 26, 2020
  37. fanquake referenced this in commit 01c563708f on Jul 10, 2020
  38. backpacker69 referenced this in commit 2463b02ccf on Sep 8, 2020
  39. backpacker69 referenced this in commit bf9f8441a5 on Sep 8, 2020
  40. Bushstar referenced this in commit 4770dc9d56 on Oct 21, 2020
  41. Bushstar referenced this in commit 11f44af7f1 on Oct 21, 2020
  42. Platinumwrist referenced this in commit 05e2740d92 on Oct 25, 2020
  43. janus referenced this in commit df1ce628bf on Nov 15, 2020
  44. random-zebra referenced this in commit e0350eda5b on Apr 14, 2021
  45. deadalnix referenced this in commit 250a0d5b25 on Aug 30, 2021
  46. Fabcien referenced this in commit baf58a5d75 on Aug 30, 2021
  47. DrahtBot locked this on Aug 18, 2022


ryanofsky DrahtBot ariard jonatack MarcoFalke

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me