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

Filter Interface Addresses #936

Merged
merged 7 commits into from
May 29, 2020
Merged

Conversation

aarshkshah1992
Copy link
Contributor

Copy link
Contributor

@willscott willscott left a comment

Choose a reason for hiding this comment

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

probably need to listen for 'local addresses changed' events to invalidate the cached default source addresses.

p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
@aarshkshah1992
Copy link
Contributor Author

aarshkshah1992 commented May 19, 2020

@willscott

probably need to listen for 'local addresses changed' events to invalidate the cached default source addresses.

The local address change event will happen when the libp2p node starts listening on a new address.

Please can you explain why the default source address we get from the netroute library would change if libp2p starts listening on a new address/how would the address we've started listening on "flow"/be available to the netroute library and why it would deem that new address a "better" source than the previous one ?

@willscott
Copy link
Contributor

I think the point is that if you get a new interface (connect to wifi, plug in an ethernet cable) it'll affect both the OS routing table / default route, and the addresses libp2p believes it is listening on. It seems reasonable to want this view of 'our current source address' to reflect the OS's view of its default route.

@aarshkshah1992
Copy link
Contributor Author

@Stebalien @willscott I think it looks much better now. Please take a look.

Copy link
Contributor

@willscott willscott left a comment

Choose a reason for hiding this comment

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

I wonder if we can get away with just running updateLocalIPAddr when emitAddrChange occurs. Presumably the irregular poll of h.Addrs(), which should pick up changed external addresses, will also vary in the same 5 minute interval as local addresses changed, so that might be a good enough signal directly, and remove the need for the extra event loop complexity.

p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
@aarshkshah1992
Copy link
Contributor Author

aarshkshah1992 commented May 21, 2020

@willscott

I wonder if we can get away with just running updateLocalIPAddr when emitAddrChange occurs. Presumably the irregular poll of h.Addrs(), which should pick up changed external addresses, will also vary in the same 5 minute interval as local addresses changed, so that might be a good enough signal directly, and remove the need for the extra event loop complexity.

Let me reason through this so I can clear my head as well:

The emitAddrChange event is emitted when h.Addrs() returns an address we haven't seen before. This can happen in two cases:

  • The user has explicitly started listening on a new address by calling host.Network.Listen().
    • If the user pro-actively starts listening on the "new" local addresses, we are good here.
  • Our local IP addresses have changed and we were listening on an unspecified IP address(0.0.0.0 and "::") before. In this case h.Addrs() will replace the unspecified address with the new local address instead of the old one and we will detect an address change.
    • However, you see, there is a catch-22 here. h.Addrs() needs to know the new local address to return them so that we can emit an addr change event. Hence, changing our local address when we emit an addr change event wont work here if the user dosen't explicitly call Listen for them.

@aarshkshah1992
Copy link
Contributor Author

ping @Stebalien for one last review.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Code LGTM but I really think the extra goroutine+channel makes this harder to read.

p2p/host/basic/basic_host.go Outdated Show resolved Hide resolved
@aarshkshah1992 aarshkshah1992 merged commit 3a1d20b into master May 29, 2020
@aarshkshah1992
Copy link
Contributor Author

@Stebalien Have made the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants