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

vasild · 2025-10-07T14:30:21Z

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 · 2025-10-07T14:30:26Z

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:

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

src/net_processing.h

optout21 · 2025-10-08T11:01:30Z

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

ACK 44a7261.

cedwies

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

src/net_processing.cpp

andrewtoth · 2025-10-09T13:56:37Z

ACK 44a726133a880f818228c01b55236b3cb3eb1a67

vasild · 2025-10-15T13:40:49Z

44a726133a...84b2ad0334: address suggestions

glozow · 2025-10-15T14:02:36Z

utACK 84b2ad0334

andrewtoth

ACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb

l0rinc

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

src/net_processing.h

optout21 · 2025-10-16T05:54:40Z

reACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb

cedwies

ACK 84b2ad0

naiyoma

utACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb

ajtowns · 2025-10-21T12:38:01Z

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 · 2025-10-24T08:33:30Z

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., eb7cc9f as a motivating example where I still think it can be worthwhile

vasild · 2025-11-10T10:00:55Z

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.

l0rinc · 2025-11-10T10:20:19Z

I also prefer extracting these smaller changes from big PR to smaller, dedicated ones to have some progress and to have focused discussions

w0xlt

crACK 9989853

ajtowns · 2025-11-13T04:42:33Z

#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 Revert compact block cache inefficiencies #33253)
changing parsing of abnormal pubkeys for CHECKMULTISIG (see Revert compact block cache inefficiencies #33253)
changing logging RPC behaviour with a PR claiming "This is purely a refactor with no behavior changes." (see Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs #27231 (comment))
and of course the classic performance improvement / inflation bug (Remove duplicatable duplicate-input check from CheckTransaction #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.

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

vasild · 2025-11-13T09:47:57Z

9989853447...78595ae0e7: take suggestions

l0rinc

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 78595ae, redid the change locally, verified the flow and the comment seems to describe what's happening accurately.

l0rinc · 2025-11-13T11:43:51Z

src/node/transaction.cpp

        break;
    case TxBroadcast::MEMPOOL_AND_BROADCAST_TO_ALL:
-        node.peerman->RelayTransaction(txid, wtxid);
+        node.peerman->InitiateTxBroadcastToAll(txid, wtxid);


the enum name aligns more closely with the method name now 👍

w0xlt

reACK 78595ae

optout21 · 2025-11-14T09:29:39Z

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.

optout21 · 2025-11-14T09:31:18Z

reACK 78595ae

achow101 · 2025-11-21T22:06:46Z

I agree with @ajtowns and don't entirely see the benefit of having split this commit into its own PR.

As someone who frequently writes gigantic PRs that need to be split up, this commit for me would not have met the bar to be split into its own PR. It isn't a complicated refactor that needs focused review on its own; it isn't something that is useful across multiple PRs; and it isn't itself a feature that is useful on its own.

I get that having smaller PRs can make it feel like things are moving faster for a project, but it also means that there is an intermediate state where we will have merged a bunch of prep work but not the feature itself. In that past this has led to both unnecessary code churn (a refactor was done for a feature, but then it gets refactored again before the feature is completed, so yet another refactor needs to be done), technical debt, and unfinished features.

Changing a method's name to better reflect its usage fits very well into that picture, in my opinion.

We rarely do PRs that are just renaming things. IIRC renaming PRs are generally limited to things that are particularly misleading and actively causing confusion.

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?

That change is made independent of the main PR, and in fact, the main PR does not even include the commit at all. The small change could simply never be merged and it would have no effect on the big change.

In contrast, the commit in this PR is included in #29415 and is a currently a prerequisite for that PR to be merged.

l0rinc · 2025-11-22T13:25:17Z

Thanks for the explanation @achow101!

renaming PRs are generally limited to things that are particularly misleading and actively causing confusion.

I assumed that was the case here - if not, maybe the parent PR could also just avoid the rename in the first place.

I get that having smaller PRs can make it feel like things are moving faster for a project, but it also means that there is an intermediate state where we will have merged a bunch of prep work but not the feature itself.

Valid points, I also don't like that the same code is now reviewed in a lot of places.
Given that some of the parts were successfully split out, is my understanding correct that the problem is not that some features are split off, but that those splits may not make sense on their own (and GitHub doesn't yet have cluster-PRs to model these relationships :p)?

The PR is in review for 2.5 years, what is the best way forward, how can we make progress there?
An alternative I mentioned in #29415 (review) was to try a smaller coherent feature, something like:

I wonder how much we could simplify if we added a fire-and-forget type transaction in the first version of the PR, without any additional functionality? Just send and don't care what happens, we assume it will just arrive - and implement the waiting in a follow-up.

fanquake · 2025-12-06T13:51:39Z

This is going to get merged via #29415, which at this point, is getting ACKs.

vasild mentioned this pull request Oct 7, 2025

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Merged

4 tasks

DrahtBot mentioned this pull request Oct 7, 2025

node: change a tx-relay on/off flag to enum #33567

Merged

glozow added the Refactoring label Oct 7, 2025

This was referenced Oct 8, 2025

[DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation) #30277

Draft

p2p: Fill reconciliation sets (Erlay) attempt 2 #30116

Open

optout21 reviewed Oct 8, 2025

View reviewed changes

src/net_processing.h Outdated Show resolved Hide resolved

optout21 reviewed Oct 8, 2025

View reviewed changes

src/net_processing.h Outdated Show resolved Hide resolved

stratospher reviewed Oct 9, 2025

View reviewed changes

cedwies reviewed Oct 9, 2025

View reviewed changes

src/net_processing.cpp Show resolved Hide resolved

vasild force-pushed the rename_RelayTransaction branch from 44a7261 to 84b2ad0 Compare October 15, 2025 13:40

DrahtBot requested review from andrewtoth, cedwies, optout21 and stratospher October 15, 2025 14:02

andrewtoth approved these changes Oct 16, 2025

View reviewed changes

l0rinc approved these changes Oct 16, 2025

View reviewed changes

src/net_processing.h Outdated Show resolved Hide resolved

src/net_processing.h Outdated Show resolved Hide resolved

src/net_processing.h Outdated Show resolved Hide resolved

cedwies reviewed Oct 16, 2025

View reviewed changes

naiyoma reviewed Oct 20, 2025

View reviewed changes

glozow closed this Oct 24, 2025

glozow reopened this Nov 10, 2025

DrahtBot added the Needs rebase label Nov 10, 2025

vasild force-pushed the rename_RelayTransaction branch from 84b2ad0 to 9989853 Compare November 10, 2025 09:48

DrahtBot removed the Needs rebase label Nov 10, 2025

w0xlt reviewed Nov 12, 2025

View reviewed changes

DrahtBot requested review from andrewtoth, cedwies, glozow, l0rinc and naiyoma November 12, 2025 19:11

vasild force-pushed the rename_RelayTransaction branch from 9989853 to 78595ae Compare November 13, 2025 09:47

l0rinc approved these changes Nov 13, 2025

View reviewed changes

DrahtBot requested a review from w0xlt November 13, 2025 11:48

w0xlt reviewed Nov 13, 2025

View reviewed changes

achow101 mentioned this pull request Nov 21, 2025

net_processing: reorder the code that handles the VERSION message #33792

Closed

fanquake closed this Dec 6, 2025

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

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

Uh oh!

Conversation

vasild commented Oct 7, 2025 • edited by DrahtBot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

optout21 commented Oct 8, 2025

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

cedwies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewtoth commented Oct 9, 2025

Uh oh!

vasild commented Oct 15, 2025

Uh oh!

glozow commented Oct 15, 2025

Uh oh!

andrewtoth left a comment

Choose a reason for hiding this comment

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

optout21 commented Oct 16, 2025

Uh oh!

cedwies left a comment

Choose a reason for hiding this comment

Uh oh!

naiyoma left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Oct 21, 2025

Uh oh!

instagibbs commented Oct 24, 2025

Uh oh!

vasild commented Nov 10, 2025

Uh oh!

l0rinc commented Nov 10, 2025

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Nov 13, 2025

Uh oh!

vasild commented Nov 13, 2025

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

l0rinc Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

optout21 commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

optout21 commented Nov 14, 2025

Uh oh!

achow101 commented Nov 21, 2025

Uh oh!

l0rinc commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

vasild commented Oct 7, 2025 •

edited by DrahtBot

Loading

DrahtBot commented Oct 7, 2025 •

edited

Loading

l0rinc Nov 13, 2025 •

edited

Loading

optout21 commented Nov 14, 2025 •

edited

Loading

l0rinc commented Nov 22, 2025 •

edited

Loading