Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

routed host: return Connect error if FindPeer doesn't yield new addresses #1946

Merged
merged 1 commit into from
Dec 12, 2022
Merged

routed host: return Connect error if FindPeer doesn't yield new addresses #1946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions p2p/host/routed/routed.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Jorropo marked this conversation as resolved.
Show resolved Hide resolved
}

return nil
}

Expand Down
53 changes: 35 additions & 18 deletions p2p/host/routed/routed_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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()
Expand All @@ -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(),
Expand All @@ -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")
}