search

vacp2p / nim-libp2p

libp2p implementation in Nim
https://vacp2p.github.io/nim-libp2p/docs/
MIT License
240 stars 52 forks source link

feat(service): add wildcard address resolver #1099

Closed diegomrsantos closed 3 weeks ago

diegomrsantos commented 1 month ago

Summary

This PR adds a service that expands wildcard addresses to their respective interface addresses equivalent. closes #1087

Context

Users of nim-libp2p often listen to wildcard addresses like 0.0.0.0 (IPv4) and [::] (IPv6) to allow the OS to bind the transports to all available network interfaces. This is useful for applications that need to be accessible on multiple networks without specifying each interface.

When listening on wildcard addresses, the OS binds to all available IP addresses, simplifying network configuration but requiring a mechanism to resolve these addresses to their specific network interfaces for communication with other peers.

The listenAddrs field contains addresses the node listens on, which may include wildcard and private addresses (not directly reachable). The addrs field contains resolved addresses that other peers can use to connect, including public-facing NAT and port-forwarded addresses.

Before this PR, nim-libp2p didn't offer a way to resolve wildcard addresses in the addrs field.

This was reported on https://github.com/status-im/nimbus-eth2/issues/6060 and https://github.com/vacp2p/nim-libp2p/issues/1087.

Changes

lchenut commented 1 month ago

Is it possible to add a withWilcardAddressResolverService() function in the switch builder? To have a simple way of creating it. This can work well in addition to withServices(@[ .... ])

diegomrsantos commented 1 month ago

Is it possible to add a withWilcardAddressResolverService() function in the switch builder? To have a simple way of creating it. This can work well in addition to withServices(@[ .... ])

Not a big fan tbh. Maybe we should always add it to the switch.

kaiserd commented 1 month ago

Is it possible to add a withWilcardAddressResolverService() function in the switch builder? To have a simple way of creating it. This can work well in addition to withServices(@[ .... ])

Not a big fan tbh. Maybe we should always add it to the switch.

Is there a reason to model the resolver as a service? Is there a situation in which this service would no be desired? Imo, the switch should offer this per default.

diegomrsantos commented 1 month ago

Is it possible to add a withWilcardAddressResolverService() function in the switch builder? To have a simple way of creating it. This can work well in addition to withServices(@[ .... ])

Not a big fan tbh. Maybe we should always add it to the switch.

Is there a reason to model the resolver as a service? Is there a situation in which this service would no be desired? Imo, the switch should offer this per default.

The motivation was to make it optional. It might be a breaking change if we make it the default behavior tho.

kaiserd commented 1 month ago

The motivation was to make it optional. It might be a breaking change if we make it the default behavior tho.

With our plan to keep only the public marked API as stable in versions 1.x, we could break that.

diegomrsantos commented 1 month ago

The motivation was to make it optional. It might be a breaking change if we make it the default behavior tho.

With our plan to keep only the public marked API as stable in versions 1.x, we could break that.

But I think keeping it as a Service is still nice cause the setup/stop is handled automatically by the Switch.

diegomrsantos commented 1 month ago

The last commits that enabled the service by default were reverted as they caused strange errors when running the tests. Initially, it was nil access errors in the tor transport test. After those were fixed, random tests failed as the transports were not closed properly. This happened when running on my macOS m1. On CI the test would hang as it's possible to see in the commits.

diegomrsantos commented 1 month ago

I added back the wildcard service enabled by default. There were some issues in the tests that I had to fix, but it seems that the weird errors happened cause of what was explained here https://github.com/vacp2p/nim-libp2p/pull/1105.

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 83.47826% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 84.48%. Comparing base (02c96fc) to head (1bb1c1a). Report is 8 commits behind head on master.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1099/graphs/tree.svg?width=650&height=150&src=pr&token=M88zHaQffJ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p)](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1099?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p) ```diff @@ Coverage Diff @@ ## master #1099 +/- ## ========================================== - Coverage 84.53% 84.48% -0.06% ========================================== Files 91 92 +1 Lines 15517 15640 +123 ========================================== + Hits 13118 13213 +95 - Misses 2399 2427 +28 ``` | [Files](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1099?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p) | Coverage Δ | | |---|---|---| | [libp2p/builders.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1099?src=pr&el=tree&filepath=libp2p%2Fbuilders.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL2J1aWxkZXJzLm5pbQ==) | `94.59% <100.00%> (+0.07%)` | :arrow_up: | | [libp2p/multiaddress.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1099?src=pr&el=tree&filepath=libp2p%2Fmultiaddress.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL211bHRpYWRkcmVzcy5uaW0=) | `87.25% <ø> (+0.32%)` | :arrow_up: | | [libp2p/services/autorelayservice.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1099?src=pr&el=tree&filepath=libp2p%2Fservices%2Fautorelayservice.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL3NlcnZpY2VzL2F1dG9yZWxheXNlcnZpY2Uubmlt) | `94.89% <100.00%> (ø)` | | | [libp2p/utility.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1099?src=pr&el=tree&filepath=libp2p%2Futility.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL3V0aWxpdHkubmlt) | `30.39% <100.00%> (+0.39%)` | :arrow_up: | | [libp2p/peerinfo.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1099?src=pr&el=tree&filepath=libp2p%2Fpeerinfo.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL3BlZXJpbmZvLm5pbQ==) | `69.23% <0.00%> (-6.70%)` | :arrow_down: | | [libp2p/services/wildcardresolverservice.nim](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1099?src=pr&el=tree&filepath=libp2p%2Fservices%2Fwildcardresolverservice.nim&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p#diff-bGlicDJwL3NlcnZpY2VzL3dpbGRjYXJkcmVzb2x2ZXJzZXJ2aWNlLm5pbQ==) | `81.13% <82.52%> (ø)` | | ... and [18 files with indirect coverage changes](https://app.codecov.io/gh/vacp2p/nim-libp2p/pull/1099/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vacp2p)