From c876a892ec0b04851bea0a688d7681b6aaca4cb7 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Mon, 31 Mar 2025 16:28:50 +0100 Subject: [PATCH] Replace GenTxid with Txid/Wtxid overloads in `txmempool` Co-authored-by: stickies-v --- src/interfaces/chain.h | 2 +- src/net_processing.cpp | 20 ++++++----- src/node/interfaces.cpp | 4 +-- src/node/mempool_persist.cpp | 2 +- src/node/mini_miner.cpp | 2 +- src/node/txdownloadman_impl.cpp | 4 +-- src/policy/rbf.cpp | 4 +-- src/rpc/mempool.cpp | 6 ++-- src/test/fuzz/mini_miner.cpp | 2 +- src/test/fuzz/package_eval.cpp | 4 +-- src/test/fuzz/partially_downloaded_block.cpp | 2 +- src/test/fuzz/tx_pool.cpp | 4 +-- src/test/mempool_tests.cpp | 36 ++++++++++---------- src/test/rbf_tests.cpp | 2 +- src/test/txpackage_tests.cpp | 2 +- src/test/util/txmempool.cpp | 6 ++-- src/txmempool.cpp | 22 ++---------- src/txmempool.h | 32 ++++++++++++----- src/util/transaction_identifier.h | 4 +++ src/validation.cpp | 26 +++++++------- 20 files changed, 95 insertions(+), 91 deletions(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index f6e831624d0..511ba41568e 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -208,7 +208,7 @@ public: virtual RBFTransactionState isRBFOptIn(const CTransaction& tx) = 0; //! Check if transaction is in mempool. - virtual bool isInMempool(const uint256& txid) = 0; + virtual bool isInMempool(const Txid& txid) = 0; //! Check if transaction has descendants in mempool. virtual bool hasDescendantsInMempool(const uint256& txid) = 0; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f390b0f944b..c5ca246ddef 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -947,7 +947,7 @@ private: std::atomic m_last_tip_update{0s}; /** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */ - CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const GenTxid& gtxid) + CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, NetEventsInterface::g_msgproc_mutex); void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic& interruptMsgProc) @@ -2391,10 +2391,15 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } } -CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const GenTxid& gtxid) +CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const CInv& inv) { + auto gtxid{ToGenTxid(inv).ToVariant()}; // If a tx was in the mempool prior to the last INV for this peer, permit the request. - auto txinfo = m_mempool.info_for_relay(gtxid, tx_relay.m_last_inv_sequence); + auto txinfo{std::visit( + [&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) { + return m_mempool.info_for_relay(id, tx_relay.m_last_inv_sequence); + }, + gtxid)}; if (txinfo.tx) { return std::move(txinfo.tx); } @@ -2403,7 +2408,7 @@ CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, { LOCK(m_most_recent_block_mutex); if (m_most_recent_block_txs != nullptr) { - auto it = m_most_recent_block_txs->find(gtxid.GetHash()); + auto it = m_most_recent_block_txs->find(gtxid.ToUint256()); if (it != m_most_recent_block_txs->end()) return it->second; } } @@ -2437,8 +2442,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic continue; } - CTransactionRef tx = FindTxForGetData(*tx_relay, ToGenTxid(inv)); - if (tx) { + if (auto tx{FindTxForGetData(*tx_relay, inv)}) { // WTX and WITNESS_TX imply we serialize with witness const auto maybe_with_witness = (inv.IsMsgTx() ? TX_NO_WITNESS : TX_WITH_WITNESS); MakeAndPushMessage(pfrom, NetMsgType::TX, maybe_with_witness(*tx)); @@ -4306,7 +4310,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, // Always relay transactions received from peers with forcerelay // permission, even if they were already in the mempool, allowing // the node to function as a gateway for nodes hidden behind it. - if (!m_mempool.exists(GenTxid::Txid(tx.GetHash()))) { + if (!m_mempool.exists(tx.GetHash())) { LogPrintf("Not relaying non-mempool transaction %s (wtxid=%s) from forcerelay peer=%d\n", tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), pfrom.GetId()); } else { @@ -5820,7 +5824,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) continue; } // Not in the mempool anymore? don't bother sending it. - auto txinfo = m_mempool.info(ToGenTxid(inv)); + auto txinfo{std::visit([&](const auto& id) { return m_mempool.info(id); }, hash)}; if (!txinfo.tx) { continue; } diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 04afc852e43..0b7cea347ce 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -669,11 +669,11 @@ public: LOCK(m_node.mempool->cs); return IsRBFOptIn(tx, *m_node.mempool); } - bool isInMempool(const uint256& txid) override + bool isInMempool(const Txid& txid) override { if (!m_node.mempool) return false; LOCK(m_node.mempool->cs); - return m_node.mempool->exists(GenTxid::Txid(txid)); + return m_node.mempool->exists(txid); } bool hasDescendantsInMempool(const uint256& txid) override { diff --git a/src/node/mempool_persist.cpp b/src/node/mempool_persist.cpp index ff7de8c64a2..437645f139e 100644 --- a/src/node/mempool_persist.cpp +++ b/src/node/mempool_persist.cpp @@ -106,7 +106,7 @@ bool LoadMempool(CTxMemPool& pool, const fs::path& load_path, Chainstate& active // wallet(s) having loaded it while we were processing // mempool transactions; consider these as valid, instead of // failed, but mark them as 'already there' - if (pool.exists(GenTxid::Txid(tx->GetHash()))) { + if (pool.exists(tx->GetHash())) { ++already_there; } else { ++failed; diff --git a/src/node/mini_miner.cpp b/src/node/mini_miner.cpp index d7d15554b32..dec1060ab7f 100644 --- a/src/node/mini_miner.cpp +++ b/src/node/mini_miner.cpp @@ -27,7 +27,7 @@ MiniMiner::MiniMiner(const CTxMemPool& mempool, const std::vector& ou // Anything that's spent by the mempool is to-be-replaced // Anything otherwise unavailable just has a bump fee of 0 for (const auto& outpoint : outpoints) { - if (!mempool.exists(GenTxid::Txid(outpoint.hash))) { + if (!mempool.exists(outpoint.hash)) { // This UTXO is either confirmed or not yet submitted to mempool. // If it's confirmed, no bump fee is required. // If it's not yet submitted, we have no information, so return 0. diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index f319414042f..ec2d772c1cf 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -148,7 +148,7 @@ bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_rec if (RecentConfirmedTransactionsFilter().contains(hash)) return true; - return RecentRejectsFilter().contains(hash) || m_opts.m_mempool.exists(gtxid); + return RecentRejectsFilter().contains(hash) || std::visit([&](const auto& id) { return m_opts.m_mempool.exists(id); }, gtxid.ToVariant()); } void TxDownloadManagerImpl::ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info) @@ -383,7 +383,7 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction fRejectedParents = true; break; } else if (RecentRejectsReconsiderableFilter().contains(parent_txid) && - !m_opts.m_mempool.exists(GenTxid::Txid(parent_txid))) { + !m_opts.m_mempool.exists(Txid::FromUint256(parent_txid))) { // More than 1 parent in m_lazy_recent_rejects_reconsiderable: 1p1c will not be // sufficient to accept this package, so just give up here. if (rejected_parent_reconsiderable.has_value()) { diff --git a/src/policy/rbf.cpp b/src/policy/rbf.cpp index bc76361fba3..dc74f43b354 100644 --- a/src/policy/rbf.cpp +++ b/src/policy/rbf.cpp @@ -32,7 +32,7 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool) // If this transaction is not in our mempool, then we can't be sure // we will know about all its inputs. - if (!pool.exists(GenTxid::Txid(tx.GetHash()))) { + if (!pool.exists(tx.GetHash())) { return RBFTransactionState::UNKNOWN; } @@ -107,7 +107,7 @@ std::optional HasNoNewUnconfirmed(const CTransaction& tx, if (!parents_of_conflicts.count(tx.vin[j].prevout.hash)) { // Rather than check the UTXO set - potentially expensive - it's cheaper to just check // if the new input refers to a tx that's in the mempool. - if (pool.exists(GenTxid::Txid(tx.vin[j].prevout.hash))) { + if (pool.exists(tx.vin[j].prevout.hash)) { return strprintf("replacement %s adds unconfirmed input, idx %d", tx.GetHash().ToString(), j); } diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 8538c764794..7b5597916c7 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -311,7 +311,7 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool std::set setDepends; for (const CTxIn& txin : tx.vin) { - if (pool.exists(GenTxid::Txid(txin.prevout.hash))) + if (pool.exists(txin.prevout.hash)) setDepends.insert(txin.prevout.hash.ToString()); } @@ -1038,7 +1038,7 @@ static RPCHelpMan submitpackage() // Belt-and-suspenders check; everything should be successful here CHECK_NONFATAL(package_result.m_tx_results.size() == txns.size()); for (const auto& tx : txns) { - CHECK_NONFATAL(mempool.exists(GenTxid::Txid(tx->GetHash()))); + CHECK_NONFATAL(mempool.exists(tx->GetHash())); } break; } @@ -1062,7 +1062,7 @@ static RPCHelpMan submitpackage() size_t num_broadcast{0}; for (const auto& tx : txns) { // We don't want to re-submit the txn for validation in BroadcastTransaction - if (!mempool.exists(GenTxid::Txid(tx->GetHash()))) { + if (!mempool.exists(tx->GetHash())) { continue; } diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp index baa28affcec..9c3ec7debf5 100644 --- a/src/test/fuzz/mini_miner.cpp +++ b/src/test/fuzz/mini_miner.cpp @@ -173,7 +173,7 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner) if (!pool.GetConflictTx(coin)) outpoints.push_back(coin); } for (const auto& tx : transactions) { - assert(pool.exists(GenTxid::Txid(tx->GetHash()))); + assert(pool.exists(tx->GetHash())); for (uint32_t n{0}; n < tx->vout.size(); ++n) { COutPoint coin{tx->GetHash(), n}; if (!pool.GetConflictTx(coin)) outpoints.push_back(coin); diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp index ff20f12fc73..19851b0593f 100644 --- a/src/test/fuzz/package_eval.cpp +++ b/src/test/fuzz/package_eval.cpp @@ -310,8 +310,8 @@ FUZZ_TARGET(ephemeral_package_eval, .init = initialize_tx_pool) const auto delta = fuzzed_data_provider.ConsumeIntegralInRange(-50 * COIN, +50 * COIN); // We only prioritise out of mempool transactions since PrioritiseTransaction doesn't // filter for ephemeral dust - if (tx_pool.exists(GenTxid::Txid(txid))) { - const auto tx_info{tx_pool.info(GenTxid::Txid(txid))}; + if (tx_pool.exists(txid)) { + const auto tx_info{tx_pool.info(txid)}; if (GetDust(*tx_info.tx, tx_pool.m_opts.dust_relay_feerate).empty()) { tx_pool.PrioritiseTransaction(txid.ToUint256(), delta); } diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp index e3a2f75d8e0..90bdf521266 100644 --- a/src/test/fuzz/partially_downloaded_block.cpp +++ b/src/test/fuzz/partially_downloaded_block.cpp @@ -83,7 +83,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb) available.insert(i); } - if (add_to_mempool && !pool.exists(GenTxid::Txid(tx->GetHash()))) { + if (add_to_mempool && !pool.exists(tx->GetHash())) { LOCK2(cs_main, pool.cs); AddToMempool(pool, ConsumeTxMemPoolEntry(fuzzed_data_provider, *tx)); available.insert(i); diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index a697ee9d838..6f848edf2da 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -315,8 +315,8 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool) node.validation_signals->SyncWithValidationInterfaceQueue(); node.validation_signals->UnregisterSharedValidationInterface(txr); - bool txid_in_mempool = tx_pool.exists(GenTxid::Txid(tx->GetHash())); - bool wtxid_in_mempool = tx_pool.exists(GenTxid::Wtxid(tx->GetWitnessHash())); + bool txid_in_mempool = tx_pool.exists(tx->GetHash()); + bool wtxid_in_mempool = tx_pool.exists(tx->GetWitnessHash()); CheckATMPInvariants(res, txid_in_mempool, wtxid_in_mempool); Assert(accepted != added.empty()); diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index f0094dce59f..5992d2a41eb 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -454,12 +454,12 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) AddToMempool(pool, entry.Fee(5000LL).FromTx(tx2)); pool.TrimToSize(pool.DynamicMemoryUsage()); // should do nothing - BOOST_CHECK(pool.exists(GenTxid::Txid(tx1.GetHash()))); - BOOST_CHECK(pool.exists(GenTxid::Txid(tx2.GetHash()))); + BOOST_CHECK(pool.exists(tx1.GetHash())); + BOOST_CHECK(pool.exists(tx2.GetHash())); pool.TrimToSize(pool.DynamicMemoryUsage() * 3 / 4); // should remove the lower-feerate transaction - BOOST_CHECK(pool.exists(GenTxid::Txid(tx1.GetHash()))); - BOOST_CHECK(!pool.exists(GenTxid::Txid(tx2.GetHash()))); + BOOST_CHECK(pool.exists(tx1.GetHash())); + BOOST_CHECK(!pool.exists(tx2.GetHash())); AddToMempool(pool, entry.FromTx(tx2)); CMutableTransaction tx3 = CMutableTransaction(); @@ -472,14 +472,14 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) AddToMempool(pool, entry.Fee(20000LL).FromTx(tx3)); pool.TrimToSize(pool.DynamicMemoryUsage() * 3 / 4); // tx3 should pay for tx2 (CPFP) - BOOST_CHECK(!pool.exists(GenTxid::Txid(tx1.GetHash()))); - BOOST_CHECK(pool.exists(GenTxid::Txid(tx2.GetHash()))); - BOOST_CHECK(pool.exists(GenTxid::Txid(tx3.GetHash()))); + BOOST_CHECK(!pool.exists(tx1.GetHash())); + BOOST_CHECK(pool.exists(tx2.GetHash())); + BOOST_CHECK(pool.exists(tx3.GetHash())); pool.TrimToSize(GetVirtualTransactionSize(CTransaction(tx1))); // mempool is limited to tx1's size in memory usage, so nothing fits - BOOST_CHECK(!pool.exists(GenTxid::Txid(tx1.GetHash()))); - BOOST_CHECK(!pool.exists(GenTxid::Txid(tx2.GetHash()))); - BOOST_CHECK(!pool.exists(GenTxid::Txid(tx3.GetHash()))); + BOOST_CHECK(!pool.exists(tx1.GetHash())); + BOOST_CHECK(!pool.exists(tx2.GetHash())); + BOOST_CHECK(!pool.exists(tx3.GetHash())); CFeeRate maxFeeRateRemoved(25000, GetVirtualTransactionSize(CTransaction(tx3)) + GetVirtualTransactionSize(CTransaction(tx2))); BOOST_CHECK_EQUAL(pool.GetMinFee(1).GetFeePerK(), maxFeeRateRemoved.GetFeePerK() + 1000); @@ -539,19 +539,19 @@ BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) // we only require this to remove, at max, 2 txn, because it's not clear what we're really optimizing for aside from that pool.TrimToSize(pool.DynamicMemoryUsage() - 1); - BOOST_CHECK(pool.exists(GenTxid::Txid(tx4.GetHash()))); - BOOST_CHECK(pool.exists(GenTxid::Txid(tx6.GetHash()))); - BOOST_CHECK(!pool.exists(GenTxid::Txid(tx7.GetHash()))); + BOOST_CHECK(pool.exists(tx4.GetHash())); + BOOST_CHECK(pool.exists(tx6.GetHash())); + BOOST_CHECK(!pool.exists(tx7.GetHash())); - if (!pool.exists(GenTxid::Txid(tx5.GetHash()))) + if (!pool.exists(tx5.GetHash())) AddToMempool(pool, entry.Fee(1000LL).FromTx(tx5)); AddToMempool(pool, entry.Fee(9000LL).FromTx(tx7)); pool.TrimToSize(pool.DynamicMemoryUsage() / 2); // should maximize mempool size by only removing 5/7 - BOOST_CHECK(pool.exists(GenTxid::Txid(tx4.GetHash()))); - BOOST_CHECK(!pool.exists(GenTxid::Txid(tx5.GetHash()))); - BOOST_CHECK(pool.exists(GenTxid::Txid(tx6.GetHash()))); - BOOST_CHECK(!pool.exists(GenTxid::Txid(tx7.GetHash()))); + BOOST_CHECK(pool.exists(tx4.GetHash())); + BOOST_CHECK(!pool.exists(tx5.GetHash())); + BOOST_CHECK(pool.exists(tx6.GetHash())); + BOOST_CHECK(!pool.exists(tx7.GetHash())); AddToMempool(pool, entry.Fee(1000LL).FromTx(tx5)); AddToMempool(pool, entry.Fee(9000LL).FromTx(tx7)); diff --git a/src/test/rbf_tests.cpp b/src/test/rbf_tests.cpp index 41bfe28c37d..0052276eac4 100644 --- a/src/test/rbf_tests.cpp +++ b/src/test/rbf_tests.cpp @@ -293,7 +293,7 @@ BOOST_FIXTURE_TEST_CASE(rbf_helper_functions, TestChain100Setup) const auto spends_unconfirmed = make_tx({tx1}, {36 * CENT}); for (const auto& input : spends_unconfirmed->vin) { // Spends unconfirmed inputs. - BOOST_CHECK(pool.exists(GenTxid::Txid(input.prevout.hash))); + BOOST_CHECK(pool.exists(input.prevout.hash)); } BOOST_CHECK(HasNoNewUnconfirmed(/*tx=*/ *spends_unconfirmed.get(), /*pool=*/ pool, diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 3c36b782604..9b6d22dde32 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -1090,7 +1090,7 @@ BOOST_AUTO_TEST_CASE(package_rbf_tests) BOOST_CHECK_EQUAL(m_node.mempool->size(), expected_pool_size); // child1 has been replaced - BOOST_CHECK(!m_node.mempool->exists(GenTxid::Txid(tx_child_1->GetHash()))); + BOOST_CHECK(!m_node.mempool->exists(tx_child_1->GetHash())); } // Test package rbf. diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index f1ca33bec78..5febb6791c6 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -122,17 +122,17 @@ std::optional CheckPackageMempoolAcceptResult(const Package& txns, if (mempool) { // The tx by txid should be in the mempool iff the result was not INVALID. const bool txid_in_mempool{atmp_result.m_result_type != MempoolAcceptResult::ResultType::INVALID}; - if (mempool->exists(GenTxid::Txid(tx->GetHash())) != txid_in_mempool) { + if (mempool->exists(tx->GetHash()) != txid_in_mempool) { return strprintf("tx %s should %sbe in mempool", wtxid.ToString(), txid_in_mempool ? "" : "not "); } // Additionally, if the result was DIFFERENT_WITNESS, we shouldn't be able to find the tx in mempool by wtxid. if (tx->HasWitness() && atmp_result.m_result_type == MempoolAcceptResult::ResultType::DIFFERENT_WITNESS) { - if (mempool->exists(GenTxid::Wtxid(wtxid))) { + if (mempool->exists(wtxid)) { return strprintf("wtxid %s should not be in mempool", wtxid.ToString()); } } for (const auto& tx_ref : atmp_result.m_replaced_transactions) { - if (mempool->exists(GenTxid::Txid(tx_ref->GetHash()))) { + if (mempool->exists(tx_ref->GetHash())) { return strprintf("tx %s should not be in mempool as it was replaced", tx_ref->GetWitnessHash().ToString()); } } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index f3b617e11d0..3c22909ad6b 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -886,24 +886,6 @@ CTransactionRef CTxMemPool::get(const uint256& hash) const return i->GetSharedTx(); } -TxMempoolInfo CTxMemPool::info(const GenTxid& gtxid) const -{ - LOCK(cs); - auto i = (gtxid.IsWtxid() ? GetIter(Wtxid::FromUint256(gtxid.GetHash())) : GetIter(Txid::FromUint256(gtxid.GetHash()))); - return i.has_value() ? GetInfo(*i) : TxMempoolInfo{}; -} - -TxMempoolInfo CTxMemPool::info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const -{ - LOCK(cs); - auto i = (gtxid.IsWtxid() ? GetIter(Wtxid::FromUint256(gtxid.GetHash())) : GetIter(Txid::FromUint256(gtxid.GetHash()))); - if (i.has_value() && (*i)->GetSequence() < last_sequence) { - return GetInfo(*i); - } else { - return TxMempoolInfo(); - } -} - void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeDelta) { { @@ -1018,7 +1000,7 @@ std::vector CTxMemPool::GetIterVec(const std::vector* pvNoSpends if (pvNoSpendsRemaining) { for (const CTransaction& tx : txn) { for (const CTxIn& txin : tx.vin) { - if (exists(GenTxid::Txid(txin.prevout.hash))) continue; + if (exists(txin.prevout.hash)) continue; pvNoSpendsRemaining->push_back(txin.prevout); } } diff --git a/src/txmempool.h b/src/txmempool.h index c6290c0cdc7..bc5af815baf 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -651,22 +651,38 @@ public: return m_total_fee; } - bool exists(const GenTxid& gtxid) const + bool exists(const Txid& txid) const { LOCK(cs); - if (gtxid.IsWtxid()) { - return (mapTx.get().count(gtxid.GetHash()) != 0); - } - return (mapTx.count(gtxid.GetHash()) != 0); + return (mapTx.count(txid) != 0); + } + + bool exists(const Wtxid& wtxid) const + { + LOCK(cs); + return (mapTx.get().count(wtxid) != 0); } const CTxMemPoolEntry* GetEntry(const Txid& txid) const LIFETIMEBOUND EXCLUSIVE_LOCKS_REQUIRED(cs); CTransactionRef get(const uint256& hash) const; - TxMempoolInfo info(const GenTxid& gtxid) const; + + template + TxMempoolInfo info(const T& id) const + { + LOCK(cs); + auto i{GetIter(id)}; + return i.has_value() ? GetInfo(*i) : TxMempoolInfo{}; + } /** Returns info for a transaction if its entry_sequence < last_sequence */ - TxMempoolInfo info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const; + template + TxMempoolInfo info_for_relay(const T& id, uint64_t last_sequence) const + { + LOCK(cs); + auto i{GetIter(id)}; + return (i.has_value() && i.value()->GetSequence() < last_sequence) ? GetInfo(*i) : TxMempoolInfo{}; + } std::vector entryAll() const EXCLUSIVE_LOCKS_REQUIRED(cs); std::vector infoAll() const; @@ -679,7 +695,7 @@ public: LOCK(cs); // Sanity check the transaction is in the mempool & insert into // unbroadcast set. - if (exists(GenTxid::Txid(txid))) m_unbroadcast_txids.insert(txid); + if (exists(Txid::FromUint256(txid))) m_unbroadcast_txids.insert(txid); }; /** Removes a transaction from the unbroadcast set */ diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h index 22ef43ea510..95b755f4a67 100644 --- a/src/util/transaction_identifier.h +++ b/src/util/transaction_identifier.h @@ -10,6 +10,7 @@ #include #include +#include #include #include @@ -80,6 +81,9 @@ using Txid = transaction_identifier; /** Wtxid commits to all transaction fields including the witness. */ using Wtxid = transaction_identifier; +template +concept TxidOrWtxid = std::is_same_v || std::is_same_v; + class GenTxidVariant : public std::variant { public: diff --git a/src/validation.cpp b/src/validation.cpp index 18c775a9db9..c0bc5f28fc9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -321,7 +321,7 @@ void Chainstate::MaybeUpdateMempoolForReorg( // 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); - } else if (m_mempool->exists(GenTxid::Txid((*it)->GetHash()))) { + } else if (m_mempool->exists((*it)->GetHash())) { vHashUpdate.push_back((*it)->GetHash()); } ++it; @@ -372,7 +372,7 @@ void Chainstate::MaybeUpdateMempoolForReorg( // If the transaction spends any coinbase outputs, it must be mature. if (it->GetSpendsCoinbase()) { for (const CTxIn& txin : tx.vin) { - if (m_mempool->exists(GenTxid::Txid(txin.prevout.hash))) continue; + if (m_mempool->exists(txin.prevout.hash)) continue; const Coin& coin{CoinsTip().AccessCoin(txin.prevout)}; assert(!coin.IsSpent()); const auto mempool_spend_height{m_chain.Tip()->nHeight + 1}; @@ -807,10 +807,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "non-final"); } - if (m_pool.exists(GenTxid::Wtxid(tx.GetWitnessHash()))) { + if (m_pool.exists(tx.GetWitnessHash())) { // Exact transaction already exists in the mempool. return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-already-in-mempool"); - } else if (m_pool.exists(GenTxid::Txid(tx.GetHash()))) { + } else if (m_pool.exists(tx.GetHash())) { // Transaction with the same non-witness data but different witness (same txid, different // wtxid) already exists in the mempool. return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-same-nonwitness-data-in-mempool"); @@ -1135,8 +1135,7 @@ bool MemPoolAccept::PackageMempoolChecks(const std::vector& txn AssertLockHeld(m_pool.cs); // CheckPackageLimits expects the package transactions to not already be in the mempool. - assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx) - { return !m_pool.exists(GenTxid::Txid(tx->GetHash()));})); + assert(std::all_of(txns.cbegin(), txns.cend(), [this](const auto& tx) { return !m_pool.exists(tx->GetHash()); })); assert(txns.size() == workspaces.size()); @@ -1346,8 +1345,7 @@ bool MemPoolAccept::SubmitPackage(const ATMPArgs& args, std::vector& AssertLockHeld(m_pool.cs); // Sanity check: none of the transactions should be in the mempool, and none of the transactions // should have a same-txid-different-witness equivalent in the mempool. - assert(std::all_of(workspaces.cbegin(), workspaces.cend(), [this](const auto& ws){ - return !m_pool.exists(GenTxid::Txid(ws.m_ptx->GetHash())); })); + assert(std::all_of(workspaces.cbegin(), workspaces.cend(), [this](const auto& ws) { return !m_pool.exists(ws.m_ptx->GetHash()); })); bool all_submitted = true; FinalizeSubpackage(args); @@ -1472,7 +1470,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef // Limit the mempool, if appropriate. if (!args.m_package_submission && !args.m_bypass_limits) { LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip()); - if (!m_pool.exists(GenTxid::Txid(ws.m_hash))) { + if (!m_pool.exists(ws.m_hash)) { // The tx no longer meets our (new) mempool minimum feerate but could be reconsidered in a package. ws.m_state.Invalid(TxValidationResult::TX_RECONSIDERABLE, "mempool full"); return MempoolAcceptResult::FeeFailure(ws.m_state, CFeeRate(ws.m_modified_fees, ws.m_vsize), {ws.m_ptx->GetWitnessHash()}); @@ -1767,7 +1765,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // There are 3 possibilities: already in mempool, same-txid-diff-wtxid already in mempool, // or not in mempool. An already confirmed tx is treated as one not in mempool, because all // we know is that the inputs aren't available. - if (m_pool.exists(GenTxid::Wtxid(wtxid))) { + if (m_pool.exists(wtxid)) { // Exact transaction already exists in the mempool. // Node operators are free to set their mempool policies however they please, nodes may receive // transactions in different orders, and malicious counterparties may try to take advantage of @@ -1779,7 +1777,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // checking ancestor/descendant limits, or double-count transaction fees for fee-related policy. const auto& entry{*Assert(m_pool.GetEntry(txid))}; results_final.emplace(wtxid, MempoolAcceptResult::MempoolTx(entry.GetTxSize(), entry.GetFee())); - } else if (m_pool.exists(GenTxid::Txid(txid))) { + } else if (m_pool.exists(txid)) { // Transaction with the same non-witness data but different witness (same txid, // different wtxid) already exists in the mempool. // @@ -1798,7 +1796,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, if (single_res.m_result_type == MempoolAcceptResult::ResultType::VALID) { // The transaction succeeded on its own and is now in the mempool. Don't include it // in package validation, because its fees should only be "used" once. - assert(m_pool.exists(GenTxid::Wtxid(wtxid))); + assert(m_pool.exists(wtxid)); results_final.emplace(wtxid, single_res); } else if (package.size() == 1 || // If there is only one transaction, no need to retry it "as a package" (single_res.m_state.GetResult() != TxValidationResult::TX_RECONSIDERABLE && @@ -1843,7 +1841,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, // If it was submitted, check to see if the tx is still in the mempool. It could have // been evicted due to LimitMempoolSize() above. const auto& txresult = multi_submission_result.m_tx_results.at(wtxid); - if (txresult.m_result_type == MempoolAcceptResult::ResultType::VALID && !m_pool.exists(GenTxid::Wtxid(wtxid))) { + if (txresult.m_result_type == MempoolAcceptResult::ResultType::VALID && !m_pool.exists(wtxid)) { package_state_final.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); TxValidationState mempool_full_state; mempool_full_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full"); @@ -1857,7 +1855,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package, Assume(it->second.m_result_type != MempoolAcceptResult::ResultType::INVALID); Assume(individual_results_nonfinal.count(wtxid) == 0); // Query by txid to include the same-txid-different-witness ones. - if (!m_pool.exists(GenTxid::Txid(tx->GetHash()))) { + if (!m_pool.exists(tx->GetHash())) { package_state_final.Invalid(PackageValidationResult::PCKG_TX, "transaction failed"); TxValidationState mempool_full_state; mempool_full_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "mempool full");