-
Notifications
You must be signed in to change notification settings - Fork 1.1k
docs: add discovery spec for shrex #4516
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## shrex_spec #4516 +/- ##
=============================================
Coverage ? 35.41%
=============================================
Files ? 304
Lines ? 24297
Branches ? 0
=============================================
Hits ? 8605
Misses ? 14745
Partials ? 947 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Wondertan
left a comment
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.
First pass. Please add a link to the rendered markdown to see the rendered diagram.
Directionally, the spec is alright. My main concern is that implementation details are mixed into the part of specification. Particularly, parts that have to be specified in main body of the doc with words are somewhere hidden in implementation details in code, like tags, params and rationale for them.
renaynay
left a comment
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.
We should make it very clear that from a discovery standpoint - full and bridge nodes are identical in the services they can provide to light nodes and that is the rationale for both advertising under full tag. I think even though we specify that full nodes are deprecated, it's a bit confusing to follow in this doc.
Also mention the connectedness of the peers and what kind of connectedness we expect peers to have that we maintain in our limited peer set. Make sure to specify that the criteria for staying in the limited peer set is that it is actively connected to the node at all times, and once that changes, that we kick peer + discover new one that meets connectedness criteria.
specs/src/shrex/discovery.md
Outdated
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.
one thing is we need to mention connectedness status and that the peers in the limited set must active connections - otherwise they must drop the peer for which connectedness changes to anything but connected and discover a new active connection.
renaynay
left a comment
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.
Besides these comments, I think it's pretty good coverage of discovery!
specs/src/shrex/discovery.md
Outdated
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.
sentence here is duplicated
specs/src/shrex/discovery.md
Outdated
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.
The discovery service maintains a limited peer set in which all peers MUST have active connections with the host service. When connectedness changes from active to any other state, the peer MUST be immediately dropped and replaced through new discovery. The dropped peer SHOULD be added to a cache with backoff so it will not be prematurely re-connected to through discovery.
specs/src/shrex/discovery.md
Outdated
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.
we can use active here to be clear, as we use it elsewhere.
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.
removed a limited set everywhere and changed to Active Set in glossary
|
Need to add network level params for dht configuration (per node type etc) |
84408d5 to
b90263a
Compare
specs/src/shrex/discovery.md
Outdated
| DHT Configuration: The kad-dht is configured with default libp2p parameters and operates in different modes based on node type: | ||
|
|
||
| Light Nodes: dht.ModeClient - consume DHT services without serving DHT queries | ||
| Bridge/Full Nodes: dht.ModeServer - both consume and serve DHT queries to support network infrastructure |
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.
When rendered, this looks like a single paragraph. Also the overview is not section to put DHT configuration and modes.
Added a specification for the peer discovery in shrex.
Rendered