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: remove old addresses in identify immediately #953

Merged
merged 2 commits into from May 28, 2020

Conversation

Stebalien
Copy link
Member

Previously, we'd keep addresses discovered through the DHT for up to 2 minutes (temporary TTL) and previously seen addresses (recently connected) for up to 10 minutes (the TTL).

  1. Make sure to downgrade both connected and recently connected addresses to the "temporary" ttl before adding new addresses.
  2. Finally, downgrade addresses with the temporary TTL to 0.

This could be more efficient with a better peerstore abstraction, but this is better than nothing.

Previously, we'd keep addresses discovered through the DHT for up to 2
minutes (temporary TTL) and previously seen addresses (recently connected) for
up to 10 minutes (the TTL).

1. Make sure to downgrade both connected and recently connected addresses to the
"temporary" ttl before adding new addresses.
2. Finally, downgrade addresses with the temporary TTL to 0.

This could be more efficient with a better peerstore abstraction, but this is
better than nothing.
Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

Generally makes sense, just left a clarification question on the test changes to make sure I understand them.

Comment on lines +83 to 84
testKnowsAddrs(t, h1, h2p, h2.Addrs()) // has them
testHasCertifiedAddrs(t, h1, h2p, h2.Peerstore().Addrs(h2p)) // should have signed addrs also
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand all the test changes. Why are we sometimes using h.Addrs() and sometimes using h.Peerstore().Addrs(h.ID)?

@Stebalien Stebalien merged commit 294ba3d into master May 28, 2020
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.

None yet

2 participants