Skip to content

feat(nameresolving): add system DNS discovery - #3379

Open
richard-ramos wants to merge 6 commits into
masterfrom
feat/system-name-resolver
Open

richard-ramos wants to merge 6 commits into
masterfrom
feat/system-name-resolver

Conversation

@richard-ramos

@richard-ramos richard-ramos commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Add system DNS server discovery from /etc/resolv.conf on Unix and adapter configuration on Windows, with the existing public DNS servers as a fallback. Discovery skips unusable IPv6 link-local servers and down Windows adapters, and retries Windows adapter queries when the required buffer grows.

Keep platform discovery in separate POSIX and Windows modules with a common getNameServers entry point. The common module applies the fallback; dnsresolver re-exports the public discovery API.

Usage

Construct a resolver explicitly with DnsResolver.new(getSystemNameServers()) and pass it to withNameResolver(resolver). The caller retains ownership. DnsResolver has no persistent worker resources to start or stop.

Stack

1 of 3 — merge in this order:

  1. This PR: system DNS discovery; base master.
  2. #3384: system resolver worker pool and factory; base feat/system-name-resolver.
  3. #3385: bounded multiaddress expansion; base feat/system-resolver-backend.

Validation

  • Switch suites: 49 checks passed.
  • Name-resolution suite: 12 checks passed.
  • Nameserver discovery: 5 checks passed.
  • Windows discovery tests cross-compiled successfully with MinGW; Windows runtime tests were not run.

References

Continues the DNS discovery work from #3114, originally authored by @nate-staked. Related: #3378.

@richard-ramos
richard-ramos force-pushed the feat/system-name-resolver branch from a944e56 to 4322731 Compare November 4, 2026 13:36
@richard-ramos richard-ramos changed the title feat(nameresolving): add system resolver and factory feat(nameresolving)!: add switch ownership and system DNS discovery Nov 4, 2026
@richard-ramos

Copy link
Copy Markdown
Collaborator Author

/review

@richard-ramos
richard-ramos added this pull request to stack #3386 November 4, 2026 13:41
@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 6 issue(s) in this PR.

  • ✅ Successfully posted inline: 6 comment(s)

Model: qwen3.8-max
Review generated in: 45m 27s

Comment thread libp2p/builders.nim Resolved
Comment thread libp2p/nameresolving/dnsresolver.nim Resolved
Comment thread libp2p/nameresolving/dnsresolver.nim Resolved
Comment thread libp2p/nameresolving/dnsresolver.nim Resolved
Comment thread libp2p/nameresolving/dnsresolver.nim Resolved
Comment thread libp2p/switch.nim Resolved
@richard-ramos
richard-ramos marked this pull request as ready for review November 4, 2026 15:35
@richard-ramos
richard-ramos requested review from a team, gmelodie and vladopajic November 4, 2026 15:35

@gmelodie gmelodie left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: maybe have linux/windows specific behavior on respective files, and a top-level files calls the common code that chooses which (also, maybe have similar procs be called the same in both)

@vladopajic vladopajic left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

there is to be key principle that Switch / SwitchBuilders are going to follow.

  • when it comes to managing lifecycle of components created by user, they should be one to manage it. Switch should just use the interface it needs. if library does not need lifecycle, for it's own purpose, than library should not care about what user does. library only cares about interface it needs and depends on.

  • building anything with SwitchBuilder's withXXX procs should only set config (they build configuration), and build finally builds everything into Switch. meaning user should not care about types. meaning withNameResolver() will set some property useNameResolver to true, then we create default name resolver. only if we want to allow custom implementations then there could be overloaded withNameResolver(resolver: NameResolverInterface), but in this case they will manage it (start/stop).

Comment thread libp2p/nameresolving/dnsresolver.nim Outdated
Comment on lines +220 to +224
method start*(self: DnsResolver) {.gcsafe.} =
discard

method stop*(self: DnsResolver) {.async: (raises: []).} =
discard

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why is strat/stop added when all code here never use it? all are discard / raiseAssert?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@richard-ramos now i see that next PR in stack has start/stop. so likely all name resolver should have it...

also the way users should use this in SwitchBuilder should be via 2 procs

.withDNSNameResolver( nameServers: seq[TransportAddress] )
.withSystemNameResolver() 

then each of these will create appropriate NameResolver and hook it to Switch. user shouldn't care about state (because it is created by SwithcBuilder).

NameResolver should be treated as a service. So that Switch reuses same logic for starting/stoping.

For example, all NameResolvers could have method .Service() that will provide accessor this name resolver from Service interface, and SwitchBuilder would do services.add(nameResolver.Service()) in addition to what it does in this pr

Comment thread libp2p/nameresolving/dnsresolver.nim Resolved
Comment thread libp2p/switch.nim Resolved
Comment thread tests/libp2p/test_switch_resolver.nim Resolved
@richard-ramos richard-ramos changed the title feat(nameresolving)!: add switch ownership and system DNS discovery feat(nameresolving): add system DNS discovery Nov 5, 2026

This branch has not been deployed

No deployments
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.

3 participants