Event driven AutoNAT #98

aarshkshah1992 · 2020-11-18T15:29:02Z

AutoNAT server will attempt to successfully dial multiple addresses rather than just one address.
Server groups addresses by (IP addr + transport protocol) and attempts to dial addresses across groups for diversity.
Server will return all addresses that worked and all addresses that failed.
We derive the reachability on the client based on number of successful/failed addresses.

Next PR: Have per address confidence rather than reachability confidence and use the above information to increment/decrement confidence.

willscott · 2020-11-18T16:46:52Z

client.go

 	}

 	if res.GetType() != pb.Message_DIAL_RESPONSE {
-		return nil, fmt.Errorf("Unexpected response: %s", res.GetType().String())
+		return nil, nil, fmt.Errorf("Unexpected response: %s", res.GetType().String())
 	}

 	status := res.GetDialResponse().GetStatus()


can we call res.GetDialResponse() once, and use status / successAddrs / FailedAddrs off that object below?

willscott · 2020-11-18T16:48:50Z

client.go

-		return ma.NewMultiaddrBytes(addr)
+		res := res.GetDialResponse()
+		success = make([]ma.Multiaddr, 0, len(res.SuccessAddrs))
+		for _, ab := range res.SuccessAddrs {


do we have any way to check these addresses are plausibly our own? does this mean someone we ask to dial us can give us a list of thousands of addresses back and either cause us to hold on to a lot of bogus addresses or do work?

We should get confirmation from multiple peers. We'll need to shift from confirming that we're dialable with some confidence to confirming that some set of addresses are dialable with confidence.

Ah, I see that's planned in the next patch. That'll make this easier to review.

willscott · 2020-11-18T16:54:00Z

svc.go

@@ -167,21 +171,83 @@ func (as *autoNATService) doDial(pi peer.AddrInfo) *pb.Message_DialResponse {
 	ctx, cancel := context.WithTimeout(as.ctx, as.config.dialTimeout)
 	defer cancel()

-	as.config.dialer.Peerstore().ClearAddrs(pi.ID)
+	// START DIALING


This logic, while i agree this is a logical approach from the service side, does make our overall reasoning really tricky, and is why we were cautious about doing something like this before:

The client who's asking for dial backs is going to expect that any individual auto nat service isn't going to dial it back on all of it's potential candidate addresses. However, it is also the entity in the position of knowing which addresses it has more confidence on being public/private, and which ones haven't been tested yet.

That means it has more knowledge. and need to prioritize which addresses should be tested. Having there be additional selection logic opaque to the client on which subset of candidates it sends to the service for testing makes the client's life much harder.

as such, it's worth considering: can we have this selection logic happen on the client, where it only sends the candidates that make it through this selection process to the server, and the server tests all of them.

Early on, the autonat service will have more information because it'll be able to see external addresses.

However, I agree in general. We should test addresses in an order specified by the client.

But there may be some room to mix in some additional address guesses. (maybe let the client specify a guess true/false in the request)

willscott · 2020-11-18T16:54:41Z

svc.go

-	as.config.dialer.Peerstore().ClearAddrs(pi.ID)
+	// START DIALING
+	// 1. Aim is to diversify dial attempts across address groups where the grouping key is (IP address + transport protocol(port dosen't matter))
+	// 2. We will continue dialing addresses in a group till we see a successful dial for an address.


The server should have a bounded number of dial attempts it makes for a response. that shouldn't extend the quota of work it's willing to do for a client just because some of the addresses are failing.

aarshkshah1992 · 2020-11-23T05:08:03Z

pb/autonat.proto

@@ -28,7 +28,8 @@ message Message {
  message DialResponse {
    optional ResponseStatus status = 1;
    optional string statusText = 2;
-    optional bytes addr = 3;
+    repeated bytes successAddrs = 3;
+    repeated bytes failedAddrs = 4;


@Stebalien

Since the old servers wont return this information, we will incorrectly mark these addresses as unreachable instead of private.

Umm, how do we make this backward compatible ?

if we only get a single subset address result back we should only record the change in that address, and can try again pinging the server only requesting other addresses?

willscott · 2020-11-23T20:38:21Z

addrset/addrset.go

+)
+
+// AddrState describes the state of a dialable addresses as seen by AutoNAT.
+type AddrState int


can we keep these states this with the network reach-ability ones?

More generally, it seems that there are two axes here that this enum can maybe track separately: those being *source and *confidence in reachability

willscott · 2020-11-23T20:43:25Z

dialed_addrs.go

+func (oa *observedAddr) activated() bool {
+
+	// We only activate if other peers observed the same address
+	// of ours at least 4 times. SeenBy peers are removed by GC if


this seems high? can we activate after 1 with no false negatives, but have a confidence tracking how sure we are?

willscott · 2020-11-23T20:45:23Z

autonat.go

+	for addr, reach := range observation.addrReachability {
+		switch reach {
+		case network.ReachabilityPublic:
+			as.publicAddrsManager.record(observation.observer, addr)


presumably you also want to record inbound dials normally received and not just as a result of autonat? how does that work?

willscott · 2020-11-23T20:47:38Z

pb/autonat.proto

@@ -28,7 +28,8 @@ message Message {
  message DialResponse {
    optional ResponseStatus status = 1;
    optional string statusText = 2;
-    optional bytes addr = 3;
+    repeated bytes successAddrs = 3;
+    repeated bytes failedAddrs = 4;


if we only get a single subset address result back we should only record the change in that address, and can try again pinging the server only requesting other addresses?

willscott · 2020-11-23T20:49:50Z

autonat.go

@@ -364,23 +447,41 @@ func (as *AmbientAutoNAT) tryProbe(p peer.ID) bool {
 }

 func (as *AmbientAutoNAT) probe(pi *peer.AddrInfo) {


as this manager gets more involved, it might be worth splitting the probing / dialing logic (which partially resides in the ephemeral Client) into one file (e.g. "natprobingmanager"), and the tracking event queue ("natstatusmanager") into a different one?

Stebalien

Some initial feedback.

Stebalien · 2020-11-24T05:20:11Z

addrset/addrset.go

+// The state is a set of  multi-addresses, each labeled with a AddrState.
+type DialAddrsSet struct {
+	// all known addresses
+	all []dialAddrState


I'd consider just using a map keyed on string(maddr.Bytes()). The number of allocations should be pretty minimal, relatively.

Stebalien · 2020-11-24T05:36:57Z

autonat.go

+	addrsSet *addrset.DialAddrsSet
+	// returns addresses that have been confirmed to be public by the AutoNAT server
+	publicAddrsManager *dialedAddrsManager
+	// returns addresses that have been confirmed to be private by the AutoNAT server
+	privateAddrsManager *dialedAddrsManager
+
+	// current NAT status and known dialable addresses.
+	status atomic.Value


I haven't dug into this, but it looks like there's going to be quite a bit of duplicate state here:

addrSet will mostly duplicate the information in the addr managers, right?

status just reflects the sum of the information in these sets.

Does this split exist for performance?

Stebalien · 2020-11-24T05:40:44Z

autonat.go

+		if conf.reachability == network.ReachabilityPublic && len(conf.dialAddrs) == 0 {
+			return nil, xerrors.New("dial addrs must be configured if forced reachability is public")
+		}
+


I disagree. "forced public" just means that the user knows that the node will be reachable so we should behave as such. That doesn't mean the user will statically configure addresses ahead of time.

Stebalien · 2020-11-24T05:47:05Z

autonat.go

+						}
+					}
+
+				case event.NetRouteChange:


Is this an address change event? This seems like something else.

Stebalien · 2020-11-24T05:54:54Z

autonat.go

 						as.tryProbe(e.Peer)
 					}
 				}
+
+			case event.EvtDiscoveredAddresses:
+				switch e.Source {


I think these are the right categories (approximately), but I'd make them more about the category/confidence and less about the source:

Configured (user config, maybe that's not even an event) - the user told me.

Interface (listen address) - my computer told me.

Network(?) (NAT, gateway, docker, aws, etc.). - a computer on the network told me.

Reported (observation). - a random computer on the internet told me.

It doesn't really matter that the observation came in from identify, it's an observation. It doesn't really matter if the event was from the NAT port mapping system, or some docker probe, etc.

BigLep · 2021-10-24T04:45:56Z

@aarshkshah1992 : what are the next steps? I'm moving to draft for now.

probe multiple addresses using AutoNAT

c04ecd8

aarshkshah1992 requested review from Stebalien and willscott November 18, 2020 15:29

willscott reviewed Nov 18, 2020

View reviewed changes

aarshkshah1992 added 2 commits November 22, 2020 20:04

draft 1.0

bed9022

doc changes

085a4e1

aarshkshah1992 changed the title ~~Probe multiple addresses using AutoNAT~~ Event driven AutoNAT Nov 23, 2020

aarshkshah1992 changed the title ~~Event driven AutoNAT~~ [WIP: DO NOT REVIEW] Event driven AutoNAT Nov 23, 2020

cleaned up PR

8b26946

aarshkshah1992 mentioned this pull request Nov 23, 2020

Disovered Addresses Event libp2p/go-libp2p-core#171

Open

aarshkshah1992 changed the title ~~[WIP: DO NOT REVIEW] Event driven AutoNAT~~ Event driven AutoNAT Nov 23, 2020

aarshkshah1992 commented Nov 23, 2020

View reviewed changes

willscott reviewed Nov 23, 2020

View reviewed changes

Stebalien reviewed Nov 24, 2020

View reviewed changes

BigLep marked this pull request as draft October 24, 2021 04:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Event driven AutoNAT #98

Event driven AutoNAT #98

aarshkshah1992 commented Nov 18, 2020

willscott Nov 18, 2020

willscott Nov 18, 2020

Stebalien Nov 18, 2020

Stebalien Nov 18, 2020

willscott Nov 18, 2020

Stebalien Nov 18, 2020

willscott Nov 18, 2020

aarshkshah1992 Nov 23, 2020

willscott Nov 23, 2020

willscott Nov 23, 2020

willscott Nov 23, 2020

willscott Nov 23, 2020

willscott Nov 23, 2020

willscott Nov 23, 2020

Stebalien left a comment

Stebalien Nov 24, 2020

Stebalien Nov 24, 2020

Stebalien Nov 24, 2020

Stebalien Nov 24, 2020

Stebalien Nov 24, 2020

BigLep commented Oct 24, 2021

		@@ -364,23 +447,41 @@ func (as *AmbientAutoNAT) tryProbe(p peer.ID) bool {
		}

		func (as AmbientAutoNAT) probe(pi peer.AddrInfo) {

Event driven AutoNAT #98

Are you sure you want to change the base?

Event driven AutoNAT #98

Conversation

aarshkshah1992 commented Nov 18, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a 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

Choose a reason for hiding this comment

BigLep commented Oct 24, 2021