The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

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:

• #27509 (Relay 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.

While renaming this variable is technically correct, I don't think it's useful to have these kind of PRs. Although it is small and trivial, it does take away time from review of the hundreds of other PRs we have, has an effect on git blaming, and possibly conflicts with other PRs in this area that do introduce meaningful changes.

@achow101 The code comment needs to be fixed as it is now incorrect, and the naming is now misleading. I think the change is clearly justified here.

I don't think it's useful to have these kind of PRs

Ok, this is your opinion. Sometimes I feel the same for some PRs. My reaction is then to ignore them - don't spend my time on them if I think it is not worth it. But I don't try to impose my opinion on others - if somebody was thinking it is worth to open a PR and somebody else was thinking it is worth to review, then let it be. Those other people have different opinion than mine and I am not going to tell them what to work on or what not to work on.

This is the true beauty of decentralized development. The opposite is centralized management - where somebody decides what is important and imposes it on others.

I don't think it's useful to have these kind of PRs

Ok, this is your opinion. Sometimes I feel the same for some PRs. My reaction is then to ignore them - don't spend my time on them if I think it is not worth it. But I don't try to impose my opinion on others

I agree with you that at a project / maintenance level we should be neutral about "I don't think this PR is a good use of effort but I can't point out any actual problems with it" review feedback. If a PR has sufficient review and no substantive technical objections, it should be merged regardless of these comments.

But I think these comments are really valuable for helping people decide what to spend time on, and I personally want to encourage more not less "I don't think this PR is useful" comments. Sometimes a PR is more useful than it initially appears, and more discussion could help clarify that. Other times, maybe the PR is not really very useful, and having the feedback could help the contributor decide not to put more work into the PR, and avoid having them wonder for months if the PR isn't getting review because it isn't interesting, or is too hard to understand, or just wasn't noticed.

The code comment needs to be fixed as it is now incorrect, and the naming is now misleading. I think the change is clearly justified here.

Could you say more about how the current code is misleading in the PR description? I'm sure the new code is better, but knowing what is incorrect or misleading in the current code could help motivate the PR a little more

Per the first line of the diff and the first line of the PR description, a well-named data structure is preferable to one with an out-of-date name that needs a comment to explain what it now does after its role changed. Our p2p code is sufficiently critical for this to be important, and for that reason we often see review comments requesting clearer naming in p2p changes.

I think the canned response in #27467 (comment), and its use, is problematic. Many commits are merged that change naming for style reasons only, or even do nothing but change indentation, clean up code, change or drop a word, etc. Here, we have a data structure that changed its role, which seems as good a reason as can be to update the naming in order to make sense and not be a footgun for developers or source of confusion. Coming down on renaming when the semantics changed out from under a data structure, but being fine with PRs containing several renaming commits for style reasons only, is a little baroque. ACKing and merging changes that would fall under that canned response, while punching down on as-valid-or-more other ones inevitably looks arbitrary, hypocritical or favoritist. A rule or canned response, like the developer notes, ought to be applied consistently or not at all. This particular one is too wide in interpretation and subjective in practice, so when it’s not misapplied, it is at best arbitrary and unhelpful, and at worst is disruptive or negative social signaling that shouldn't be done. For these reasons, my suggestion would be to bin that canned response and not use it at all.

Let's please encourage review, including post-merge review and occasional fixups that may come from that. If there's one thing that is perennially desired on this project, it's more eyes on the code and helpful, positive, complementary reviewers to move things forward. As long as a reviewer is well-intentioned and trying to help Bitcoin, let's encourage them with kindness, indulgence and wisdom. Thanks!

One more thing to consider - this PR is a followup to another one (not an out of the blue, standalone one). We use the will-do-in-a-followup practice to avoid invalidating ACKs on the original PR with non-critical changes. If such followup PRs start getting frowned upon or ignored, then that could create a backward pressure to insist to include the small/non-critical suggestions in the original PR because the followup PR will not make it.

@stratospher, @mzumsande, maybe you want to review this followup since you authored/reviewed #27374? (or feel free to ignore)

Code Review ACK 11bb31c

This fixes an outdated doc, so it's an improvement.

The naming is now misleading.

I don't think the previous naming was too bad: #27343 added an explanatory comment after all. If anything, it was much more misleading before #27343 when there was no comment, because the name setConnected doesn't imply "netgroups of outbound peers from all networks" any more than it implies "netgroups of ipv4/ipv6 outbound peers" - after all we are "connected" to inbound peers too.

But I'm not against renaming.