-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat: use DNS discovery as default bootstrap discovery #1114
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we ready for that?
Todo list because we can consider this change:
- Test DNS Discovery with an example
- Ask a number of people to use said example to do some dog fooding
I've added 3 tests for the same:
You can find the tests in
Until we release these changes to npm, we can't consume them in our example app unless we build manually. What do you suggest? |
libp2p wasn't by default tagging peers with dns-discovery as "bootstrap" -- we are manually now tagging peers with "dns-discovery", and then running tests according to that
19abac7
to
2fc1246
Compare
I've refactored the tests to be more explicit and added usage of |
Let me rephrase: we need to dogfood dns discovery before we make it the default discovery strategy. To dogfood it, I suggest to make the |
Gotcha. I misinterpreted. Thanks for explaining. I should be able to integrate it with |
peerInfos.forEach(async (peerInfo) => { | ||
await this._components.peerStore.tagPeer( | ||
peerInfo.id, | ||
DEFAULT_BOOTSTRAP_TAG_NAME, | ||
{ | ||
value: this._options.tagValue ?? DEFAULT_BOOTSTRAP_TAG_VALUE, | ||
ttl: this._options.tagTTL ?? DEFAULT_BOOTSTRAP_TAG_TTL, | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these changes? If more modification to dns discovery is necessary then lets' do that and dog food that first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code just tags these peers in the peer store, and doesn't change any functionality
relay: maxQuantity, | ||
store: maxQuantity, | ||
filter: maxQuantity, | ||
lightPush: maxQuantity, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have to pass this? what about if relay
is undefined
it means "return all node with relay".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if undefined is passed for any particular protocol, it initialised it to 0
🥳 |
Problem
ref: #517
Solution
moves away from predefined list of nodes, and uses DNS discovery as bootstrap discovery