netaddress: Simplify reachability logic

dongcarl commented at 8:53 pm on January 16, 2020: member

 0Our reachability logic was overly complicated, and most likely
 1incorrect. In particular, we would prefer advertising our IPv4 services
 2for peers on Toredo, which would most likely just result in us sending
 3packets to their Toredo provider.
 4
 5The new logic is a bit simpler:
 6
 7- Assume all peers can reach us through IPv4 (same as before)
 8- If peer is unknown or unroutable, assume they can reach through every
 9  network (same as before)
10- IPv6 peers can reach through IPv6
11- Onion peers can reach through Onion

What I need help on:

Is the “If peer is unknown or unroutable, assume they can reach through every network” rule reasonable? I suspect I’m misunderstanding the original intention.
Since the unknown and unroutable cases seem to follow the same logic, can we just make them one thing and eliminate the awkward re-purposing of NET_MAX?

docs: Improve GetLocal documentation e5036ad1f8

netaddress: Simplify reachability logic

Our reachability logic was overly complicated, and most likely
incorrect. In particular, we would prefer advertising our IPv4 services
for peers on Toredo, which would most likely just result in us sending
packets to their Toredo provider.

The new logic is a bit simpler:

- Assume all peers can reach us through IPv4 (same as before)
- If peer is unknown or unroutable, assume they can reach through every
  network (same as before)
- IPv6 peers can reach through IPv6
- Onion peers can reach through Onion

2120989b9c

dongcarl force-pushed on Jan 16, 2020

DrahtBot added the label P2P on Jan 16, 2020

fanquake commented at 0:32 am on January 17, 2020: member

cc @EthanHeilman

in src/netaddress.cpp:541 in 2120989b9c

544     case NET_IPV6:
545-        switch(ourNet) {
546-        default:         return REACH_DEFAULT;
547-        case NET_TEREDO: return REACH_TEREDO;
548-        case NET_IPV4:   return REACH_IPV4;
549-        case NET_IPV6:   return fTunnel ? REACH_IPV6_WEAK : REACH_IPV6_STRONG; // only prefer giving our IPv6 address if it's not tunnelled

ariard commented at 6:56 pm on January 21, 2020:

Isn’t the bias to favor IPv4 announcement over IPv6-tunneled lost with the change ? It may make connection relying on 6to4 routers less reliable..

dongcarl commented at 8:30 pm on January 23, 2020:

Yes, I think we don’t want to favor IPv4 over IPv6-tunneled, because it might be the case that the only connection they have is thru IPv6-tunneled.

ariard commented at 11:55 pm on February 13, 2020:

Okay to be clear we do want to favor connection through IPv6-tunneled, this is behavior change. Maybe code should be better documented but I think commit description is right “IPv6 peers can reach through IPV6”

in src/netaddress.cpp:515 in 2120989b9c

528         return REACH_UNREACHABLE;
529 
530-    int ourNet = GetExtNetwork(this);
531-    int theirNet = GetExtNetwork(paddrPartner);
532-    bool fTunnel = IsRFC3964() || IsRFC6052() || IsRFC6145();
533+    int ourNet = GetNetwork();

ariard commented at 7:14 pm on January 21, 2020:

If ourNet is nullptr you will now mark its reachability to NET_IPV6 which may be the highest-scored one and announce this one (but can we have null entries in mapLocalHost anyway ?)

dongcarl commented at 8:28 pm on January 23, 2020:

Perhaps I don’t understand C++ well enough but from my understanding, this cannot be nullptr?

sipa commented at 8:44 pm on January 23, 2020:

this cannot be nullptr, because if it was, you’re invoking a member of nullptr which is UB.

ariard commented at 7:17 pm on January 21, 2020: member

On the interpretation of this rule, seems to me it’s contrary to the first one, if we assume every peer can reach us through IPv4 we should announce an IPv4 address instead of a IPv6. A IPv6 host is likely to be able to reach a IPv4 peer, do we hold the reverse for true ?
I think so, see my comment.

DrahtBot commented at 0:03 am on May 21, 2020: member

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

Conflicts

No conflicts as of last run.

glowang commented at 5:59 pm on May 25, 2020: contributor

This cleanup looks very helpful! I organized the old reachability logic flow in this table (vertically listed are our networks), and it looks like the handling for Teredo is changed. The bolded grid values are now all changed to REACH_DEFAULT in the simplified logic. Is this the intended effect of

In particular, we would prefer advertising our IPv4 services for peers on Toredo, which would most likely just result in us sending packets to their Toredo provider.

?

	IPv4	IPv6	Onion	Teredo	Unknown	Unroutable
IPv4	REACH_IPV4	REACH_IPV4	REACH_IPV4	REACH_IPV4	REACH_IPV4	REACH_IPV4
IPv6	REACH_DEFAULT	REACH_IPV6 WEAK/STRONG	REACH_DEFAULT	REACH_IPV6_WEAK	REACH_IPV6_WEAK	REACH_IPV6_WEAK
Onion	REACH_DEFAULT	REACH_DEFAULT	REACH_PRIVATE	REACH_DEFAULT	REACH_PRIVATE	REACH_PRIVATE
Teredo	REACH_DEFAULT	REACH_TEREDO	REACH_DEFAULT	REACH_TEREDO	REACH_TEREDO	REACH_TEREDO

in src/netaddress.cpp:508 in 2120989b9c

518         REACH_DEFAULT,
519-        REACH_TEREDO,
520-        REACH_IPV6_WEAK,
521         REACH_IPV4,
522-        REACH_IPV6_STRONG,
523+        REACH_IPV6,

glowang commented at 6:00 pm on May 25, 2020:

Could you provide some reasoning behind merging IPv6 WEAK and STRONG into one case?

dongcarl commented at 7:34 pm on January 22, 2021: member

Not going to work on this in the immediate future, closing

dongcarl closed this on Jan 22, 2021

DrahtBot locked this on Aug 18, 2022

netaddress: Simplify reachability logic #17944

Conflicts