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

feat: use DNS discovery as default bootstrap discovery #1114

Merged
merged 15 commits into from
Jan 31, 2023

Conversation

danisharora099
Copy link
Collaborator

Problem

ref: #517

Solution

moves away from predefined list of nodes, and uses DNS discovery as bootstrap discovery

@danisharora099 danisharora099 marked this pull request as ready for review January 6, 2023 13:41
@danisharora099 danisharora099 changed the title use DNS discovery as default bootstrap discovery feat: use DNS discovery as default bootstrap discovery Jan 6, 2023
@github-actions
Copy link

github-actions bot commented Jan 6, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 132.96 KB (0%) 2.7 s (0%) 2.7 s (-19.05% 🔽) 5.3 s
Waku default setup 424.46 KB (+4.96% 🔺) 8.5 s (+4.96% 🔺) 6.4 s (+19.06% 🔺) 14.9 s
ECIES encryption 44.48 KB (0%) 890 ms (0%) 1.1 s (-36.99% 🔽) 2 s
Symmetric encryption 44.48 KB (0%) 890 ms (0%) 2 s (+37.46% 🔺) 2.9 s
DNS discovery 108.08 KB (+0.24% 🔺) 2.2 s (+0.24% 🔺) 3.3 s (+19.32% 🔺) 5.5 s
Privacy preserving protocols 131.95 KB (0%) 2.7 s (0%) 2.9 s (-20.15% 🔽) 5.5 s
Light protocols 133.97 KB (0%) 2.7 s (0%) 2.6 s (+11.21% 🔺) 5.3 s
History retrieval protocols 133.76 KB (0%) 2.7 s (0%) 3 s (+37.95% 🔺) 5.7 s

Copy link
Collaborator

@fryorcraken fryorcraken left a 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:

  1. Test DNS Discovery with an example
  2. Ask a number of people to use said example to do some dog fooding

@danisharora099
Copy link
Collaborator Author

danisharora099 commented Jan 9, 2023

Are we ready for that?

Todo list because we can consider this change:

1. Test DNS Discovery with an example

I've added 3 tests for the same:

  1. libp2p-compliance-test
  2. passing dnsDiscovery in peerDiscovery with light node
  3. retrieving multiaddrs for test.waku.nodes.status.im

You can find the tests in packages/tests/tests/dns-peer-discovery.spec.ts (ref: #1084)

2. Ask a number of people to use said example to do some dog fooding

Until we release these changes to npm, we can't consume them in our example app unless we build manually. What do you suggest?

@danisharora099 danisharora099 mentioned this pull request Jan 9, 2023
7 tasks
    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
@danisharora099
Copy link
Collaborator Author

I've refactored the tests to be more explicit and added usage of components while initializing the discovery while tagging peers.

@fryorcraken
Copy link
Collaborator

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 web-chat example use it and ask colleagues to use the examples and let us know any feedback.

@danisharora099
Copy link
Collaborator Author

danisharora099 commented Jan 13, 2023

Gotcha. I misinterpreted.

Thanks for explaining. I should be able to integrate it with web-chat once we can release the latest @waku packages to NPM.
Holding off on this PR until then.

packages/dns-discovery/src/index.ts Outdated Show resolved Hide resolved
.cspell.json Outdated Show resolved Hide resolved
packages/create/package.json Show resolved Hide resolved
packages/create/src/index.ts Outdated Show resolved Hide resolved
Comment on lines +92 to +100
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,
}
);
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

packages/tests/tests/dns-peer-discovery.spec.ts Outdated Show resolved Hide resolved
packages/create/src/index.ts Outdated Show resolved Hide resolved
Comment on lines +278 to +281
relay: maxQuantity,
store: maxQuantity,
filter: maxQuantity,
lightPush: maxQuantity,
Copy link
Collaborator

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

Copy link
Collaborator Author

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

@danisharora099 danisharora099 merged commit 11819fc into master Jan 31, 2023
@danisharora099 danisharora099 deleted the feat/dns-disc-default-bootstrap branch January 31, 2023 14:17
@danisharora099
Copy link
Collaborator Author

🥳

This was referenced Feb 9, 2023
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