net_processing: rename RelayTransaction to better describe what it does

vasild commented at 2:30 pm on October 7, 2025: contributor

Rename PeerManager::RelayTransaction() to PeerManager::ScheduleTxForBroadcastToAll(). The transaction is not relayed when the method returns. It is only scheduled for broadcasting at a later time. Also, there will be another method which only schedules for broadcast to Tor or I2P peers.

This is part of #29415 Broadcast own transactions only via short-lived Tor or I2P connections. Putting it in its own PR to reduce the size of #29415 and because it does not depend on the other commits from there.

Rename `PeerManager::RelayTransaction()` to
`PeerManager::ScheduleTxForBroadcastToAll()`. The transaction is not relayed
when the method returns. It is only scheduled for broadcasting at a later
time. Also, there will be another method which only schedules for broadcast
to Tor or I2P peers.

44a726133a

DrahtBot commented at 2:30 pm on October 7, 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/33565.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	optout21, stratospher, cedwies, andrewtoth

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:

#33567 (node: change a tx-relay on/off flag to enum by vasild)
#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)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

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.

glozow added the label Refactoring on Oct 7, 2025

in src/net_processing.h:120 in 44a726133a

115@@ -116,8 +116,13 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
116     /** Get peer manager info. */
117     virtual PeerManagerInfo GetInfo() const = 0;
118 
119-    /** Relay transaction to all peers. */
120-    virtual void RelayTransaction(const Txid& txid, const Wtxid& wtxid) = 0;
121+    /**
122+     * Schedule a transaction to be broadcast to all peers at a later time.

optout21 commented at 10:57 am on October 8, 2025:

nit: “at a later time” is implied by “schedule”, unnecessary to mention, plus it may suggest to the reader that you can control the later time here (which is not the case).

0     * Schedule a transaction for broadcasting to all peers.

glozow commented at 3:17 pm on October 9, 2025:

The transaction isn’t really scheduled; its broadcast time hasn’t been decided. Apologies for the pedantry but given this PR aims to rename something for clarity, I point out that all other schedules in net_processing involve deciding on the time at which something will happen.

My suggestion: here, we QueueAnnouncement. Later, depending on what’s in the priority queue and what else we receive, might inv it. “Announce” is more specific than “Broadcast” for tx relay.

andrewtoth commented at 5:19 pm on October 9, 2025:

The transaction isn’t really scheduled; its broadcast time hasn’t been decided.

Hmm but isn’t the announcements being queued just an implementation detail? The tx may or may not be queued for announcement to each individual peer. If they are queued, then we can say they have been scheduled for those peers at the next periodic send.

Later, depending on what’s in the priority queue and what else we receive, might inv it. “Announce” is more specific than “Broadcast” for tx relay.

Same for this just being an implementation detail. Sending announcements is the first step of a tx being broadcast. The intention is to broadcast them.

glozow commented at 5:22 pm on October 9, 2025:

If they are queued, then we can say they have been scheduled for those peers at the next periodic send.

No, we can’t. The next send will only include the highest feerate transactions in the queue. If we keep receiving higher feerate transactions, it won’t be inv’d for some time. This transaction just has a spot in the priority queue.

andrewtoth commented at 6:30 pm on October 9, 2025:

The next send will only include the highest feerate transactions in the queue

Ah, right, but then can we say it is scheduled for the next periodic send which will include it as one of the highest feerate transactions?

If we keep receiving higher feerate transactions, it won’t be inv’d for some time

Sure, but this is I believe an implementation detail. The tx has been scheduled when we call this method, and newly received txs can alter the broadcast schedule after the fact. The intention of the method is still to schedule the tx for broadcast.

I don’t think having the verb of this method be Queue describes the intention of the method. The callers of this method don’t need to know that there is a queue of txs that our tx needs to be added to, that is an implementation detail. The callers want to know that a best effort will be made to broadcast this tx to all peers sometime in the future, which ScheduleTxForBroadcastToAll describes.

in src/net_processing.h:123 in 44a726133a

120-    virtual void RelayTransaction(const Txid& txid, const Wtxid& wtxid) = 0;
121+    /**
122+     * Schedule a transaction to be broadcast to all peers at a later time.
123+     * This function saves the transaction id to `Peer::TxRelay::m_tx_inventory_to_send` for each peer.
124+     * Later, depending on `Peer::TxRelay::m_next_inv_send_time` and if the transaction is in the
125+     * mempool an `INV` about it is sent to the peer.

optout21 commented at 10:57 am on October 8, 2025:

nit: comma

0     * mempool, an `INV` about it is sent to the peer.

optout21 commented at 11:01 am on October 8, 2025: none

ACK 44a726133a880f818228c01b55236b3cb3eb1a67

Straightforward renaming of one method, plus doc comment change. The change is rather small for a PR, but since it’s part of a larger change (#29415), it’s ok.

stratospher commented at 9:00 am on October 9, 2025: contributor

ACK 44a7261.

in src/net_processing.cpp:4287 in 44a726133a

4285@@ -4286,7 +4286,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4286                 } else {
4287                     LogPrintf("Force relaying tx %s (wtxid=%s) from peer=%d\n",

cedwies commented at 1:29 pm on October 9, 2025:

Nit: Maybe we should change this log message. Something like LogPrint(BCLog::NET, "Force scheduling tx %s for broadcast\n", tx.GetHash().ToString()); That wording would better match the new method name and clarifies that we are queuing it, not sending instantly.

glozow commented at 4:57 pm on October 9, 2025:

Fwiw I don’t agree with this suggestion as we are not scheduling anything, and ForceRelay is the name of the permission.

cedwies commented at 1:35 pm on October 9, 2025: none

Tested ACK 44a7261 Unit/Functional tests pass. Checked git grep RelayTransaction

andrewtoth commented at 1:56 pm on October 9, 2025: contributor

ACK 44a726133a880f818228c01b55236b3cb3eb1a67

net_processing: rename RelayTransaction to better describe what it does #33565

Code Coverage & Benchmarks

Reviews

Conflicts