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

fix: re-validate peers whenever their state changes #607

Merged
merged 2 commits into from
Apr 25, 2020

Conversation

Stebalien
Copy link
Member

  1. When a peer changes their listening addresses, we need to re-run our routing table filters, possibly removing them.
  2. When a peer starts supporting the DHT protocol, we need to add them to our routing table. Previously, we'd only do the inverse.
  3. Don't add temporary, discovered addresses to our peerstore if we have better ones. That's a very bad idea.

Depends on libp2p/go-libp2p#898

1. When a peer changes their listening addresses, we need to re-run our routing
table filters, possibly removing them.
2. When a peer _starts_ supporting the DHT protocol, we need to add them to our
routing table. Previously, we'd only do the inverse.
They've told us better ones via identify.

func (dht *IpfsDHT) maybeAddAddrs(p peer.ID, addrs []ma.Multiaddr, ttl time.Duration) {
// Don't add addresses for self or our connected peers. We have better ones.
if p == dht.self || dht.host.Network().Connectedness(p) == network.Connected {
Copy link
Contributor

Choose a reason for hiding this comment

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

not adding addresses because the peer is connected seems counter intuitive. when are the peer's addresses added in that case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we don't want someone else to tell us the wrong addresses for the peer, overwriting the good ones in our peerstore.

@Stebalien Stebalien merged commit 09896cd into master Apr 25, 2020
@Stebalien
Copy link
Member Author

@aarshkshah1992 I'd like you to take a look at this after the fact. I think I got this right, but I want to double check.

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.

2 participants