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.

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

Multiaddrs with peer ID are not considered dialable #2001

Closed
masih opened this issue Jan 19, 2023 · 2 comments · Fixed by #2007
Closed

Multiaddrs with peer ID are not considered dialable #2001

masih opened this issue Jan 19, 2023 · 2 comments · Fixed by #2007
Labels
effort/hours Estimated to take one or several hours exp/beginner Can be confidently tackled by newcomers good first issue Good issue for new contributors

Comments

@masih
Copy link
Member

masih commented Jan 19, 2023

A multiaddr can optionally contain the peer ID, i.e. ../p2p/<peer-ID>. When such multiaddrs are added to the libp2p host peerstore, they are not considered to be dialable, and filtered on host.Connect resulting in no good addresses error.

For example, the following test fails with error no good addresses:

func TestHost_ConnectWithMultiaddrAndID(t *testing.T) {
	host, err := libp2p.New()
	require.NoError(t, err)
	pid, err := peer.Decode("12D3KooWNSRG5wTShNu6EXCPTkoH7dWsphKAPrbvQchHa5arfsDC")
	require.NoError(t, err)

	addrWithPid, err := multiaddr.NewMultiaddr("/ip4/209.94.92.6/tcp/24001/p2p/" + pid.String())
	require.NoError(t, err)

	host.Peerstore().AddAddr(pid, addrWithPid, peerstore.PermanentAddrTTL)

	addrs := host.Peerstore().Addrs(pid)
	t.Log(addrs)

	err = host.Connect(context.Background(), peer.AddrInfo{ID: pid})
	require.NoError(t, err)
}

While this one succeeds:

func TestHost_ConnectWithMultiaddr(t *testing.T) {
	host, err := libp2p.New()
	require.NoError(t, err)
	pid, err := peer.Decode("12D3KooWNSRG5wTShNu6EXCPTkoH7dWsphKAPrbvQchHa5arfsDC")
	require.NoError(t, err)

	addrWithPid, err := multiaddr.NewMultiaddr("/ip4/209.94.92.6/tcp/24001")
	require.NoError(t, err)

	host.Peerstore().AddAddr(pid, addrWithPid, peerstore.PermanentAddrTTL)

	addrs := host.Peerstore().Addrs(pid)
	t.Log(addrs)

	err = host.Connect(context.Background(), peer.AddrInfo{ID: pid})
	require.NoError(t, err)
}

The only difference being that the multiaddr added to peerstore in the first test contained peer ID.

In the specific case above, looks like the address is being filtered by CanDial of TcpTransport which uses this matcher.

Is this behaviour expected and or documented? Otherwise, it seems surprising that a valid multiaddr being successfully added to the peerstore and yet no good address are found on connection.

@MarcoPolo
Copy link
Collaborator

This is definitely a foot gun that has bitten me as well. Thanks for the issue. Should be a straight forward fix.

@MarcoPolo MarcoPolo added good first issue Good issue for new contributors exp/beginner Can be confidently tackled by newcomers effort/hours Estimated to take one or several hours labels Jan 19, 2023
@sukunrt
Copy link
Member

sukunrt commented Jan 23, 2023

Can I pick this up?

sukunrt added a commit to sukunrt/go-libp2p that referenced this issue Jan 23, 2023
MarcoPolo added a commit that referenced this issue Jan 25, 2023
* Clean addresses with peer id before adding to addrbook

Fixes: #2001

* Filter p2p addresses with invalid peerid

Add error reporting from Host.Connect

* Nits

Co-authored-by: Marco Munizaga <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/hours Estimated to take one or several hours exp/beginner Can be confidently tackled by newcomers good first issue Good issue for new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants