refactor: GenTxid type safety followups #33005

marcofleon commented at 6:50 PM on July 17, 2025: contributor

This is a followup to #32631.

Addresses: #32631 (review) #32631 (review) #32631 (review) #32631 (review) #32631 (comment)

DrahtBot added the label Refactoring on Jul 17, 2025

DrahtBot commented at 6:50 PM on July 17, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33005.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, stickies-v, maflcko
Approach ACK	ajtowns
Stale ACK	hodlinator, janb84

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33029 (net: make m_mempool optional in PeerManager by naiyoma)
#30277 ([DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation) by sr-gi)
#30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
#28676 (Cluster mempool implementation by sdaftuar)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

marcofleon commented at 7:01 PM on July 17, 2025: contributor

@JeremyRubin in your comment you mentioned that there are multiple data structures that should be switched to homogeneous. I was only able to spot m_tx_inventory_to_send as one that should be changed. Are there others that I'm missing?

If it is only the one, then I'm not sure about making such a generic container. Might be better to just keep it specific to Txid/Wtxid or have a class for m_tx_inventory_to_send only.

marcofleon force-pushed on Jul 17, 2025

DrahtBot added the label CI failed on Jul 17, 2025

DrahtBot commented at 7:38 PM on July 17, 2025: contributor

🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/runs/46205797248</sub> <sub>LLM reason (✨ experimental): The CI failure is caused by a missing include guard in src/util/variantset.h, as reported by the lint check.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

JeremyRubin commented at 9:58 PM on July 17, 2025: contributor

i'll check if there are others but this looks cleaner

in src/net_processing.cpp:306 in 67837bb426 outdated

 302 | @@ -302,7 +303,7 @@ struct Peer {
 303 |           *  non-wtxid-relay peers, wtxid for wtxid-relay peers). We use the
 304 |           *  mempool to sort transactions in dependency order before relay, so
 305 |           *  this does not have to be sorted. */
 306 | -        std::set<GenTxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
 307 | +        VariantSet<Txid, Wtxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);

glozow commented at 1:27 PM on July 18, 2025:

67837bb426049918004fe48525db71b515940e6f seems like an unnecessarily complex approach - looks like we are introducing VariantSet solely for this purpose? We can just keep a set of Wtxids and, if the peer does not support wtxidrelay, grab the txid from info.tx (which we have to look up anyway) to construct the inv message. It's also going to be Wtxids pretty much 100% of the time.

marcofleon commented at 8:49 PM on July 18, 2025:

Done. Thanks for the suggestion, this is quite a bit simpler. Let me know if there's something that can be improved/fixed.

marcofleon force-pushed on Jul 18, 2025

DrahtBot removed the label CI failed on Jul 18, 2025

marcofleon force-pushed on Jul 18, 2025

fanquake added this to the milestone 30.0 on Jul 19, 2025

in src/net_processing.cpp:5764 in e991cac062 outdated

5762 |                              peer->m_wtxid_relay ? MSG_WTX : MSG_TX,
5763 | -                            peer->m_wtxid_relay ?
5764 | -                                txinfo.tx->GetWitnessHash().ToUint256() :
5765 | -                                txinfo.tx->GetHash().ToUint256(),
5766 | +                            peer->m_wtxid_relay ? wtxid.ToUint256() : txid.ToUint256(),
5767 |                          };

hodlinator commented at 6:30 PM on July 19, 2025:

nit: Could be made more consistent with: https://github.com/bitcoin/bitcoin/blob/e991cac062ca644f3c4ac5c5144eb0daf445fd09/src/net_processing.cpp#L5815-L5817

<details><summary>(Personally I prefer 1 branch and curly-braces).</summary>

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index ebb3db39a5..f38dc4b90d 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5758,10 +5758,9 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
                     for (const auto& txinfo : vtxinfo) {
                         const auto& txid{txinfo.tx->GetHash()};
                         const auto& wtxid{txinfo.tx->GetWitnessHash()};
-                        CInv inv{
-                            peer->m_wtxid_relay ? MSG_WTX : MSG_TX,
-                            peer->m_wtxid_relay ? wtxid.ToUint256() : txid.ToUint256(),
-                        };
+                        CInv inv = peer->m_wtxid_relay ?
+                                       CInv{MSG_WTX, wtxid.ToUint256()} :
+                                       CInv{MSG_TX, txid.ToUint256()};
                         tx_relay->m_tx_inventory_to_send.erase(wtxid);
 
                         // Don't send transactions that peers will not put into their mempool
@@ -5813,8 +5812,8 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
                             continue;
                         }
                         const auto inv = peer->m_wtxid_relay ?
-                                             CInv(MSG_WTX, wtxid.ToUint256()) :
-                                             CInv(MSG_TX, txinfo.tx->GetHash());
+                                             CInv{MSG_WTX, wtxid.ToUint256()} :
+                                             CInv{MSG_TX, txinfo.tx->GetHash()};
                         // Check if not in the filter already
                         if (tx_relay->m_tx_inventory_known_filter.contains(inv.hash)) {
                             continue;

</details>

marcofleon commented at 2:18 PM on July 23, 2025:

Fixed.

hodlinator approved

hodlinator commented at 7:08 PM on July 19, 2025: contributor

crACK e991cac062ca644f3c4ac5c5144eb0daf445fd09

Thanks for doing the follow-up! The major change is that Peer::TxRelay::m_tx_inventory_to_send now uses Wtxid instead of GenTxid while the Peer::TxRelay::m_tx_inventory_known_filter bloom filter still uses either Wtxid or Txid consistently depending on the type of peer.

Only one style-nit in case you re-touch.

in src/util/transaction_identifier.h:102 in e662bca72a outdated

  97 | @@ -98,7 +98,7 @@ class GenTxid : public std::variant<Txid, Wtxid>
  98 |  
  99 |      friend auto operator<=>(const GenTxid& a, const GenTxid& b)
 100 |      {
 101 | -        return std::tuple(a.IsWtxid(), a.ToUint256()) <=> std::tuple(b.IsWtxid(), b.ToUint256());
 102 | +        return std::tuple<bool, const uint256&>(a.IsWtxid(), a.ToUint256()) <=> std::tuple<bool, const uint256&>(b.IsWtxid(), b.ToUint256());

ajtowns commented at 10:26 AM on July 21, 2025:

Maybe add a comment to call attention to the const&and the reason for it?

marcofleon commented at 2:18 PM on July 23, 2025:

Added.

in src/net_processing.cpp:2398 in bc56473f43 outdated

2401 | -    auto txinfo{std::visit(
2402 | -        [&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) {
2403 | -            return m_mempool.info_for_relay(id, tx_relay.m_last_inv_sequence);
2404 | -        },
2405 | -        gtxid)};
2406 | +    auto txinfo{m_mempool.info_for_relay(id, tx_relay.m_last_inv_sequence)};

ajtowns commented at 10:40 AM on July 21, 2025:

Passing in the GenTxid and keeping the visit inside the function (rather than an if outside it along with templatizing) seems nicer?

marcofleon commented at 1:46 PM on July 21, 2025:

Fair, it does feel cleaner using GenTxid and avoiding the branch in ProcessGetData. I chose to templatize in response to this comment from the original PR. The templating takes care of everything at compile-time, which is nice. But m_most_recent_block_txs holds GenTxids, so in the templated version we're creating a temporary GenTxid anyway for that map lookup vs creating a GenTxid once upfront.

I think it's largely a stylistic choice and don't have a strong preference either way. @stickies-v @dergoegge if you have anything to add, feel free.

edit: Of course, if there's something I'm missing as to why one approach is clearly better, let me know

maflcko commented at 9:56 AM on July 22, 2025:

less relevant here, but below you are removing the assert on the CInv&. Seems fine to at least restore this as an Assume?

ajtowns commented at 4:56 AM on July 23, 2025:

You might consider:


diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 10fbae5e7ed..42102973c31 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -947,7 +947,7 @@ private:
     std::atomic<std::chrono::seconds> 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 CInv& inv)
+    CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const GenTxid& gtxid)
         EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, NetEventsInterface::g_msgproc_mutex);

     void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc)
@@ -2391,15 +2391,10 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
     }
 }

-CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const CInv& inv)
+CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const GenTxid& gtxid)
 {
-    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) {
-            return m_mempool.info_for_relay(id, tx_relay.m_last_inv_sequence);
-        },
-        gtxid)};
+    auto txinfo = m_mempool.info_for_relay(gtxid, tx_relay.m_last_inv_sequence);
     if (txinfo.tx) {
         return std::move(txinfo.tx);
     }
@@ -2442,7 +2437,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
             continue;
         }

-        if (auto tx{FindTxForGetData(*tx_relay, inv)}) {
+        if (auto tx{FindTxForGetData(*tx_relay, ToGenTxid(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));
diff --git a/src/txmempool.h b/src/txmempool.h
index 687c474f8e0..31e168bc277 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -652,12 +652,13 @@ public:
     }

     /** Returns info for a transaction if its entry_sequence < last_sequence */
-    template <TxidOrWtxid T>
-    TxMempoolInfo info_for_relay(const T& id, uint64_t last_sequence) const
+    TxMempoolInfo info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const
     {
-        LOCK(cs);
-        auto i{GetIter(id)};
-        return (i.has_value() && i.value()->GetSequence() < last_sequence) ? GetInfo(*i) : TxMempoolInfo{};
+        return std::visit([&](const auto& id) {
+            LOCK(cs);
+            auto i{GetIter(id)};
+            return (i.has_value() && i.value()->GetSequence() < last_sequence) ? GetInfo(*i) : TxMempoolInfo{};
+        }, gtxid);
     }

     std::vector<CTxMemPoolEntryRef> entryAll() const EXCLUSIVE_LOCKS_REQUIRED(cs);

ajtowns commented at 5:03 AM on July 23, 2025:

Or adding GetIter(GenTxid) could work too:

     std::optional<txiter> GetIter(const GenTxid& gtxid) const EXCLUSIVE_LOCKS_REQUIRED(cs)
     {
         return std::visit([&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(cs) {
             return GetIter(id);
         }, gtxid);
     }

     /** Returns info for a transaction if its entry_sequence < last_sequence */
     TxMempoolInfo info_for_relay(const GenTxid& gtxid, uint64_t last_sequence) const
     {
         LOCK(cs);
         auto i{GetIter(gtxid)};
         return (i.has_value() && i.value()->GetSequence() < last_sequence) ? GetInfo(*i) : TxMempoolInfo{};
     }

marcofleon commented at 2:17 PM on July 23, 2025:

I opted for passing in a GenTxid to FindTxForGetData and doing the visit there over the templated version for a couple of reasons:

It feels slightly cleaner to me.
We need a GenTxid anyway for m_most_recent_block_txs.
It avoids repeating the branching logic that is already present in ToGenTxid.

I’m keeping the functions in txmempool Txid/Wtxid, as after this PR there won’t be any GenTxids at all in the current mempool code, which is nice.

below you are removing the assert on the CInv&. Seems fine to at least restore this as an Assume?

This approach takes care of this as well.

ajtowns commented at 3:02 PM on July 28, 2025:

Sounds fine.

Another option would be to move info_for_relay from txmempool.h into net_processing.cpp:

/** Returns info for a transaction if its entry_sequence < last_sequence */
static TxMempoolInfo info_for_relay(const CTxMemPool& mempool, const GenTxid& gtxid, uint64_t last_sequence)
{
    return std::visit([&](const auto& id) {
        LOCK(mempool.cs);
        auto i{mempool.GetIter(id)};
        return (i.has_value() && i.value()->GetSequence() < last_sequence) ? mempool.GetInfo(*i) : TxMempoolInfo{};
    }, gtxid);
}

// FindTxForGetData:
    auto txinfo = info_for_relay(m_mempool, gtxid, tx_relay.m_last_inv_sequence);

That removes p2p-specific code from the mempool (and kernel), keeps the mempool with specific Txid/Wtxid things, and simplifies the locking logic a little. Requires exposing CTxMemPool::GetInfo() as public, though that doesn't seem terrible afaics.

stickies-v commented at 5:23 PM on July 28, 2025:

Sorry, need to read up on this thread properly in the next few days, but quickly wanted to add: moving info_for_relay to net_processing.cpp absolutely makes sense to me, that logic should not be in mempool interface. However, leaking mempool internals into p2p (through mempool.GetIter(id)) I really don't like.

ajtowns commented at 4:03 AM on July 29, 2025:

GetIter is already exposed, it's used in validation.cpp for looking up txs. This would also expose GetInfo which converts an iterator into a index-neutral summary of info about the tx.

stickies-v commented at 5:18 PM on July 29, 2025:

tl;dr - I think the current implementation a9819b0e9d3c74970a94cb674fe8fd771e60f6df (i.e. ajtown's suggestion in #33005 (review)) is pragmatically the best way forward.

GetIter is already exposed

I think "still" is more appropriate than "already", see e.g. work done in #28391. Non-mempool code using mapTx iterators imo remains an anti-pattern and we should strive to reduce, not increase it.

That removes p2p-specific code from the mempool (and kernel)

Looking at the code in more detail, I actually think the only thing p2p-specific about the function is the name. E.g. the function still makes sense if we turn it into a TxMempoolInfo info(const T& id, uint64_t last_sequence) const overload. Besides the sequence number, it doesn't use any p2p symbols or logic, so I think keeping as-is makes sense for now.

The true p2p-specific code in the mempool appears to be the existence of sequence numbers, but getting rid of that seems like a much bigger code change than is warranted for this follow-up PR.

marcofleon commented at 10:54 AM on July 30, 2025:

I'll leave as is. Maybe another PR could find a cleaner way to separate relay logic from mempool if we wanted.

ajtowns commented at 2:21 AM on July 31, 2025:

The true p2p-specific code in the mempool appears to be the existence of sequence numbers, but getting rid of that seems like a much bigger code change than is warranted for this follow-up PR.

I have a bunch of thoughts about separating "kernel" and "relay" mempool logic like this... hmm, maybe should just do a brain dump to a gist sometime.

Non-mempool code using mapTx iterators imo remains an anti-pattern

I think if they were (treated as) opaque handles and you had to use GetInfo() or similar accessors to get information out from them, that would be fine fwiw.

stickies-v commented at 1:13 PM on July 31, 2025:

hmm, maybe should just do a brain dump to a gist sometime.

Please cc me if you do, would be very interested to see that.

I think if they were (treated as) opaque handles and you had to use GetInfo() ... that would be fine fwiw.

I agree. Although not opaque, I think that's pretty much what the GetEntry() methods and CTxMemPoolEntry should support doing? But most of the mempool interface is still expecting iterators rather than entries, so it would require a bit of adding/changing methods. Might be a sensible project after cluster mempool?

ajtowns commented at 10:49 AM on July 21, 2025: contributor

Approach ACK

refactor: miscellaneous GenTxid followups

Addresses a few comments from #32631:
https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2199951996
https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201874252
https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2201918072

d588575ed1

refactor: Change `FindTxForGetData` to take GenTxid instead of CInv

Addresses
https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2200291621
from #32631

a9819b0e9d

marcofleon force-pushed on Jul 23, 2025

in src/net_processing.cpp:305 in 8e19a4375e outdated

 301 | @@ -302,7 +302,7 @@ struct Peer {
 302 |           *  non-wtxid-relay peers, wtxid for wtxid-relay peers). We use the
 303 |           *  mempool to sort transactions in dependency order before relay, so
 304 |           *  this does not have to be sorted. */
 305 | -        std::set<GenTxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);
 306 | +        std::set<Wtxid> m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex);

glozow commented at 7:57 PM on July 24, 2025:

Comment above this declaration needs to be changed

marcofleon commented at 5:25 PM on July 28, 2025:

Good looks, fixed.

in src/net_processing.cpp:2170 in 8e19a4375e outdated

2165 | @@ -2166,9 +2166,9 @@ void PeerManagerImpl::RelayTransaction(const Txid& txid, const Wtxid& wtxid)
2166 |          // in the announcement.
2167 |          if (tx_relay->m_next_inv_send_time == 0s) continue;
2168 |  
2169 | -        const auto gtxid{peer.m_wtxid_relay ? GenTxid{wtxid} : GenTxid{txid}};
2170 | -        if (!tx_relay->m_tx_inventory_known_filter.contains(gtxid.ToUint256())) {
2171 | -            tx_relay->m_tx_inventory_to_send.insert(gtxid);
2172 | +        const uint256& hash{peer.m_wtxid_relay ? wtxid.ToUint256() : txid.ToUint256()};
2173 | +        if (!tx_relay->m_tx_inventory_known_filter.contains(hash)) {

glozow commented at 8:01 PM on July 24, 2025:

Hm, is there a reason why m_tx_inventory_known_filter should contain sometimes-txids instead of wtxids?

marcofleon commented at 1:03 PM on July 28, 2025:

I'm seeing a couple of potential reasons. First, when we receive an announcement from a non-wtxid-relay peer, we put that inv's hash (a txid) into the known inventory. And then second, when we receive an orphan we add the parent txids that we don't already have into that peer's known inventory.

glozow commented at 1:26 PM on July 28, 2025:

You're right. Might think about this some more but would be out of scope here anyway. Resolving!

janb84 commented at 12:05 PM on July 25, 2025: contributor

concept ACK 8e19a4375e1db6e59cdcb38e0cc3e1dab79e88ec

This PR is a followup of #32631 which addresses some of the NITS / comments that were placed in that PR. Additionally the PR simplifies m_tx_inventory_to_send a bit to make it std::set<Wtxid> based in stead of std::set<GenTxid> imho a good change.

code reviewed ✅
build and tested ✅

DrahtBot requested review from ajtowns on Jul 25, 2025

DrahtBot requested review from hodlinator on Jul 25, 2025

in src/net_processing.cpp:5818 in 8e19a4375e outdated

5819 |                          if (!txinfo.tx) {
5820 |                              continue;
5821 |                          }
5822 | +                        const auto inv = peer->m_wtxid_relay ?
5823 | +                                             CInv{MSG_WTX, wtxid.ToUint256()} :
5824 | +                                             CInv{MSG_TX, txinfo.tx->GetHash().ToUint256()};

glozow commented at 1:25 PM on July 28, 2025:

Might be worth a comment because non-obvious: m_tx_inventory_known_filter hashes are based on the wtxidrelay-ness of the peer, so it's important that we compare the hash from inv and not the wtxid that was in m_tx_inventory_to_send.

marcofleon commented at 5:25 PM on July 28, 2025:

Done.

refactor: Change `m_tx_inventory_to_send` from `std::set<GenTxid>` to `std::set<Wtxid>`

Simplifies `m_tx_inventory_to_send` a bit by making it a set of Wtxids.
Wtxid relay is prevalent throughout the network, so the complexity of
dealing with a GenTxid in this data structure isn't necessary.

For peers that aren't wtxid relay, the txid is now retrieved from our
mempool entry when the inv is constructed.

94b39ce738

marcofleon force-pushed on Jul 28, 2025

marcofleon commented at 5:24 PM on July 28, 2025: contributor

Most recent push updates the m_tx_inventory_to_send comment and adds a clarifying comment as to why we create the inv before checking m_tx_inventory_known_filter.

I'm still considering #33005 (review).

glozow commented at 7:15 PM on July 28, 2025: member

ACK 94b39ce73831acc4c94c7f0d1347d5991b27ef0b

DrahtBot requested review from janb84 on Jul 28, 2025

in src/net_processing.cpp:5762 in 94b39ce738

5764 | -                        tx_relay->m_tx_inventory_to_send.erase(ToGenTxid(inv));
5765 | +                        const Txid& txid{txinfo.tx->GetHash()};
5766 | +                        const Wtxid& wtxid{txinfo.tx->GetWitnessHash()};
5767 | +                        const auto inv = peer->m_wtxid_relay ?
5768 | +                                             CInv{MSG_WTX, wtxid.ToUint256()} :
5769 | +                                             CInv{MSG_TX, txid.ToUint256()};

stickies-v commented at 1:16 PM on July 29, 2025:

nit: txid declaration not really helpful imo

                        const Wtxid& wtxid{txinfo.tx->GetWitnessHash()};
                        const auto inv = peer->m_wtxid_relay ?
                                             CInv{MSG_WTX, wtxid.ToUint256()} :
                                             CInv{MSG_TX, txinfo.tx->GetHash().ToUint256()};

in src/net_processing.cpp:5784 in 94b39ce738

5780 | @@ -5783,9 +5781,9 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
5781 |                  // Determine transactions to relay
5782 |                  if (fSendTrickle) {
5783 |                      // Produce a vector with all candidates for sending
5784 | -                    std::vector<std::set<GenTxid>::iterator> vInvTx;
5785 | +                    std::vector<std::set<Wtxid>::iterator> vInvTx;

stickies-v commented at 2:23 PM on July 29, 2025:

nit: this is touching a lot more code than you are, but modernizing this bit of code would make it more readable:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 165f6e822f..a9dafb7160 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5780,28 +5780,23 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
 
                 // Determine transactions to relay
                 if (fSendTrickle) {
-                    // Produce a vector with all candidates for sending
-                    std::vector<std::set<Wtxid>::iterator> vInvTx;
-                    vInvTx.reserve(tx_relay->m_tx_inventory_to_send.size());
-                    for (std::set<Wtxid>::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);
-                    std::make_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder);
+                    auto tx_inv_iters{std::views::iota(tx_relay->m_tx_inventory_to_send.begin(),
+                                                       tx_relay->m_tx_inventory_to_send.end())};
+                    // A priority queue is used so that not all items need sorting if only a few are being sent.
+                    std::priority_queue tx_inv_to_send{tx_inv_iters.begin(), tx_inv_iters.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.
                     unsigned int nRelayedTransactions = 0;
                     LOCK(tx_relay->m_bloom_filter_mutex);
                     size_t broadcast_max{INVENTORY_BROADCAST_TARGET + (tx_relay->m_tx_inventory_to_send.size()/1000)*5};
                     broadcast_max = std::min<size_t>(INVENTORY_BROADCAST_MAX, broadcast_max);
-                    while (!vInvTx.empty() && nRelayedTransactions < broadcast_max) {
-                        // Fetch the top element from the heap
-                        std::pop_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder);
-                        std::set<Wtxid>::iterator it = vInvTx.back();
-                        vInvTx.pop_back();
+                    while (!tx_inv_to_send.empty() && nRelayedTransactions < broadcast_max) {
+                        std::set<Wtxid>::iterator it = tx_inv_to_send.top();
+                        tx_inv_to_send.pop();
                         auto wtxid = *it;
                         // Remove it from the to-be-sent set
                         tx_relay->m_tx_inventory_to_send.erase(it);

</details>

in src/net_processing.cpp:2401 in 94b39ce738

2399 |      auto txinfo{std::visit(
2400 |          [&](const auto& id) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) {
2401 |              return m_mempool.info_for_relay(id, tx_relay.m_last_inv_sequence);
2402 |          },
2403 |          gtxid)};
2404 | +

stickies-v commented at 2:57 PM on July 29, 2025:

ghost nit

stickies-v approved

stickies-v commented at 5:27 PM on July 29, 2025: contributor

ACK 94b39ce73831acc4c94c7f0d1347d5991b27ef0b

maflcko commented at 5:26 PM on July 30, 2025: member

review ACK 94b39ce73831acc4c94c7f0d1347d5991b27ef0b 🎲

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 94b39ce73831acc4c94c7f0d1347d5991b27ef0b 🎲
oeMvE0HV8g7Aop9tgh9W10FYJLCk8QCUAj7Iy4uReCklfeTSpvizOzp9hdytYtSC1Y+hsNfZUHITsxIooV24Dw==

</details>

in src/net_processing.cpp:2397 in 94b39ce738

2390 | @@ -2391,15 +2391,14 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
2391 |      }
2392 |  }
2393 |  
2394 | -CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const CInv& inv)
2395 | +CTransactionRef PeerManagerImpl::FindTxForGetData(const Peer::TxRelay& tx_relay, const GenTxid& gtxid)
2396 |  {
2397 | -    auto gtxid{ToGenTxid(inv)};
2398 | -    // If a tx was in the mempool prior to the last INV for this peer, permit the request.

maflcko commented at 5:29 PM on July 30, 2025:

nit in the second commit: Why remove this comment, but not the "Or ..." continuation below. Better to keep both or none?

marcofleon commented at 8:11 AM on July 31, 2025:

Second commit was a bit sloppy it seems😔 I'll clean up this function in the next PR

glozow merged this on Jul 30, 2025

glozow closed this on Jul 30, 2025