net: do not `break` when `addr` is not from a distinct network group #27863

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

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

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.

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

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 break. This means that we're going to leave the loop, sleep again, add seed nodes, recalculates already-connected network ranges and so on. However, we could instead do a continue and try to find another address that can meet our requirement (distinct network group). That is, we could try it 100x (nTried) before really leaving this loop.

utACK 5fa4055452861ca1700008e1761815e88b29fae7

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 5fa4055452861ca1700008e1761815e88b29fae7

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 continue to the next iteration of the inner loop to try again with another address, rather than break to jump back to the outer loop, sleeping for 1/2 second then iterating through all the nodes to aggregate stats again.

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:

if (!addr.IsValid() || IsLocal(addr)) {
    break;
}

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 break. It's an extreme conditional, because we don't expect to have any invalid or local addresses in addrman.

I think we can leave that one with break. It's an extreme conditional, because we don't expect to have any invalid or local addresses in addrman.

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 ThreadOpenConnections is a significant influencer to addrman info). but no worries, I don't think it matters too much either way & not necessary for this PR.

ACK 5fa4055452861ca1700008e1761815e88b29fae7

Agree that continue makes sense if we're simply unlucky.