maxmustermann/dogecoin
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

wallet: Minimal fix to restore conflicted transaction notifications

Browse source 
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:
a31be09bfd and
7e89994133 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>
This commit is contained in:
Russell Yanofsky 2020-05-15 09:23:55 -04:00
parent e2f6866cca
commit b604c5c8b5
9 changed files with 55 additions and 19 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
doc/release-notes-18982.md Normal file
View file

@ -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)).

4
src/interfaces/chain.cpp
View file

@ -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
{

3
src/interfaces/chain.h
View file

@ -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() {}

2
src/txmempool.cpp
View file

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

6
src/validationinterface.cpp
View file

@ -208,9 +208,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(),

5
src/validationinterface.h
View file

@ -21,6 +21,7 @@ class CConnman;
class CValidationInterface;
class uint256;
class CScheduler;
enum class MemPoolRemovalReason;
/** Register subscriber */
void RegisterValidationInterface(CValidationInterface* callbacks);
@ -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 &);

35
src/wallet/wallet.cpp
View file

@ -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>
@ -1111,12 +1112,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)
@ -1129,7 +1160,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);
}
}

2
src/wallet/wallet.h
View file

@ -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) override;
void transactionRemovedFromMempool(const CTransactionRef &ptx, MemPoolRemovalReason reason) override;
void ReacceptWalletTransactions() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
void ResendWalletTransactions();
struct Balance {

9
test/functional/feature_notifications.py
View file

@ -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