net: do not break when addr is not from a distinct network group
#27863
Conversation
When the address is from a network group we already caught, do a `continue` and try to find another address until conditions are met or we reach the limit (`nTries`).
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee 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. |
|
cc: @mzumsande |
|
can you contextualize this change? generally it's helpful when the OP & commit message explain why the change is meaningful vs what the change is doing. that'd be valuable for me to evaluate this patch |
@amitiuttarwar, sure! We require outbound IPv4/IPv6 connections to be to distinct network groups. E.g. in case asmap has been used, we expect to have our outbound connections from different Autonomous System (AS). In the current implementation, we basically select an address (from addrman) and if it's not from a distinct network group, we do a |
There was a problem hiding this comment.
utACK 5fa4055
I once wrote up my understanding of the continue/break question here on a related question. In short, think it's better to continue if we just picked an unlucky address from addrman, but nothing went wrong fundamentally, so it seems better to try another address right away instead of leaving the loop and having a sleep.
I think the upside is that we would find new connection faster (less sleeping for half a second), a possible downside might be that if for some reason we wouldn't have a suitable peer in addrman (because we might have run out of netgroups) we would spend more resources in the net thread. But after #27374, that shouldn't be possible.
|
utACK 5fa4055 thanks for the context, I was initially unclear if it was a performance optimization or causing another issue. makes sense to me for this case to it seems like the same logic could also be applied to the next conditional, which is also focused on the address that was randomly selected: |
I think we can leave that one with |
ok sure. I agree it should be unlikely / unexpected to hit, but I don't see why it would be preferable to jump back to the outer loop & sleep, etc. vs have a tighter iteration (esp since |
|
ACK 5fa4055 Agree that |
…stinct network group 5fa4055 net: do not `break` when `addr` is not from a distinct network group (brunoerg) Pull request description: When the address is from a network group we already caught, do a `continue` and try to find another address until conditions are met or we reach the limit (`nTries`). ACKs for top commit: amitiuttarwar: utACK 5fa4055 achow101: ACK 5fa4055 mzumsande: utACK 5fa4055 Tree-SHA512: 225bb6df450b46960db934983c583e862d1a17bacfc46d3657a0eb25a0204e106e8cd18de36764e210e0a92489ab4b5773437e4a641c9b455bde74ff8a041787
When the address is from a network group we already caught, do a `continue` and try to find another address until conditions are met or we reach the limit (`nTries`). Github-Pull: bitcoin#27863 Rebased-From: 5fa4055
When the address is from a network group we already caught,
do a
continueand try to find another address until conditionsare met or we reach the limit (
nTries).