Skip to content

feat(nat): absorb autonat / hole-punching into NATService#2604

Open
gmelodie wants to merge 10 commits into
masterfrom
feat/nat/absorb-autonat
Open

feat(nat): absorb autonat / hole-punching into NATService#2604
gmelodie wants to merge 10 commits into
masterfrom
feat/nat/absorb-autonat

Conversation

@gmelodie

@gmelodie gmelodie commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Move AutoNAT (v1/v2) and hole-punching wiring out of the builder and into
NATService, so withNAT(NATConfig) is the single entry point for all NAT
traversal concerns.

NATConfig is now three independent, optional concerns:

  • portMapping — UPnP / NAT-PMP / explicit-IP, built via upnpConfig,
    natPmpConfig, explicitIpConfig.
  • reachability — AutoNAT v1/v2 probing, built via autonatConfig(version, …).
  • holePunching — DCUtR + AutoRelay + AutoNAT v1, built via holePunchingConfig(…).

withNAT may be called once per distinct concern and merges them; configuring
the same concern twice is a programmer error. reachability and holePunching
are mutually exclusive (hole-punching already drives its own AutoNAT v1) and
pairing them is rejected at setup with ServiceSetupError.

withAutonatV2 and withHolePunching are deprecated — kept as thin wrappers
over withNAT, not removed.

Affected Areas

  • Protocol Logic — NAT, AutoNAT, hole-punching
  • Build / Tooling — builder API surface

Impact on Library Users

Breaking API changes:

  • NATMode is split into PortMappingMode (ExplicitIp/Upnp/NatPmp) and
    AutonatVersion (AutonatV1/AutonatV2). The old Auto mode is gone — an
    empty NATConfig() is the no-op.
  • Construct NATConfig via the helper procs (upnpConfig / natPmpConfig /
    explicitIpConfig / autonatConfig / holePunchingConfig) instead of
    NATConfig(mode: …).
  • withAutonatV2(…) / withHolePunching(…) now emit deprecation warnings;
    migrate to withNAT(autonatConfig(AutonatV2, …)) /
    withNAT(holePunchingConfig(…)).
  • NATService.new now takes rng as a required second argument.
  • PortMapperFactory now receives PortMappingMode instead of NATMode.
  • The live AutoNAT v2 service is exposed via the
    autonatV2Service: Opt[AutonatV2Service] accessor on NATService (derived
    from the active reachability service).

References

Follows on from the feat/nat/upnp-pmp series; replaces the standalone
chore/nat/include-autonat-hole-punching branch.

@gmelodie gmelodie changed the base branch from master to feat/nat/upnp-natpmp-6-tests June 5, 2026 17:54
@gmelodie gmelodie force-pushed the feat/nat/absorb-autonat branch from 6b25e5a to 15de6e7 Compare June 5, 2026 18:02
Base automatically changed from feat/nat/upnp-natpmp-6-tests to master June 5, 2026 18:06
@gmelodie gmelodie force-pushed the feat/nat/absorb-autonat branch from 15de6e7 to 52b122b Compare June 5, 2026 18:12
@gmelodie gmelodie changed the title feat(nat): absorb autonat / hole-punching into NATService feat(nat): absorb autonat / hole-punching into NATService ! Jun 5, 2026
@gmelodie gmelodie marked this pull request as ready for review June 5, 2026 18:18
@gmelodie gmelodie requested review from a team, richard-ramos and vladopajic June 5, 2026 18:18
@codecov-commenter

codecov-commenter commented Jun 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.81967% with 59 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.82%. Comparing base (ad5b9b3) to head (844b15b).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
libp2p/services/natservice.nim 74.45% 59 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2604      +/-   ##
==========================================
- Coverage   84.98%   84.82%   -0.17%     
==========================================
  Files         165      165              
  Lines       28229    28498     +269     
  Branches        7        7              
==========================================
+ Hits        23991    24173     +182     
- Misses       4238     4325      +87     
Files with missing lines Coverage Δ
libp2p/builders.nim 80.07% <100.00%> (-3.64%) ⬇️
libp2p/services/autorelayservice.nim 93.54% <ø> (ø)
libp2p/services/natservice.nim 77.29% <74.45%> (-10.21%) ⬇️

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@richard-ramos richard-ramos 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.

I like the idea of moving NAT traversal setup out of SwitchBuilder. It makes sense that NATService owns this logic, but in general we should be more careful with breaking changes. I think this should be done in a backwards compatible way.

Previously (when discussing the removal of {.public.} pragma, it was said the following:

If we introduce breaking changes or things suddenly stop being backward compatible, lets bump major versions. Minor versions are for backward compatible additions only or just new stuff. And the patch version just for bugs (tbf, it's rarelly used). If we can avoid the breaking change via {.deprecated.} , no need to increment new version

And this is for sure something that we can avoid in this PR. (Let's leave major version updates when they can cause more impact).

I do think as well that the API was made harder to understand with this PR, due to reasons described below

Comment thread libp2p/services/natservice.nim Outdated
Comment thread libp2p/builders.nim Outdated
Comment thread libp2p/services/natservice.nim Outdated
Comment thread libp2p/services/natservice.nim Outdated
Comment thread libp2p/services/natservice.nim Outdated
Comment on lines +359 to +364
if self.config.autonat == Opt.some(AutonatV2):
raise newException(
ServiceSetupError,
"NATService: enableHolePunching currently requires AutoNAT v1; " &
"set NATConfig.autonat to none or some(AutonatV1).",
)

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.

wth. Holepunching should be independent of the autonat version. This seems to be something happening prior to this PR. Seems like we missed it when we were working on Autonat. Not a blocker for this PR but please create a follow up issue.

@gmelodie gmelodie changed the title feat(nat): absorb autonat / hole-punching into NATService ! feat(nat): absorb autonat / hole-punching into NATService Jun 9, 2026
@gmelodie gmelodie force-pushed the feat/nat/absorb-autonat branch 3 times, most recently from 344d57a to b31796e Compare June 9, 2026 13:15
@gmelodie gmelodie force-pushed the feat/nat/absorb-autonat branch from b31796e to ec9cbcd Compare June 9, 2026 13:29
@gmelodie gmelodie force-pushed the feat/nat/absorb-autonat branch from a2828b3 to 5517f5b Compare June 9, 2026 14:23
Comment thread tests/libp2p/services/test_natservice.nim Outdated

@richard-ramos richard-ramos 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.

Image

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR consolidates NAT traversal wiring by moving AutoNAT v1/v2 and hole-punching composition out of SwitchBuilder and into NATService, making withNAT(NATConfig) the single entry point for NAT traversal configuration.

Changes:

  • Refactors NATConfig into three independent optional concerns (portMapping, reachability, holePunching) with helper constructors (upnpConfig, natPmpConfig, explicitIpConfig, autonatConfig, holePunchingConfig) and merge semantics.
  • Updates SwitchBuilder.withNAT to merge distinct concerns across multiple calls; deprecates withAutonatV2/withHolePunching as wrappers over withNAT.
  • Updates tests and C bindings to use the new withNAT(autonatConfig(...)) / NATService accessors.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/libp2p/services/test_natservice.nim Expands NATService test coverage for reachability (AutoNAT v1/v2), HP composition, and merged configs.
tests/libp2p/protocols/test_autonat_v2_service.nim Switch creation now enables AutoNAT v2 via withNAT(autonatConfig(AutonatV2)).
tests/interop/test_autonatv2.nim Interop switch setup updated to use withNAT(autonatConfig(...)) for v2 service configuration.
tests/interop/autonatv2.nim Uses NATService.natService() + autonatV2Service accessor instead of relying on service ordering/casts.
libp2p/services/natservice.nim Core refactor: splits config concerns, wires AutoNAT v1/v2 + HP internally, adds natService and autonatV2Service accessors.
libp2p/services/autorelayservice.nim Exports OnReservationHandler so hole-punching config can reference it via NATService API.
libp2p/builders.nim Removes AutoNAT v2/HP wiring from builder; adds config merging in withNAT; introduces deprecated wrappers.
cbind/libp2p_thread/inter_thread_communication/requests/libp2p_lifecycle_requests.nim C bindings now enable AutoNAT v2 via withNAT(autonatConfig(AutonatV2)).

Comment thread libp2p/services/natservice.nim Outdated
Comment on lines +193 to +204
proc mergeInto*(dst: var NATConfig, src: NATConfig) =
## Fold ``src``'s set concerns into ``dst``; setting one concern twice is a
## programmer error (build-time misuse), so it fails fast with a Defect.
template mergeField(field: untyped) =
src.field.withValue(v):
doAssert dst.field.isNone(),
"withNAT: " & astToStr(field) & " configured more than once"
dst.field = Opt.some(v)

mergeField(portMapping)
mergeField(reachability)
mergeField(holePunching)
@gmelodie gmelodie enabled auto-merge (squash) June 9, 2026 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

5 participants