diff options
author | Russell Yanofsky <russ@yanofsky.org> | 2020-05-15 09:23:55 -0400 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2020-06-09 21:12:38 +0800 |
commit | 654420d6dfb455ca4030055881db4e3aa9ec6e8b (patch) | |
tree | 15e81a6f0d90f0e011d6be5d2d3a3c8e0ce7a6bd | |
parent | febebc4ea68104bba9ad2cf4468fc50e6136f803 (diff) | |
download | bitcoin-654420d6dfb455ca4030055881db4e3aa9ec6e8b.tar.xz |
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>
Github-Pull: #18982
Rebased-From: b604c5c8b5892842f13dee89ae31812a28ab25d1
-rw-r--r-- | doc/release-notes-18982.md | 8 | ||||
-rw-r--r-- | src/interfaces/chain.cpp | 4 | ||||
-rw-r--r-- | src/interfaces/chain.h | 3 | ||||
-rw-r--r-- | src/txmempool.cpp | 2 | ||||
-rw-r--r-- | src/validationinterface.cpp | 6 | ||||
-rw-r--r-- | src/validationinterface.h | 5 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 35 | ||||
-rw-r--r-- | src/wallet/wallet.h | 2 | ||||
-rwxr-xr-x | test/functional/feature_notifications.py | 9 |
9 files changed, 55 insertions, 19 deletions
diff --git a/doc/release-notes-18982.md b/doc/release-notes-18982.md new file mode 100644 index 0000000000..d1b8528d13 --- /dev/null +++ b/doc/release-notes-18982.md @@ -0,0 +1,8 @@ +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 had been +broken since that release (bug +[#18325](https://github.com/bitcoin/bitcoin/issues/18325)). diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 0b3cd08e22..7c58273f82 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -158,9 +158,9 @@ public: { m_notifications->transactionAddedToMempool(tx); } - void TransactionRemovedFromMempool(const CTransactionRef& tx) override + void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) override { - m_notifications->transactionRemovedFromMempool(tx); + m_notifications->transactionRemovedFromMempool(tx, reason); } void BlockConnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* index) override { diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index e1bc9bbbf3..538aaae23b 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -20,6 +20,7 @@ class CRPCCommand; class CScheduler; class Coin; class uint256; +enum class MemPoolRemovalReason; enum class RBFTransactionState; struct CBlockLocator; struct FeeCalculation; @@ -221,7 +222,7 @@ public: public: virtual ~Notifications() {} virtual void transactionAddedToMempool(const CTransactionRef& tx) {} - virtual void transactionRemovedFromMempool(const CTransactionRef& ptx) {} + virtual void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {} virtual void blockConnected(const CBlock& block, int height) {} virtual void blockDisconnected(const CBlock& block, int height) {} virtual void updatedBlockTip() {} diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 47b0d39ea4..c14417a847 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -410,7 +410,7 @@ 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); } const uint256 hash = it->GetTx().GetHash(); diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 11000774c0..81a8268684 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -199,9 +199,9 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) { ptx->GetWitnessHash().ToString()); } -void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx) { - auto event = [ptx, this] { - m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(ptx); }); +void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) { + auto event = [ptx, reason, this] { + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(ptx, reason); }); }; ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s", __func__, ptx->GetHash().ToString(), diff --git a/src/validationinterface.h b/src/validationinterface.h index cb0204a555..2c03d8cd85 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -21,6 +21,7 @@ class CConnman; class CValidationInterface; class uint256; class CScheduler; +enum class MemPoolRemovalReason; // These functions dispatch to one or all registered wallets @@ -129,7 +130,7 @@ protected: * * 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. @@ -197,7 +198,7 @@ public: 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> &, const CBlockIndex* pindex); void ChainStateFlushed(const CBlockLocator &); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 45f5542cad..80b4b1f4bf 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -21,6 +21,7 @@ #include <script/descriptor.h> #include <script/script.h> #include <script/signingprovider.h> +#include <txmempool.h> #include <util/bip32.h> #include <util/error.h> #include <util/fees.h> @@ -1105,12 +1106,42 @@ 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->GetHash()); 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, /* block hash */ {}, /* index */ 0}); + } } void CWallet::blockConnected(const CBlock& block, int height) @@ -1124,7 +1155,7 @@ void CWallet::blockConnected(const CBlock& block, int height) 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); - transactionRemovedFromMempool(block.vtx[index]); + transactionRemovedFromMempool(block.vtx[index], MemPoolRemovalReason::BLOCK); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 7446a4889a..20f0306c63 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -910,7 +910,7 @@ public: uint256 last_failed_block; }; ScanResult ScanForWalletTransactions(const uint256& first_block, const uint256& last_block, 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 { diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py index 47200b6cc6..fb0c7ceed4 100755 --- a/test/functional/feature_notifications.py +++ b/test/functional/feature_notifications.py @@ -125,12 +125,7 @@ class NotificationsTest(BitcoinTestFramework): # Bump tx2 as bump2 and generate a block on node 0 while # disconnected, then reconnect and check for notifications on node 1 - # about newly confirmed bump2 and newly conflicted tx2. Currently - # only the bump2 notification is sent. Ideally, notifications would - # be sent both for bump2 and tx2, which was the previous behavior - # before being broken by an accidental change in PR - # https://github.com/bitcoin/bitcoin/pull/16624. The bug is reported - # in issue https://github.com/bitcoin/bitcoin/issues/18325. + # about newly confirmed bump2 and newly conflicted tx2. disconnect_nodes(self.nodes[0], 1) bump2 = self.nodes[0].bumpfee(tx2)["txid"] self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE) @@ -138,7 +133,7 @@ class NotificationsTest(BitcoinTestFramework): assert_equal(tx2 in self.nodes[1].getrawmempool(), True) connect_nodes(self.nodes[0], 1) self.sync_blocks() - self.expect_wallet_notify([bump2]) + self.expect_wallet_notify([bump2, tx2]) assert_equal(self.nodes[1].gettransaction(bump2)["confirmations"], 1) # TODO: add test for `-alertnotify` large fork notifications |