diff --git a/doc/release-notes.md b/doc/release-notes.md
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -12,3 +12,10 @@
followed by `generatetoaddress`, can generate blocks for command line testing
purposes. This is a client-side version of the former `generate` RPC. See
the help for details.
+
+## Notification changes
+
+`-walletnotify` notifications are now sent for wallet transactions that are
+removed from the mempool because they conflict with a new block. These
+notifications were sent previously before the v0.21.13 release, but had been
+broken since that release.
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -25,6 +25,8 @@
class CScheduler;
class TxValidationState;
+enum class MemPoolRemovalReason;
+
struct BlockHash;
struct bilingual_str;
struct CBlockLocator;
@@ -271,8 +273,9 @@
public:
virtual ~Notifications() {}
virtual void transactionAddedToMempool(const CTransactionRef &tx) {}
- virtual void transactionRemovedFromMempool(const CTransactionRef &ptx) {
- }
+ virtual void
+ transactionRemovedFromMempool(const CTransactionRef &ptx,
+ MemPoolRemovalReason reason) {}
virtual void blockConnected(const CBlock &block, int height) {}
virtual void blockDisconnected(const CBlock &block, int height) {}
virtual void updatedBlockTip() {}
diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -75,8 +75,10 @@
void TransactionAddedToMempool(const CTransactionRef &tx) override {
m_notifications->transactionAddedToMempool(tx);
}
- void TransactionRemovedFromMempool(const CTransactionRef &tx) override {
- m_notifications->transactionRemovedFromMempool(tx);
+ void
+ TransactionRemovedFromMempool(const CTransactionRef &tx,
+ MemPoolRemovalReason reason) override {
+ m_notifications->transactionRemovedFromMempool(tx, reason);
}
void BlockConnected(const std::shared_ptr<const CBlock> &block,
const CBlockIndex *index) override {
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -469,7 +469,8 @@
// for any reason except being included in a block. Clients interested
// in transactions included in blocks can subscribe to the
// BlockConnected notification.
- GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx());
+ GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(),
+ reason);
}
for (const CTxIn &txin : it->GetTx().vin) {
diff --git a/src/validationinterface.h b/src/validationinterface.h
--- a/src/validationinterface.h
+++ b/src/validationinterface.h
@@ -21,6 +21,7 @@
class CValidationInterface;
class uint256;
class CScheduler;
+enum class MemPoolRemovalReason;
/** Register subscriber */
void RegisterValidationInterface(CValidationInterface *callbacks);
@@ -137,7 +138,8 @@
*
* Called on a background thread.
*/
- virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx) {}
+ virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx,
+ MemPoolRemovalReason reason) {}
/**
* Notifies listeners of a block being connected.
* Provides a vector of transactions evicted from the mempool as a result.
@@ -218,7 +220,8 @@
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> &,
diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -213,10 +213,11 @@
ptx->GetHash().ToString());
}
-void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx) {
- auto event = [ptx, this] {
+void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx,
+ MemPoolRemovalReason reason) {
+ auto event = [ptx, reason, this] {
m_internals->Iterate([&](CValidationInterface &callbacks) {
- callbacks.TransactionRemovedFromMempool(ptx);
+ callbacks.TransactionRemovedFromMempool(ptx, reason);
});
};
ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s", __func__,
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1089,7 +1089,8 @@
std::optional<int> max_height,
const WalletRescanReserver &reserver,
bool fUpdate);
- void transactionRemovedFromMempool(const CTransactionRef &ptx) override;
+ void transactionRemovedFromMempool(const CTransactionRef &ptx,
+ MemPoolRemovalReason reason) override;
void ReacceptWalletTransactions() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
void ResendWalletTransactions();
struct Balance {
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -23,6 +23,7 @@
#include <script/sighashtype.h>
#include <script/sign.h>
#include <script/signingprovider.h>
+#include <txmempool.h>
#include <util/bip32.h>
#include <util/check.h>
#include <util/error.h>
@@ -1202,12 +1203,45 @@
}
}
-void CWallet::transactionRemovedFromMempool(const CTransactionRef &ptx) {
+void CWallet::transactionRemovedFromMempool(const CTransactionRef &ptx,
+ MemPoolRemovalReason reason) {
LOCK(cs_wallet);
auto it = mapWallet.find(ptx->GetId());
if (it != mapWallet.end()) {
it->second.fInMempool = false;
}
+ // Handle transactions that were removed from the mempool because they
+ // conflict with transactions in a newly connected block.
+ if (reason == MemPoolRemovalReason::CONFLICT) {
+ // Call SyncNotifications, so external -walletnotify notifications will
+ // be triggered for these transactions. Set Status::UNCONFIRMED instead
+ // of Status::CONFLICTED for a few reasons:
+ //
+ // 1. The transactionRemovedFromMempool callback does not currently
+ // provide the conflicting block's hash and height, and for backwards
+ // compatibility reasons it may not be not safe to store conflicted
+ // wallet transactions with a null block hash. See
+ // https://github.com/bitcoin/bitcoin/pull/18600#discussion_r420195993.
+ // 2. For most of these transactions, the wallet's internal conflict
+ // detection in the blockConnected handler will subsequently call
+ // MarkConflicted and update them with CONFLICTED status anyway. This
+ // applies to any wallet transaction that has inputs spent in the
+ // block, or that has ancestors in the wallet with inputs spent by
+ // the block.
+ // 3. Longstanding behavior since the sync implementation in
+ // https://github.com/bitcoin/bitcoin/pull/9371 and the prior sync
+ // implementation before that was to mark these transactions
+ // unconfirmed rather than conflicted.
+ //
+ // Nothing described above should be seen as an unchangeable requirement
+ // when improving this code in the future. The wallet's heuristics for
+ // distinguishing between conflicted and unconfirmed transactions are
+ // imperfect, and could be improved in general, see
+ // https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking
+ SyncTransaction(ptx,
+ {CWalletTx::Status::UNCONFIRMED, /* block height */ 0,
+ BlockHash(), /* index */ 0});
+ }
}
void CWallet::blockConnected(const CBlock &block, int height) {
@@ -1220,7 +1254,8 @@
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/test/functional/feature_notifications.py b/test/functional/feature_notifications.py
--- a/test/functional/feature_notifications.py
+++ b/test/functional/feature_notifications.py
@@ -12,6 +12,7 @@
assert_equal,
connect_nodes,
wait_until,
+ disconnect_nodes,
hex_str_to_bytes,
)
@@ -118,7 +119,7 @@
for tx_file in os.listdir(self.walletnotify_dir):
os.remove(os.path.join(self.walletnotify_dir, tx_file))
- # Transactions tests. Give node 0 same wallet seed as
+ # Conflicting transactions tests. Give node 0 same wallet seed as
# node 1, generate spends from node 0, and check notifications
# triggered by node 1
self.log.info("test -walletnotify with conflicting transactions")
@@ -145,6 +146,34 @@
self.expect_wallet_notify([tx1])
assert_equal(self.nodes[1].gettransaction(tx1)["confirmations"], 1)
+ # Generate conflicting transactions with the nodes disconnected.
+ # Sending almost the entire available balance on each node, but
+ # with a slightly different amount, ensures that there will be
+ # a conflict.
+ balance = self.nodes[0].getbalance()
+ disconnect_nodes(self.nodes[0], self.nodes[1])
+ tx2_node0 = self.nodes[0].sendtoaddress(
+ address=ADDRESS_ECREG_UNSPENDABLE, amount=balance - 20)
+ tx2_node1 = self.nodes[1].sendtoaddress(
+ address=ADDRESS_ECREG_UNSPENDABLE, amount=balance - 21)
+ assert tx2_node0 != tx2_node1
+ self.expect_wallet_notify([tx2_node1])
+ # So far tx2_node1 has no conflicting tx
+ assert not self.nodes[1].gettransaction(
+ tx2_node1)['walletconflicts']
+
+ # Mine a block on node0, reconnect the nodes, check that tx2_node1
+ # has a conflicting tx after syncing with node0.
+ self.nodes[0].generatetoaddress(1, ADDRESS_ECREG_UNSPENDABLE)
+ connect_nodes(self.nodes[0], self.nodes[1])
+ self.sync_blocks()
+ assert tx2_node0 in self.nodes[1].gettransaction(tx2_node1)[
+ 'walletconflicts']
+
+ # node1's wallet will notify of the new confirmed transaction tx2_0
+ # and about the conflicted transaction tx2_1.
+ self.expect_wallet_notify([tx2_node0, tx2_node1])
+
# Create an invalid chain and ensure the node warns.
self.log.info("test -alertnotify for forked chain")
fork_block = self.nodes[0].getbestblockhash()