In #27374 the role of the setConnected data structure in CConnman::ThreadOpenConnections changed from the set of outbound peer netgroups to those of outbound IPv4/6 peers only.
In accordance with the changed semantics, this pull fixes a code comment regarding feeler connections and updates the naming of setConnected to outbound_ipv46_peer_netgroups.
Addresses #27374 (review).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | vasild, mzumsande, ryanofsky |
| Concept ACK | Ayush170-Future |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
ACK on the concept.
The code also looks good to me.
1706@@ -1707,7 +1707,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1707 int nOutboundFullRelay = 0;
1708 int nOutboundBlockRelay = 0;
1709 int outbound_privacy_network_peers = 0;
1710- std::set<std::vector<unsigned char>> setConnected; // netgroups of our ipv4/ipv6 outbound peers
1711+ std::set<std::vector<unsigned char>> outbound_ipv46_peer_netgroups_set;
_set seems unnecessary. E.g. apples vs apples_vector (I guess the first one is preferable).
In PR 27374, the semantics of the `setConnected` data structure in
CConnman::ThreadOpenConnections changed from the set of outbound peer
netgroups to those of outbound IPv4/6 peers only.
This commit updates a code comment in this regard about feeler connections and
updates the naming of `setConnected` to `outbound_ipv46_peer_netgroups` to
reflect its new role.
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
1854@@ -1855,8 +1855,8 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1855 std::tie(addr, addr_last_try) = addrman.Select();
1856 }
1857
1858- // Require outbound connections, other than feelers, to be to distinct network groups
1859- if (!fFeeler && setConnected.count(m_netgroupman.GetGroup(addr))) {
1860+ // Require outbound IPv4/IPv6 connections, other than feelers, to be to distinct network groups
to be two
Since we are talking about direction (outbound) - either removing it or changing it to “toward” makes more sense - now that you mention it.
Note: Since the comment mentions “groups” it seems like a typo.
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!
Code Review ACK 11bb31c1c43b5da36ca8509b5747abfb3278ffcd
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.
Code review ACK 11bb31c1c43b5da36ca8509b5747abfb3278ffcd
This change seems like an improvement to me, but I also think the early comment calling it “small and trivial” was appropriate and useful for clarifying the purpose of the change. Reviewers need to be able to express their honest opinions so limited review bandwidth goes to the right places, and so PR authors have a chance to improve and justify their PRs and not sit around wondering why a PR isn’t being reviewed