From 735891d22983e2859815b4c18bcce4be4fe5ba06 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sun, 11 Dec 2022 16:59:38 -0800 Subject: [PATCH] routed host: return Connect error if FindPeer doesn't yield new addresses (#1946) --- p2p/host/routed/routed.go | 9 +++--- p2p/host/routed/routed_test.go | 53 ++++++++++++++++++++++------------ 2 files changed, 39 insertions(+), 23 deletions(-) diff --git a/p2p/host/routed/routed.go b/p2p/host/routed/routed.go index 8f00c25b75..4188d2fcd8 100644 --- a/p2p/host/routed/routed.go +++ b/p2p/host/routed/routed.go @@ -107,8 +107,7 @@ func (rh *RoutedHost) Connect(ctx context.Context, pi peer.AddrInfo) error { // if we're here, we got some addrs. let's use our wrapped host to connect. pi.Addrs = addrs - err := rh.host.Connect(ctx, pi) - if err != nil { + if cerr := rh.host.Connect(ctx, pi); cerr != nil { // We couldn't connect. Let's check if we have the most // up-to-date addresses for the given peer. If there // are addresses we didn't know about previously, we @@ -135,10 +134,10 @@ func (rh *RoutedHost) Connect(ctx context.Context, pi peer.AddrInfo) error { pi.Addrs = newAddrs return rh.host.Connect(ctx, pi) } - - return err + // No appropriate new address found. + // Return the original dial error. + return cerr } - return nil } diff --git a/p2p/host/routed/routed_test.go b/p2p/host/routed/routed_test.go index ccae2fdb58..5c5f8078a9 100644 --- a/p2p/host/routed/routed_test.go +++ b/p2p/host/routed/routed_test.go @@ -7,8 +7,8 @@ import ( "github.com/libp2p/go-libp2p/core/peer" basic "github.com/libp2p/go-libp2p/p2p/host/basic" swarmt "github.com/libp2p/go-libp2p/p2p/net/swarm/testing" + ma "github.com/multiformats/go-multiaddr" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -25,8 +25,6 @@ func (m *mockRouting) FindPeer(ctx context.Context, pid peer.ID) (peer.AddrInfo, } func TestRoutedHostConnectToObsoleteAddresses(t *testing.T) { - ctx := context.Background() - h1, err := basic.NewHost(swarmt.GenSwarm(t), nil) require.NoError(t, err) defer h1.Close() @@ -35,23 +33,19 @@ func TestRoutedHostConnectToObsoleteAddresses(t *testing.T) { require.NoError(t, err) defer h2.Close() - // construct a wrong multi address for host 2, so that - // the initial connection attempt will fail - // (we have obsolete, old multi address information) - maddr, err := ma.NewMultiaddr("/ip4/127.0.0.1/tcp/1234") - require.NoError(t, err) - // assemble the AddrInfo struct to use for the connection attempt pi := peer.AddrInfo{ - ID: h2.ID(), - Addrs: []ma.Multiaddr{maddr}, + ID: h2.ID(), + // Use a wrong multi address for host 2, so that the initial connection attempt will fail + // (we have obsolete, old multi address information) + Addrs: []ma.Multiaddr{ma.StringCast("/ip4/127.0.0.1/tcp/1234")}, } // Build mock routing module and replace the FindPeer function. // Now, that function will return the correct multi addresses for host 2 // (we have fetched the most up-to-date data from the DHT) mr := &mockRouting{ - findPeerFn: func(ctx context.Context, pi peer.ID) (peer.AddrInfo, error) { + findPeerFn: func(context.Context, peer.ID) (peer.AddrInfo, error) { return peer.AddrInfo{ ID: h2.ID(), Addrs: h2.Addrs(), @@ -61,13 +55,36 @@ func TestRoutedHostConnectToObsoleteAddresses(t *testing.T) { // Build routed host rh := Wrap(h1, mr) + // Connection establishment should have worked without an error + require.NoError(t, rh.Connect(context.Background(), pi)) + require.Equal(t, 1, mr.callCount, "the mocked FindPeer function should have been called") +} - // Try to connect - err = rh.Connect(ctx, pi) +func TestRoutedHostConnectFindPeerNoUsefulAddrs(t *testing.T) { + h1, err := basic.NewHost(swarmt.GenSwarm(t), nil) + require.NoError(t, err) + defer h1.Close() - // Connection establishment should have worked without an error - assert.NoError(t, err) + h2, err := basic.NewHost(swarmt.GenSwarm(t), nil) + require.NoError(t, err) + defer h2.Close() + + // assemble the AddrInfo struct to use for the connection attempt + pi := peer.AddrInfo{ + ID: h2.ID(), + // Use a wrong multi address for host 2, so that the initial connection attempt will fail + // (we have obsolete, old multi address information) + Addrs: []ma.Multiaddr{ma.StringCast("/ip4/127.0.0.1/tcp/1234")}, + } - // The mocked FindPeer function should have been called - assert.Equal(t, 1, mr.callCount) + // Build mock routing module and replace the FindPeer function. + // Now, that function will return the correct multi addresses for host 2 + // (we have fetched the most up-to-date data from the DHT) + mr := &mockRouting{findPeerFn: func(context.Context, peer.ID) (peer.AddrInfo, error) { return pi, nil }} + + // Build routed host + rh := Wrap(h1, mr) + // Connection establishment should fail, since we didn't provide any useful addresses in FindPeer. + require.Error(t, rh.Connect(context.Background(), pi)) + require.Equal(t, 1, mr.callCount, "the mocked FindPeer function should have been called") }