Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

netaddress: Simplify reachability logic #17944

Closed

Conversation

dongcarl
Copy link
Contributor

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

What I need help on:

  1. 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.
  2. 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?

dongcarl and others added 2 commits January 16, 2020 16:07
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
@dongcarl dongcarl force-pushed the 2020-01-simplify-reachability branch from 98b1365 to 2120989 Compare January 16, 2020 21:08
@DrahtBot DrahtBot added the P2P label Jan 16, 2020
@fanquake
Copy link
Member

cc @EthanHeilman

Copy link
Contributor

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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 ?

  2. I think so, see my comment.

default: return REACH_DEFAULT;
case NET_TEREDO: return REACH_TEREDO;
case NET_IPV4: return REACH_IPV4;
case NET_IPV6: return fTunnel ? REACH_IPV6_WEAK : REACH_IPV6_STRONG; // only prefer giving our IPv6 address if it's not tunnelled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

src/netaddress.cpp Show resolved Hide resolved
@DrahtBot
Copy link
Contributor

DrahtBot commented May 21, 2020

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

Conflicts

No conflicts as of last run.

@glowang
Copy link
Contributor

glowang commented May 25, 2020

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

REACH_IPV4,
REACH_IPV6_STRONG,
REACH_IPV6,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dongcarl
Copy link
Contributor Author

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

@dongcarl dongcarl closed this Jan 22, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants