Merge Autonat-svc #54

willscott · 2020-03-13T03:13:36Z

This PR subsumes the go-libp2p-autonat-svc repository into this one.

It builds on #53 to expose both through functional arguments.
The only functional change is that the dialer used by the autonat-svc is constructed in a bit more tricky of a way to avoid a dependency on go-libp2p.

Note: This PR is structured to import the history from autonat-svc. The final 4 commits are the new changes: https://github.com/libp2p/go-libp2p-autonat/pull/54/files/62f42d93464d523fccc59bb7740975301d7bc1c7..dd12f0a09f839cec420686c40e8161b0a0b0158e

Known remaining work:

Combine the service and client's background goroutines into a single primary event loop
Pass over external interface for which constructors / methods should no longer be public

Extract service implementation from go-libp2p-autonat

gx publish 1.0.1

gx publish 1.0.2

gx publish 1.0.3

gx publish 1.0.4

gx publish 1.0.7

gx publish 1.0.8

gx publish 1.0.12

…ibp2p/go-libp2p-core-0.3.1 Bump github.com/libp2p/go-libp2p-core from 0.3.0 to 0.3.1

use ip.Equal for comparison add test on jitter simplify addrToIP

fix #44

remove global request limit when forced on.

Bumps [github.com/libp2p/go-libp2p-core](https://github.com/libp2p/go-libp2p-core) from 0.3.1 to 0.4.0. - [Release notes](https://github.com/libp2p/go-libp2p-core/releases) - [Commits](libp2p/go-libp2p-core@v0.3.1...v0.4.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

…ibp2p/go-libp2p-core-0.4.0 Bump github.com/libp2p/go-libp2p-core from 0.3.1 to 0.4.0

… feat/delaystart

Limiting autonat service responses/startup

vyzo · 2020-03-13T07:06:11Z

I think this is a mistake; it was done separately precisely to avoid the dependency dance.

willscott · 2020-03-13T07:43:45Z

I think this is a mistake; it was done separately precisely to avoid the dependency dance.

Separating the motivation and the mechanics of dependencies:
I think the main motivations are
(1) less chance of two versions getting out of sync and
(2) It seems like it would be nice for the consumer to say "i would like an autonat subsystem" in a relatively succinct way and have things work.

The current split between the client - which is itself of interest to the DHT and autorelay, and the service that is outside of the libp2p Host isn't the same pattern as network / routing / discovery, and is a bit of an odd interface.

In terms of dependency resolution:

providing an option at creation for the consumer to directly choose a dialer to be used mostly removes the only dependency the service has currently on the upper libp2p repo. The rest of the dance is newDialerFromHost. I think we could arguably simplify this to just attempt the default TCP transport and error if anything else is used without an appropriate dialer passed in.

options.go

svc.go

proto.go

Stebalien · 2020-03-16T08:29:49Z

svc.go

+	ctx         context.Context
+	h           host.Host
+	dialer      network.Dialer
+	dialerStore peerstore.Peerstore


We should just use the one one the host.

Ah, now I see. We still don't need the separate peerstore if we use the network.Network interface.

Stebalien · 2020-03-16T08:30:30Z

svc.go

-	dialer host.Host
+	ctx         context.Context
+	h           host.Host
+	dialer      network.Dialer


These interfaces are a bit wonky and need to be refactored. Let's just use a host.

(or network.Network)

went with just a network.Dialer, since the dialer interface includes .Peerstore.

network.Network embeds the network.Dialer interface.

Stebalien · 2020-03-16T09:04:22Z

@vyzo the context here is:

The two repos are annoying to keep in sync.
Importantly, the service depends on events from the client to determine if it should even turn on in the first place. Otherwise, two nodes behind the same NAT could decide that they're both publicly reachable.

@willscott

So, this is a bit nasty. What if we made the host a required constructor argument (for non-client services)?

We can put tests in a separate autonat_test package and let that package import go-libp2p (circular module dependencies are fine).

Stebalien · 2020-03-17T04:51:56Z

options.go

 // are "its own". Useful for testing, or for more exotic port-forwarding
 // scenarios where the host may be listening on different ports than it wants
 // to externally advertise or verify connectability on.
-func WithAddresses(addrFunc GetAddrs) Option {
+func AddressGuesser(addrFunc AddrFunc) Option {


Hm. I now hate myself, that was a bad suggestion. Can you think of a better alternative?

Stebalien · 2020-03-17T04:53:03Z

options.go

+// default network/dialer of the host passed to `New`, as the NAT system will need to
+// make parallel connections, and as such will modify both the associated peerstore
+// and terminate connections of this dialer. The dialer provided
+// should be compatible (TCP/UDP) however with the transports of the libp2p network.


This isn't strictly speaking necessary. Ideally, the dialer would support dialing all available transports.

Stebalien · 2020-03-17T04:53:16Z

options.go

+func EnableService(dialer network.Network, forceServer bool) Option {
+	return func(c *config) error {
+		if dialer == c.host.Network() || dialer.Peerstore() == c.host.Peerstore() {
+			return errors.New("dialer should not be that of the host")


Stebalien · 2020-03-17T21:46:37Z

autonat.go

@@ -236,6 +255,9 @@ func (as *AmbientAutoNAT) recordObservation(observation autoNATResult) {
 				// we are flipping our NATStatus, so confidence drops to 0
 				as.confidence = 0
 				as.status.Store(observation)
+				if as.serviceCancel != nil {
+					as.serviceCancel()


nit: might as well have as.serviceCancel = nil.

Stebalien · 2020-03-17T21:46:48Z

autonat.go

@@ -252,13 +274,16 @@ func (as *AmbientAutoNAT) recordObservation(observation autoNATResult) {
 		log.Debugf("NAT status is unknown")
 		as.status.Store(autoNATResult{network.ReachabilityUnknown, nil})
 		if currentStatus.Reachability != network.ReachabilityUnknown {
+			if as.serviceCancel != nil {
+				as.serviceCancel()


vyzo and others added 30 commits October 16, 2018 11:59

Initial commit

2d02597

service implementation

3e5abf1

IsPublicAddr lives in multiaddr-net

136d3ca

fix readme bugs

61b34a1

gx package

9813687

gx import go-libp2p-autonat

a87b01c

update test for AutoNATClient constructor change

3b3a6d0

gx update

8c74386

gx publish 1.0.0

9553d46

Merge pull request #1 from libp2p/implementation

1e10777

Extract service implementation from go-libp2p-autonat

gx publish 1.0.1

96e6ccb

Merge pull request #2 from libp2p/gx/update

576f94a

gx publish 1.0.1

gx publish 1.0.2

b74ed6d

Merge pull request #3 from libp2p/gx/update

9694f62

gx publish 1.0.2

update go-libp2p

8e7527d

Merge pull request #4 from libp2p/gx/update

1422874

gx publish 1.0.3

gx publish 1.0.4

5b4147e

Merge pull request #5 from libp2p/update/manet

a84dbb2

gx publish 1.0.4

gx publish 1.0.5

585596f

gx publish 1.0.6

dc1eb4b

gx publish 1.0.7

34ce426

Merge pull request #7 from libp2p/gx/update

066eb88

gx publish 1.0.7

gx publish 1.0.8

f033d2a

Merge pull request #8 from libp2p/gx/update

0f14198

gx publish 1.0.8

gx publish 1.0.9

fee49b4

gx publish 1.0.10

5ff689d

gx publish 1.0.11

2c179c8

gx publish 1.0.12

a5158f4

Merge pull request #11 from libp2p/gx/update-9hkcv6

76cd2be

gx publish 1.0.12

gx publish 1.0.13

77939d4

Stebalien and others added 18 commits February 25, 2020 22:54

Merge pull request #42 from libp2p/dependabot/go_modules/github.com/l…

4d09550

…ibp2p/go-libp2p-core-0.3.1 Bump github.com/libp2p/go-libp2p-core from 0.3.0 to 0.3.1

address review comments

c345bb6

use ip.Equal for comparison add test on jitter simplify addrToIP

Add option for forcing service startup

eb051dd

bump go.mod

310ced9

skip locally held addresses

bd83737

fix #44

Merge branch 'feat/ratelimit' into feat/delaystart

ab09945

Merge branch 'master' into feat/delaystart

4e4e6f2

unadvertise autonat service when private.

7fd9224

remove global request limit when forced on.

force net connection in autonat svc

c1f9c7d

update to single event type

8f02187

Merge pull request #48 from libp2p/dependabot/go_modules/github.com/l…

7e0bdf6

…ibp2p/go-libp2p-core-0.4.0 Bump github.com/libp2p/go-libp2p-core from 0.3.1 to 0.4.0

Merge branch 'master' of github.com:libp2p/go-libp2p-autonat-svc into…

6ef469a

… feat/delaystart

mitigate race in test

183c2c9

additional tests

380bfbc

bump to new go-multiaddr-net

f95ffa9

Merge pull request #45 from libp2p/feat/delaystart

f213600

Limiting autonat service responses/startup

Merge branch 'service' into feat/merged

0f50f0c

willscott requested a review from aarshkshah1992 March 13, 2020 03:16

Stebalien reviewed Mar 16, 2020

View reviewed changes

willscott force-pushed the feat/merged branch 2 times, most recently from 08fc45f to f1cd6ba Compare March 16, 2020 22:12

Stebalien approved these changes Mar 17, 2020

View reviewed changes

Integrate service into autonat

565285b

willscott force-pushed the feat/merged branch from 02b3ad4 to 565285b Compare March 18, 2020 00:08

willscott merged commit 4a4a0ff into master Mar 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge Autonat-svc #54

Merge Autonat-svc #54

willscott commented Mar 13, 2020 •

edited

Loading

vyzo commented Mar 13, 2020

willscott commented Mar 13, 2020

Stebalien Mar 16, 2020

Stebalien Mar 16, 2020

Stebalien Mar 16, 2020

Stebalien Mar 16, 2020

willscott Mar 16, 2020

Stebalien Mar 16, 2020

Stebalien commented Mar 16, 2020

Stebalien Mar 17, 2020

Stebalien Mar 17, 2020

Stebalien Mar 17, 2020

Stebalien Mar 17, 2020

Stebalien Mar 17, 2020

Merge Autonat-svc #54

Merge Autonat-svc #54

Conversation

willscott commented Mar 13, 2020 • edited Loading

vyzo commented Mar 13, 2020

willscott commented Mar 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Mar 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willscott commented Mar 13, 2020 •

edited

Loading