wallet: Minimal fix to restore conflicted transaction notifications #18982

ryanofsky · 2020-05-15T13:41:43Z

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: a31be09 and 7e89994 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)

This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in 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: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard <ariard@student.42.fr>

DrahtBot · 2020-05-15T13:58:31Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

ariard

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.

src/validationinterface.h

src/wallet/wallet.cpp

ryanofsky

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

src/validationinterface.h

src/wallet/wallet.cpp

ryanofsky · 2020-05-21T11:53:56Z

(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)

jonatack

Code review ACK 8242b09 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...

reverted test, info log output on PR branch

(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

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

updated test, info log output on master

(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

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

src/wallet/wallet.cpp

src/validationinterface.cpp

src/wallet/wallet.cpp

jonatack · 2020-05-22T16:25:40Z

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

bitcoin#18982 (review)

ryanofsky

Thanks for reviewing and testing!

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

re: #18982 (review)

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.

src/validationinterface.cpp

src/wallet/wallet.cpp

jonatack · 2020-05-24T12:13:37Z

Re-ACK 7eaf86d

changes since last review

((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);

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

ariard · 2020-05-24T23:49:25Z

ACK 7eaf86d, reviewed, built and ran tests.

Thanks for stretching my fix to the minimal required !

maflcko · 2020-06-02T22:11:21Z

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

ACK 7eaf86d 🍡

Show signature and timestamp

Signature:

Timestamp of file with hash 7bc81c048dd3a3d462e02deb3c82e3c23ceb049acdcd359e6b7b9b2440608e3b -

…ction notifications 7eaf86d trivial: Suggested cleanups to surrounding code (Russell Yanofsky) b604c5c wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky) Pull request description: This fix is a based on the fix by Antoine Riard (ariard) in 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: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard (ariard) ACKs for top commit: jonatack: Re-ACK 7eaf86d ariard: ACK 7eaf86d, reviewed, built and ran tests. MarcoFalke: ACK 7eaf86d 🍡 Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689

This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in 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: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard <ariard@student.42.fr> Github-Pull: bitcoin#18982 Rebased-From: b604c5c

bitcoin#18982 (review) Github-Pull: bitcoin#18982 Rebased-From: 7eaf86d

This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in 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: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard <ariard@student.42.fr> Github-Pull: bitcoin#18982 Rebased-From: b604c5c

…ction notifications 7eaf86d trivial: Suggested cleanups to surrounding code (Russell Yanofsky) b604c5c wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky) Pull request description: This fix is a based on the fix by Antoine Riard (ariard) in 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: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard (ariard) ACKs for top commit: jonatack: Re-ACK 7eaf86d ariard: ACK 7eaf86d, reviewed, built and ran tests. MarcoFalke: ACK 7eaf86d dango Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689

bitcoin#18982 (review)

2b79ac7 Clean up separated ban/discourage interface (Pieter Wuille) 0477348 Replace automatic bans with discouragement filter (Pieter Wuille) e7f06f9 test: remove Cirrus CI FreeBSD job (fanquake) eb6b82a qa: Test concurrent wallet loading (João Barbosa) c9b49d2 wallet: Handle concurrent wallet loading (João Barbosa) cf0b5a9 tests: Check that segwit inputs in psbt have both UTXO types (Andrew Chow) 3228b59 psbt: always put a non_witness_utxo and don't remove it (Andrew Chow) ed5ec30 psbt: Allow both non_witness_utxo and witness_utxo (Andrew Chow) 68e0e6f rpc: show both UTXOs in decodepsbt (Andrew Chow) 27786d0 trivial: Suggested cleanups to surrounding code (Russell Yanofsky) 654420d wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky) febebc4 Fix WSL file locking by using flock instead of fcntl (Samuel Dobson) 5c7151a gui: update Qt base translations for macOS release (fanquake) c219d21 build: improved output of configure for build OS (sachinkm77) 0596a6e util: Don't reference errno when pthread fails. (MIZUTA Takeshi) Pull request description: Currently backports the following to the 0.20 branch: * #18700 - Fix locking on WSL using flock instead of fcntl * #18982 - wallet: Minimal fix to restore conflicted transaction notifications * #19059 - gui: update Qt base translations for macOS release * #19152 - build: improve build OS configure output * #19194 - util: Don't reference errno when pthread fails. * #19215 - psbt: Include and allow both non_witness_utxo and witness_utxo for segwit inputs * #19219 - Replace automatic bans with discouragement filter * #19300 - wallet: Handle concurrent wallet loading ACKs for top commit: amitiuttarwar: ACK 0477348 2b79ac7 by comparing to original changes, double checking the diff sipa: utACK 2b79ac7 laanwj: ACK 2b79ac7 Tree-SHA512: e5eb31d08288fa4a6e8aba08e60b16ce1988f14f249238b1cfd18ab2c8f6fcd9f07e3c0c491d32d2361fca26e3037fb0374f37464bddcabecea29069d6737539

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: a31be09 and 7e89994 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. Fixes #18325 Co-authored-by: Antoine Riard <ariard@student.42.fr> Github-Pull: #18982 Rebased-From: b604c5c

bitcoin/bitcoin#18982 (review) Github-Pull: #18982 Rebased-From: 7eaf86d

This fix is a based on the fix by Antoine Riard <ariard@student.42.fr> in 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: a31be09 and 7e89994 from bitcoin#16624, causing issue bitcoin#18325. The change here could be improved and replaced with a more comprehensive cleanup, so it includes a detailed comment explaining future considerations. Fixes bitcoin#18325 Co-authored-by: Antoine Riard <ariard@student.42.fr> Github-Pull: bitcoin#18982 Rebased-From: b604c5c

bitcoin#18982 (review) Github-Pull: bitcoin#18982 Rebased-From: 7eaf86d

bitcoin/bitcoin#18982 (review)

…fication fix f2734f0 [Tests] Fix policyestimator and validation_block tests (random-zebra) 3448bc8 wallet: Minimal fix to restore conflicted transaction notifications (Antoine Riard) ba9d84d feature_notifications.py mimic upstream is_wallet_compiled() function to make future back ports easier. (furszy) 37c9944 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery) 5cc619f [validation] Remove pool member from ConnectTrace (John Newbery) 1e453b7 [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery) e774bfd [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery) 389680a [validation interface] Remove vtxConflicted from BlockConnected (furszy) 19c9383 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery) 139882d Fire TransactionRemovedFromMempool from mempool This commit fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code. (furszy) Pull request description: PR built on top of #2209, #2235 and #2240 (don't get scared by the amount of commits, most of them are coming from 2209. Will disappear as soon as that one gets merged). Based on the brief talk originated in #2209 (comment) . Solving the conflicted transactions external listeners notification. Adapting the following PRs: bitcoin#14384, bitcoin#17477 and bitcoin#18982. Without functional test support, for the time being, for the lack of RBF functionality. ACKs for top commit: Fuzzbawls: ACK f2734f0 random-zebra: re-utACK f2734f0 and merging... Tree-SHA512: 07d437e0d5e5d9798b16d982b6f58585d1f0dcd82251c5ac06f47e44233433831243d451ce43068e33c6002a64b76fa4d165167a2cbaeeedc28cde2019853565

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

Summary: bitcoin/bitcoin#18982 (review) This is a backport of [[bitcoin/bitcoin#18982 | core#18982]] [2/2] bitcoin/bitcoin@7eaf86d Depends on D9962 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9963

fanquake added the Wallet label May 15, 2020

ryanofsky mentioned this pull request May 15, 2020

Restore UI notifications and -walletnotify behavior for block conflicted transactions #18325

Closed

ryanofsky force-pushed the pr/cblock branch from 261ccf7 to 58ec222 Compare May 15, 2020 14:05

maflcko added the Needs backport (0.20) label May 15, 2020

ryanofsky force-pushed the pr/cblock branch from 58ec222 to 90118ec Compare May 15, 2020 14:11

ariard reviewed May 20, 2020

View reviewed changes

src/validationinterface.h Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/cblock branch from 90118ec to 8242b09 Compare May 21, 2020 11:43

ryanofsky commented May 21, 2020

View reviewed changes

src/validationinterface.h Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

ryanofsky requested a review from jonatack May 21, 2020 11:51

jonatack reviewed May 22, 2020

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/validationinterface.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

trivial: Suggested cleanups to surrounding code

7eaf86d

bitcoin#18982 (review)

ryanofsky force-pushed the pr/cblock branch from 8242b09 to 7eaf86d Compare May 22, 2020 20:40

ryanofsky commented May 22, 2020

View reviewed changes

src/validationinterface.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request May 31, 2020

[WIP] wallet: use BlockFilterIndex in ScanForWalletTransactions #19116

Closed

maflcko merged commit 3657aee into bitcoin:master Jun 2, 2020

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 9, 2020

trivial: Suggested cleanups to surrounding code

27786d0

bitcoin#18982 (review) Github-Pull: bitcoin#18982 Rebased-From: 7eaf86d

fanquake mentioned this pull request Jun 9, 2020

[0.20] Backports #19224

Merged

fanquake removed the Needs backport (0.20) label Jun 9, 2020

stackman27 pushed a commit to stackman27/bitcoin that referenced this pull request Jun 26, 2020

trivial: Suggested cleanups to surrounding code

8bfa706

bitcoin#18982 (review)

backpacker69 pushed a commit to peercoin/peercoin that referenced this pull request Sep 8, 2020

trivial: Suggested cleanups to surrounding code

bf9f844

bitcoin/bitcoin#18982 (review) Github-Pull: #18982 Rebased-From: 7eaf86d

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Oct 21, 2020

trivial: Suggested cleanups to surrounding code

11f44af

bitcoin#18982 (review) Github-Pull: bitcoin#18982 Rebased-From: 7eaf86d

Platinumwrist referenced this pull request in syscoin/syscoin Oct 25, 2020

doc: Add historical release notes for 0.20.1

05e2740

janus pushed a commit to janus/bitgesell that referenced this pull request Nov 15, 2020

trivial: Suggested cleanups to surrounding code

df1ce62

bitcoin/bitcoin#18982 (review)

This was referenced Jan 7, 2021

wallet: encapsulate transactions state #16624

Merged

Marking chains of txs conflicted properly #8692

Closed

random-zebra mentioned this pull request Mar 3, 2021

Killing wallet and GUI cs_main locks PIVX-Project/PIVX#2209

Merged

furszy mentioned this pull request Mar 10, 2021

[wallet] Conflicted transactions external listeners notification fix PIVX-Project/PIVX#2241

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

wallet: Minimal fix to restore conflicted transaction notifications #18982

wallet: Minimal fix to restore conflicted transaction notifications #18982

Uh oh!

Conversation

ryanofsky commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented May 22, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard commented May 24, 2020

Uh oh!

maflcko commented Jun 2, 2020

Uh oh!

Uh oh!

ryanofsky commented May 15, 2020 •

edited

Loading

DrahtBot commented May 15, 2020 •

edited

Loading

ryanofsky commented May 21, 2020 •

edited

Loading

jonatack commented May 24, 2020 •

edited

Loading