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

  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: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.
  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. 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
  12. vasild force-pushed on Oct 15, 2025
  13. vasild commented at 1:40 pm on October 15, 2025: contributor
    44a726133a...84b2ad0334: address suggestions
  14. glozow commented at 2:02 pm on October 15, 2025: member
    utACK 84b2ad0334
  15. DrahtBot requested review from andrewtoth on Oct 15, 2025
  16. DrahtBot requested review from stratospher on Oct 15, 2025
  17. DrahtBot requested review from cedwies on Oct 15, 2025
  18. DrahtBot requested review from optout21 on Oct 15, 2025
  19. andrewtoth approved
  20. andrewtoth commented at 0:36 am on October 16, 2025: contributor
    ACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb
  21. 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.
    
  22. 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!
  23. l0rinc approved
  24. 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

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

  30. glozow closed this on Oct 24, 2025


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-10-31 15:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me