From 1718bcab6a41fc2edb90a56aa5aa560573028c1a Mon Sep 17 00:00:00 2001 From: gammazero Date: Tue, 22 Feb 2022 08:34:31 -0800 Subject: [PATCH 1/2] Resolve addresses when creating a new stream BasicHost.NewStream will try to establish a connection if one doesn't already exist. This will fail if the hosts addresses have not yet been resolved. This PR resolves the hosts addresses before creating the stream and possible new connection. Fixes #1302 --- p2p/host/basic/basic_host.go | 6 ++++ p2p/host/basic/basic_host_test.go | 46 +++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/p2p/host/basic/basic_host.go b/p2p/host/basic/basic_host.go index 5bcbf5433f..5e55f905b8 100644 --- a/p2p/host/basic/basic_host.go +++ b/p2p/host/basic/basic_host.go @@ -605,6 +605,12 @@ func (h *BasicHost) RemoveStreamHandler(pid protocol.ID) { // to create one. If ProtocolID is "", writes no header. // (Threadsafe) func (h *BasicHost) NewStream(ctx context.Context, p peer.ID, pids ...protocol.ID) (network.Stream, error) { + resolved, err := h.resolveAddrs(ctx, h.Peerstore().PeerInfo(p)) + if err != nil { + return nil, err + } + h.Peerstore().AddAddrs(p, resolved, peerstore.TempAddrTTL) + s, err := h.Network().NewStream(ctx, p) if err != nil { return nil, err diff --git a/p2p/host/basic/basic_host_test.go b/p2p/host/basic/basic_host_test.go index 74671fc818..9b229b4c03 100644 --- a/p2p/host/basic/basic_host_test.go +++ b/p2p/host/basic/basic_host_test.go @@ -6,6 +6,7 @@ import ( "io" "io/ioutil" "reflect" + "strings" "sync" "testing" "time" @@ -439,6 +440,51 @@ func TestNewDialOld(t *testing.T) { require.Equal(t, s.Protocol(), protocol.ID("/testing"), "should have gotten /testing") } +func TestNewStreamResolve(t *testing.T) { + h1, err := NewHost(swarmt.GenSwarm(t), nil) + require.NoError(t, err) + h2, err := NewHost(swarmt.GenSwarm(t), nil) + require.NoError(t, err) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + // Get the tcp port that h2 is listening on. + h2pi := h2.Peerstore().PeerInfo(h2.ID()) + var dialAddr string + const tcpPrefix = "/ip4/127.0.0.1/tcp/" + for _, addr := range h2pi.Addrs { + addrStr := addr.String() + if strings.HasPrefix(addrStr, tcpPrefix) { + port := addrStr[len(tcpPrefix):] + dialAddr = "/dns4/localhost/tcp/" + port + break + } + } + assert.NotEqual(t, dialAddr, "") + + // Add the DNS multiaddr to h1's peerstore. + maddr, err := ma.NewMultiaddr(dialAddr) + require.NoError(t, err) + h1.Peerstore().AddAddr(h2.ID(), maddr, time.Second) + + connectedOn := make(chan protocol.ID) + h2.SetStreamHandler("/testing", func(s network.Stream) { + connectedOn <- s.Protocol() + s.Close() + }) + + // NewStream will make a new connection using the DNS address in h1's + // peerstore. + s, err := h1.NewStream(ctx, h2.ID(), "/testing/1.0.0", "/testing") + require.NoError(t, err) + + // force the lazy negotiation to complete + _, err = s.Write(nil) + require.NoError(t, err) + assertWait(t, connectedOn, "/testing") +} + func TestProtoDowngrade(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() From 42e0c0d1b68270b3ef4739e0021c86b61b60c58f Mon Sep 17 00:00:00 2001 From: gammazero Date: Tue, 15 Mar 2022 14:11:42 -0700 Subject: [PATCH 2/2] Changes from review comments --- p2p/host/basic/basic_host.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/p2p/host/basic/basic_host.go b/p2p/host/basic/basic_host.go index 5e55f905b8..78f4f37c1a 100644 --- a/p2p/host/basic/basic_host.go +++ b/p2p/host/basic/basic_host.go @@ -605,11 +605,16 @@ func (h *BasicHost) RemoveStreamHandler(pid protocol.ID) { // to create one. If ProtocolID is "", writes no header. // (Threadsafe) func (h *BasicHost) NewStream(ctx context.Context, p peer.ID, pids ...protocol.ID) (network.Stream, error) { - resolved, err := h.resolveAddrs(ctx, h.Peerstore().PeerInfo(p)) - if err != nil { - return nil, err + // Ensure we have a connection, with peer addresses resolved by the routing system (#207) + // It is not sufficient to let the underlying host connect, it will most likely not have + // any addresses for the peer without any prior connections. + // If the caller wants to prevent the host from dialing, it should use the NoDial option. + if nodial, _ := network.GetNoDial(ctx); !nodial { + err := h.Connect(ctx, peer.AddrInfo{ID: p}) + if err != nil { + return nil, err + } } - h.Peerstore().AddAddrs(p, resolved, peerstore.TempAddrTTL) s, err := h.Network().NewStream(ctx, p) if err != nil {