use a basichost, not a blankhost when constructing autonat

[marten-seemann]

Is this safe to do? Our default in NewHost are such that we don't construct any services unless a config option is set for those, so there shouldn't be a big difference between a BasicHost and a BlankHost.

Note that this PR is not sufficient to kill the blankhost. We still use it in tests, and refactoring those will take some more effort (import loops...).

[vyzo]

are we sure there is nothing listening? We need identify only and nothing else.

Also, whats this nil we are passing?

[marten-seemann]

Identify is always added:

go-libp2p/p2p/host/basic/basic_host.go

Lines 223 to 227 in 0f76f17

	if h.disableSignedPeerRecord {
	h.ids, err = identify.NewIDService(h, identify.UserAgent(opts.UserAgent), identify.DisableSignedPeerRecord())
	} else {
	h.ids, err = identify.NewIDService(h, identify.UserAgent(opts.UserAgent))
	}

The nil is a HostOpts:

go-libp2p/p2p/host/basic/basic_host.go

Lines 113 to 155 in 0f76f17

	type HostOpts struct {
	// MultistreamMuxer is essential for the *BasicHost and will use a sensible default value if omitted.
	MultistreamMuxer *msmux.MultistreamMuxer
	// NegotiationTimeout determines the read and write timeouts on streams.
	// If 0 or omitted, it will use DefaultNegotiationTimeout.
	// If below 0, timeouts on streams will be deactivated.
	NegotiationTimeout time.Duration
	// AddrsFactory holds a function which can be used to override or filter the result of Addrs.
	// If omitted, there's no override or filtering, and the results of Addrs and AllAddrs are the same.
	AddrsFactory AddrsFactory
	// MultiaddrResolves holds the go-multiaddr-dns.Resolver used for resolving
	// /dns4, /dns6, and /dnsaddr addresses before trying to connect to a peer.
	MultiaddrResolver *madns.Resolver
	// NATManager takes care of setting NAT port mappings, and discovering external addresses.
	// If omitted, this will simply be disabled.
	NATManager func(network.Network) NATManager
	// ConnManager is a libp2p connection manager
	ConnManager connmgr.ConnManager
	// EnablePing indicates whether to instantiate the ping service
	EnablePing bool
	// EnableRelayService enables the circuit v2 relay (if we're publicly reachable).
	EnableRelayService bool
	// RelayServiceOpts are options for the circuit v2 relay.
	RelayServiceOpts []relayv2.Option
	// UserAgent sets the user-agent for the host.
	UserAgent string
	// DisableSignedPeerRecord disables the generation of Signed Peer Records on this host.
	DisableSignedPeerRecord bool
	// EnableHolePunching enables the peer to initiate/respond to hole punching attempts for NAT traversal.
	EnableHolePunching bool
	// HolePunchingOptions are options for the hole punching service
	HolePunchingOptions []holepunch.Option
	}

.

[vyzo]

can we also neuter the peerstore?

[vyzo]

we should pass a noop one, which we may have to shim.

[marten-seemann]

What do you mean by "neuter"? We need some kind of peerstore, as autonat is first adding the peer info, and then connecting (since the swarm doesn't allow us to dial an address directly).

[vyzo]

ah yes, indeed.

Can we make a variant with minimal overhead that just immediately removes everything when the peer is disconnected?
We dont need a peer manager and we dont care about caching addrs.

[marten-seemann]

Maybe that should be part of autonat? The peerstore exposes a RemovePeer method, so we could call that one.

[vyzo]

that might be the right thing to do, esp if we move the pstoremgr responsibility to the host.

Only concern is we keep addrs for some 10min, as recently connected.

[Stebalien]

What's the motivation? We intentionally use a blank host for better performance. Do we need identify?

[marten-seemann]

What's the motivation? We intentionally use a blank host for better performance.

The motivation is that there shouldn't be a (notable) difference between a blank host and a basic host constructed without any options. For example, we recently had to add support the peer connectedness event to the blank host: libp2p/go-libp2p-blankhost#58, and stuff like this will be coming up and might break us in subtle ways.
That way, we'll be able to move the peerstore manager to the host, which after discussion in that repo (see libp2p/go-libp2p-peerstore#188) is the current plan.

Mid-term, I hope to deprecate the blankhost repo altogether.

Do we need identify?

Good point. We don't. The blankhost doesn't have identify. I've add a DisableIdentify option that we can pass to the basic host.

[Stebalien]

As long as the basic host does literally nothing, I guess that's fine. I'd consider introducing a new base constructor.

I guess my main concerns are:

1. Will we randomly panic if some service we expect isn't there?
2. We should probably avoid calling updateLocalIpAddr (or move this into some singleton service). Getting routing information isn't cheap.

Note: I think the real problem here is that the host is to "fat". Ideally, we'd have a FullHost wrapping a BaseHost where the BaseHost managed base events, and the FullHost had all the services. But we don't have to do that now.

[marten-seemann]

1. Will we randomly panic if some service we expect isn't there?

I think we will, and there seems no good way to disentangle this, other than splitting the basic host into a base and full host, as you suggest.