diff options
author | MarcoFalke <falke.marco@gmail.com> | 2020-06-02 18:11:33 -0400 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2020-06-02 18:11:52 -0400 |
commit | 3657aee2d25ce1ffefc6817af3eead7120b1d755 (patch) | |
tree | 9774d72abcbdc4c8baf28eab3f084ce81a77268e /src | |
parent | 5879bfa9a541576100d939d329a2639b79d9e4f9 (diff) | |
parent | 7eaf86d3bfc83f2beb3ef449707d5156853126fb (diff) |
Merge #18982: wallet: Minimal fix to restore conflicted transaction notifications
7eaf86d3bfc83f2beb3ef449707d5156853126fb trivial: Suggested cleanups to surrounding code (Russell Yanofsky)
b604c5c8b5892842f13dee89ae31812a28ab25d1 wallet: Minimal fix to restore conflicted transaction notifications (Russell Yanofsky)
Pull request description:
This fix is a based on the fix by Antoine Riard (ariard) 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)
ACKs for top commit:
jonatack:
Re-ACK 7eaf86d3bfc83f
ariard:
ACK 7eaf86d, reviewed, built and ran tests.
MarcoFalke:
ACK 7eaf86d3bfc83f2beb3ef449707d5156853126fb 🍡
Tree-SHA512: 9a1efe975969bb522a9dd73c41064a9348887cb67883cd92c6571fd2df4321b9f4568363891abdaae14a3b9b168ef8142e95c373fc04677e46289b251fb84689
Diffstat (limited to 'src')
-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 | 20 | ||||
-rw-r--r-- | src/validationinterface.h | 9 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 53 | ||||
-rw-r--r-- | src/wallet/wallet.h | 2 |
7 files changed, 61 insertions, 32 deletions
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 { |