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

docs(iroh-net): Improve pkarr discovery docs #2722

Merged
merged 4 commits into from
Sep 30, 2024
Merged

docs(iroh-net): Improve pkarr discovery docs #2722

merged 4 commits into from
Sep 30, 2024

Conversation

flub
Copy link
Contributor

@flub flub commented Sep 9, 2024

Description

This tries to improve the pkarr discovery docs by trying to describe
enough about how it works that users do not need to go looking
elsewhere.

Breaking Changes

none

Notes & open questions

There are so many code overlaps and naming inconsistencies here. But this was
part of docs friday so I'm just documenting things for now.

  • Discovery mechanisms are not named particularly consistent:
  • PkarrPublisher is probably a PkarrRelayPublisher
  • PkarrResolver is also a PkarrRelayResolver
  • DnsDiscovery only does resolving -> DnsResolver
  • DhtDiscovery can also use relay server, but at least it does both publish and resolve
  • The DhtDiscovery is pretty generic, it can probably be made to do both replace PkarrResolver and PkarrPublisher, maybe with custom constructors.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • [ ] Tests if relevant.
  • [ ] All breaking changes documented.

This tries to improve the pkarr discovery docs by trying to describe
enough about how it works that users do not need to go looking
elsewhere.
@flub flub requested review from Frando and rklaehn September 9, 2024 10:07
Copy link

github-actions bot commented Sep 9, 2024

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2722/docs/iroh/

Last updated: 2024-09-30T13:00:29Z

@flub
Copy link
Contributor Author

flub commented Sep 12, 2024

@Frando @rklaehn ping

Copy link
Member

@Frando Frando left a comment

Choose a reason for hiding this comment

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

Thank you @flub for writing this sorely missing piece of documentation.
A couple of comments inline.

Also: I agree to your suggested renames above. And if we can combine it all into a single PkarrDiscovery trait with feature flags and a builder that might be even better IMO.

The current situation is very much the result of us adding piece after piece, and not giving things another high-level view I think. So refactoring this into a more coherent picture would be good.

/// The production pkarr relay run by [number 0].
///
/// This server is both a pkarr relay server as well as a DNS resolver, see the [module
/// documentation]. However it does not interact with the Mainline DHT, so is a more
Copy link
Member

Choose a reason for hiding this comment

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

iroh-dns-server can interact with the mainline DHT for resolving node ids not found in the local database, see here. It does not (yet?) support publishing records to mainline DHT.

The config template for production does not enable the mainline support by defualt. IIRC we wanted to enable it on dns.iroh.link though, I do not know OTOH if that is the case. I would guess @Arqu can verify.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Im not sure, lets chat about this on the meeting today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it is currently not enabled let's leave it documented like this. I'm not trying to change anything here, merely documenting the current state.

/// `iroh-dns-server`, e.g. as running on [`N0_DNS_PKARR_RELAY_PROD`], as the home server
/// keeps the records for the domain. When using the pkarr relay no DNS is involved and the
/// setting is ignored.
// TODO(flub): huh?
Copy link
Member

Choose a reason for hiding this comment

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

I also cannot say what the last sentence is supposed to mean.

This paragraph should likely instead explain that this is the timeframe in which, after publishing new node addr info, other nodes might still receive the old outdated info when resolving via DNS.

iroh-net/src/discovery/pkarr.rs Outdated Show resolved Hide resolved
iroh-net/src/discovery/pkarr.rs Outdated Show resolved Hide resolved
@dignifiedquire
Copy link
Contributor

@flub what's missing here to merge this?

@flub flub enabled auto-merge September 30, 2024 13:00
Copy link

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: c5fa3bd

@flub flub added this pull request to the merge queue Sep 30, 2024
Merged via the queue into main with commit a0a8d56 Sep 30, 2024
27 checks passed
@flub flub deleted the flub/discovery-docs branch September 30, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants