From 11d28f21bb8f0c3094934b3fef45871f73bb216a Mon Sep 17 00:00:00 2001 From: marcofleon Date: Mon, 31 Mar 2025 12:37:38 +0100 Subject: [PATCH 01/11] Implement GenTxid as a variant Reimplements the GenTxid class as a variant for better type safety. Also adds two temporary functions to the old GenTxid class that convert to and from the new variant. --- src/primitives/transaction.h | 14 ++++++++++++++ src/util/transaction_identifier.h | 22 ++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index bf86562886a..adadb2a3e74 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -437,6 +437,20 @@ public: const uint256& GetHash() const LIFETIMEBOUND { return m_hash; } friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; } friend bool operator<(const GenTxid& a, const GenTxid& b) { return std::tie(a.m_is_wtxid, a.m_hash) < std::tie(b.m_is_wtxid, b.m_hash); } + + GenTxidVariant ToVariant() const + { + return m_is_wtxid ? + GenTxidVariant{Wtxid::FromUint256(m_hash)} : + GenTxidVariant{Txid::FromUint256(m_hash)}; + } + + static GenTxid FromVariant(const GenTxidVariant& variant) + { + return GenTxid{ + std::holds_alternative<::Wtxid>(variant), + variant.ToUint256()}; + } }; #endif // BITCOIN_PRIMITIVES_TRANSACTION_H diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h index c86c8930158..22ef43ea510 100644 --- a/src/util/transaction_identifier.h +++ b/src/util/transaction_identifier.h @@ -9,6 +9,10 @@ #include #include +#include +#include +#include + /** transaction_identifier represents the two canonical transaction identifier * types (txid, wtxid).*/ template @@ -76,4 +80,22 @@ using Txid = transaction_identifier; /** Wtxid commits to all transaction fields including the witness. */ using Wtxid = transaction_identifier; +class GenTxidVariant : public std::variant +{ +public: + using variant::variant; + + bool IsWtxid() const { return std::holds_alternative(*this); } + + const uint256& ToUint256() const LIFETIMEBOUND + { + return std::visit([](const auto& id) -> const uint256& { return id.ToUint256(); }, *this); + } + + friend auto operator<=>(const GenTxidVariant& a, const GenTxidVariant& b) + { + return std::tuple(a.IsWtxid(), a.ToUint256()) <=> std::tuple(b.IsWtxid(), b.ToUint256()); + } +}; + #endif // BITCOIN_UTIL_TRANSACTION_IDENTIFIER_H From fcf92fd640eae60d1f601136a4e1c9de8ccb68b5 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Mon, 23 Jun 2025 14:52:13 +0100 Subject: [PATCH 02/11] refactor: make CTxMemPool::GetIter strongly typed This allows adding a GetIter(const Wtxid&) overload in a next commit, making it easier to visit this function from a variant. Co-authored-by: stickies-v --- src/rpc/mempool.cpp | 4 ++-- src/test/txvalidation_tests.cpp | 6 +++--- src/txmempool.cpp | 8 ++++---- src/txmempool.h | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index e9bae93642c..8538c764794 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -523,12 +523,12 @@ static RPCHelpMan getmempooldescendants() if (!request.params[1].isNull()) fVerbose = request.params[1].get_bool(); - uint256 hash = ParseHashV(request.params[0], "parameter 1"); + Txid txid{Txid::FromUint256(ParseHashV(request.params[0], "parameter 1"))}; const CTxMemPool& mempool = EnsureAnyMemPool(request.context); LOCK(mempool.cs); - const auto it{mempool.GetIter(hash)}; + const auto it{mempool.GetIter(txid)}; if (!it) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction not in mempool"); } diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 83d6c246849..d7f78443a88 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -304,7 +304,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) Package package_v3_v2{mempool_tx_v3, tx_v2_from_v3}; BOOST_CHECK_EQUAL(*PackageTRUCChecks(tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), package_v3_v2, empty_ancestors), expected_error_str); - CTxMemPool::setEntries entries_mempool_v3{pool.GetIter(mempool_tx_v3->GetHash().ToUint256()).value()}; + CTxMemPool::setEntries entries_mempool_v3{pool.GetIter(mempool_tx_v3->GetHash()).value()}; BOOST_CHECK_EQUAL(*PackageTRUCChecks(tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), {tx_v2_from_v3}, entries_mempool_v3), expected_error_str); // mempool_tx_v3 mempool_tx_v2 @@ -339,7 +339,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) Package package_v2_v3{mempool_tx_v2, tx_v3_from_v2}; BOOST_CHECK_EQUAL(*PackageTRUCChecks(tx_v3_from_v2, GetVirtualTransactionSize(*tx_v3_from_v2), package_v2_v3, empty_ancestors), expected_error_str); - CTxMemPool::setEntries entries_mempool_v2{pool.GetIter(mempool_tx_v2->GetHash().ToUint256()).value()}; + CTxMemPool::setEntries entries_mempool_v2{pool.GetIter(mempool_tx_v2->GetHash()).value()}; BOOST_CHECK_EQUAL(*PackageTRUCChecks(tx_v3_from_v2, GetVirtualTransactionSize(*tx_v3_from_v2), {tx_v3_from_v2}, entries_mempool_v2), expected_error_str); // mempool_tx_v3 mempool_tx_v2 @@ -536,7 +536,7 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup) // Configuration where parent already has 2 other children in mempool (no sibling eviction allowed). This may happen as the result of a reorg. AddToMempool(pool, entry.FromTx(tx_v3_child2)); auto tx_v3_child3 = make_tx({COutPoint{mempool_tx_v3->GetHash(), 24}}, /*version=*/3); - auto entry_mempool_parent = pool.GetIter(mempool_tx_v3->GetHash().ToUint256()).value(); + auto entry_mempool_parent = pool.GetIter(mempool_tx_v3->GetHash()).value(); BOOST_CHECK_EQUAL(entry_mempool_parent->GetCountWithDescendants(), 3); auto ancestors_2siblings{pool.CalculateMemPoolAncestors(entry.FromTx(tx_v3_child3), m_limits)}; diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 3a5a3fb306d..36bd58bfbff 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -154,7 +154,7 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector& vHashes for (const auto& txid : descendants_to_remove) { // 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)) { + if (const std::optional txiter = GetIter(Txid::FromUint256(txid))) { removeRecursive((*txiter)->GetTx(), MemPoolRemovalReason::SIZELIMIT); } } @@ -984,9 +984,9 @@ const CTransaction* CTxMemPool::GetConflictTx(const COutPoint& prevout) const return it == mapNextTx.end() ? nullptr : it->second; } -std::optional CTxMemPool::GetIter(const uint256& txid) const +std::optional CTxMemPool::GetIter(const Txid& txid) const { - auto it = mapTx.find(txid); + auto it = mapTx.find(txid.ToUint256()); if (it != mapTx.end()) return it; return std::nullopt; } @@ -1007,7 +1007,7 @@ std::vector CTxMemPool::GetIterVec(const std::vector ret; ret.reserve(txids.size()); for (const auto& txid : txids) { - const auto it{GetIter(txid)}; + const auto it{GetIter(Txid::FromUint256(txid))}; if (!it) return {}; ret.push_back(*it); } diff --git a/src/txmempool.h b/src/txmempool.h index 10acb2aa22f..4bce0d43ab8 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -498,7 +498,7 @@ public: const CTransaction* GetConflictTx(const COutPoint& prevout) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Returns an iterator to the given hash, if found */ - std::optional GetIter(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::optional GetIter(const Txid& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Translate a set of hashes into a set of pool iterators to avoid repeated lookups. * Does not require that all of the hashes correspond to actual transactions in the mempool, From 243553d59071f3e43a42f3809706790495b17ffc Mon Sep 17 00:00:00 2001 From: stickies-v Date: Mon, 23 Jun 2025 15:14:10 +0100 Subject: [PATCH 03/11] refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) Overloading GetIter makes it easier to use std::visit patterns from a GenTxid. --- src/txmempool.cpp | 33 +++++++++++++++++++-------------- src/txmempool.h | 6 +----- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 36bd58bfbff..930fd4de18e 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -802,14 +802,14 @@ bool CTxMemPool::CompareDepthAndScore(const uint256& hasha, const uint256& hashb * both are in the mempool and a has a higher score than b */ LOCK(cs); - indexed_transaction_set::const_iterator j = wtxid ? get_iter_from_wtxid(hashb) : mapTx.find(hashb); - if (j == mapTx.end()) return false; - indexed_transaction_set::const_iterator i = wtxid ? get_iter_from_wtxid(hasha) : mapTx.find(hasha); - if (i == mapTx.end()) return true; - uint64_t counta = i->GetCountWithAncestors(); - uint64_t countb = j->GetCountWithAncestors(); + auto j = wtxid ? GetIter(Wtxid::FromUint256(hashb)) : GetIter(Txid::FromUint256(hashb)); + if (!j.has_value()) return false; + auto i = wtxid ? GetIter(Wtxid::FromUint256(hasha)) : GetIter(Txid::FromUint256(hasha)); + if (!i.has_value()) return true; + uint64_t counta = i.value()->GetCountWithAncestors(); + uint64_t countb = j.value()->GetCountWithAncestors(); if (counta == countb) { - return CompareTxMemPoolEntryByScore()(*i, *j); + return CompareTxMemPoolEntryByScore()(*i.value(), *j.value()); } return counta < countb; } @@ -893,18 +893,16 @@ CTransactionRef CTxMemPool::get(const uint256& hash) const TxMempoolInfo CTxMemPool::info(const GenTxid& gtxid) const { LOCK(cs); - indexed_transaction_set::const_iterator i = (gtxid.IsWtxid() ? get_iter_from_wtxid(gtxid.GetHash()) : mapTx.find(gtxid.GetHash())); - if (i == mapTx.end()) - return TxMempoolInfo(); - return GetInfo(i); + 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); - indexed_transaction_set::const_iterator i = (gtxid.IsWtxid() ? get_iter_from_wtxid(gtxid.GetHash()) : mapTx.find(gtxid.GetHash())); - if (i != mapTx.end() && i->GetSequence() < last_sequence) { - return GetInfo(i); + 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(); } @@ -991,6 +989,13 @@ std::optional CTxMemPool::GetIter(const Txid& txid) const return std::nullopt; } +std::optional CTxMemPool::GetIter(const Wtxid& wtxid) const +{ + AssertLockHeld(cs); + auto it{mapTx.project<0>(mapTx.get().find(wtxid))}; + return it != mapTx.end() ? std::make_optional(it) : std::nullopt; +} + CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set& hashes) const { CTxMemPool::setEntries ret; diff --git a/src/txmempool.h b/src/txmempool.h index 4bce0d43ab8..c30bc96942d 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -499,6 +499,7 @@ public: /** Returns an iterator to the given hash, if found */ std::optional GetIter(const Txid& txid) const EXCLUSIVE_LOCKS_REQUIRED(cs); + std::optional GetIter(const Wtxid& wtxid) const EXCLUSIVE_LOCKS_REQUIRED(cs); /** Translate a set of hashes into a set of pool iterators to avoid repeated lookups. * Does not require that all of the hashes correspond to actual transactions in the mempool, @@ -656,11 +657,6 @@ public: const CTxMemPoolEntry* GetEntry(const Txid& txid) const LIFETIMEBOUND EXCLUSIVE_LOCKS_REQUIRED(cs); CTransactionRef get(const uint256& hash) const; - txiter get_iter_from_wtxid(const uint256& wtxid) const EXCLUSIVE_LOCKS_REQUIRED(cs) - { - AssertLockHeld(cs); - return mapTx.project<0>(mapTx.get().find(wtxid)); - } TxMempoolInfo info(const GenTxid& gtxid) const; /** Returns info for a transaction if its entry_sequence < last_sequence */ From eee473d9f3019a0ea4ebbc9c41781813ad574a86 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Mon, 31 Mar 2025 17:42:20 +0100 Subject: [PATCH 04/11] Convert `CompareInvMempoolOrder` to GenTxidVariant Now that we are storing `CTxMemPool::CompareDepthAndScore` parameters using `std::variant` we have no portable zero-overhead way of accessing them, so use `std::visit` and drop `bool wtxid` in-parameter. Co-authored-by: stickies-v --- src/net_processing.cpp | 48 ++++++++++++++++++---------------------- src/net_processing.h | 2 +- src/node/transaction.cpp | 2 +- src/txmempool.cpp | 6 ++--- src/txmempool.h | 5 +++-- 5 files changed, 30 insertions(+), 33 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c4fe412fb8a..f390b0f944b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -302,7 +302,7 @@ struct Peer { * non-wtxid-relay peers, wtxid for wtxid-relay peers). We use the * mempool to sort transactions in dependency order before relay, so * this does not have to be sorted. */ - std::set m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex); + std::set m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex); /** Whether the peer has requested us to send our complete mempool. Only * permitted if the peer has NetPermissionFlags::Mempool or we advertise * NODE_BLOOM. See BIP35. */ @@ -536,7 +536,7 @@ public: std::vector GetOrphanTransactions() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); PeerManagerInfo GetInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); + void RelayTransaction(const Txid& txid, const Wtxid& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void SetBestBlock(int height, std::chrono::seconds time) override { m_best_height = height; @@ -1583,7 +1583,7 @@ void PeerManagerImpl::ReattemptInitialBroadcast(CScheduler& scheduler) CTransactionRef tx = m_mempool.get(txid); if (tx != nullptr) { - RelayTransaction(txid, tx->GetWitnessHash()); + RelayTransaction(Txid::FromUint256(txid), tx->GetWitnessHash()); } else { m_mempool.RemoveUnbroadcastTx(txid, true); } @@ -2150,7 +2150,7 @@ void PeerManagerImpl::SendPings() for(auto& it : m_peer_map) it.second->m_ping_queued = true; } -void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid) +void PeerManagerImpl::RelayTransaction(const Txid& txid, const Wtxid& wtxid) { LOCK(m_peer_mutex); for(auto& it : m_peer_map) { @@ -2166,11 +2166,11 @@ void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid // in the announcement. if (tx_relay->m_next_inv_send_time == 0s) continue; - const uint256& hash{peer.m_wtxid_relay ? wtxid : txid}; - if (!tx_relay->m_tx_inventory_known_filter.contains(hash)) { - tx_relay->m_tx_inventory_to_send.insert(hash); + const auto gtxid{peer.m_wtxid_relay ? GenTxidVariant{wtxid} : GenTxidVariant{txid}}; + if (!tx_relay->m_tx_inventory_known_filter.contains(gtxid.ToUint256())) { + tx_relay->m_tx_inventory_to_send.insert(gtxid); } - }; + } } void PeerManagerImpl::RelayAddress(NodeId originator, @@ -5442,20 +5442,15 @@ void PeerManagerImpl::MaybeSendFeefilter(CNode& pto, Peer& peer, std::chrono::mi namespace { class CompareInvMempoolOrder { - CTxMemPool* mp; - bool m_wtxid_relay; + const CTxMemPool* m_mempool; public: - explicit CompareInvMempoolOrder(CTxMemPool *_mempool, bool use_wtxid) - { - mp = _mempool; - m_wtxid_relay = use_wtxid; - } + explicit CompareInvMempoolOrder(CTxMemPool* mempool) : m_mempool{mempool} {} - bool operator()(std::set::iterator a, std::set::iterator b) + bool operator()(std::set::iterator a, std::set::iterator b) { /* As std::make_heap produces a max-heap, we want the entries with the * fewest ancestors/highest fee to sort later. */ - return mp->CompareDepthAndScore(*b, *a, m_wtxid_relay); + return m_mempool->CompareDepthAndScore(*b, *a); } }; } // namespace @@ -5773,7 +5768,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) txinfo.tx->GetWitnessHash().ToUint256() : txinfo.tx->GetHash().ToUint256(), }; - tx_relay->m_tx_inventory_to_send.erase(inv.hash); + tx_relay->m_tx_inventory_to_send.erase(ToGenTxid(inv).ToVariant()); // Don't send transactions that peers will not put into their mempool if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) { @@ -5794,15 +5789,15 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Determine transactions to relay if (fSendTrickle) { // Produce a vector with all candidates for sending - std::vector::iterator> vInvTx; + std::vector::iterator> vInvTx; vInvTx.reserve(tx_relay->m_tx_inventory_to_send.size()); - for (std::set::iterator it = tx_relay->m_tx_inventory_to_send.begin(); it != tx_relay->m_tx_inventory_to_send.end(); it++) { + for (std::set::iterator it = tx_relay->m_tx_inventory_to_send.begin(); it != tx_relay->m_tx_inventory_to_send.end(); it++) { vInvTx.push_back(it); } const CFeeRate filterrate{tx_relay->m_fee_filter_received.load()}; // Topologically and fee-rate sort the inventory we send for privacy and priority reasons. // A heap is used so that not all items need sorting if only a few are being sent. - CompareInvMempoolOrder compareInvMempoolOrder(&m_mempool, peer->m_wtxid_relay); + CompareInvMempoolOrder compareInvMempoolOrder(&m_mempool); std::make_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder); // No reason to drain out at many times the network's capacity, // especially since we have many peers and some will draw much shorter delays. @@ -5813,14 +5808,15 @@ bool PeerManagerImpl::SendMessages(CNode* pto) while (!vInvTx.empty() && nRelayedTransactions < broadcast_max) { // Fetch the top element from the heap std::pop_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder); - std::set::iterator it = vInvTx.back(); + std::set::iterator it = vInvTx.back(); vInvTx.pop_back(); - uint256 hash = *it; - CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash); + GenTxidVariant hash = *it; + Assume(peer->m_wtxid_relay == hash.IsWtxid()); + CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash.ToUint256()); // Remove it from the to-be-sent set tx_relay->m_tx_inventory_to_send.erase(it); // Check if not in the filter already - if (tx_relay->m_tx_inventory_known_filter.contains(hash)) { + if (tx_relay->m_tx_inventory_known_filter.contains(hash.ToUint256())) { continue; } // Not in the mempool anymore? don't bother sending it. @@ -5840,7 +5836,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) MakeAndPushMessage(*pto, NetMsgType::INV, vInv); vInv.clear(); } - tx_relay->m_tx_inventory_known_filter.insert(hash); + tx_relay->m_tx_inventory_known_filter.insert(hash.ToUint256()); } // Ensure we'll respond to GETDATA requests for anything we've just announced diff --git a/src/net_processing.h b/src/net_processing.h index 1156ec08f73..147ab681bed 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -119,7 +119,7 @@ public: virtual PeerManagerInfo GetInfo() const = 0; /** Relay transaction to all peers. */ - virtual void RelayTransaction(const uint256& txid, const uint256& wtxid) = 0; + virtual void RelayTransaction(const Txid& txid, const Wtxid& wtxid) = 0; /** Send ping message to all peers */ virtual void SendPings() = 0; diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 666597391e3..9162557bca1 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -42,7 +42,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t std::promise promise; Txid txid = tx->GetHash(); - uint256 wtxid = tx->GetWitnessHash(); + Wtxid wtxid = tx->GetWitnessHash(); bool callback_set = false; { diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 930fd4de18e..e3660d9aea2 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -794,7 +794,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei assert(innerUsage == cachedInnerUsage); } -bool CTxMemPool::CompareDepthAndScore(const uint256& hasha, const uint256& hashb, bool wtxid) +bool CTxMemPool::CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb) const { /* Return `true` if hasha should be considered sooner than hashb. Namely when: * a is not in the mempool, but b is @@ -802,9 +802,9 @@ bool CTxMemPool::CompareDepthAndScore(const uint256& hasha, const uint256& hashb * both are in the mempool and a has a higher score than b */ LOCK(cs); - auto j = wtxid ? GetIter(Wtxid::FromUint256(hashb)) : GetIter(Txid::FromUint256(hashb)); + auto j{std::visit([&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(cs) { return GetIter(id); }, hashb)}; if (!j.has_value()) return false; - auto i = wtxid ? GetIter(Wtxid::FromUint256(hasha)) : GetIter(Txid::FromUint256(hasha)); + auto i{std::visit([&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(cs) { return GetIter(id); }, hasha)}; if (!i.has_value()) return true; uint64_t counta = i.value()->GetCountWithAncestors(); uint64_t countb = j.value()->GetCountWithAncestors(); diff --git a/src/txmempool.h b/src/txmempool.h index c30bc96942d..ca1d1e870e7 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -19,9 +19,10 @@ #include #include #include +#include #include #include -#include +#include #include #include @@ -466,7 +467,7 @@ public: void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); - bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb, bool wtxid=false); + bool CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb) const; bool isSpent(const COutPoint& outpoint) const; unsigned int GetTransactionsUpdated() const; void AddTransactionsUpdated(unsigned int n); From de858ce2bea83c53635dee9a49c8c273a12440dd Mon Sep 17 00:00:00 2001 From: stickies-v Date: Mon, 23 Jun 2025 18:19:55 +0100 Subject: [PATCH 05/11] move-only: make GetInfo a private CTxMemPool member This allows it to be used by templated functions in a future commit. --- src/txmempool.cpp | 4 ---- src/txmempool.h | 5 +++++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e3660d9aea2..f3b617e11d0 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -844,10 +844,6 @@ std::vector CTxMemPool::Get return iters; } -static TxMempoolInfo GetInfo(CTxMemPool::indexed_transaction_set::const_iterator it) { - return TxMempoolInfo{it->GetSharedTx(), it->GetTime(), it->GetFee(), it->GetTxSize(), it->GetModifiedFee() - it->GetFee()}; -} - std::vector CTxMemPool::entryAll() const { AssertLockHeld(cs); diff --git a/src/txmempool.h b/src/txmempool.h index ca1d1e870e7..c6290c0cdc7 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -431,6 +431,11 @@ private: const Limits& limits ) const EXCLUSIVE_LOCKS_REQUIRED(cs); + static TxMempoolInfo GetInfo(CTxMemPool::indexed_transaction_set::const_iterator it) + { + return TxMempoolInfo{it->GetSharedTx(), it->GetTime(), it->GetFee(), it->GetTxSize(), it->GetModifiedFee() - it->GetFee()}; + } + public: indirectmap mapNextTx GUARDED_BY(cs); std::map mapDeltas GUARDED_BY(cs); From c876a892ec0b04851bea0a688d7681b6aaca4cb7 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Mon, 31 Mar 2025 16:28:50 +0100 Subject: [PATCH 06/11] 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"); From bde4579b0780aa3754af35beffbcfeb31f28045b Mon Sep 17 00:00:00 2001 From: marcofleon Date: Mon, 31 Mar 2025 19:30:35 +0100 Subject: [PATCH 07/11] Convert `txdownloadman_impl` to GenTxidVariant Convert all of `txdownloadman_impl` to the new variant except for `GetRequestsToSend`, which will be easier to switch at the same time as `txrequest`. --- src/net_processing.cpp | 6 +-- src/node/txdownloadman.h | 7 +-- src/node/txdownloadman_impl.cpp | 90 +++++++++++++++------------------ src/node/txdownloadman_impl.h | 6 +-- src/test/fuzz/txdownloadman.cpp | 24 ++++----- src/test/txdownload_tests.cpp | 24 ++++----- 6 files changed, 75 insertions(+), 82 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c5ca246ddef..8b0e6ff1a86 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4015,7 +4015,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, AddKnownTx(*peer, inv.hash); if (!m_chainman.IsInitialBlockDownload()) { - const bool fAlreadyHave{m_txdownloadman.AddTxAnnouncement(pfrom.GetId(), gtxid, current_time)}; + const bool fAlreadyHave{m_txdownloadman.AddTxAnnouncement(pfrom.GetId(), gtxid.ToVariant(), current_time)}; LogDebug(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); } } else { @@ -4942,11 +4942,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (msg_type == NetMsgType::NOTFOUND) { std::vector vInv; vRecv >> vInv; - std::vector tx_invs; + std::vector tx_invs; if (vInv.size() <= node::MAX_PEER_TX_ANNOUNCEMENTS + MAX_BLOCKS_IN_TRANSIT_PER_PEER) { for (CInv &inv : vInv) { if (inv.IsGenTxMsg()) { - tx_invs.emplace_back(inv.hash); + tx_invs.emplace_back(ToGenTxid(inv).ToVariant()); } } } diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 1ae833a6be6..328d24c686f 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include @@ -15,7 +16,7 @@ class CBlock; class CRollingBloomFilter; class CTxMemPool; -class GenTxid; +class GenTxidVariant; class TxRequestTracker; namespace node { class TxDownloadManagerImpl; @@ -137,13 +138,13 @@ public: /** Consider adding this tx hash to txrequest. Should be called whenever a new inv has been received. * Also called internally when a transaction is missing parents so that we can request them. * Returns true if this was a dropped inv (p2p_inv=true and we already have the tx), false otherwise. */ - bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now); + bool AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now); /** Get getdata requests to send. */ std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); /** Should be called when a notfound for a tx has been received. */ - void ReceivedNotFound(NodeId nodeid, const std::vector& txhashes); + void ReceivedNotFound(NodeId nodeid, const std::vector& gtxids); /** Respond to successful transaction submission to mempool */ void MempoolAcceptedTx(const CTransactionRef& tx); diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index ec2d772c1cf..5b72fd96cd2 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -39,7 +39,7 @@ void TxDownloadManager::DisconnectedPeer(NodeId nodeid) { m_impl->DisconnectedPeer(nodeid); } -bool TxDownloadManager::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now) +bool TxDownloadManager::AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now) { return m_impl->AddTxAnnouncement(peer, gtxid, now); } @@ -47,9 +47,9 @@ std::vector TxDownloadManager::GetRequestsToSend(NodeId nodeid, std::ch { return m_impl->GetRequestsToSend(nodeid, current_time); } -void TxDownloadManager::ReceivedNotFound(NodeId nodeid, const std::vector& txhashes) +void TxDownloadManager::ReceivedNotFound(NodeId nodeid, const std::vector& gtxids) { - m_impl->ReceivedNotFound(nodeid, txhashes); + m_impl->ReceivedNotFound(nodeid, gtxids); } void TxDownloadManager::MempoolAcceptedTx(const CTransactionRef& tx) { @@ -122,33 +122,28 @@ void TxDownloadManagerImpl::BlockDisconnected() RecentConfirmedTransactionsFilter().reset(); } -bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable) +bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable) { - const uint256& hash = gtxid.GetHash(); + const uint256& hash = gtxid.ToUint256(); - if (gtxid.IsWtxid()) { - // Normal query by wtxid. - if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; - } else { - // Never query by txid: it is possible that the transaction in the orphanage has the same - // txid but a different witness, which would give us a false positive result. If we decided - // not to request the transaction based on this result, an attacker could prevent us from - // downloading a transaction by intentionally creating a malleated version of it. While - // only one (or none!) of these transactions can ultimately be confirmed, we have no way of - // discerning which one that is, so the orphanage can store multiple transactions with the - // same txid. - // - // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid. - // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will - // help us find non-segwit transactions, saving bandwidth, and should have no false positives. - if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; - } + // Never query by txid: it is possible that the transaction in the orphanage has the same + // txid but a different witness, which would give us a false positive result. If we decided + // not to request the transaction based on this result, an attacker could prevent us from + // downloading a transaction by intentionally creating a malleated version of it. While + // only one (or none!) of these transactions can ultimately be confirmed, we have no way of + // discerning which one that is, so the orphanage can store multiple transactions with the + // same txid. + // + // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid. + // A non-segwit transaction's txid == wtxid. Query this txhash "casted" to a wtxid. This will + // help us find non-segwit transactions, saving bandwidth, and should have no false positives. + if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; if (include_reconsiderable && RecentRejectsReconsiderableFilter().contains(hash)) return true; if (RecentConfirmedTransactionsFilter().contains(hash)) return true; - return RecentRejectsFilter().contains(hash) || std::visit([&](const auto& id) { return m_opts.m_mempool.exists(id); }, gtxid.ToVariant()); + return RecentRejectsFilter().contains(hash) || std::visit([&](const auto& id) { return m_opts.m_mempool.exists(id); }, gtxid); } void TxDownloadManagerImpl::ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info) @@ -172,18 +167,17 @@ void TxDownloadManagerImpl::DisconnectedPeer(NodeId nodeid) } -bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now) +bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now) { // If this is an orphan we are trying to resolve, consider this peer as a orphan resolution candidate instead. // - is wtxid matching something in orphanage // - exists in orphanage // - peer can be an orphan resolution candidate - if (gtxid.IsWtxid()) { - const auto wtxid{Wtxid::FromUint256(gtxid.GetHash())}; - if (auto orphan_tx{m_orphanage.GetTx(wtxid)}) { + if (const auto* wtxid = std::get_if(>xid)) { + if (auto orphan_tx{m_orphanage.GetTx(*wtxid)}) { auto unique_parents{GetUniqueParents(*orphan_tx)}; - std::erase_if(unique_parents, [&](const auto& txid){ - return AlreadyHaveTx(GenTxid::Txid(txid), /*include_reconsiderable=*/false); + std::erase_if(unique_parents, [&](const auto& txid) { + return AlreadyHaveTx(txid, /*include_reconsiderable=*/false); }); // The missing parents may have all been rejected or accepted since the orphan was added to the orphanage. @@ -192,7 +186,7 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, return true; } - if (MaybeAddOrphanResolutionCandidate(unique_parents, wtxid, peer, now)) { + if (MaybeAddOrphanResolutionCandidate(unique_parents, *wtxid, peer, now)) { m_orphanage.AddAnnouncer(orphan_tx->GetWitnessHash(), peer); } @@ -224,7 +218,7 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, const bool overloaded = !info.m_relay_permissions && m_txrequest.CountInFlight(peer) >= MAX_PEER_TX_REQUEST_IN_FLIGHT; if (overloaded) delay += OVERLOADED_PEER_TX_DELAY; - m_txrequest.ReceivedInv(peer, gtxid, info.m_preferred, now + delay); + m_txrequest.ReceivedInv(peer, GenTxid::FromVariant(gtxid), info.m_preferred, now + delay); return false; } @@ -277,7 +271,7 @@ std::vector TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std entry.second.GetHash().ToString(), entry.first); } for (const GenTxid& gtxid : requestable) { - if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { + if (!AlreadyHaveTx(gtxid.ToVariant(), /*include_reconsiderable=*/false)) { LogDebug(BCLog::NET, "Requesting %s %s peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx", gtxid.GetHash().ToString(), nodeid); requests.emplace_back(gtxid); @@ -291,12 +285,12 @@ std::vector TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std return requests; } -void TxDownloadManagerImpl::ReceivedNotFound(NodeId nodeid, const std::vector& txhashes) +void TxDownloadManagerImpl::ReceivedNotFound(NodeId nodeid, const std::vector& gtxids) { - for (const auto& txhash : txhashes) { + for (const auto& gtxid : gtxids) { // If we receive a NOTFOUND message for a tx we requested, mark the announcement for it as // completed in TxRequestTracker. - m_txrequest.ReceivedResponse(nodeid, txhash); + m_txrequest.ReceivedResponse(nodeid, gtxid.ToUint256()); } } @@ -377,13 +371,13 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction // Distinguish between parents in m_lazy_recent_rejects and m_lazy_recent_rejects_reconsiderable. // We can tolerate having up to 1 parent in m_lazy_recent_rejects_reconsiderable since we // submit 1p1c packages. However, fail immediately if any are in m_lazy_recent_rejects. - std::optional rejected_parent_reconsiderable; - for (const uint256& parent_txid : unique_parents) { - if (RecentRejectsFilter().contains(parent_txid)) { + std::optional rejected_parent_reconsiderable; + for (const Txid& parent_txid : unique_parents) { + if (RecentRejectsFilter().contains(parent_txid.ToUint256())) { fRejectedParents = true; break; - } else if (RecentRejectsReconsiderableFilter().contains(parent_txid) && - !m_opts.m_mempool.exists(Txid::FromUint256(parent_txid))) { + } else if (RecentRejectsReconsiderableFilter().contains(parent_txid.ToUint256()) && + !m_opts.m_mempool.exists(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()) { @@ -397,8 +391,8 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction // Filter parents that we already have. // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been // previously rejected for being too low feerate. This orphan might CPFP it. - std::erase_if(unique_parents, [&](const auto& txid){ - return AlreadyHaveTx(GenTxid::Txid(txid), /*include_reconsiderable=*/false); + std::erase_if(unique_parents, [&](const auto& txid) { + return AlreadyHaveTx(txid, /*include_reconsiderable=*/false); }); const auto now{GetTime()}; const auto& wtxid = ptx->GetWitnessHash(); @@ -412,8 +406,8 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction // // Search by txid and, if the tx has a witness, wtxid std::vector orphan_resolution_candidates{nodeid}; - m_txrequest.GetCandidatePeers(ptx->GetHash().ToUint256(), orphan_resolution_candidates); - if (ptx->HasWitness()) m_txrequest.GetCandidatePeers(ptx->GetWitnessHash().ToUint256(), orphan_resolution_candidates); + m_txrequest.GetCandidatePeers(ptx->GetHash(), orphan_resolution_candidates); + if (ptx->HasWitness()) m_txrequest.GetCandidatePeers(ptx->GetWitnessHash(), orphan_resolution_candidates); for (const auto& nodeid : orphan_resolution_candidates) { if (MaybeAddOrphanResolutionCandidate(unique_parents, ptx->GetWitnessHash(), nodeid, now)) { @@ -515,8 +509,8 @@ void TxDownloadManagerImpl::MempoolRejectedPackage(const Package& package) std::pair> TxDownloadManagerImpl::ReceivedTx(NodeId nodeid, const CTransactionRef& ptx) { - const uint256& txid = ptx->GetHash(); - const uint256& wtxid = ptx->GetWitnessHash(); + const Txid& txid = ptx->GetHash(); + const Wtxid& wtxid = ptx->GetWitnessHash(); // Mark that we have received a response m_txrequest.ReceivedResponse(nodeid, txid); @@ -535,7 +529,7 @@ std::pair> TxDownloadManagerImpl::Receive // already; and an adversary can already relay us old transactions // (older than our recency filter) if trying to DoS us, without any need // for witness malleation. - if (AlreadyHaveTx(GenTxid::Wtxid(wtxid), /*include_reconsiderable=*/false)) { + if (AlreadyHaveTx(wtxid, /*include_reconsiderable=*/false)) { // If a tx is detected by m_lazy_recent_rejects it is ignored. Because we haven't // submitted the tx to our mempool, we won't have computed a DoS // score for it or determined exactly why we consider it invalid. @@ -552,7 +546,7 @@ std::pair> TxDownloadManagerImpl::Receive // peer simply for relaying a tx that our m_lazy_recent_rejects has caught, // regardless of false positives. return {false, std::nullopt}; - } else if (RecentRejectsReconsiderableFilter().contains(wtxid)) { + } else if (RecentRejectsReconsiderableFilter().contains(wtxid.ToUint256())) { // When a transaction is already in m_lazy_recent_rejects_reconsiderable, we shouldn't submit // it by itself again. However, look for a matching child in the orphanage, as it is // possible that they succeed as a package. diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index f2dee0adaf8..6387edb251f 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -155,7 +155,7 @@ public: * - m_recent_rejects_reconsiderable (if include_reconsiderable = true) * - m_recent_confirmed_transactions * */ - bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable); + bool AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable); void ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info); void DisconnectedPeer(NodeId nodeid); @@ -163,13 +163,13 @@ public: /** Consider adding this tx hash to txrequest. Should be called whenever a new inv has been received. * Also called internally when a transaction is missing parents so that we can request them. */ - bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now); + bool AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now); /** Get getdata requests to send. */ std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); /** Marks a tx as ReceivedResponse in txrequest. */ - void ReceivedNotFound(NodeId nodeid, const std::vector& txhashes); + void ReceivedNotFound(NodeId nodeid, const std::vector& gtxids); /** Look for a child of this transaction in the orphanage to form a 1-parent-1-child package, * skipping any combinations that have already been tried. Return the resulting package along with diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp index 06385e7bddb..30c69f0a461 100644 --- a/src/test/fuzz/txdownloadman.cpp +++ b/src/test/fuzz/txdownloadman.cpp @@ -227,9 +227,9 @@ FUZZ_TARGET(txdownloadman, .init = initialize) Assert(first_time_failure || !todo.m_should_add_extra_compact_tx); }, [&] { - GenTxid gtxid = fuzzed_data_provider.ConsumeBool() ? - GenTxid::Txid(rand_tx->GetHash()) : - GenTxid::Wtxid(rand_tx->GetWitnessHash()); + auto gtxid = fuzzed_data_provider.ConsumeBool() ? + GenTxidVariant{rand_tx->GetHash()} : + GenTxidVariant{rand_tx->GetWitnessHash()}; txdownloadman.AddTxAnnouncement(rand_peer, gtxid, time); }, [&] { @@ -260,8 +260,7 @@ FUZZ_TARGET(txdownloadman, .init = initialize) // returned true. Assert(expect_work); } - } - ); + }); // Jump forwards or backwards auto time_skip = fuzzed_data_provider.PickValueInArray(TIME_SKIPS); if (fuzzed_data_provider.ConsumeBool()) time_skip *= -1; @@ -373,9 +372,9 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize) if (!reject_contains_wtxid) Assert(todo.m_unique_parents.size() <= rand_tx->vin.size()); }, [&] { - GenTxid gtxid = fuzzed_data_provider.ConsumeBool() ? - GenTxid::Txid(rand_tx->GetHash()) : - GenTxid::Wtxid(rand_tx->GetWitnessHash()); + auto gtxid = fuzzed_data_provider.ConsumeBool() ? + GenTxidVariant{rand_tx->GetHash()} : + GenTxidVariant{rand_tx->GetWitnessHash()}; txdownload_impl.AddTxAnnouncement(rand_peer, gtxid, time); }, [&] { @@ -383,7 +382,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize) // TxDownloadManager should not be telling us to request things we already have. // Exclude m_lazy_recent_rejects_reconsiderable because it may request low-feerate parent of orphan. for (const auto& gtxid : getdata_requests) { - Assert(!txdownload_impl.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)); + Assert(!txdownload_impl.AlreadyHaveTx(gtxid.ToVariant(), /*include_reconsiderable=*/false)); } }, [&] { @@ -395,7 +394,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize) // The only combination that doesn't make sense is validate both tx and package. Assert(!(should_validate && maybe_package.has_value())); if (should_validate) { - Assert(!txdownload_impl.AlreadyHaveTx(GenTxid::Wtxid(rand_tx->GetWitnessHash()), /*include_reconsiderable=*/true)); + Assert(!txdownload_impl.AlreadyHaveTx(rand_tx->GetWitnessHash(), /*include_reconsiderable=*/true)); } if (maybe_package.has_value()) { CheckPackageToValidate(*maybe_package, rand_peer); @@ -424,7 +423,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize) // However, if there was a non-null tx in the workset, HaveMoreWork should have // returned true. Assert(expect_work); - Assert(txdownload_impl.AlreadyHaveTx(GenTxid::Wtxid(ptx->GetWitnessHash()), /*include_reconsiderable=*/false)); + Assert(txdownload_impl.AlreadyHaveTx(ptx->GetWitnessHash(), /*include_reconsiderable=*/false)); // Presumably we have validated this tx. Use "missing inputs" to keep it in the // orphanage longer. Later iterations might call MempoolAcceptedTx or // MempoolRejectedTx with a different error. @@ -432,8 +431,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize) state_missing_inputs.Invalid(TxValidationResult::TX_MISSING_INPUTS, ""); txdownload_impl.MempoolRejectedTx(ptx, state_missing_inputs, rand_peer, fuzzed_data_provider.ConsumeBool()); } - } - ); + }); auto time_skip = fuzzed_data_provider.PickValueInArray(TIME_SKIPS); if (fuzzed_data_provider.ConsumeBool()) time_skip *= -1; diff --git a/src/test/txdownload_tests.cpp b/src/test/txdownload_tests.cpp index 463eb210139..f9d50108c24 100644 --- a/src/test/txdownload_tests.cpp +++ b/src/test/txdownload_tests.cpp @@ -126,10 +126,10 @@ BOOST_FIXTURE_TEST_CASE(tx_rejection_types, TestChain100Setup) for (const auto segwit_child : {true, false}) { const auto ptx_parent = CreatePlaceholderTx(segwit_parent); const auto ptx_child = CreatePlaceholderTx(segwit_child); - const auto& parent_txid = ptx_parent->GetHash().ToUint256(); - const auto& parent_wtxid = ptx_parent->GetWitnessHash().ToUint256(); - const auto& child_txid = ptx_child->GetHash().ToUint256(); - const auto& child_wtxid = ptx_child->GetWitnessHash().ToUint256(); + const auto& parent_txid = ptx_parent->GetHash(); + const auto& parent_wtxid = ptx_parent->GetWitnessHash(); + const auto& child_txid = ptx_child->GetHash(); + const auto& child_wtxid = ptx_child->GetWitnessHash(); for (const auto& [result, expected_behavior] : expected_behaviors) { node::TxDownloadManagerImpl txdownload_impl{DEFAULT_OPTS}; @@ -141,13 +141,13 @@ BOOST_FIXTURE_TEST_CASE(tx_rejection_types, TestChain100Setup) // No distinction between txid and wtxid caching for nonsegwit transactions, so only test these specific // behaviors for segwit transactions. Behaviors actual_behavior{ - /*txid_rejects=*/txdownload_impl.RecentRejectsFilter().contains(parent_txid), - /*wtxid_rejects=*/txdownload_impl.RecentRejectsFilter().contains(parent_wtxid), - /*txid_recon=*/txdownload_impl.RecentRejectsReconsiderableFilter().contains(parent_txid), - /*wtxid_recon=*/txdownload_impl.RecentRejectsReconsiderableFilter().contains(parent_wtxid), + /*txid_rejects=*/txdownload_impl.RecentRejectsFilter().contains(parent_txid.ToUint256()), + /*wtxid_rejects=*/txdownload_impl.RecentRejectsFilter().contains(parent_wtxid.ToUint256()), + /*txid_recon=*/txdownload_impl.RecentRejectsReconsiderableFilter().contains(parent_txid.ToUint256()), + /*wtxid_recon=*/txdownload_impl.RecentRejectsReconsiderableFilter().contains(parent_wtxid.ToUint256()), /*keep=*/keep, - /*txid_inv=*/txdownload_impl.AddTxAnnouncement(nodeid, GenTxid::Txid(parent_txid), now), - /*wtxid_inv=*/txdownload_impl.AddTxAnnouncement(nodeid, GenTxid::Wtxid(parent_wtxid), now), + /*txid_inv=*/txdownload_impl.AddTxAnnouncement(nodeid, parent_txid, now), + /*wtxid_inv=*/txdownload_impl.AddTxAnnouncement(nodeid, parent_wtxid, now), }; BOOST_TEST_MESSAGE("Testing behavior for " << result << (segwit_parent ? " segwit " : " nonsegwit")); actual_behavior.CheckEqual(expected_behavior, /*segwit=*/segwit_parent); @@ -158,8 +158,8 @@ BOOST_FIXTURE_TEST_CASE(tx_rejection_types, TestChain100Setup) // If parent (by txid) was rejected, child is too. const bool parent_txid_rejected{segwit_parent ? expected_behavior.m_txid_in_rejects : expected_behavior.m_wtxid_in_rejects}; - BOOST_CHECK_EQUAL(parent_txid_rejected, txdownload_impl.RecentRejectsFilter().contains(child_txid)); - BOOST_CHECK_EQUAL(parent_txid_rejected, txdownload_impl.RecentRejectsFilter().contains(child_wtxid)); + BOOST_CHECK_EQUAL(parent_txid_rejected, txdownload_impl.RecentRejectsFilter().contains(child_txid.ToUint256())); + BOOST_CHECK_EQUAL(parent_txid_rejected, txdownload_impl.RecentRejectsFilter().contains(child_wtxid.ToUint256())); // Unless rejected, the child should be in orphanage. BOOST_CHECK_EQUAL(!parent_txid_rejected, txdownload_impl.m_orphanage.HaveTx(ptx_child->GetWitnessHash())); From 1b528391c79497ae19f7e481439e350533c7cd1a Mon Sep 17 00:00:00 2001 From: marcofleon Date: Mon, 31 Mar 2025 20:46:48 +0100 Subject: [PATCH 08/11] Convert `txrequest` to GenTxidVariant Switch all instances of GenTxid to the new variant in `txrequest` and complete `txdownloadman_impl` by converting `GetRequestsToSend`. --- src/net_processing.cpp | 4 +- src/node/txdownloadman.h | 2 +- src/node/txdownloadman_impl.cpp | 28 ++++++------ src/node/txdownloadman_impl.h | 2 +- src/test/fuzz/txdownloadman.cpp | 2 +- src/test/fuzz/txrequest.cpp | 14 +++--- src/test/txrequest_tests.cpp | 75 +++++++++++++++++---------------- src/txrequest.cpp | 67 +++++++++++++---------------- src/txrequest.h | 17 ++++---- 9 files changed, 103 insertions(+), 108 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8b0e6ff1a86..ea0b747d09b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5962,8 +5962,8 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // { LOCK(m_tx_download_mutex); - for (const GenTxid& gtxid : m_txdownloadman.GetRequestsToSend(pto->GetId(), current_time)) { - vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*peer)), gtxid.GetHash()); + for (const GenTxidVariant& gtxid : m_txdownloadman.GetRequestsToSend(pto->GetId(), current_time)) { + vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*peer)), gtxid.ToUint256()); if (vGetData.size() >= MAX_GETDATA_SZ) { MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); vGetData.clear(); diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 328d24c686f..5c5e61cdca7 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -141,7 +141,7 @@ public: bool AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now); /** Get getdata requests to send. */ - std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); + std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); /** Should be called when a notfound for a tx has been received. */ void ReceivedNotFound(NodeId nodeid, const std::vector& gtxids); diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 5b72fd96cd2..a8fe8971bed 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -43,7 +43,7 @@ bool TxDownloadManager::AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtx { return m_impl->AddTxAnnouncement(peer, gtxid, now); } -std::vector TxDownloadManager::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) +std::vector TxDownloadManager::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) { return m_impl->GetRequestsToSend(nodeid, current_time); } @@ -218,7 +218,7 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxidVariant& const bool overloaded = !info.m_relay_permissions && m_txrequest.CountInFlight(peer) >= MAX_PEER_TX_REQUEST_IN_FLIGHT; if (overloaded) delay += OVERLOADED_PEER_TX_DELAY; - m_txrequest.ReceivedInv(peer, GenTxid::FromVariant(gtxid), info.m_preferred, now + delay); + m_txrequest.ReceivedInv(peer, gtxid, info.m_preferred, now + delay); return false; } @@ -255,31 +255,31 @@ bool TxDownloadManagerImpl::MaybeAddOrphanResolutionCandidate(const std::vector< // Treat finding orphan resolution candidate as equivalent to the peer announcing all missing parents. // In the future, orphan resolution may include more explicit steps for (const auto& parent_txid : unique_parents) { - m_txrequest.ReceivedInv(nodeid, GenTxid::Txid(parent_txid), info.m_preferred, now + delay); + m_txrequest.ReceivedInv(nodeid, parent_txid, info.m_preferred, now + delay); } LogDebug(BCLog::TXPACKAGES, "added peer=%d as a candidate for resolving orphan %s\n", nodeid, wtxid.ToString()); return true; } -std::vector TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) +std::vector TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) { - std::vector requests; - std::vector> expired; + std::vector requests; + std::vector> expired; auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired); - for (const auto& entry : expired) { - LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", entry.second.IsWtxid() ? "wtx" : "tx", - entry.second.GetHash().ToString(), entry.first); + for (const auto& [expired_nodeid, gtxid] : expired) { + LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx", + gtxid.ToUint256().ToString(), expired_nodeid); } - for (const GenTxid& gtxid : requestable) { - if (!AlreadyHaveTx(gtxid.ToVariant(), /*include_reconsiderable=*/false)) { + for (const GenTxidVariant& gtxid : requestable) { + if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { LogDebug(BCLog::NET, "Requesting %s %s peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx", - gtxid.GetHash().ToString(), nodeid); + gtxid.ToUint256().ToString(), nodeid); requests.emplace_back(gtxid); - m_txrequest.RequestedTx(nodeid, gtxid.GetHash(), current_time + GETDATA_TX_INTERVAL); + m_txrequest.RequestedTx(nodeid, gtxid.ToUint256(), current_time + GETDATA_TX_INTERVAL); } else { // We have already seen this transaction, no need to download. This is just a belt-and-suspenders, as // this should already be called whenever a transaction becomes AlreadyHaveTx(). - m_txrequest.ForgetTxHash(gtxid.GetHash()); + m_txrequest.ForgetTxHash(gtxid.ToUint256()); } } return requests; diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index 6387edb251f..e5bbc1e8030 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -166,7 +166,7 @@ public: bool AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now); /** Get getdata requests to send. */ - std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); + std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); /** Marks a tx as ReceivedResponse in txrequest. */ void ReceivedNotFound(NodeId nodeid, const std::vector& gtxids); diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp index 30c69f0a461..af6265678d6 100644 --- a/src/test/fuzz/txdownloadman.cpp +++ b/src/test/fuzz/txdownloadman.cpp @@ -382,7 +382,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize) // TxDownloadManager should not be telling us to request things we already have. // Exclude m_lazy_recent_rejects_reconsiderable because it may request low-feerate parent of orphan. for (const auto& gtxid : getdata_requests) { - Assert(!txdownload_impl.AlreadyHaveTx(gtxid.ToVariant(), /*include_reconsiderable=*/false)); + Assert(!txdownload_impl.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)); } }, [&] { diff --git a/src/test/fuzz/txrequest.cpp b/src/test/fuzz/txrequest.cpp index 931ddf03288..1cab9bc471c 100644 --- a/src/test/fuzz/txrequest.cpp +++ b/src/test/fuzz/txrequest.cpp @@ -19,7 +19,7 @@ namespace { constexpr int MAX_TXHASHES = 16; constexpr int MAX_PEERS = 16; -//! Randomly generated GenTxids used in this test (length is MAX_TXHASHES). +//! Randomly generated txhashes used in this test (length is MAX_TXHASHES). uint256 TXHASHES[MAX_TXHASHES]; //! Precomputed random durations (positive and negative, each ~exponentially distributed). @@ -204,7 +204,8 @@ public: } // Call TxRequestTracker's implementation. - m_tracker.ReceivedInv(peer, is_wtxid ? GenTxid::Wtxid(TXHASHES[txhash]) : GenTxid::Txid(TXHASHES[txhash]), preferred, reqtime); + auto gtxid = is_wtxid ? GenTxidVariant{Wtxid::FromUint256(TXHASHES[txhash])} : GenTxidVariant{Txid::FromUint256(TXHASHES[txhash])}; + m_tracker.ReceivedInv(peer, gtxid, preferred, reqtime); } void RequestedTx(int peer, int txhash, std::chrono::microseconds exptime) @@ -246,13 +247,14 @@ public: //! list of (sequence number, txhash, is_wtxid) tuples. std::vector> result; - std::vector> expected_expired; + std::vector> expected_expired; for (int txhash = 0; txhash < MAX_TXHASHES; ++txhash) { // Mark any expired REQUESTED announcements as COMPLETED. for (int peer2 = 0; peer2 < MAX_PEERS; ++peer2) { Announcement& ann2 = m_announcements[txhash][peer2]; if (ann2.m_state == State::REQUESTED && ann2.m_time <= m_now) { - expected_expired.emplace_back(peer2, ann2.m_is_wtxid ? GenTxid::Wtxid(TXHASHES[txhash]) : GenTxid::Txid(TXHASHES[txhash])); + auto gtxid = ann2.m_is_wtxid ? GenTxidVariant{Wtxid::FromUint256(TXHASHES[txhash])} : GenTxidVariant{Txid::FromUint256(TXHASHES[txhash])}; + expected_expired.emplace_back(peer2, gtxid); ann2.m_state = State::COMPLETED; break; } @@ -270,7 +272,7 @@ public: std::sort(expected_expired.begin(), expected_expired.end()); // Compare with TxRequestTracker's implementation. - std::vector> expired; + std::vector> expired; const auto actual = m_tracker.GetRequestable(peer, m_now, &expired); std::sort(expired.begin(), expired.end()); assert(expired == expected_expired); @@ -278,7 +280,7 @@ public: m_tracker.PostGetRequestableSanityCheck(m_now); assert(result.size() == actual.size()); for (size_t pos = 0; pos < actual.size(); ++pos) { - assert(TXHASHES[std::get<1>(result[pos])] == actual[pos].GetHash()); + assert(TXHASHES[std::get<1>(result[pos])] == actual[pos].ToUint256()); assert(std::get<2>(result[pos]) == actual[pos].IsWtxid()); } } diff --git a/src/test/txrequest_tests.cpp b/src/test/txrequest_tests.cpp index d73692d8f98..0278f824f6c 100644 --- a/src/test/txrequest_tests.cpp +++ b/src/test/txrequest_tests.cpp @@ -61,7 +61,7 @@ struct Runner /** Which (peer, gtxid) combinations are known to be expired. These need to be accumulated here instead of * checked directly in the GetRequestable return value to avoid introducing a dependency between the various * parallel tests. */ - std::multiset> expired; + std::multiset> expired; }; std::chrono::microseconds TxRequestTest::RandomTime8s() { return std::chrono::microseconds{1 + m_rng.randbits(23)}; } @@ -103,17 +103,17 @@ public: void ForgetTxHash(const uint256& txhash) { auto& runner = m_runner; - runner.actions.emplace_back(m_now, [=,&runner]() { + runner.actions.emplace_back(m_now, [=, &runner]() { runner.txrequest.ForgetTxHash(txhash); runner.txrequest.SanityCheck(); }); } /** Schedule a ReceivedInv call at the Scheduler's current time. */ - void ReceivedInv(NodeId peer, const GenTxid& gtxid, bool pref, std::chrono::microseconds reqtime) + void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool pref, std::chrono::microseconds reqtime) { auto& runner = m_runner; - runner.actions.emplace_back(m_now, [=,&runner]() { + runner.actions.emplace_back(m_now, [=, &runner]() { runner.txrequest.ReceivedInv(peer, gtxid, pref, reqtime); runner.txrequest.SanityCheck(); }); @@ -123,7 +123,7 @@ public: void DisconnectedPeer(NodeId peer) { auto& runner = m_runner; - runner.actions.emplace_back(m_now, [=,&runner]() { + runner.actions.emplace_back(m_now, [=, &runner]() { runner.txrequest.DisconnectedPeer(peer); runner.txrequest.SanityCheck(); }); @@ -133,7 +133,7 @@ public: void RequestedTx(NodeId peer, const uint256& txhash, std::chrono::microseconds exptime) { auto& runner = m_runner; - runner.actions.emplace_back(m_now, [=,&runner]() { + runner.actions.emplace_back(m_now, [=, &runner]() { runner.txrequest.RequestedTx(peer, txhash, exptime); runner.txrequest.SanityCheck(); }); @@ -143,7 +143,7 @@ public: void ReceivedResponse(NodeId peer, const uint256& txhash) { auto& runner = m_runner; - runner.actions.emplace_back(m_now, [=,&runner]() { + runner.actions.emplace_back(m_now, [=, &runner]() { runner.txrequest.ReceivedResponse(peer, txhash); runner.txrequest.SanityCheck(); }); @@ -160,18 +160,20 @@ public: * @param offset Offset with the current time to use (must be <= 0). This allows simulations of time going * backwards (but note that the ordering of this event only follows the scenario's m_now. */ - void Check(NodeId peer, const std::vector& expected, size_t candidates, size_t inflight, - size_t completed, const std::string& checkname, - std::chrono::microseconds offset = std::chrono::microseconds{0}) + void Check(NodeId peer, const std::vector& expected, size_t candidates, size_t inflight, + size_t completed, const std::string& checkname, + std::chrono::microseconds offset = std::chrono::microseconds{0}) { const auto comment = m_testname + " " + checkname; auto& runner = m_runner; const auto now = m_now; assert(offset.count() <= 0); - runner.actions.emplace_back(m_now, [=,&runner]() { - std::vector> expired_now; + runner.actions.emplace_back(m_now, [=, &runner]() { + std::vector> expired_now; auto ret = runner.txrequest.GetRequestable(peer, now + offset, &expired_now); - for (const auto& entry : expired_now) runner.expired.insert(entry); + for (const auto& entry : expired_now) { + runner.expired.insert(entry); + } runner.txrequest.SanityCheck(); runner.txrequest.PostGetRequestableSanityCheck(now + offset); size_t total = candidates + inflight + completed; @@ -189,12 +191,12 @@ public: * * Every expected expiration should be accounted for through exactly one call to this function. */ - void CheckExpired(NodeId peer, GenTxid gtxid) + void CheckExpired(NodeId peer, GenTxidVariant gtxid) { const auto& testname = m_testname; auto& runner = m_runner; - runner.actions.emplace_back(m_now, [=,&runner]() { - auto it = runner.expired.find(std::pair{peer, gtxid}); + runner.actions.emplace_back(m_now, [=, &runner]() { + auto it = runner.expired.find(std::pair{peer, gtxid}); BOOST_CHECK_MESSAGE(it != runner.expired.end(), "[" + testname + "] missing expiration"); if (it != runner.expired.end()) runner.expired.erase(it); }); @@ -233,10 +235,11 @@ public: return ret; } - /** Generate a random GenTxid; the txhash follows NewTxHash; the is_wtxid flag is random. */ - GenTxid NewGTxid(const std::vector>& orders = {}) + /** Generate a random GenTxid; the txhash follows NewTxHash; the transaction identifier is random. */ + GenTxidVariant NewGTxid(const std::vector>& orders = {}) { - return m_rng.randbool() ? GenTxid::Wtxid(NewTxHash(orders)) : GenTxid::Txid(NewTxHash(orders)); + const uint256 txhash{NewTxHash(orders)}; + return m_rng.randbool() ? GenTxidVariant{Wtxid::FromUint256(txhash)} : GenTxidVariant{Txid::FromUint256(txhash)}; } /** Generate a new random NodeId to use as peer. The same NodeId is never returned twice @@ -285,7 +288,7 @@ void TxRequestTest::BuildSingleTest(Scenario& scenario, int config) scenario.AdvanceTime(RandomTime8s()); auto expiry = RandomTime8s(); scenario.Check(peer, {gtxid}, 1, 0, 0, "s5"); - scenario.RequestedTx(peer, gtxid.GetHash(), scenario.Now() + expiry); + scenario.RequestedTx(peer, gtxid.ToUint256(), scenario.Now() + expiry); scenario.Check(peer, {}, 0, 1, 0, "s6"); if ((config >> 3) == 1) { // The request will time out @@ -299,7 +302,7 @@ void TxRequestTest::BuildSingleTest(Scenario& scenario, int config) scenario.AdvanceTime(std::chrono::microseconds{m_rng.randrange(expiry.count())}); scenario.Check(peer, {}, 0, 1, 0, "s9"); if ((config >> 3) == 3) { // A response will arrive for the transaction - scenario.ReceivedResponse(peer, gtxid.GetHash()); + scenario.ReceivedResponse(peer, gtxid.ToUint256()); scenario.Check(peer, {}, 0, 0, 0, "s10"); return; } @@ -309,7 +312,7 @@ void TxRequestTest::BuildSingleTest(Scenario& scenario, int config) if (config & 4) { // The peer will go offline scenario.DisconnectedPeer(peer); } else { // The transaction is no longer needed - scenario.ForgetTxHash(gtxid.GetHash()); + scenario.ForgetTxHash(gtxid.ToUint256()); } scenario.Check(peer, {}, 0, 0, 0, "s11"); } @@ -355,7 +358,7 @@ void TxRequestTest::BuildPriorityTest(Scenario& scenario, int config) // We possibly request from the selected peer. if (config & 8) { - scenario.RequestedTx(priopeer, gtxid.GetHash(), MAX_TIME); + scenario.RequestedTx(priopeer, gtxid.ToUint256(), MAX_TIME); scenario.Check(priopeer, {}, 0, 1, 0, "p7"); scenario.Check(otherpeer, {}, 1, 0, 0, "p8"); if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s()); @@ -365,7 +368,7 @@ void TxRequestTest::BuildPriorityTest(Scenario& scenario, int config) if (config & 16) { scenario.DisconnectedPeer(priopeer); } else { - scenario.ReceivedResponse(priopeer, gtxid.GetHash()); + scenario.ReceivedResponse(priopeer, gtxid.ToUint256()); } if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s()); scenario.Check(priopeer, {}, 0, 0, !(config & 16), "p8"); @@ -449,7 +452,7 @@ void TxRequestTest::BuildBigPriorityTest(Scenario& scenario, int peers) scenario.DisconnectedPeer(peer); scenario.Check(peer, {}, 0, 0, 0, "b4"); } else { - scenario.ReceivedResponse(peer, gtxid.GetHash()); + scenario.ReceivedResponse(peer, gtxid.ToUint256()); scenario.Check(peer, {}, 0, 0, request_order.size() > 0, "b5"); } if (request_order.size()) { @@ -510,8 +513,8 @@ void TxRequestTest::BuildWtxidTest(Scenario& scenario, int config) auto peerT = scenario.NewPeer(); auto peerW = scenario.NewPeer(); auto txhash = scenario.NewTxHash(); - auto txid{GenTxid::Txid(txhash)}; - auto wtxid{GenTxid::Wtxid(txhash)}; + auto txid{Txid::FromUint256(txhash)}; + auto wtxid{Wtxid::FromUint256(txhash)}; auto reqtimeT = m_rng.randbool() ? MIN_TIME : scenario.Now() + RandomTime8s(); auto reqtimeW = m_rng.randbool() ? MIN_TIME : scenario.Now() + RandomTime8s(); @@ -542,11 +545,11 @@ void TxRequestTest::BuildWtxidTest(Scenario& scenario, int config) // Let the preferred announcement be requested. It's not going to be delivered. auto expiry = RandomTime8s(); if (config & 2) { - scenario.RequestedTx(peerT, txid.GetHash(), scenario.Now() + expiry); + scenario.RequestedTx(peerT, txid.ToUint256(), scenario.Now() + expiry); scenario.Check(peerT, {}, 0, 1, 0, "w5"); scenario.Check(peerW, {}, 1, 0, 0, "w6"); } else { - scenario.RequestedTx(peerW, wtxid.GetHash(), scenario.Now() + expiry); + scenario.RequestedTx(peerW, wtxid.ToUint256(), scenario.Now() + expiry); scenario.Check(peerT, {}, 1, 0, 0, "w7"); scenario.Check(peerW, {}, 0, 1, 0, "w8"); } @@ -599,7 +602,7 @@ void TxRequestTest::BuildTimeBackwardsTest(Scenario& scenario) // Request from peer1. if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s()); auto expiry = scenario.Now() + RandomTime8s(); - scenario.RequestedTx(peer1, gtxid.GetHash(), expiry); + scenario.RequestedTx(peer1, gtxid.ToUint256(), expiry); scenario.Check(peer1, {}, 0, 1, 0, "r7"); scenario.Check(peer2, {}, 1, 0, 0, "r8"); @@ -638,20 +641,20 @@ void TxRequestTest::BuildWeirdRequestsTest(Scenario& scenario) // We request gtxid2 from *peer1* - no effect. if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s()); - scenario.RequestedTx(peer1, gtxid2.GetHash(), MAX_TIME); + scenario.RequestedTx(peer1, gtxid2.ToUint256(), MAX_TIME); scenario.Check(peer1, {gtxid1}, 1, 0, 0, "q4"); scenario.Check(peer2, {gtxid2}, 1, 0, 0, "q5"); // Now request gtxid1 from peer1 - marks it as REQUESTED. if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s()); auto expiryA = scenario.Now() + RandomTime8s(); - scenario.RequestedTx(peer1, gtxid1.GetHash(), expiryA); + scenario.RequestedTx(peer1, gtxid1.ToUint256(), expiryA); scenario.Check(peer1, {}, 0, 1, 0, "q6"); scenario.Check(peer2, {gtxid2}, 1, 0, 0, "q7"); // Request it a second time - nothing happens, as it's already REQUESTED. auto expiryB = expiryA + RandomTime8s(); - scenario.RequestedTx(peer1, gtxid1.GetHash(), expiryB); + scenario.RequestedTx(peer1, gtxid1.ToUint256(), expiryB); scenario.Check(peer1, {}, 0, 1, 0, "q8"); scenario.Check(peer2, {gtxid2}, 1, 0, 0, "q9"); @@ -668,7 +671,7 @@ void TxRequestTest::BuildWeirdRequestsTest(Scenario& scenario) // Requesting it yet again from peer1 doesn't do anything, as it's already COMPLETED. if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s()); - scenario.RequestedTx(peer1, gtxid1.GetHash(), MAX_TIME); + scenario.RequestedTx(peer1, gtxid1.ToUint256(), MAX_TIME); scenario.Check(peer1, {}, 0, 0, 1, "q14"); scenario.Check(peer2, {gtxid2, gtxid1}, 2, 0, 0, "q15"); @@ -680,13 +683,13 @@ void TxRequestTest::BuildWeirdRequestsTest(Scenario& scenario) // And request it from peer1 (weird as peer2 has the preference). if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s()); - scenario.RequestedTx(peer1, gtxid2.GetHash(), MAX_TIME); + scenario.RequestedTx(peer1, gtxid2.ToUint256(), MAX_TIME); scenario.Check(peer1, {}, 0, 1, 1, "q18"); scenario.Check(peer2, {gtxid1}, 2, 0, 0, "q19"); // If peer2 now (normally) requests gtxid2, the existing request by peer1 becomes COMPLETED. if (m_rng.randbool()) scenario.AdvanceTime(RandomTime8s()); - scenario.RequestedTx(peer2, gtxid2.GetHash(), MAX_TIME); + scenario.RequestedTx(peer2, gtxid2.ToUint256(), MAX_TIME); scenario.Check(peer1, {}, 0, 0, 2, "q20"); scenario.Check(peer2, {gtxid1}, 1, 1, 0, "q21"); diff --git a/src/txrequest.cpp b/src/txrequest.cpp index 03b4cd6b161..02f03435154 100644 --- a/src/txrequest.cpp +++ b/src/txrequest.cpp @@ -60,7 +60,7 @@ using SequenceNumber = uint64_t; /** An announcement. This is the data we track for each txid or wtxid that is announced to us by each peer. */ struct Announcement { /** Txid or wtxid that was announced. */ - const uint256 m_txhash; + const GenTxidVariant m_gtxid; /** For CANDIDATE_{DELAYED,BEST,READY} the reqtime; for REQUESTED the expiry. */ std::chrono::microseconds m_time; /** What peer the request was from. */ @@ -69,9 +69,6 @@ struct Announcement { const SequenceNumber m_sequence : 59; /** Whether the request is preferred. */ const bool m_preferred : 1; - /** Whether this is a wtxid request. */ - const bool m_is_wtxid : 1; - /** What state this announcement is in. */ State m_state : 3 {State::CANDIDATE_DELAYED}; State GetState() const { return m_state; } @@ -96,10 +93,9 @@ struct Announcement { } /** Construct a new announcement from scratch, initially in CANDIDATE_DELAYED state. */ - Announcement(const GenTxid& gtxid, NodeId peer, bool preferred, std::chrono::microseconds reqtime, + Announcement(const GenTxidVariant& gtxid, NodeId peer, bool preferred, std::chrono::microseconds reqtime, SequenceNumber sequence) - : m_txhash(gtxid.GetHash()), m_time(reqtime), m_peer(peer), m_sequence(sequence), m_preferred(preferred), - m_is_wtxid{gtxid.IsWtxid()} {} + : m_gtxid(gtxid), m_time(reqtime), m_peer(peer), m_sequence(sequence), m_preferred(preferred) {} }; //! Type alias for priorities. @@ -124,7 +120,7 @@ public: Priority operator()(const Announcement& ann) const { - return operator()(ann.m_txhash, ann.m_peer, ann.m_preferred); + return operator()(ann.m_gtxid.ToUint256(), ann.m_peer, ann.m_preferred); } }; @@ -148,7 +144,7 @@ struct ByPeerViewExtractor using result_type = ByPeerView; result_type operator()(const Announcement& ann) const { - return ByPeerView{ann.m_peer, ann.GetState() == State::CANDIDATE_BEST, ann.m_txhash}; + return ByPeerView{ann.m_peer, ann.GetState() == State::CANDIDATE_BEST, ann.m_gtxid.ToUint256()}; } }; @@ -172,7 +168,7 @@ public: result_type operator()(const Announcement& ann) const { const Priority prio = (ann.GetState() == State::CANDIDATE_READY) ? m_computer(ann) : 0; - return ByTxHashView{ann.m_txhash, ann.GetState(), prio}; + return ByTxHashView{ann.m_gtxid.ToUint256(), ann.GetState(), prio}; } }; @@ -280,7 +276,7 @@ std::map ComputeTxHashInfo(const Index& index, const Priori { std::map ret; for (const Announcement& ann : index) { - TxHashInfo& info = ret[ann.m_txhash]; + TxHashInfo& info = ret[ann.m_gtxid.ToUint256()]; // Classify how many announcements of each state we have for this txhash. info.m_candidate_delayed += (ann.GetState() == State::CANDIDATE_DELAYED); info.m_candidate_ready += (ann.GetState() == State::CANDIDATE_READY); @@ -299,11 +295,6 @@ std::map ComputeTxHashInfo(const Index& index, const Priori return ret; } -GenTxid ToGenTxid(const Announcement& ann) -{ - return ann.m_is_wtxid ? GenTxid::Wtxid(ann.m_txhash) : GenTxid::Txid(ann.m_txhash); -} - } // namespace /** Actual implementation for TxRequestTracker's data structure. */ @@ -409,7 +400,7 @@ private: // priority) comes last. Thus, if an existing _BEST exists for the same txhash that this announcement may // be preferred over, it must immediately follow the newly created _READY. auto it_next = std::next(it); - if (it_next == m_index.get().end() || it_next->m_txhash != it->m_txhash || + if (it_next == m_index.get().end() || it_next->m_gtxid.ToUint256() != it->m_gtxid.ToUint256() || it_next->GetState() == State::COMPLETED) { // This is the new best CANDIDATE_READY, and there is no IsSelected() announcement for this txhash // already. @@ -435,7 +426,7 @@ private: auto it_prev = std::prev(it); // The next best CANDIDATE_READY, if any, immediately precedes the REQUESTED or CANDIDATE_BEST // announcement in the ByTxHash index. - if (it_prev->m_txhash == it->m_txhash && it_prev->GetState() == State::CANDIDATE_READY) { + if (it_prev->m_gtxid.ToUint256() == it->m_gtxid.ToUint256() && it_prev->GetState() == State::CANDIDATE_READY) { // If one such CANDIDATE_READY exists (for this txhash), convert it to CANDIDATE_BEST. Modify(it_prev, [](Announcement& ann){ ann.SetState(State::CANDIDATE_BEST); }); } @@ -451,10 +442,10 @@ private: // This announcement has a predecessor that belongs to the same txhash. Due to ordering, and the // fact that 'it' is not COMPLETED, its predecessor cannot be COMPLETED here. - if (it != m_index.get().begin() && std::prev(it)->m_txhash == it->m_txhash) return false; + if (it != m_index.get().begin() && std::prev(it)->m_gtxid.ToUint256() == it->m_gtxid.ToUint256()) return false; // This announcement has a successor that belongs to the same txhash, and is not COMPLETED. - if (std::next(it) != m_index.get().end() && std::next(it)->m_txhash == it->m_txhash && + if (std::next(it) != m_index.get().end() && std::next(it)->m_gtxid.ToUint256() == it->m_gtxid.ToUint256() && std::next(it)->GetState() != State::COMPLETED) return false; return true; @@ -472,10 +463,10 @@ private: if (IsOnlyNonCompleted(it)) { // This is the last non-COMPLETED announcement for this txhash. Delete all. - uint256 txhash = it->m_txhash; + uint256 txhash = it->m_gtxid.ToUint256(); do { it = Erase(it); - } while (it != m_index.get().end() && it->m_txhash == txhash); + } while (it != m_index.get().end() && it->m_gtxid.ToUint256() == txhash); return false; } @@ -490,7 +481,7 @@ private: //! - REQUESTED announcements with expiry <= now are turned into COMPLETED. //! - CANDIDATE_DELAYED announcements with reqtime <= now are turned into CANDIDATE_{READY,BEST}. //! - CANDIDATE_{READY,BEST} announcements with reqtime > now are turned into CANDIDATE_DELAYED. - void SetTimePoint(std::chrono::microseconds now, std::vector>* expired) + void SetTimePoint(std::chrono::microseconds now, std::vector>* expired) { if (expired) expired->clear(); @@ -501,7 +492,7 @@ private: if (it->GetState() == State::CANDIDATE_DELAYED && it->m_time <= now) { PromoteCandidateReady(m_index.project(it)); } else if (it->GetState() == State::REQUESTED && it->m_time <= now) { - if (expired) expired->emplace_back(it->m_peer, ToGenTxid(*it)); + if (expired) expired->emplace_back(it->m_peer, it->m_gtxid); MakeCompleted(m_index.project(it)); } else { break; @@ -569,7 +560,7 @@ public: void ForgetTxHash(const uint256& txhash) { auto it = m_index.get().lower_bound(ByTxHashView{txhash, State::CANDIDATE_DELAYED, 0}); - while (it != m_index.get().end() && it->m_txhash == txhash) { + while (it != m_index.get().end() && it->m_gtxid.ToUint256() == txhash) { it = Erase(it); } } @@ -577,19 +568,19 @@ public: void GetCandidatePeers(const uint256& txhash, std::vector& result_peers) const { auto it = m_index.get().lower_bound(ByTxHashView{txhash, State::CANDIDATE_DELAYED, 0}); - while (it != m_index.get().end() && it->m_txhash == txhash && it->GetState() != State::COMPLETED) { + while (it != m_index.get().end() && it->m_gtxid.ToUint256() == txhash && it->GetState() != State::COMPLETED) { result_peers.push_back(it->m_peer); ++it; } } - void ReceivedInv(NodeId peer, const GenTxid& gtxid, bool preferred, - std::chrono::microseconds reqtime) + void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred, + std::chrono::microseconds reqtime) { // Bail out if we already have a CANDIDATE_BEST announcement for this (txhash, peer) combination. The case // where there is a non-CANDIDATE_BEST announcement already will be caught by the uniqueness property of the // ByPeer index when we try to emplace the new object below. - if (m_index.get().count(ByPeerView{peer, true, gtxid.GetHash()})) return; + if (m_index.get().count(ByPeerView{peer, true, gtxid.ToUint256()})) return; // Try creating the announcement with CANDIDATE_DELAYED state (which will fail due to the uniqueness // of the ByPeer index if a non-CANDIDATE_BEST announcement already exists with the same txhash and peer). @@ -603,8 +594,8 @@ public: } //! Find the GenTxids to request now from peer. - std::vector GetRequestable(NodeId peer, std::chrono::microseconds now, - std::vector>* expired) + std::vector GetRequestable(NodeId peer, std::chrono::microseconds now, + std::vector>* expired) { // Move time. SetTimePoint(now, expired); @@ -624,10 +615,10 @@ public: }); // Convert to GenTxid and return. - std::vector ret; + std::vector ret; ret.reserve(selected.size()); std::transform(selected.begin(), selected.end(), std::back_inserter(ret), [](const Announcement* ann) { - return ToGenTxid(*ann); + return ann->m_gtxid; }); return ret; } @@ -654,7 +645,7 @@ public: // found announcement had a different state than CANDIDATE_BEST. If it did, invariants guarantee that no // other CANDIDATE_BEST or REQUESTED can exist. auto it_old = m_index.get().lower_bound(ByTxHashView{txhash, State::CANDIDATE_BEST, 0}); - if (it_old != m_index.get().end() && it_old->m_txhash == txhash) { + if (it_old != m_index.get().end() && it_old->m_gtxid.ToUint256() == txhash) { if (it_old->GetState() == State::CANDIDATE_BEST) { // The data structure's invariants require that there can be at most one CANDIDATE_BEST or one // REQUESTED announcement per txhash (but not both simultaneously), so we have to convert any @@ -738,8 +729,8 @@ void TxRequestTracker::PostGetRequestableSanityCheck(std::chrono::microseconds n m_impl->PostGetRequestableSanityCheck(now); } -void TxRequestTracker::ReceivedInv(NodeId peer, const GenTxid& gtxid, bool preferred, - std::chrono::microseconds reqtime) +void TxRequestTracker::ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred, + std::chrono::microseconds reqtime) { m_impl->ReceivedInv(peer, gtxid, preferred, reqtime); } @@ -754,8 +745,8 @@ void TxRequestTracker::ReceivedResponse(NodeId peer, const uint256& txhash) m_impl->ReceivedResponse(peer, txhash); } -std::vector TxRequestTracker::GetRequestable(NodeId peer, std::chrono::microseconds now, - std::vector>* expired) +std::vector TxRequestTracker::GetRequestable(NodeId peer, std::chrono::microseconds now, + std::vector>* expired) { return m_impl->GetRequestable(peer, now, expired); } diff --git a/src/txrequest.h b/src/txrequest.h index 98ad43b79aa..f38f4e486ec 100644 --- a/src/txrequest.h +++ b/src/txrequest.h @@ -132,8 +132,8 @@ public: * fetched. The new announcement is given the specified preferred and reqtime values, and takes its is_wtxid * from the specified gtxid. */ - void ReceivedInv(NodeId peer, const GenTxid& gtxid, bool preferred, - std::chrono::microseconds reqtime); + void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred, + std::chrono::microseconds reqtime); /** Deletes all announcements for a given peer. * @@ -158,14 +158,13 @@ public: * exists, and for which the specified peer is the best choice among all (reqtime <= now) CANDIDATE * announcements with the same txhash (subject to preferredness rules, and tiebreaking using a deterministic * salted hash of peer and txhash). - * - The selected announcements are converted to GenTxids using their is_wtxid flag, and returned in - * announcement order (even if multiple were added at the same time, or when the clock went backwards while - * they were being added). This is done to minimize disruption from dependent transactions being requested - * out of order: if multiple dependent transactions are announced simultaneously by one peer, and end up - * being requested from them, the requests will happen in announcement order. + * - The selected announcements are returned in announcement order (even if multiple were added at the same + * time, or when the clock went backwards while they were being added). This is done to minimize disruption + * from dependent transactions being requested out of order: if multiple dependent transactions are announced + * simultaneously by one peer, and end up being requested from them, the requests will happen in announcement order. */ - std::vector GetRequestable(NodeId peer, std::chrono::microseconds now, - std::vector>* expired = nullptr); + std::vector GetRequestable(NodeId peer, std::chrono::microseconds now, + std::vector>* expired = nullptr); /** Marks a transaction as requested, with a specified expiry. * From 072a198ea4bc9f1e8449cd31e55d397b75ce4ad5 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Tue, 1 Apr 2025 11:47:49 +0100 Subject: [PATCH 09/11] Convert remaining instances of GenTxid to GenTxidVariant --- src/net_processing.cpp | 16 ++++++++-------- src/protocol.cpp | 4 ++-- src/protocol.h | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ea0b747d09b..cf367aac67d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -856,7 +856,7 @@ private: std::shared_ptr m_most_recent_block GUARDED_BY(m_most_recent_block_mutex); std::shared_ptr m_most_recent_compact_block GUARDED_BY(m_most_recent_block_mutex); uint256 m_most_recent_block_hash GUARDED_BY(m_most_recent_block_mutex); - std::unique_ptr> m_most_recent_block_txs GUARDED_BY(m_most_recent_block_mutex); + std::unique_ptr> m_most_recent_block_txs GUARDED_BY(m_most_recent_block_mutex); // Data about the low-work headers synchronization, aggregated from all peers' HeadersSyncStates. /** Mutex guarding the other m_headers_presync_* variables. */ @@ -2027,7 +2027,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha std::async(std::launch::deferred, [&] { return NetMsg::Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })}; { - auto most_recent_block_txs = std::make_unique>(); + auto most_recent_block_txs = std::make_unique>(); for (const auto& tx : pblock->vtx) { most_recent_block_txs->emplace(tx->GetHash(), tx); most_recent_block_txs->emplace(tx->GetWitnessHash(), tx); @@ -2393,7 +2393,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const CInv& inv) { - auto gtxid{ToGenTxid(inv).ToVariant()}; + auto gtxid{ToGenTxid(inv)}; // If a tx was in the mempool prior to the last INV for this peer, permit the request. auto txinfo{std::visit( [&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) { @@ -2408,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.ToUint256()); + auto it = m_most_recent_block_txs->find(gtxid); if (it != m_most_recent_block_txs->end()) return it->second; } } @@ -4011,11 +4011,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, pfrom.fDisconnect = true; return; } - const GenTxid gtxid = ToGenTxid(inv); + const GenTxidVariant gtxid = ToGenTxid(inv); AddKnownTx(*peer, inv.hash); if (!m_chainman.IsInitialBlockDownload()) { - const bool fAlreadyHave{m_txdownloadman.AddTxAnnouncement(pfrom.GetId(), gtxid.ToVariant(), current_time)}; + const bool fAlreadyHave{m_txdownloadman.AddTxAnnouncement(pfrom.GetId(), gtxid, current_time)}; LogDebug(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); } } else { @@ -4946,7 +4946,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (vInv.size() <= node::MAX_PEER_TX_ANNOUNCEMENTS + MAX_BLOCKS_IN_TRANSIT_PER_PEER) { for (CInv &inv : vInv) { if (inv.IsGenTxMsg()) { - tx_invs.emplace_back(ToGenTxid(inv).ToVariant()); + tx_invs.emplace_back(ToGenTxid(inv)); } } } @@ -5772,7 +5772,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) txinfo.tx->GetWitnessHash().ToUint256() : txinfo.tx->GetHash().ToUint256(), }; - tx_relay->m_tx_inventory_to_send.erase(ToGenTxid(inv).ToVariant()); + tx_relay->m_tx_inventory_to_send.erase(ToGenTxid(inv)); // Don't send transactions that peers will not put into their mempool if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) { diff --git a/src/protocol.cpp b/src/protocol.cpp index bdf0a669863..a8ad50ab728 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -118,8 +118,8 @@ std::vector serviceFlagsToStr(uint64_t flags) return str_flags; } -GenTxid ToGenTxid(const CInv& inv) +GenTxidVariant ToGenTxid(const CInv& inv) { assert(inv.IsGenTxMsg()); - return inv.IsMsgWtx() ? GenTxid::Wtxid(inv.hash) : GenTxid::Txid(inv.hash); + return inv.IsMsgWtx() ? GenTxidVariant{Wtxid::FromUint256(inv.hash)} : GenTxidVariant{Txid::FromUint256(inv.hash)}; } diff --git a/src/protocol.h b/src/protocol.h index 804597b86d8..35b2722afb9 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -526,6 +526,6 @@ public: }; /** Convert a TX/WITNESS_TX/WTX CInv to a GenTxid. */ -GenTxid ToGenTxid(const CInv& inv); +GenTxidVariant ToGenTxid(const CInv& inv); #endif // BITCOIN_PROTOCOL_H From c8ba1995986323cd9e76097acc1f15eed7c60943 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Tue, 1 Apr 2025 11:59:19 +0100 Subject: [PATCH 10/11] Remove old GenTxid class --- src/primitives/transaction.h | 30 ------------------------------ 1 file changed, 30 deletions(-) diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index adadb2a3e74..78ddebcbd48 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -423,34 +423,4 @@ struct CMutableTransaction typedef std::shared_ptr CTransactionRef; template static inline CTransactionRef MakeTransactionRef(Tx&& txIn) { return std::make_shared(std::forward(txIn)); } -/** A generic txid reference (txid or wtxid). */ -class GenTxid -{ - bool m_is_wtxid; - uint256 m_hash; - GenTxid(bool is_wtxid, const uint256& hash) : m_is_wtxid(is_wtxid), m_hash(hash) {} - -public: - static GenTxid Txid(const uint256& hash) { return GenTxid{false, hash}; } - static GenTxid Wtxid(const uint256& hash) { return GenTxid{true, hash}; } - bool IsWtxid() const { return m_is_wtxid; } - const uint256& GetHash() const LIFETIMEBOUND { return m_hash; } - friend bool operator==(const GenTxid& a, const GenTxid& b) { return a.m_is_wtxid == b.m_is_wtxid && a.m_hash == b.m_hash; } - friend bool operator<(const GenTxid& a, const GenTxid& b) { return std::tie(a.m_is_wtxid, a.m_hash) < std::tie(b.m_is_wtxid, b.m_hash); } - - GenTxidVariant ToVariant() const - { - return m_is_wtxid ? - GenTxidVariant{Wtxid::FromUint256(m_hash)} : - GenTxidVariant{Txid::FromUint256(m_hash)}; - } - - static GenTxid FromVariant(const GenTxidVariant& variant) - { - return GenTxid{ - std::holds_alternative<::Wtxid>(variant), - variant.ToUint256()}; - } -}; - #endif // BITCOIN_PRIMITIVES_TRANSACTION_H From a60f863d3e276534444571282f432b913d3967db Mon Sep 17 00:00:00 2001 From: marcofleon Date: Tue, 1 Jul 2025 22:06:01 +0100 Subject: [PATCH 11/11] scripted-diff: Replace GenTxidVariant with GenTxid -BEGIN VERIFY SCRIPT- sed -i 's/GenTxidVariant/GenTxid/g' $(git grep -l 'GenTxidVariant') -END VERIFY SCRIPT- --- src/net_processing.cpp | 24 ++++++++++++------------ src/node/txdownloadman.h | 8 ++++---- src/node/txdownloadman_impl.cpp | 20 ++++++++++---------- src/node/txdownloadman_impl.h | 8 ++++---- src/protocol.cpp | 4 ++-- src/protocol.h | 2 +- src/test/fuzz/txdownloadman.cpp | 8 ++++---- src/test/fuzz/txrequest.cpp | 8 ++++---- src/test/txrequest_tests.cpp | 16 ++++++++-------- src/txmempool.cpp | 2 +- src/txmempool.h | 2 +- src/txrequest.cpp | 20 ++++++++++---------- src/txrequest.h | 6 +++--- src/util/transaction_identifier.h | 4 ++-- 14 files changed, 66 insertions(+), 66 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index cf367aac67d..7d3bb5f8ab3 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -302,7 +302,7 @@ struct Peer { * non-wtxid-relay peers, wtxid for wtxid-relay peers). We use the * mempool to sort transactions in dependency order before relay, so * this does not have to be sorted. */ - std::set m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex); + std::set m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex); /** Whether the peer has requested us to send our complete mempool. Only * permitted if the peer has NetPermissionFlags::Mempool or we advertise * NODE_BLOOM. See BIP35. */ @@ -856,7 +856,7 @@ private: std::shared_ptr m_most_recent_block GUARDED_BY(m_most_recent_block_mutex); std::shared_ptr m_most_recent_compact_block GUARDED_BY(m_most_recent_block_mutex); uint256 m_most_recent_block_hash GUARDED_BY(m_most_recent_block_mutex); - std::unique_ptr> m_most_recent_block_txs GUARDED_BY(m_most_recent_block_mutex); + std::unique_ptr> m_most_recent_block_txs GUARDED_BY(m_most_recent_block_mutex); // Data about the low-work headers synchronization, aggregated from all peers' HeadersSyncStates. /** Mutex guarding the other m_headers_presync_* variables. */ @@ -2027,7 +2027,7 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha std::async(std::launch::deferred, [&] { return NetMsg::Make(NetMsgType::CMPCTBLOCK, *pcmpctblock); })}; { - auto most_recent_block_txs = std::make_unique>(); + auto most_recent_block_txs = std::make_unique>(); for (const auto& tx : pblock->vtx) { most_recent_block_txs->emplace(tx->GetHash(), tx); most_recent_block_txs->emplace(tx->GetWitnessHash(), tx); @@ -2166,7 +2166,7 @@ void PeerManagerImpl::RelayTransaction(const Txid& txid, const Wtxid& wtxid) // in the announcement. if (tx_relay->m_next_inv_send_time == 0s) continue; - const auto gtxid{peer.m_wtxid_relay ? GenTxidVariant{wtxid} : GenTxidVariant{txid}}; + const auto gtxid{peer.m_wtxid_relay ? GenTxid{wtxid} : GenTxid{txid}}; if (!tx_relay->m_tx_inventory_known_filter.contains(gtxid.ToUint256())) { tx_relay->m_tx_inventory_to_send.insert(gtxid); } @@ -4011,7 +4011,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, pfrom.fDisconnect = true; return; } - const GenTxidVariant gtxid = ToGenTxid(inv); + const GenTxid gtxid = ToGenTxid(inv); AddKnownTx(*peer, inv.hash); if (!m_chainman.IsInitialBlockDownload()) { @@ -4942,7 +4942,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, if (msg_type == NetMsgType::NOTFOUND) { std::vector vInv; vRecv >> vInv; - std::vector tx_invs; + std::vector tx_invs; if (vInv.size() <= node::MAX_PEER_TX_ANNOUNCEMENTS + MAX_BLOCKS_IN_TRANSIT_PER_PEER) { for (CInv &inv : vInv) { if (inv.IsGenTxMsg()) { @@ -5450,7 +5450,7 @@ class CompareInvMempoolOrder public: explicit CompareInvMempoolOrder(CTxMemPool* mempool) : m_mempool{mempool} {} - bool operator()(std::set::iterator a, std::set::iterator b) + bool operator()(std::set::iterator a, std::set::iterator b) { /* As std::make_heap produces a max-heap, we want the entries with the * fewest ancestors/highest fee to sort later. */ @@ -5793,9 +5793,9 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // Determine transactions to relay if (fSendTrickle) { // Produce a vector with all candidates for sending - std::vector::iterator> vInvTx; + std::vector::iterator> vInvTx; vInvTx.reserve(tx_relay->m_tx_inventory_to_send.size()); - for (std::set::iterator it = tx_relay->m_tx_inventory_to_send.begin(); it != tx_relay->m_tx_inventory_to_send.end(); it++) { + for (std::set::iterator it = tx_relay->m_tx_inventory_to_send.begin(); it != tx_relay->m_tx_inventory_to_send.end(); it++) { vInvTx.push_back(it); } const CFeeRate filterrate{tx_relay->m_fee_filter_received.load()}; @@ -5812,9 +5812,9 @@ bool PeerManagerImpl::SendMessages(CNode* pto) while (!vInvTx.empty() && nRelayedTransactions < broadcast_max) { // Fetch the top element from the heap std::pop_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder); - std::set::iterator it = vInvTx.back(); + std::set::iterator it = vInvTx.back(); vInvTx.pop_back(); - GenTxidVariant hash = *it; + GenTxid hash = *it; Assume(peer->m_wtxid_relay == hash.IsWtxid()); CInv inv(peer->m_wtxid_relay ? MSG_WTX : MSG_TX, hash.ToUint256()); // Remove it from the to-be-sent set @@ -5962,7 +5962,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // { LOCK(m_tx_download_mutex); - for (const GenTxidVariant& gtxid : m_txdownloadman.GetRequestsToSend(pto->GetId(), current_time)) { + for (const GenTxid& gtxid : m_txdownloadman.GetRequestsToSend(pto->GetId(), current_time)) { vGetData.emplace_back(gtxid.IsWtxid() ? MSG_WTX : (MSG_TX | GetFetchFlags(*peer)), gtxid.ToUint256()); if (vGetData.size() >= MAX_GETDATA_SZ) { MakeAndPushMessage(*pto, NetMsgType::GETDATA, vGetData); diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 5c5e61cdca7..9eb246740b9 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -16,7 +16,7 @@ class CBlock; class CRollingBloomFilter; class CTxMemPool; -class GenTxidVariant; +class GenTxid; class TxRequestTracker; namespace node { class TxDownloadManagerImpl; @@ -138,13 +138,13 @@ public: /** Consider adding this tx hash to txrequest. Should be called whenever a new inv has been received. * Also called internally when a transaction is missing parents so that we can request them. * Returns true if this was a dropped inv (p2p_inv=true and we already have the tx), false otherwise. */ - bool AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now); + bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now); /** Get getdata requests to send. */ - std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); + std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); /** Should be called when a notfound for a tx has been received. */ - void ReceivedNotFound(NodeId nodeid, const std::vector& gtxids); + void ReceivedNotFound(NodeId nodeid, const std::vector& gtxids); /** Respond to successful transaction submission to mempool */ void MempoolAcceptedTx(const CTransactionRef& tx); diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index a8fe8971bed..5bb435f5321 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -39,15 +39,15 @@ void TxDownloadManager::DisconnectedPeer(NodeId nodeid) { m_impl->DisconnectedPeer(nodeid); } -bool TxDownloadManager::AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now) +bool TxDownloadManager::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now) { return m_impl->AddTxAnnouncement(peer, gtxid, now); } -std::vector TxDownloadManager::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) +std::vector TxDownloadManager::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) { return m_impl->GetRequestsToSend(nodeid, current_time); } -void TxDownloadManager::ReceivedNotFound(NodeId nodeid, const std::vector& gtxids) +void TxDownloadManager::ReceivedNotFound(NodeId nodeid, const std::vector& gtxids) { m_impl->ReceivedNotFound(nodeid, gtxids); } @@ -122,7 +122,7 @@ void TxDownloadManagerImpl::BlockDisconnected() RecentConfirmedTransactionsFilter().reset(); } -bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable) +bool TxDownloadManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable) { const uint256& hash = gtxid.ToUint256(); @@ -167,7 +167,7 @@ void TxDownloadManagerImpl::DisconnectedPeer(NodeId nodeid) } -bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now) +bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now) { // If this is an orphan we are trying to resolve, consider this peer as a orphan resolution candidate instead. // - is wtxid matching something in orphanage @@ -261,16 +261,16 @@ bool TxDownloadManagerImpl::MaybeAddOrphanResolutionCandidate(const std::vector< return true; } -std::vector TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) +std::vector TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) { - std::vector requests; - std::vector> expired; + std::vector requests; + std::vector> expired; auto requestable = m_txrequest.GetRequestable(nodeid, current_time, &expired); for (const auto& [expired_nodeid, gtxid] : expired) { LogDebug(BCLog::NET, "timeout of inflight %s %s from peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx", gtxid.ToUint256().ToString(), expired_nodeid); } - for (const GenTxidVariant& gtxid : requestable) { + for (const GenTxid& gtxid : requestable) { if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { LogDebug(BCLog::NET, "Requesting %s %s peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx", gtxid.ToUint256().ToString(), nodeid); @@ -285,7 +285,7 @@ std::vector TxDownloadManagerImpl::GetRequestsToSend(NodeId node return requests; } -void TxDownloadManagerImpl::ReceivedNotFound(NodeId nodeid, const std::vector& gtxids) +void TxDownloadManagerImpl::ReceivedNotFound(NodeId nodeid, const std::vector& gtxids) { for (const auto& gtxid : gtxids) { // If we receive a NOTFOUND message for a tx we requested, mark the announcement for it as diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index e5bbc1e8030..3e0213352ca 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -155,7 +155,7 @@ public: * - m_recent_rejects_reconsiderable (if include_reconsiderable = true) * - m_recent_confirmed_transactions * */ - bool AlreadyHaveTx(const GenTxidVariant& gtxid, bool include_reconsiderable); + bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable); void ConnectedPeer(NodeId nodeid, const TxDownloadConnectionInfo& info); void DisconnectedPeer(NodeId nodeid); @@ -163,13 +163,13 @@ public: /** Consider adding this tx hash to txrequest. Should be called whenever a new inv has been received. * Also called internally when a transaction is missing parents so that we can request them. */ - bool AddTxAnnouncement(NodeId peer, const GenTxidVariant& gtxid, std::chrono::microseconds now); + bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now); /** Get getdata requests to send. */ - std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); + std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); /** Marks a tx as ReceivedResponse in txrequest. */ - void ReceivedNotFound(NodeId nodeid, const std::vector& gtxids); + void ReceivedNotFound(NodeId nodeid, const std::vector& gtxids); /** Look for a child of this transaction in the orphanage to form a 1-parent-1-child package, * skipping any combinations that have already been tried. Return the resulting package along with diff --git a/src/protocol.cpp b/src/protocol.cpp index a8ad50ab728..5217c45daaf 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -118,8 +118,8 @@ std::vector serviceFlagsToStr(uint64_t flags) return str_flags; } -GenTxidVariant ToGenTxid(const CInv& inv) +GenTxid ToGenTxid(const CInv& inv) { assert(inv.IsGenTxMsg()); - return inv.IsMsgWtx() ? GenTxidVariant{Wtxid::FromUint256(inv.hash)} : GenTxidVariant{Txid::FromUint256(inv.hash)}; + return inv.IsMsgWtx() ? GenTxid{Wtxid::FromUint256(inv.hash)} : GenTxid{Txid::FromUint256(inv.hash)}; } diff --git a/src/protocol.h b/src/protocol.h index 35b2722afb9..804597b86d8 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -526,6 +526,6 @@ public: }; /** Convert a TX/WITNESS_TX/WTX CInv to a GenTxid. */ -GenTxidVariant ToGenTxid(const CInv& inv); +GenTxid ToGenTxid(const CInv& inv); #endif // BITCOIN_PROTOCOL_H diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp index af6265678d6..b65ba0eb383 100644 --- a/src/test/fuzz/txdownloadman.cpp +++ b/src/test/fuzz/txdownloadman.cpp @@ -228,8 +228,8 @@ FUZZ_TARGET(txdownloadman, .init = initialize) }, [&] { auto gtxid = fuzzed_data_provider.ConsumeBool() ? - GenTxidVariant{rand_tx->GetHash()} : - GenTxidVariant{rand_tx->GetWitnessHash()}; + GenTxid{rand_tx->GetHash()} : + GenTxid{rand_tx->GetWitnessHash()}; txdownloadman.AddTxAnnouncement(rand_peer, gtxid, time); }, [&] { @@ -373,8 +373,8 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize) }, [&] { auto gtxid = fuzzed_data_provider.ConsumeBool() ? - GenTxidVariant{rand_tx->GetHash()} : - GenTxidVariant{rand_tx->GetWitnessHash()}; + GenTxid{rand_tx->GetHash()} : + GenTxid{rand_tx->GetWitnessHash()}; txdownload_impl.AddTxAnnouncement(rand_peer, gtxid, time); }, [&] { diff --git a/src/test/fuzz/txrequest.cpp b/src/test/fuzz/txrequest.cpp index 1cab9bc471c..c1585d7c00e 100644 --- a/src/test/fuzz/txrequest.cpp +++ b/src/test/fuzz/txrequest.cpp @@ -204,7 +204,7 @@ public: } // Call TxRequestTracker's implementation. - auto gtxid = is_wtxid ? GenTxidVariant{Wtxid::FromUint256(TXHASHES[txhash])} : GenTxidVariant{Txid::FromUint256(TXHASHES[txhash])}; + auto gtxid = is_wtxid ? GenTxid{Wtxid::FromUint256(TXHASHES[txhash])} : GenTxid{Txid::FromUint256(TXHASHES[txhash])}; m_tracker.ReceivedInv(peer, gtxid, preferred, reqtime); } @@ -247,13 +247,13 @@ public: //! list of (sequence number, txhash, is_wtxid) tuples. std::vector> result; - std::vector> expected_expired; + std::vector> expected_expired; for (int txhash = 0; txhash < MAX_TXHASHES; ++txhash) { // Mark any expired REQUESTED announcements as COMPLETED. for (int peer2 = 0; peer2 < MAX_PEERS; ++peer2) { Announcement& ann2 = m_announcements[txhash][peer2]; if (ann2.m_state == State::REQUESTED && ann2.m_time <= m_now) { - auto gtxid = ann2.m_is_wtxid ? GenTxidVariant{Wtxid::FromUint256(TXHASHES[txhash])} : GenTxidVariant{Txid::FromUint256(TXHASHES[txhash])}; + auto gtxid = ann2.m_is_wtxid ? GenTxid{Wtxid::FromUint256(TXHASHES[txhash])} : GenTxid{Txid::FromUint256(TXHASHES[txhash])}; expected_expired.emplace_back(peer2, gtxid); ann2.m_state = State::COMPLETED; break; @@ -272,7 +272,7 @@ public: std::sort(expected_expired.begin(), expected_expired.end()); // Compare with TxRequestTracker's implementation. - std::vector> expired; + std::vector> expired; const auto actual = m_tracker.GetRequestable(peer, m_now, &expired); std::sort(expired.begin(), expired.end()); assert(expired == expected_expired); diff --git a/src/test/txrequest_tests.cpp b/src/test/txrequest_tests.cpp index 0278f824f6c..ba82a986a1b 100644 --- a/src/test/txrequest_tests.cpp +++ b/src/test/txrequest_tests.cpp @@ -61,7 +61,7 @@ struct Runner /** Which (peer, gtxid) combinations are known to be expired. These need to be accumulated here instead of * checked directly in the GetRequestable return value to avoid introducing a dependency between the various * parallel tests. */ - std::multiset> expired; + std::multiset> expired; }; std::chrono::microseconds TxRequestTest::RandomTime8s() { return std::chrono::microseconds{1 + m_rng.randbits(23)}; } @@ -110,7 +110,7 @@ public: } /** Schedule a ReceivedInv call at the Scheduler's current time. */ - void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool pref, std::chrono::microseconds reqtime) + void ReceivedInv(NodeId peer, const GenTxid& gtxid, bool pref, std::chrono::microseconds reqtime) { auto& runner = m_runner; runner.actions.emplace_back(m_now, [=, &runner]() { @@ -160,7 +160,7 @@ public: * @param offset Offset with the current time to use (must be <= 0). This allows simulations of time going * backwards (but note that the ordering of this event only follows the scenario's m_now. */ - void Check(NodeId peer, const std::vector& expected, size_t candidates, size_t inflight, + void Check(NodeId peer, const std::vector& expected, size_t candidates, size_t inflight, size_t completed, const std::string& checkname, std::chrono::microseconds offset = std::chrono::microseconds{0}) { @@ -169,7 +169,7 @@ public: const auto now = m_now; assert(offset.count() <= 0); runner.actions.emplace_back(m_now, [=, &runner]() { - std::vector> expired_now; + std::vector> expired_now; auto ret = runner.txrequest.GetRequestable(peer, now + offset, &expired_now); for (const auto& entry : expired_now) { runner.expired.insert(entry); @@ -191,12 +191,12 @@ public: * * Every expected expiration should be accounted for through exactly one call to this function. */ - void CheckExpired(NodeId peer, GenTxidVariant gtxid) + void CheckExpired(NodeId peer, GenTxid gtxid) { const auto& testname = m_testname; auto& runner = m_runner; runner.actions.emplace_back(m_now, [=, &runner]() { - auto it = runner.expired.find(std::pair{peer, gtxid}); + auto it = runner.expired.find(std::pair{peer, gtxid}); BOOST_CHECK_MESSAGE(it != runner.expired.end(), "[" + testname + "] missing expiration"); if (it != runner.expired.end()) runner.expired.erase(it); }); @@ -236,10 +236,10 @@ public: } /** Generate a random GenTxid; the txhash follows NewTxHash; the transaction identifier is random. */ - GenTxidVariant NewGTxid(const std::vector>& orders = {}) + GenTxid NewGTxid(const std::vector>& orders = {}) { const uint256 txhash{NewTxHash(orders)}; - return m_rng.randbool() ? GenTxidVariant{Wtxid::FromUint256(txhash)} : GenTxidVariant{Txid::FromUint256(txhash)}; + return m_rng.randbool() ? GenTxid{Wtxid::FromUint256(txhash)} : GenTxid{Txid::FromUint256(txhash)}; } /** Generate a new random NodeId to use as peer. The same NodeId is never returned twice diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 3c22909ad6b..9d78a6e716c 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -794,7 +794,7 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei assert(innerUsage == cachedInnerUsage); } -bool CTxMemPool::CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb) const +bool CTxMemPool::CompareDepthAndScore(const GenTxid& hasha, const GenTxid& hashb) const { /* Return `true` if hasha should be considered sooner than hashb. Namely when: * a is not in the mempool, but b is diff --git a/src/txmempool.h b/src/txmempool.h index bc5af815baf..40be8025c49 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -472,7 +472,7 @@ public: void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); - bool CompareDepthAndScore(const GenTxidVariant& hasha, const GenTxidVariant& hashb) const; + bool CompareDepthAndScore(const GenTxid& hasha, const GenTxid& hashb) const; bool isSpent(const COutPoint& outpoint) const; unsigned int GetTransactionsUpdated() const; void AddTransactionsUpdated(unsigned int n); diff --git a/src/txrequest.cpp b/src/txrequest.cpp index 02f03435154..37496614129 100644 --- a/src/txrequest.cpp +++ b/src/txrequest.cpp @@ -60,7 +60,7 @@ using SequenceNumber = uint64_t; /** An announcement. This is the data we track for each txid or wtxid that is announced to us by each peer. */ struct Announcement { /** Txid or wtxid that was announced. */ - const GenTxidVariant m_gtxid; + const GenTxid m_gtxid; /** For CANDIDATE_{DELAYED,BEST,READY} the reqtime; for REQUESTED the expiry. */ std::chrono::microseconds m_time; /** What peer the request was from. */ @@ -93,7 +93,7 @@ struct Announcement { } /** Construct a new announcement from scratch, initially in CANDIDATE_DELAYED state. */ - Announcement(const GenTxidVariant& gtxid, NodeId peer, bool preferred, std::chrono::microseconds reqtime, + Announcement(const GenTxid& gtxid, NodeId peer, bool preferred, std::chrono::microseconds reqtime, SequenceNumber sequence) : m_gtxid(gtxid), m_time(reqtime), m_peer(peer), m_sequence(sequence), m_preferred(preferred) {} }; @@ -481,7 +481,7 @@ private: //! - REQUESTED announcements with expiry <= now are turned into COMPLETED. //! - CANDIDATE_DELAYED announcements with reqtime <= now are turned into CANDIDATE_{READY,BEST}. //! - CANDIDATE_{READY,BEST} announcements with reqtime > now are turned into CANDIDATE_DELAYED. - void SetTimePoint(std::chrono::microseconds now, std::vector>* expired) + void SetTimePoint(std::chrono::microseconds now, std::vector>* expired) { if (expired) expired->clear(); @@ -574,7 +574,7 @@ public: } } - void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred, + void ReceivedInv(NodeId peer, const GenTxid& gtxid, bool preferred, std::chrono::microseconds reqtime) { // Bail out if we already have a CANDIDATE_BEST announcement for this (txhash, peer) combination. The case @@ -594,8 +594,8 @@ public: } //! Find the GenTxids to request now from peer. - std::vector GetRequestable(NodeId peer, std::chrono::microseconds now, - std::vector>* expired) + std::vector GetRequestable(NodeId peer, std::chrono::microseconds now, + std::vector>* expired) { // Move time. SetTimePoint(now, expired); @@ -615,7 +615,7 @@ public: }); // Convert to GenTxid and return. - std::vector ret; + std::vector ret; ret.reserve(selected.size()); std::transform(selected.begin(), selected.end(), std::back_inserter(ret), [](const Announcement* ann) { return ann->m_gtxid; @@ -729,7 +729,7 @@ void TxRequestTracker::PostGetRequestableSanityCheck(std::chrono::microseconds n m_impl->PostGetRequestableSanityCheck(now); } -void TxRequestTracker::ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred, +void TxRequestTracker::ReceivedInv(NodeId peer, const GenTxid& gtxid, bool preferred, std::chrono::microseconds reqtime) { m_impl->ReceivedInv(peer, gtxid, preferred, reqtime); @@ -745,8 +745,8 @@ void TxRequestTracker::ReceivedResponse(NodeId peer, const uint256& txhash) m_impl->ReceivedResponse(peer, txhash); } -std::vector TxRequestTracker::GetRequestable(NodeId peer, std::chrono::microseconds now, - std::vector>* expired) +std::vector TxRequestTracker::GetRequestable(NodeId peer, std::chrono::microseconds now, + std::vector>* expired) { return m_impl->GetRequestable(peer, now, expired); } diff --git a/src/txrequest.h b/src/txrequest.h index f38f4e486ec..084c5ffffea 100644 --- a/src/txrequest.h +++ b/src/txrequest.h @@ -132,7 +132,7 @@ public: * fetched. The new announcement is given the specified preferred and reqtime values, and takes its is_wtxid * from the specified gtxid. */ - void ReceivedInv(NodeId peer, const GenTxidVariant& gtxid, bool preferred, + void ReceivedInv(NodeId peer, const GenTxid& gtxid, bool preferred, std::chrono::microseconds reqtime); /** Deletes all announcements for a given peer. @@ -163,8 +163,8 @@ public: * from dependent transactions being requested out of order: if multiple dependent transactions are announced * simultaneously by one peer, and end up being requested from them, the requests will happen in announcement order. */ - std::vector GetRequestable(NodeId peer, std::chrono::microseconds now, - std::vector>* expired = nullptr); + std::vector GetRequestable(NodeId peer, std::chrono::microseconds now, + std::vector>* expired = nullptr); /** Marks a transaction as requested, with a specified expiry. * diff --git a/src/util/transaction_identifier.h b/src/util/transaction_identifier.h index 95b755f4a67..02e8ec077db 100644 --- a/src/util/transaction_identifier.h +++ b/src/util/transaction_identifier.h @@ -84,7 +84,7 @@ using Wtxid = transaction_identifier; template concept TxidOrWtxid = std::is_same_v || std::is_same_v; -class GenTxidVariant : public std::variant +class GenTxid : public std::variant { public: using variant::variant; @@ -96,7 +96,7 @@ public: return std::visit([](const auto& id) -> const uint256& { return id.ToUint256(); }, *this); } - friend auto operator<=>(const GenTxidVariant& a, const GenTxidVariant& b) + friend auto operator<=>(const GenTxid& a, const GenTxid& b) { return std::tuple(a.IsWtxid(), a.ToUint256()) <=> std::tuple(b.IsWtxid(), b.ToUint256()); }