aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--doc/release-notes-18982.md8
-rw-r--r--src/interfaces/chain.cpp4
-rw-r--r--src/interfaces/chain.h3
-rw-r--r--src/txmempool.cpp2
-rw-r--r--src/validationinterface.cpp20
-rw-r--r--src/validationinterface.h9
-rw-r--r--src/wallet/wallet.cpp53
-rw-r--r--src/wallet/wallet.h2
-rwxr-xr-xtest/functional/feature_notifications.py9
9 files changed, 71 insertions, 39 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 d8e459a8e8..d1e04b114d 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -63,9 +63,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 7dfc77db7b..61d7ddb934 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 bilingual_str;
struct CBlockLocator;
@@ -239,7 +240,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 c5c0208d8f..7d8eb8a323 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 9437f9c817..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) {
- auto event = [ptx, this] {
- m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(ptx); });
+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 9c23965bc1..e96f2883fc 100644
--- a/src/validationinterface.h
+++ b/src/validationinterface.h
@@ -21,6 +21,7 @@ class CConnman;
class CValidationInterface;
class uint256;
class CScheduler;
+enum class MemPoolRemovalReason;
/** Register subscriber */
void RegisterValidationInterface(CValidationInterface* callbacks);
@@ -96,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.
*
@@ -129,7 +130,7 @@ protected:
*
* Called on a background thread.
*/
- virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx) {}
+ 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.
@@ -196,8 +197,8 @@ public:
void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload);
- void TransactionAddedToMempool(const CTransactionRef &);
- void TransactionRemovedFromMempool(const CTransactionRef &);
+ 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 7824563254..89737ca7b5 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/check.h>
#include <util/error.h>
@@ -1100,23 +1101,52 @@ 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) {
+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;
}
+ // 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(tx, {CWalletTx::Status::UNCONFIRMED, /* block height */ 0, /* block hash */ {}, /* index */ 0});
+ }
}
void CWallet::blockConnected(const CBlock& block, int height)
@@ -1127,9 +1157,8 @@ 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);
- transactionRemovedFromMempool(block.vtx[index]);
+ SyncTransaction(block.vtx[index], {CWalletTx::Status::CONFIRMED, height, block_hash, (int)index});
+ transactionRemovedFromMempool(block.vtx[index], MemPoolRemovalReason::BLOCK);
}
}
@@ -1144,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});
}
}
@@ -1685,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 e3141baef0..67331dc3be 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -921,7 +921,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) override;
+ void transactionRemovedFromMempool(const CTransactionRef& tx, 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
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-02 17:10:53 +0000