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::InitiateTxBroadcastToAll(). 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.

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	glozow, andrewtoth, l0rinc, optout21, cedwies, naiyoma
Stale ACK	stratospher

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.

glozow commented at 7:09 pm on October 10, 2025:

If Queue is confusing, then I can also suggest MarkTxForRelay or UpdateTxBroadcastSet

I recognize that we can all read verbs differently. I’m just pointing out that “schedule” in this codebase almost always means putting a task on the scheduler background thread with a specific time.

vasild commented at 8:03 am on October 14, 2025:

I meant “schedule” in the more general English sense, however I can see that:

“schedule” in this codebase almost always means putting a task on the scheduler …

So, looking at “schedule (verb)” synonyms, what about:

0ArrageTxForBroadcastToAll() 
1BookTxForBroadcastToAll()
2LineUpTxForBroadcastToAll()
3MarkTxForBroadcastToAll()
4NoteTxForBroadcastToAll()
5RegisterTxForBroadcastToAll()
6SetTxForBroadcastToAll()
7SetUpTxForBroadcastToAll()

?

optout21 commented at 9:10 am on October 14, 2025:

There have been several valid points mentioned about naming:

it doesn’t really broadcast, just initiates the broadcast process
‘schedule’ suggest scheduling at a given future time
‘queue’ suggests placing in some concrete queue

I can also add one:

for the a user of this method, the intent is to broadcast, the other details are secondary

I can also propose some more alternatives:

InitiateTxBroadcastToAll()
EnqueueTxForBroadcastToAll()
BackgroundBroadcastTxToAll()
BroadcastTxForAllAsync()
BroadcastTxForAllBg()

My preference is the last one, as it is specific enough to indicate that broadcast doesn’t actually happens during the call, but without putting to much focus on this detail.

While it’s important to pick a good name when naming/renaming, this starts to have a bit of bikeshedding vibe… There are many good and acceptable proposals already mentioned.

vasild commented at 1:17 pm on October 14, 2025:

Change to InitiateTxBroadcastToAll()?

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.

vasild commented at 1:41 pm on October 15, 2025:

Added.

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 outdated

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.

vasild commented at 1:41 pm on October 15, 2025:

Leaving it as it is.

cedwies commented at 2:04 pm on October 16, 2025:

Thanks for the correction, that makes sense. I was focusing on the mechanism rather than the permission. I appreciate the clarification.

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

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

84b2ad0334

vasild force-pushed on Oct 15, 2025

vasild commented at 1:40 pm on October 15, 2025: contributor

44a726133a...84b2ad0334: address suggestions

glozow commented at 2:02 pm on October 15, 2025: member

utACK 84b2ad0334

DrahtBot requested review from andrewtoth on Oct 15, 2025

DrahtBot requested review from stratospher on Oct 15, 2025

DrahtBot requested review from cedwies on Oct 15, 2025

DrahtBot requested review from optout21 on Oct 15, 2025

andrewtoth approved

andrewtoth commented at 0:36 am on October 16, 2025: contributor

ACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb

in src/net_processing.h:121 in 84b2ad0334

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+     * Initiate a transaction broadcast to all peers.
123+     * This function saves the transaction id to `Peer::TxRelay::m_tx_inventory_to_send` for each peer.

l0rinc commented at 0:52 am on October 16, 2025:

Nit: now that they’re split properly by type

0     * This function saves the witness transaction id to `Peer::TxRelay::m_tx_inventory_to_send` for each peer.

or

0     * This function saves the transaction wtxid to `Peer::TxRelay::m_tx_inventory_to_send` for each peer.

l0rinc commented at 1:04 am on October 16, 2025:

0     * Queue the transaction id to `Peer::TxRelay::m_tx_inventory_to_send` for each peer.

in src/net_processing.h:120 in 84b2ad0334

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+     * Initiate a transaction broadcast to all peers.

l0rinc commented at 0:57 am on October 16, 2025:

Given that there are at least 3 gates in the code before the wtxid is actually queued (TxRelay must exist, version handshake must be complete and peer doesn’t already have it), it may be more precise to state:

0     * Initiate a transaction broadcast to eligible peers.

vasild commented at 7:06 am on October 16, 2025:

I will address if I have to re-touch, thanks!

l0rinc approved

l0rinc commented at 1:10 am on October 16, 2025: contributor

Left a few code comment suggestions, I think the newly added one is a bit imprecise, but I also don’t mind merging as is if others don’t think they’re serious.

ACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb

optout21 commented at 5:54 am on October 16, 2025: none

reACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb

cedwies commented at 6:33 pm on October 16, 2025: none

ACK 84b2ad0

naiyoma commented at 12:53 pm on October 20, 2025: contributor

utACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb

ajtowns commented at 12:38 pm on October 21, 2025: contributor

I don’t think it makes much sense to split this out; it doesn’t reduce the size of the parent PR meaningfully, and renaming a function doesn’t really seem like it justifies a PR on its own.

instagibbs commented at 8:33 am on October 24, 2025: member

Have to concur with @ajtowns . I could see it if it were especially misleading that could cause issues as is, but I don’t think it is?

e.g., eb7cc9fd2140df77acc6eb42004cf45b260bc629 as a motivating example where I still think it can be worthwhile

glozow closed this on Oct 24, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts