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

pull vasild wants to merge 1 commits into bitcoin:master from vasild:rename_RelayTransaction changing 3 files +13 −8
  1. 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.

  2. 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 l0rinc, w0xlt, optout21
    Concept NACK ajtowns
    Stale ACK stratospher, glozow, andrewtoth, cedwies, naiyoma

    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:

    • #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.

  3. glozow added the label Refactoring on Oct 7, 2025
  4. 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()?
  5. 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.
  6. 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.

  7. stratospher commented at 9:00 am on October 9, 2025: contributor
    ACK 44a7261.
  8. in src/net_processing.cpp:4292 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.
  9. cedwies commented at 1:35 pm on October 9, 2025: none
    Tested ACK 44a7261 Unit/Functional tests pass. Checked git grep RelayTransaction
  10. andrewtoth commented at 1:56 pm on October 9, 2025: contributor
    ACK 44a726133a880f818228c01b55236b3cb3eb1a67
  11. vasild force-pushed on Oct 15, 2025
  12. vasild commented at 1:40 pm on October 15, 2025: contributor
    44a726133a...84b2ad0334: address suggestions
  13. glozow commented at 2:02 pm on October 15, 2025: member
    utACK 84b2ad0334
  14. DrahtBot requested review from andrewtoth on Oct 15, 2025
  15. DrahtBot requested review from stratospher on Oct 15, 2025
  16. DrahtBot requested review from cedwies on Oct 15, 2025
  17. DrahtBot requested review from optout21 on Oct 15, 2025
  18. andrewtoth approved
  19. andrewtoth commented at 0:36 am on October 16, 2025: contributor
    ACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb
  20. in src/net_processing.h:121 in 84b2ad0334 outdated 
    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.
    vasild commented at 9:50 am on November 13, 2025:
    Done
    vasild commented at 9:51 am on November 13, 2025:
    Done
  21. in src/net_processing.h:120 in 84b2ad0334 outdated 
    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 commented at 11:14 pm on November 12, 2025:
    Could you please address them, now that you had to rebase anyway?
    vasild commented at 9:50 am on November 13, 2025:
    This slipped through, thanks for the reminder. Done now and also took the changes into https://github.com/bitcoin/bitcoin/pull/29415
  22. l0rinc approved
  23. 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

  24. optout21 commented at 5:54 am on October 16, 2025: none
    reACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb
  25. cedwies commented at 6:33 pm on October 16, 2025: none
    ACK 84b2ad0
  26. naiyoma commented at 12:53 pm on October 20, 2025: contributor
    utACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb
  27. 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.
  28. 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

  29. glozow closed this on Oct 24, 2025
  30. glozow reopened this on Nov 10, 2025
  31. DrahtBot added the label Needs rebase on Nov 10, 2025
  32. vasild force-pushed on Nov 10, 2025
  33. vasild commented at 10:00 am on November 10, 2025: contributor

    84b2ad0334...9989853447: rebase due to conflicts

    I don’t think it makes much sense to split this out; it doesn’t reduce the size of the parent PR meaningfully,

    I agree it does not help much. It helps a little. And that is the point - to get some smaller changes out of the main PR.

    and renaming a function doesn’t really seem like it justifies a PR on its own.

    Having this in its own PR helped have a fresh discussion that resulted in picking up a better name. This did not happen in the main PR for ~1.5 years. That is a positive outcome from having a PR on its own.

    I could see it if it were especially misleading that could cause issues as is, but I don’t think it is?

    For me Relay...() is confusing because it does not relay anything. It gets further confusing in #29415 where another similar function is added. So I decided to clarify and change to a better name. I initially thought about Schedule...() but (only here in this PR) it was suggested that it could be associated with the scheduler, so we ended up with Initiate...(). I like that.

  34. l0rinc commented at 10:20 am on November 10, 2025: contributor
    I also prefer extracting these smaller changes from big PR to smaller, dedicated ones to have some progress and to have focused discussions
  35. DrahtBot removed the label Needs rebase on Nov 10, 2025
  37. DrahtBot requested review from l0rinc on Nov 12, 2025
  38. DrahtBot requested review from glozow on Nov 12, 2025
  39. DrahtBot requested review from andrewtoth on Nov 12, 2025
  40. DrahtBot requested review from naiyoma on Nov 12, 2025
  41. DrahtBot requested review from cedwies on Nov 12, 2025
  42. ajtowns commented at 4:42 am on November 13, 2025: contributor

    #33565 has no NACKs

    To be explicit: NACK. Reviewing bitcoin PRs well is hard, we should not be increasing that burden for, at best, marginal benefits to the author of the PR. For this particular PR, bikeshedding about the names of functions is not a good use of anyone’s time here. Further, small PRs that seem to just be a mild refactoring with no (expected) functional effect have repeatedly resulted in meaningful changes; ones that come to mind currently include:

    • removing the “redundant” Wtxid accompanying CTransactionRef for compact block reconstruction (see #33253)
    • changing parsing of abnormal pubkeys for CHECKMULTISIG (see #33253)
    • changing logging RPC behaviour with a PR claiming “This is purely a refactor with no behavior changes.” (see #27231 (comment))
    • and of course the classic performance improvement / inflation bug (#9049)

    To me, the lesson from examples like that is that all our PRs, even the ones that seem like they should be trivial, should receive serious, in-depth review. But we’re already constrained on review, and should therefore be prioritising what review resources we have to the most valuable PRs we have, not the easiest ones. This PR in particular, and the strategy of “let’s split off easy commits into separate PRs” more generally, does the opposite. Worse, if we do make a habit of merging PRs that (a) seem trivial, and (b) have cursory review from a couple of possibly newer contributors because there’s not enough benefit to justify deeper review, and no apparent need for it; then that opens up a relatively straightforward way to introduce severe bugs (or one that’s at least as straightforward as our approach to fixing severe bugs). Far better to focus on PRs that make non-trivial improvements, and apply correspondingly non-trivial levels of review before merging.

  43. 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.
    78595ae0e7
  44. vasild force-pushed on Nov 13, 2025
  45. vasild commented at 9:47 am on November 13, 2025: contributor
    9989853447...78595ae0e7: take suggestions
  46. in src/node/transaction.cpp:136 in 78595ae0e7 
    132@@ -133,7 +133,7 @@ TransactionError BroadcastTransaction(NodeContext& node,
133     case TxBroadcast::MEMPOOL_NO_BROADCAST:
134         break;
135     case TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL:
136-        node.peerman->RelayTransaction(txid, wtxid);
137+        node.peerman->InitiateTxBroadcastToAll(txid, wtxid);
    l0rinc commented at 11:43 am on November 13, 2025:
    the enum name aligns more closely with the method name now 👍
  47. l0rinc approved
  48. l0rinc commented at 11:48 am on November 13, 2025: contributor

    I don’t want to be dismissive of @ajtowns’ claims. I agree they’re all valid, and please allow me to push back respectfully.

    bikeshedding about the names of functions is not a good use of anyone’s time here.

    I’ve been tricked many times by misnamed methods. If you think the old name is good, or the new name is worse, or the new comment isn’t adding anything, I can empathize with that. But nobody is forcing us to review this. I personally do it because I want some progress with the original, and I know how difficult it is when a sub-change that everyone agrees on keeps getting rebased for months or years without any progress. If people decided that this change is a good use of their time, I don’t think it’s fair to deny them that.

    refactoring with no (expected) functional effect have repeatedly resulted in meaningful changes

    Most of our work is refactoring. Changing a method’s name to better reflect its usage fits very well into that picture, in my opinion. Again, if you don’t agree that the new name is better, that’s fair, but I don’t see how this refactor differs from the hundreds of others we do regularly.

    Far better to focus on PRs that make non-trivial improvements, and apply correspondingly non-trivial levels of review before merging.

    I’m not sure I follow. How is this different from extracting changes into commits to separate noise from signal and to allow proportional review time to the risk implied? How is this different from, e.g., #33862, which also extracts a small change from the big one to unburden it and make some progress?

    Let the record show that I’m personally fine with this change: ACK 78595ae0e71b833ebe7640d5120eea5da14f55ab, redid the change locally, verified the flow and the comment seems to describe what’s happening accurately.

  49. DrahtBot requested review from w0xlt on Nov 13, 2025
  51. optout21 commented at 9:29 am on November 14, 2025: none

    we’re already constrained on review, and should therefore be prioritising what review resources we have to the most valuable PRs we have, not the easiest ones. This PR in particular, and the strategy of “let’s split off easy commits into separate PRs” more generally, does the opposite

    I get your points, however, to nuance it a bit: Splitting of smaller parts into breakout PRs: Cons:

    • higher total overhead
    • may draw review resources from more important PRs
    • risk of shallow review as it’s “a simple change”
    • risk of focus on minor change, bikeshedding

    Pros:

    • Can give momentum to otherwise stalling large review
    • Reduce the cost of branch rebasing
    • Increase the clarity of the heart of the larger change
    • Smaller PRs may attract less experiences reviewers, who can learn about the larger change graudually

    A minor change like this in itself is a no-go. As part of a larger (exisitng) PR is possible. Breaking up a larger change into smaller – self-sufficient and marginally useful – smaller parts is helpful. But there is a tradeoff due to larger total overhead, where the limit lies can be subjective.

    In this particular case I lean towards go, though not strongly.

  52. optout21 commented at 9:31 am on November 14, 2025: none
    reACK 78595ae0e71b833ebe7640d5120eea5da14f55ab


vasild DrahtBot optout21 glozow andrewtoth stratospher cedwies l0rinc naiyoma ajtowns instagibbs w0xlt


andrewtoth optout21 glozow naiyoma stratospher cedwies

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-11-21 00:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me