Skip to content

Commit 654420d

Browse files
ryanofskyfanquake
ryanofsky
authored and
fanquake
committed
wallet: Minimal fix to restore conflicted transaction notifications
This fix is a based on the fix by Antoine Riard <[email protected]> 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 <[email protected]> Github-Pull: #18982 Rebased-From: b604c5c
1 parent febebc4 commit 654420d

File tree

9 files changed

+55
-19
lines changed

9 files changed

+55
-19
lines changed

‎doc/release-notes-18982.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
Notification changes
2+
--------------------
3+
4+
`-walletnotify` notifications are now sent for wallet transactions that are
5+
removed from the mempool because they conflict with a new block. These
6+
notifications were sent previously before the v0.19 release, but had been
7+
broken since that release (bug
8+
[#18325](https://github.com/bitcoin/bitcoin/issues/18325)).

‎src/interfaces/chain.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -158,9 +158,9 @@ class NotificationsProxy : public CValidationInterface
158158
{
159159
m_notifications->transactionAddedToMempool(tx);
160160
}
161-
void TransactionRemovedFromMempool(const CTransactionRef& tx) override
161+
void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) override
162162
{
163-
m_notifications->transactionRemovedFromMempool(tx);
163+
m_notifications->transactionRemovedFromMempool(tx, reason);
164164
}
165165
void BlockConnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* index) override
166166
{

‎src/interfaces/chain.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ class CRPCCommand;
2020
class CScheduler;
2121
class Coin;
2222
class uint256;
23+
enum class MemPoolRemovalReason;
2324
enum class RBFTransactionState;
2425
struct CBlockLocator;
2526
struct FeeCalculation;
@@ -221,7 +222,7 @@ class Chain
221222
public:
222223
virtual ~Notifications() {}
223224
virtual void transactionAddedToMempool(const CTransactionRef& tx) {}
224-
virtual void transactionRemovedFromMempool(const CTransactionRef& ptx) {}
225+
virtual void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {}
225226
virtual void blockConnected(const CBlock& block, int height) {}
226227
virtual void blockDisconnected(const CBlock& block, int height) {}
227228
virtual void updatedBlockTip() {}

‎src/txmempool.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -410,7 +410,7 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
410410
// for any reason except being included in a block. Clients interested
411411
// in transactions included in blocks can subscribe to the BlockConnected
412412
// notification.
413-
GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx());
413+
GetMainSignals().TransactionRemovedFromMempool(it->GetSharedTx(), reason);
414414
}
415415

416416
const uint256 hash = it->GetTx().GetHash();

‎src/validationinterface.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,9 +199,9 @@ void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
199199
ptx->GetWitnessHash().ToString());
200200
}
201201

202-
void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx) {
203-
auto event = [ptx, this] {
204-
m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(ptx); });
202+
void CMainSignals::TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {
203+
auto event = [ptx, reason, this] {
204+
m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(ptx, reason); });
205205
};
206206
ENQUEUE_AND_LOG_EVENT(event, "%s: txid=%s wtxid=%s", __func__,
207207
ptx->GetHash().ToString(),

‎src/validationinterface.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ class CConnman;
2121
class CValidationInterface;
2222
class uint256;
2323
class CScheduler;
24+
enum class MemPoolRemovalReason;
2425

2526
// These functions dispatch to one or all registered wallets
2627

@@ -129,7 +130,7 @@ class CValidationInterface {
129130
*
130131
* Called on a background thread.
131132
*/
132-
virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx) {}
133+
virtual void TransactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {}
133134
/**
134135
* Notifies listeners of a block being connected.
135136
* Provides a vector of transactions evicted from the mempool as a result.
@@ -197,7 +198,7 @@ class CMainSignals {
197198

198199
void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload);
199200
void TransactionAddedToMempool(const CTransactionRef &);
200-
void TransactionRemovedFromMempool(const CTransactionRef &);
201+
void TransactionRemovedFromMempool(const CTransactionRef &, MemPoolRemovalReason);
201202
void BlockConnected(const std::shared_ptr<const CBlock> &, const CBlockIndex *pindex);
202203
void BlockDisconnected(const std::shared_ptr<const CBlock> &, const CBlockIndex* pindex);
203204
void ChainStateFlushed(const CBlockLocator &);

‎src/wallet/wallet.cpp‎

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <script/descriptor.h>
2222
#include <script/script.h>
2323
#include <script/signingprovider.h>
24+
#include <txmempool.h>
2425
#include <util/bip32.h>
2526
#include <util/error.h>
2627
#include <util/fees.h>
@@ -1105,12 +1106,42 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& ptx) {
11051106
}
11061107
}
11071108

1108-
void CWallet::transactionRemovedFromMempool(const CTransactionRef &ptx) {
1109+
void CWallet::transactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) {
11091110
LOCK(cs_wallet);
11101111
auto it = mapWallet.find(ptx->GetHash());
11111112
if (it != mapWallet.end()) {
11121113
it->second.fInMempool = false;
11131114
}
1115+
// Handle transactions that were removed from the mempool because they
1116+
// conflict with transactions in a newly connected block.
1117+
if (reason == MemPoolRemovalReason::CONFLICT) {
1118+
// Call SyncNotifications, so external -walletnotify notifications will
1119+
// be triggered for these transactions. Set Status::UNCONFIRMED instead
1120+
// of Status::CONFLICTED for a few reasons:
1121+
//
1122+
// 1. The transactionRemovedFromMempool callback does not currently
1123+
// provide the conflicting block's hash and height, and for backwards
1124+
// compatibility reasons it may not be not safe to store conflicted
1125+
// wallet transactions with a null block hash. See
1126+
// https://github.com/bitcoin/bitcoin/pull/18600#discussion_r420195993.
1127+
// 2. For most of these transactions, the wallet's internal conflict
1128+
// detection in the blockConnected handler will subsequently call
1129+
// MarkConflicted and update them with CONFLICTED status anyway. This
1130+
// applies to any wallet transaction that has inputs spent in the
1131+
// block, or that has ancestors in the wallet with inputs spent by
1132+
// the block.
1133+
// 3. Longstanding behavior since the sync implementation in
1134+
// https://github.com/bitcoin/bitcoin/pull/9371 and the prior sync
1135+
// implementation before that was to mark these transactions
1136+
// unconfirmed rather than conflicted.
1137+
//
1138+
// Nothing described above should be seen as an unchangeable requirement
1139+
// when improving this code in the future. The wallet's heuristics for
1140+
// distinguishing between conflicted and unconfirmed transactions are
1141+
// imperfect, and could be improved in general, see
1142+
// https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking
1143+
SyncTransaction(ptx, {CWalletTx::Status::UNCONFIRMED, /* block height */ 0, /* block hash */ {}, /* index */ 0});
1144+
}
11141145
}
11151146

11161147
void CWallet::blockConnected(const CBlock& block, int height)
@@ -1124,7 +1155,7 @@ void CWallet::blockConnected(const CBlock& block, int height)
11241155
for (size_t index = 0; index < block.vtx.size(); index++) {
11251156
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, height, block_hash, index);
11261157
SyncTransaction(block.vtx[index], confirm);
1127-
transactionRemovedFromMempool(block.vtx[index]);
1158+
transactionRemovedFromMempool(block.vtx[index], MemPoolRemovalReason::BLOCK);
11281159
}
11291160
}
11301161

‎src/wallet/wallet.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -910,7 +910,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
910910
uint256 last_failed_block;
911911
};
912912
ScanResult ScanForWalletTransactions(const uint256& first_block, const uint256& last_block, const WalletRescanReserver& reserver, bool fUpdate);
913-
void transactionRemovedFromMempool(const CTransactionRef &ptx) override;
913+
void transactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) override;
914914
void ReacceptWalletTransactions() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
915915
void ResendWalletTransactions();
916916
struct Balance {

‎test/functional/feature_notifications.py‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -125,20 +125,15 @@ def run_test(self):
125125

126126
# Bump tx2 as bump2 and generate a block on node 0 while
127127
# disconnected, then reconnect and check for notifications on node 1
128-
# about newly confirmed bump2 and newly conflicted tx2. Currently
129-
# only the bump2 notification is sent. Ideally, notifications would
130-
# be sent both for bump2 and tx2, which was the previous behavior
131-
# before being broken by an accidental change in PR
132-
# https://github.com/bitcoin/bitcoin/pull/16624. The bug is reported
133-
# in issue https://github.com/bitcoin/bitcoin/issues/18325.
128+
# about newly confirmed bump2 and newly conflicted tx2.
134129
disconnect_nodes(self.nodes[0], 1)
135130
bump2 = self.nodes[0].bumpfee(tx2)["txid"]
136131
self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)
137132
assert_equal(self.nodes[0].gettransaction(bump2)["confirmations"], 1)
138133
assert_equal(tx2 in self.nodes[1].getrawmempool(), True)
139134
connect_nodes(self.nodes[0], 1)
140135
self.sync_blocks()
141-
self.expect_wallet_notify([bump2])
136+
self.expect_wallet_notify([bump2, tx2])
142137
assert_equal(self.nodes[1].gettransaction(bump2)["confirmations"], 1)
143138

144139
# TODO: add test for `-alertnotify` large fork notifications

0 commit comments

Comments
 (0)