From eee473d9f3019a0ea4ebbc9c41781813ad574a86 Mon Sep 17 00:00:00 2001 From: marcofleon Date: Mon, 31 Mar 2025 17:42:20 +0100 Subject: [PATCH] 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);