fix: return the best _acceptable_ conn in NewStream

[Stebalien]

Otherwise, we can return, e.g., a transient connection that can't actually be used.

I noticed bitswap was trying to use transient connections, which shouldn't be possible. To really fix this issue, we'd also need to fix #1603 as well, but that's harder.

[marten-seemann]

Not sure I understand the swarm code correctly. Let's assume we only have a transient conn to a peer. We will therefore call dialPeer:

go-libp2p/p2p/net/swarm/swarm.go

Lines 354 to 369 in 8840285

	if c == nil {
	if nodial, _ := network.GetNoDial(ctx); nodial {
	return nil, network.ErrNoConn
	}
	if dials >= DialAttempts {
	return nil, errors.New("max dial attempts exceeded")
	}
	dials++
	var err error
	c, err = s.dialPeer(ctx, p)
	if err != nil {
	return nil, err
	}
	}

dialPeer will then start a new dial sync (there are currently no active dials), as we only have a transient conn:

go-libp2p/p2p/net/swarm/swarm_dial.go

Lines 251 to 264 in 8840285

	// check if we already have an open (usable) connection first
	conn := s.bestAcceptableConnToPeer(ctx, p)
	if conn != nil {
	return conn, nil
	}
	// apply the DialPeer timeout
	ctx, cancel := context.WithTimeout(ctx, network.GetDialPeerTimeout(ctx))
	defer cancel()
	conn, err = s.dsync.Dial(ctx, p)
	if err == nil {
	return conn, nil
	}

Does that mean we'll end up with two transient connections now?

[Stebalien]

Ah.... ok, this is all broken.

[Stebalien]

Ok, it's "fixed". Horribly broken, but at least it's correct.

[MarcoPolo]

It's a bit confusing that we have two different concepts of "transient" connections in go-libp2p. Transient as in a limited connection over a relay and transient as in a connection that hasn't been upgraded yet and doesn't have a peerid associated with it yet.

[MarcoPolo]

What happens here? We may have a transient connection but we'll try to dial again?

[Stebalien]

Yes. We need to do that for hole-punching.