diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index c9ef46243c..89af848c30 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -7,6 +7,7 @@ #include #include +#include #include // For CTransactionRef #include @@ -26,7 +27,6 @@ class CRPCCommand; class CScheduler; class Coin; class uint256; -enum class MemPoolRemovalReason; enum class RBFTransactionState; enum class ChainstateRole; struct bilingual_str; @@ -319,7 +319,7 @@ public: public: virtual ~Notifications() = default; virtual void transactionAddedToMempool(const CTransactionRef& tx) {} - virtual void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {} + virtual void transactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& reason) {} virtual void blockConnected(ChainstateRole role, const BlockInfo& block) {} virtual void blockDisconnected(const BlockInfo& block) {} virtual void updatedBlockTip() {} diff --git a/src/kernel/mempool_removal_reason.cpp b/src/kernel/mempool_removal_reason.cpp index df27590c7a..9dc14e5449 100644 --- a/src/kernel/mempool_removal_reason.cpp +++ b/src/kernel/mempool_removal_reason.cpp @@ -9,13 +9,8 @@ std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept { - switch (r) { - case MemPoolRemovalReason::EXPIRY: return "expiry"; - case MemPoolRemovalReason::SIZELIMIT: return "sizelimit"; - case MemPoolRemovalReason::REORG: return "reorg"; - case MemPoolRemovalReason::BLOCK: return "block"; - case MemPoolRemovalReason::CONFLICT: return "conflict"; - case MemPoolRemovalReason::REPLACED: return "replaced"; - } - assert(false); + return std::visit([](auto&& arg) { + return arg.toString(); + }, + r); } diff --git a/src/kernel/mempool_removal_reason.h b/src/kernel/mempool_removal_reason.h index 53c2ff1c31..65666d4b47 100644 --- a/src/kernel/mempool_removal_reason.h +++ b/src/kernel/mempool_removal_reason.h @@ -5,20 +5,77 @@ #ifndef BITCOIN_KERNEL_MEMPOOL_REMOVAL_REASON_H #define BITCOIN_KERNEL_MEMPOOL_REMOVAL_REASON_H +#include +#include + #include +#include /** Reason why a transaction was removed from the mempool, * this is passed to the notification signal. */ -enum class MemPoolRemovalReason { - EXPIRY, //!< Expired from mempool - SIZELIMIT, //!< Removed in size limiting - REORG, //!< Removed for reorganization - BLOCK, //!< Removed for block - CONFLICT, //!< Removed for conflict with in-block transaction - REPLACED, //!< Removed for replacement + +struct ExpiryReason { + std::string toString() const noexcept + { + return "expiry"; + } }; +struct SizeLimitReason { + std::string toString() const noexcept + { + return "sizelimit"; + } +}; + +struct ReorgReason { + std::string toString() const noexcept + { + return "reorg"; + } +}; + +struct BlockReason { + std::string toString() const noexcept + { + return "block"; + } +}; + +struct ConflictReason { + uint256 conflicting_block_hash; + unsigned int conflicting_block_height; + + explicit ConflictReason(const uint256& conflicting_block_hash, int conflicting_block_height) : conflicting_block_hash(conflicting_block_hash), conflicting_block_height(conflicting_block_height) {} + ConflictReason(std::nullptr_t, int conflicting_block_height) = delete; + + std::string toString() const noexcept + { + return "conflict"; + } +}; + +struct ReplacedReason { + std::optional replacement_tx; + + explicit ReplacedReason(const std::optional replacement_tx) : replacement_tx(replacement_tx) {} + ReplacedReason(std::nullptr_t) = delete; + + std::string toString() const noexcept + { + return "replaced"; + } +}; + +using MemPoolRemovalReason = std::variant; + std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept; +template +bool IsReason(const MemPoolRemovalReason& reason) +{ + return std::get_if(&reason) != nullptr; +} + #endif // BITCOIN_KERNEL_MEMPOOL_REMOVAL_REASON_H diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 73ce927f71..ffd134e582 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -459,7 +459,7 @@ public: { m_notifications->transactionAddedToMempool(tx.info.m_tx); } - void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override + void TransactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& reason, uint64_t mempool_sequence) override { m_notifications->transactionRemovedFromMempool(tx, reason); } diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index e85b2f2caa..a14366120c 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -582,7 +582,7 @@ void CBlockPolicyEstimator::TransactionAddedToMempool(const NewMempoolTransactio processTransaction(tx); } -void CBlockPolicyEstimator::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason /*unused*/, uint64_t /*unused*/) +void CBlockPolicyEstimator::TransactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& /*unused*/, uint64_t /*unused*/) { removeTx(tx->GetHash()); } diff --git a/src/policy/fees.h b/src/policy/fees.h index a95cc19dd4..8cce3a4664 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -266,7 +266,7 @@ protected: /** Overridden from CValidationInterface. */ void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t /*unused*/) override EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); - void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason /*unused*/, uint64_t /*unused*/) override + void TransactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& /*unused*/, uint64_t /*unused*/) override EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); void MempoolTransactionsRemovedForBlock(const std::vector& txs_removed_for_block, unsigned int nBlockHeight) override EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator); diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp index ed95a8831e..680e756eff 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -89,7 +90,7 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest) BOOST_CHECK_EQUAL(pool.get(block.vtx[2]->GetHash()).use_count(), SHARED_TX_OFFSET + 1); size_t poolSize = pool.size(); - pool.removeRecursive(*block.vtx[2], MemPoolRemovalReason::REPLACED); + pool.removeRecursive(*block.vtx[2], ReplacedReason(MakeTransactionRef(CTransaction(CMutableTransaction())))); BOOST_CHECK_EQUAL(pool.size(), poolSize - 1); CBlock block2; diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index 8e3d84a9e6..45b97c8448 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -72,7 +72,7 @@ struct OutpointsUpdater final : public CValidationInterface { } } - void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override + void TransactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& reason, uint64_t /* mempool_sequence */) override { // outpoints spent by this tx are now available for (const auto& input : tx->vin) { @@ -98,7 +98,7 @@ struct TransactionsDelta final : public CValidationInterface { m_added.insert(tx.info.m_tx); } - void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override + void TransactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& reason, uint64_t /* mempool_sequence */) override { // Transactions may be entered and booted any number of times m_added.erase(tx); diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index a697ee9d83..6b9d9fe56b 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -71,7 +71,7 @@ struct TransactionsDelta final : public CValidationInterface { Assert(m_added.insert(tx.info.m_tx).second); } - void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t /* mempool_sequence */) override + void TransactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& reason, uint64_t /* mempool_sequence */) override { Assert(m_removed.insert(tx).second); } @@ -107,7 +107,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha const auto info_all = tx_pool.infoAll(); if (!info_all.empty()) { const auto& tx_to_remove = *PickValue(fuzzed_data_provider, info_all).tx; - WITH_LOCK(tx_pool.cs, tx_pool.removeRecursive(tx_to_remove, MemPoolRemovalReason::BLOCK /* dummy */)); + WITH_LOCK(tx_pool.cs, tx_pool.removeRecursive(tx_to_remove, BlockReason{} /* dummy */)); assert(tx_pool.size() < info_all.size()); WITH_LOCK(::cs_main, tx_pool.check(chainstate.CoinsTip(), chainstate.m_chain.Height() + 1)); } diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index f0094dce59..1746601141 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -4,8 +4,10 @@ #include #include +#include #include #include +#include #include #include @@ -15,7 +17,7 @@ BOOST_FIXTURE_TEST_SUITE(mempool_tests, TestingSetup) -static constexpr auto REMOVAL_REASON_DUMMY = MemPoolRemovalReason::REPLACED; +static const auto REMOVAL_REASON_DUMMY = ReplacedReason(MakeTransactionRef(CTransaction(CMutableTransaction()))); class MemPoolTest final : public CTxMemPool { @@ -401,7 +403,7 @@ BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest) /* after tx6 is mined, tx7 should move up in the sort */ std::vector vtx; vtx.push_back(MakeTransactionRef(tx6)); - pool.removeForBlock(vtx, 1); + pool.removeForBlock(vtx, uint256::ZERO, 1); sortedOrder.erase(sortedOrder.begin()+1); // Ties are broken by hash @@ -561,7 +563,7 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) SetMockTime(42 + CTxMemPool::ROLLING_FEE_HALFLIFE); BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), maxFeeRateRemoved.GetFeePerK() + 1000); // ... we should keep the same min fee until we get a block - pool.removeForBlock(vtx, 1); + pool.removeForBlock(vtx, uint256::ZERO, 1); SetMockTime(42 + 2*CTxMemPool::ROLLING_FEE_HALFLIFE); BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), llround((maxFeeRateRemoved.GetFeePerK() + 1000)/2.0)); // ... then feerate should drop 1/2 each halflife diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 5bfcde7718..8953edd5af 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -192,7 +192,7 @@ void MinerTestingSetup::TestPackageSelection(const CScript& scriptPubKey, const // Test that packages above the min relay fee do get included, even if one // of the transactions is below the min relay fee // Remove the low fee transaction and replace with a higher fee transaction - tx_mempool.removeRecursive(CTransaction(tx), MemPoolRemovalReason::REPLACED); + tx_mempool.removeRecursive(CTransaction(tx), ReplacedReason(MakeTransactionRef(CTransaction(CMutableTransaction())))); tx.vout[0].nValue -= 2; // Now we should be just over the min relay fee hashLowFeeTx = tx.GetHash(); AddToMempool(tx_mempool, entry.Fee(feeToUse + 2).FromTx(tx)); diff --git a/src/test/policyestimator_tests.cpp b/src/test/policyestimator_tests.cpp index f767cd4287..b34bade7a4 100644 --- a/src/test/policyestimator_tests.cpp +++ b/src/test/policyestimator_tests.cpp @@ -96,7 +96,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) { LOCK(mpool.cs); - mpool.removeForBlock(block, ++blocknum); + mpool.removeForBlock(block, uint256::ZERO, ++blocknum); } block.clear(); @@ -143,7 +143,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) // We haven't decayed the moving average enough so we still have enough data points in every bucket while (blocknum < 250) { LOCK(mpool.cs); - mpool.removeForBlock(block, ++blocknum); + mpool.removeForBlock(block, uint256::ZERO, ++blocknum); } // Wait for fee estimator to catch up @@ -184,7 +184,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) } { LOCK(mpool.cs); - mpool.removeForBlock(block, ++blocknum); + mpool.removeForBlock(block, uint256::ZERO, ++blocknum); } } @@ -208,7 +208,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) { LOCK(mpool.cs); - mpool.removeForBlock(block, 266); + mpool.removeForBlock(block, uint256::ZERO, 266); } block.clear(); @@ -252,7 +252,7 @@ BOOST_AUTO_TEST_CASE(BlockPolicyEstimates) { LOCK(mpool.cs); - mpool.removeForBlock(block, ++blocknum); + mpool.removeForBlock(block, uint256::ZERO, ++blocknum); } block.clear(); diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index af36a95693..c244f0828e 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -81,7 +81,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup) BOOST_CHECK(m_node.chainman->ActiveChain().Tip()->GetBlockHash() != block.GetHash()); } BOOST_CHECK_EQUAL(m_node.mempool->size(), 1U); - WITH_LOCK(m_node.mempool->cs, m_node.mempool->removeRecursive(CTransaction{spends[0]}, MemPoolRemovalReason::CONFLICT)); + WITH_LOCK(m_node.mempool->cs, m_node.mempool->removeRecursive(CTransaction{spends[0]}, ConflictReason(uint256::ZERO, 1))); BOOST_CHECK_EQUAL(m_node.mempool->size(), 0U); // Test 3: ... and should be rejected if spend2 is in the memory pool @@ -92,7 +92,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup) BOOST_CHECK(m_node.chainman->ActiveChain().Tip()->GetBlockHash() != block.GetHash()); } BOOST_CHECK_EQUAL(m_node.mempool->size(), 1U); - WITH_LOCK(m_node.mempool->cs, m_node.mempool->removeRecursive(CTransaction{spends[1]}, MemPoolRemovalReason::CONFLICT)); + WITH_LOCK(m_node.mempool->cs, m_node.mempool->removeRecursive(CTransaction{spends[1]}, ConflictReason(uint256::ZERO, 1))); BOOST_CHECK_EQUAL(m_node.mempool->size(), 0U); // Final sanity test: first spend in *m_node.mempool, second in block, that's OK: diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 3a5a3fb306..2174252e3d 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -155,7 +155,7 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector& vHashes // This txid may have been removed already in a prior call to removeRecursive. // Therefore we ensure it is not yet removed already. if (const std::optional txiter = GetIter(txid)) { - removeRecursive((*txiter)->GetTx(), MemPoolRemovalReason::SIZELIMIT); + removeRecursive((*txiter)->GetTx(), SizeLimitReason{}); } } } @@ -435,7 +435,15 @@ void CTxMemPool::AddTransactionsUpdated(unsigned int n) void CTxMemPool::Apply(ChangeSet* changeset) { AssertLockHeld(cs); - RemoveStaged(changeset->m_to_remove, false, MemPoolRemovalReason::REPLACED); + if (!changeset->m_entry_vec.empty()) { + // The replacement_tx is the transaction added in this changeset or, + // in the case of packages, the last child in the package added in this changeset + const auto replacement_tx = changeset->m_entry_vec[changeset->m_entry_vec.size() - 1]->GetSharedTx(); + RemoveStaged(changeset->m_to_remove, false, ReplacedReason(replacement_tx)); + } else { + Assume(false); + RemoveStaged(changeset->m_to_remove, false, ReplacedReason(std::nullopt)); + } for (size_t i=0; im_entry_vec.size(); ++i) { auto tx_entry = changeset->m_entry_vec[i]; @@ -517,13 +525,13 @@ void CTxMemPool::addNewTransaction(CTxMemPool::txiter newit, CTxMemPool::setEntr ); } -void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) +void CTxMemPool::removeUnchecked(txiter it, const MemPoolRemovalReason& reason) { // We increment mempool sequence value no matter removal reason // even if not directly reported below. uint64_t mempool_sequence = GetAndIncrementSequence(); - if (reason != MemPoolRemovalReason::BLOCK && m_opts.signals) { + if (!IsReason(reason) && m_opts.signals) { // Notify clients that a transaction has been removed from the mempool // for any reason except being included in a block. Clients interested // in transactions included in blocks can subscribe to the BlockConnected @@ -592,7 +600,7 @@ void CTxMemPool::CalculateDescendants(txiter entryit, setEntries& setDescendants } } -void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReason reason) +void CTxMemPool::removeRecursive(const CTransaction& origTx, const MemPoolRemovalReason& reason) { // Remove transaction from memory pool AssertLockHeld(cs); @@ -638,13 +646,13 @@ void CTxMemPool::removeForReorg(CChain& chain, std::function check for (txiter it : txToRemove) { CalculateDescendants(it, setAllRemoves); } - RemoveStaged(setAllRemoves, false, MemPoolRemovalReason::REORG); + RemoveStaged(setAllRemoves, false, ReorgReason{}); for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { assert(TestLockPointValidity(chain, it->GetLockPoints())); } } -void CTxMemPool::removeConflicts(const CTransaction &tx) +void CTxMemPool::removeConflicts(const CTransaction& tx, const uint256& blockHash, unsigned int nBlockHeight) { // Remove transactions which depend on inputs of tx, recursively AssertLockHeld(cs); @@ -655,7 +663,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) if (txConflict != tx) { ClearPrioritisation(txConflict.GetHash()); - removeRecursive(txConflict, MemPoolRemovalReason::CONFLICT); + removeRecursive(txConflict, ConflictReason(blockHash, nBlockHeight)); } } } @@ -664,7 +672,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) /** * Called when a block is connected. Removes from mempool. */ -void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) +void CTxMemPool::removeForBlock(const std::vector& vtx, const uint256& blockHash, unsigned int nBlockHeight) { AssertLockHeld(cs); Assume(!m_have_changeset); @@ -677,9 +685,9 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigne setEntries stage; stage.insert(it); txs_removed_for_block.emplace_back(*it); - RemoveStaged(stage, true, MemPoolRemovalReason::BLOCK); + RemoveStaged(stage, true, BlockReason{}); } - removeConflicts(*tx); + removeConflicts(*tx, blockHash, nBlockHeight); ClearPrioritisation(tx->GetHash()); } if (m_opts.signals) { @@ -1075,7 +1083,8 @@ void CTxMemPool::RemoveUnbroadcastTx(const uint256& txid, const bool unchecked) } } -void CTxMemPool::RemoveStaged(setEntries &stage, bool updateDescendants, MemPoolRemovalReason reason) { +void CTxMemPool::RemoveStaged(setEntries& stage, bool updateDescendants, const MemPoolRemovalReason& reason) +{ AssertLockHeld(cs); UpdateForRemoveFromMempool(stage, updateDescendants); for (txiter it : stage) { @@ -1097,7 +1106,7 @@ int CTxMemPool::Expire(std::chrono::seconds time) for (txiter removeit : toremove) { CalculateDescendants(removeit, stage); } - RemoveStaged(stage, false, MemPoolRemovalReason::EXPIRY); + RemoveStaged(stage, false, ExpiryReason{}); return stage.size(); } @@ -1183,7 +1192,7 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector* pvNoSpends for (txiter iter : stage) txn.push_back(iter->GetTx()); } - RemoveStaged(stage, false, MemPoolRemovalReason::SIZELIMIT); + RemoveStaged(stage, false, SizeLimitReason{}); if (pvNoSpendsRemaining) { for (const CTransaction& tx : txn) { for (const CTxIn& txin : tx.vin) { diff --git a/src/txmempool.h b/src/txmempool.h index 10acb2aa22..7d0a3766c5 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -18,10 +18,11 @@ #include #include #include +#include #include +#include #include #include -#include #include #include @@ -454,7 +455,7 @@ public: void check(const CCoinsViewCache& active_coins_tip, int64_t spendheight) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main); - void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); + void removeRecursive(const CTransaction& tx, const MemPoolRemovalReason& reason) EXCLUSIVE_LOCKS_REQUIRED(cs); /** After reorg, filter the entries that would no longer be valid in the next block, and update * the entries' cached LockPoints if needed. The mempool does not have any knowledge of * consensus rules. It just applies the callable function and removes the ones for which it @@ -463,8 +464,8 @@ public: * and updates an entry's LockPoints. * */ void removeForReorg(CChain& chain, std::function filter_final_and_mature) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); - void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs); - void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); + void removeConflicts(const CTransaction& tx, const uint256& blockHash, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); + void removeForBlock(const std::vector& vtx, const uint256& blockHash, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb, bool wtxid=false); bool isSpent(const COutPoint& outpoint) const; @@ -719,7 +720,7 @@ private: * Set updateDescendants to true when removing a tx that was in a block, so * that any in-mempool descendants have their ancestor state updated. */ - void RemoveStaged(setEntries& stage, bool updateDescendants, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); + void RemoveStaged(setEntries& stage, bool updateDescendants, const MemPoolRemovalReason& reason) EXCLUSIVE_LOCKS_REQUIRED(cs); /** UpdateForDescendants is used by UpdateTransactionsFromBlock to update * the descendants for a single transaction that has been added to the @@ -770,7 +771,8 @@ private: * transactions in a chain before we've updated all the state for the * removal. */ - void removeUnchecked(txiter entry, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); + void removeUnchecked(txiter entry, const MemPoolRemovalReason& reason) EXCLUSIVE_LOCKS_REQUIRED(cs); + public: /** visited marks a CTxMemPoolEntry as having been traversed * during the lifetime of the most recently created Epoch::Guard diff --git a/src/validation.cpp b/src/validation.cpp index 0384018bc3..9c19a1e480 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -318,7 +318,7 @@ void Chainstate::MaybeUpdateMempoolForReorg( MempoolAcceptResult::ResultType::VALID) { // If the transaction doesn't make it in to the mempool, remove any // transactions that depend on it (which would now be orphans). - m_mempool->removeRecursive(**it, MemPoolRemovalReason::REORG); + m_mempool->removeRecursive(**it, ReorgReason{}); } else if (m_mempool->exists(GenTxid::Txid((*it)->GetHash()))) { vHashUpdate.push_back((*it)->GetHash()); } @@ -3107,7 +3107,7 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra // Save transactions to re-add to mempool at end of reorg. If any entries are evicted for // exceeding memory limits, remove them and their descendants from the mempool. for (auto&& evicted_tx : disconnectpool->AddTransactionsFromBlock(block.vtx)) { - m_mempool->removeRecursive(*evicted_tx, MemPoolRemovalReason::REORG); + m_mempool->removeRecursive(*evicted_tx, ReorgReason{}); } } @@ -3235,7 +3235,7 @@ bool Chainstate::ConnectTip(BlockValidationState& state, CBlockIndex* pindexNew, Ticks(m_chainman.time_chainstate) / m_chainman.num_blocks_total); // Remove conflicting transactions from the mempool.; if (m_mempool) { - m_mempool->removeForBlock(blockConnecting.vtx, pindexNew->nHeight); + m_mempool->removeForBlock(blockConnecting.vtx, pindexNew->GetBlockHash(), pindexNew->nHeight); disconnectpool.removeForBlock(blockConnecting.vtx); } // Update m_chain & related variables. diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index da2685d771..a0e184d0a5 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -198,7 +198,8 @@ void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInf tx.info.m_tx->GetWitnessHash().ToString()); } -void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) { +void ValidationSignals::TransactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& reason, uint64_t mempool_sequence) +{ auto event = [tx, reason, mempool_sequence, this] { m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.TransactionRemovedFromMempool(tx, reason, mempool_sequence); }); }; diff --git a/src/validationinterface.h b/src/validationinterface.h index 3cf75aa210..22a17f350d 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -8,6 +8,7 @@ #include #include +#include #include // CTransaction(Ref) #include @@ -25,7 +26,6 @@ class BlockValidationState; class CBlock; class CBlockIndex; struct CBlockLocator; -enum class MemPoolRemovalReason; struct RemovedMempoolTransactionInfo; struct NewMempoolTransactionInfo; @@ -104,7 +104,7 @@ protected: * * Called on a background thread. */ - virtual void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) {} + virtual void TransactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& reason, uint64_t mempool_sequence) {} /* * Notifies listeners of transactions removed from the mempool as * as a result of new block being connected. @@ -220,7 +220,7 @@ public: void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); void ActiveTipChange(const CBlockIndex&, bool); void TransactionAddedToMempool(const NewMempoolTransactionInfo&, uint64_t mempool_sequence); - void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); + void TransactionRemovedFromMempool(const CTransactionRef&, const MemPoolRemovalReason&, uint64_t mempool_sequence); void MempoolTransactionsRemovedForBlock(const std::vector&, unsigned int nBlockHeight); void BlockConnected(ChainstateRole, const std::shared_ptr &, const CBlockIndex *pindex); void BlockDisconnected(const std::shared_ptr &, const CBlockIndex* pindex); diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h index 9079f6dd82..569dac5ec3 100644 --- a/src/wallet/transaction.h +++ b/src/wallet/transaction.h @@ -78,7 +78,7 @@ struct TxStateUnrecognized { using TxState = std::variant; //! Subset of states transaction sync logic is implemented to handle. -using SyncTxState = std::variant; +using SyncTxState = std::variant; //! Try to interpret deserialized TxStateUnrecognized data as a recognized state. static inline TxState TxStateInterpretSerialized(TxStateUnrecognized data) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index dd9dfcd516..41feff2561 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1248,6 +1248,18 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const SyncTxS } } + if (auto* conf = std::get_if(&state)) { + WalletLogPrintf("A transaction in block %s conflicts with wallet transaction %s\n", conf->conflicting_block_hash.ToString(), ptx->GetHash().ToString()); + auto try_updating_state = [&](CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { + if (std::get_if(&wtx.m_state)) return TxUpdate::UNCHANGED; + + wtx.m_state = TxStateBlockConflicted(conf->conflicting_block_hash, conf->conflicting_block_height); + return TxUpdate::CHANGED; + }; + // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too. + RecursiveUpdateTxState(ptx->GetHash(), try_updating_state); + } + bool fExisted = mapWallet.count(tx.GetHash()) != 0; if (fExisted && !fUpdate) return false; if (fExisted || IsMine(tx) || IsFromMe(tx)) @@ -1457,40 +1469,39 @@ void CWallet::transactionAddedToMempool(const CTransactionRef& tx) { } } -void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) { +void CWallet::transactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& reason) +{ LOCK(cs_wallet); auto it = mapWallet.find(tx->GetHash()); if (it != mapWallet.end()) { RefreshMempoolStatus(it->second, chain()); } + + auto* replaced_reason = std::get_if(&reason); + + // Check if wallet transaction is being replaced by a parent transaction which is not from this wallet + if (IsFromMe(*tx) && replaced_reason != nullptr && replaced_reason->replacement_tx.has_value() && !IsFromMe(*replaced_reason->replacement_tx.value())) { + m_unrelated_conflict_tx_watchlist.insert(std::make_pair(replaced_reason->replacement_tx.value()->GetHash(), tx)); + } + + auto iter = m_unrelated_conflict_tx_watchlist.find(tx->GetHash()); + if (iter != m_unrelated_conflict_tx_watchlist.end()) { + // The replacement tx was removed from the mempool, remove it from map + // This new replacement tx may not conflict with the original tx + // so leave wallet tx to remain as TxStateInactive + m_unrelated_conflict_tx_watchlist.erase(iter); + } + + auto* conflict_reason = std::get_if(&reason); + // Handle transactions that were removed from the mempool because they // conflict with transactions in a newly connected block. - if (reason == MemPoolRemovalReason::CONFLICT) { - // Trigger external -walletnotify notifications 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 + if (conflict_reason != nullptr && IsFromMe(*tx)) { + // 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, TxStateInactive{}); + SyncTransaction(tx, TxStateBlockConflicted(conflict_reason->conflicting_block_hash, conflict_reason->conflicting_block_height)); } const Txid& txid = tx->GetHash(); @@ -1526,8 +1537,15 @@ void CWallet::blockConnected(ChainstateRole role, const interfaces::BlockInfo& b // Scan block for (size_t index = 0; index < block.data->vtx.size(); index++) { + auto it = m_unrelated_conflict_tx_watchlist.find(block.data->vtx[index]->GetHash()); + if (it != m_unrelated_conflict_tx_watchlist.end()) { + // A conflicting unrelated parent transaction was confirmed in a block + // Mark child wallet tx as conflicted + SyncTransaction(it->second, TxStateBlockConflicted(block.hash, block.height)); + } + SyncTransaction(block.data->vtx[index], TxStateConfirmed{block.hash, block.height, static_cast(index)}); - transactionRemovedFromMempool(block.data->vtx[index], MemPoolRemovalReason::BLOCK); + transactionRemovedFromMempool(block.data->vtx[index], BlockReason{}); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 0afe71a40e..49bae2b4b6 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -56,7 +57,6 @@ class CKeyID; class CPubKey; class Coin; class SigningProvider; -enum class MemPoolRemovalReason; enum class SigningResult; namespace common { enum class PSBTError; @@ -428,6 +428,15 @@ private: //! Cache of descriptor ScriptPubKeys used for IsMine. Maps ScriptPubKey to set of spkms std::unordered_map, SaltedSipHasher> m_cached_spks; + /** + * Tracks txids of txs that replace parents of wallet transactions. + * The key is the txid of the replacing transaction which may be unrelated to this wallet and + * the value is the replaced transaction which is related to this wallet. + * On blockConnected, the wallet checks the block for replacement txids stored in this map, + * if found, the wallet marks the child wallet tx as 'CONFLICTED' + */ + std::unordered_map m_unrelated_conflict_tx_watchlist GUARDED_BY(cs_wallet); + /** * Catch wallet up to current chain, scanning new blocks, updating the best * block locator and m_last_block_processed, and registering for @@ -631,7 +640,7 @@ public: uint256 last_failed_block; }; ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress); - void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) override; + void transactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& reason) override; /** Set the next time this wallet should resend transactions to 12-36 hours from now, ~1 day on average. */ void SetNextResend() { m_next_resend = GetDefaultNextResend(); } /** Return true if all conditions for periodically resending transactions are met. */ diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index 0039f55698..2d18e41666 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -166,7 +166,7 @@ void CZMQNotificationInterface::TransactionAddedToMempool(const NewMempoolTransa }); } -void CZMQNotificationInterface::TransactionRemovedFromMempool(const CTransactionRef& ptx, MemPoolRemovalReason reason, uint64_t mempool_sequence) +void CZMQNotificationInterface::TransactionRemovedFromMempool(const CTransactionRef& ptx, const MemPoolRemovalReason& reason, uint64_t mempool_sequence) { // Called for all non-block inclusion reasons const CTransaction& tx = *ptx; diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index c879fdd0dd..947582f47a 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -34,7 +34,7 @@ protected: // CValidationInterface void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) override; - void TransactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override; + void TransactionRemovedFromMempool(const CTransactionRef& tx, const MemPoolRemovalReason& reason, uint64_t mempool_sequence) override; void BlockConnected(ChainstateRole role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override; void BlockDisconnected(const std::shared_ptr& pblock, const CBlockIndex* pindexDisconnected) override; void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override; diff --git a/test/functional/wallet_conflicts.py b/test/functional/wallet_conflicts.py index 7a950ffae6..8721f056d4 100755 --- a/test/functional/wallet_conflicts.py +++ b/test/functional/wallet_conflicts.py @@ -9,9 +9,11 @@ Test that wallet correctly tracks transactions that have been conflicted by bloc from decimal import Decimal +from test_framework.messages import COIN from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( - assert_equal, + assert_equal, + find_vout_for_address, ) class TxConflicts(BitcoinTestFramework): @@ -36,6 +38,7 @@ class TxConflicts(BitcoinTestFramework): """ self.test_block_conflicts() + self.test_unknown_parent_conflict() self.test_mempool_conflict() self.test_mempool_and_block_conflicts() self.test_descendants_with_mempool_conflicts() @@ -422,5 +425,61 @@ class TxConflicts(BitcoinTestFramework): bob.unloadwallet() carol.unloadwallet() + def test_unknown_parent_conflict(self): + self.log.info("Test that a conflict with parent not belonging to the wallet causes in-wallet children to also conflict") + + def_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + self.nodes[0].createwallet("parent_conflict") + wallet = self.nodes[0].get_wallet_rpc("parent_conflict") + + # two utxos to target wallet + addr = wallet.getnewaddress() + addr_desc = wallet.getaddressinfo(addr)["desc"] + def_wallet.sendtoaddress(addr, 10) + def_wallet.sendtoaddress(addr, 10) + + self.generate(self.nodes[0], 1) + + txids = [] + for node in [self.nodes[0], self.nodes[1]]: + utxo = wallet.listunspent()[0] + # Make utxo in grandparent that will be double spent + gp_addr = def_wallet.getnewaddress() + gp_txid = def_wallet.sendtoaddress(gp_addr, 7) + gp_utxo = {"txid": gp_txid, "vout": find_vout_for_address(self.nodes[0], gp_txid, gp_addr)} + + self.generate(self.nodes[0], 1) + + # make unconfirmed parent tx + parent_addr = def_wallet.getnewaddress() + parent_txid = def_wallet.send(outputs=[{parent_addr: 5}], inputs=[gp_utxo])["txid"] + parent_utxo = {"txid": parent_txid, "vout": find_vout_for_address(self.nodes[0], parent_txid, parent_addr)} + parent_me = self.nodes[0].getmempoolentry(parent_txid) + parent_feerate = parent_me["fees"]["base"] * COIN / parent_me["vsize"] + self.nodes[1].sendrawtransaction(def_wallet.gettransaction(parent_txid)["hex"]) + + # make child tx that has one input belonging to target wallet, and one input not + psbt = def_wallet.walletcreatefundedpsbt(inputs=[utxo, parent_utxo], outputs=[{def_wallet.getnewaddress(): 13}], solving_data={"descriptors":[addr_desc]})["psbt"] + psbt = def_wallet.walletprocesspsbt(psbt)["psbt"] + psbt_proc = wallet.walletprocesspsbt(psbt) + psbt = psbt_proc["psbt"] + assert_equal(psbt_proc["complete"], True) + child_txid = self.nodes[0].sendrawtransaction(psbt_proc["hex"]) + txids.append(child_txid) + self.nodes[1].sendrawtransaction(psbt_proc["hex"]) + + # Make a conflict spending parent + conflict_psbt = def_wallet.walletcreatefundedpsbt(inputs=[gp_utxo], outputs=[{def_wallet.getnewaddress(): 2}], fee_rate="{:.8f}".format(Decimal(parent_feerate*3)))["psbt"] + conflict_proc = def_wallet.walletprocesspsbt(conflict_psbt) + node.sendrawtransaction(conflict_proc["hex"]) + self.generate(node, 1, sync_fun=self.no_op) + + assert_equal(wallet.gettransaction(txids[0])["confirmations"], -3) + + self.log.info("Test that receiving a block with a conflict with parent not belonging to the wallet causes in-wallet children to also conflict") + # Sync block with conflict tx + self.sync_blocks([self.nodes[0], self.nodes[1]]) + assert_equal(wallet.gettransaction(txids[1])["confirmations"], -1) + if __name__ == '__main__': TxConflicts(__file__).main()