3

In the inner loop of ThreadOpenConnections() in net.cpp sometimes there is a break to the outer loop and sometimes a continue. This is my understanding of the decision between break and continue in this part of the code:

  • A break fully 'restarts' the search for an outward connection, the type of which will be determined again. Also new incoming potential peers will be included in the addrman tables.
  • A continue effectivily just asks addrman for a next candidate peer from the existing tables. The type of outward connection that we wish for remains the same.

My question is about the choice of break versus continue in two cases. Here the code does a break:

// if we selected an invalid or local address, restart
if (!addr.IsValid() || IsLocal(addr)) {
    break;
}

While a few lines below there is a continue:

if (!IsReachable(addr))
    continue;

My question is: why 'restart' in the first code snippet and not continue? In other words: why not ask addrman a few times more, as it might supply a valid address soon? Or, for the second snippet, if we got an unreachable address: why not restart, so addrman might get refreshed with more reachable addresses?

I tried to find the answer by tracking back the history of this part of the code. The break in the first code snippet above appears to stem from the introduction of the address manager (pr/787). It did not seem to answer my question.

codo
  • 31
  • 2

1 Answers1

2
// if we selected an invalid or local address, restart
if (!addr.IsValid() || IsLocal(addr)) {
    break;
}

This looks like some kind of a belt-and-suspenders check which shouldn't happen under normal circumstances, because we shouldn't have added invalid or local addresses to AddrMan in the first place. I'm not sure why a break was chosen here - a continue here would probably be fine as well, it probably doesn't matter much.

if (!IsReachable(addr))
    continue;

This, on the other hand, is something that could happen frequently, because IsReachable is dynamic, it depends on user configuration - we may have run on Tor in the past and deactivated with the next restart, so that all Tor addresses that were previously reachable are now unreachable.

Using continue instead of break in this case seems important to me: Encountering an unreachable address is no reason to skip the inner loop and have a 500ms sleep in the outer loop - we'd like to continue immediately without a sleep.

why not restart, so addrman might get refreshed with more reachable addresses?

The process of AddrMan getting more addresses is independent of the ThreadOpenConnection logic - this happens in another area of the code (net_processing) and another Thread - whenever we receive addr messages from our peers, they are added to addrman.

Lightlike
  • 581
  • 1
  • 12