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

swarm: fix selection of transport for dialing #1653

Merged
merged 1 commit into from
Sep 16, 2022
Merged

swarm: fix selection of transport for dialing #1653

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
46 changes: 46 additions & 0 deletions p2p/net/swarm/swarm_addr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,25 @@ package swarm_test

import (
"context"
"fmt"
"testing"

"github.com/libp2p/go-libp2p/core/peer"

ic "github.com/libp2p/go-libp2p/core/crypto"
"github.com/libp2p/go-libp2p/core/peerstore"
"github.com/libp2p/go-libp2p/core/test"
"github.com/libp2p/go-libp2p/p2p/net/swarm"
swarmt "github.com/libp2p/go-libp2p/p2p/net/swarm/testing"
circuitv2 "github.com/libp2p/go-libp2p/p2p/protocol/circuitv2/client"
quic "github.com/libp2p/go-libp2p/p2p/transport/quic"
"github.com/libp2p/go-libp2p/p2p/transport/tcp"
webtransport "github.com/libp2p/go-libp2p/p2p/transport/webtransport"

"github.com/minio/sha256-simd"
ma "github.com/multiformats/go-multiaddr"
"github.com/multiformats/go-multibase"
"github.com/multiformats/go-multihash"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -56,3 +68,37 @@ func TestAddressesWithoutListening(t *testing.T) {
require.NoError(t, err)
require.Empty(t, a1, "expected to be listening on no addresses")
}

func TestDialAddressSelection(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly concerned about the amount of dependencies that this test adds. Not that much of a problem, now that we have a mono-repo, but maybe this would be nicer if we used a mock transport.Transport interface?

priv, _, err := test.RandTestKeyPair(ic.Ed25519, 256)
require.NoError(t, err)
id, err := peer.IDFromPrivateKey(priv)
require.NoError(t, err)
s, err := swarm.NewSwarm("local", nil)
require.NoError(t, err)

tcpTr, err := tcp.NewTCPTransport(nil, nil)
require.NoError(t, err)
require.NoError(t, s.AddTransport(tcpTr))
quicTr, err := quic.NewTransport(priv, nil, nil, nil)
require.NoError(t, err)
require.NoError(t, s.AddTransport(quicTr))
webtransportTr, err := webtransport.New(priv, nil, nil)
require.NoError(t, err)
require.NoError(t, s.AddTransport(webtransportTr))
h := sha256.Sum256([]byte("foo"))
hash, err := multihash.Encode(h[:], multihash.SHA2_256)
require.NoError(t, err)
certHash, err := multibase.Encode(multibase.Base58BTC, hash)
require.NoError(t, err)
circuitTr, err := circuitv2.New(nil, nil)
require.NoError(t, err)
require.NoError(t, s.AddTransport(circuitTr))

require.Equal(t, tcpTr, s.TransportForDialing(ma.StringCast("/ip4/127.0.0.1/tcp/1234")))
require.Equal(t, quicTr, s.TransportForDialing(ma.StringCast("/ip4/127.0.0.1/udp/1234/quic")))
require.Equal(t, circuitTr, s.TransportForDialing(ma.StringCast(fmt.Sprintf("/ip4/127.0.0.1/udp/1234/quic/p2p-circuit/p2p/%s", id))))
require.Equal(t, webtransportTr, s.TransportForDialing(ma.StringCast(fmt.Sprintf("/ip4/127.0.0.1/udp/1234/quic/webtransport/certhash/%s", certHash))))
require.Nil(t, s.TransportForDialing(ma.StringCast("/ip4/1.2.3.4")))
require.Nil(t, s.TransportForDialing(ma.StringCast("/ip4/1.2.3.4/tcp/443/ws")))
}
18 changes: 8 additions & 10 deletions p2p/net/swarm/swarm_transport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,25 +19,23 @@ func (s *Swarm) TransportForDialing(a ma.Multiaddr) transport.Transport {

s.transports.RLock()
defer s.transports.RUnlock()

if len(s.transports.m) == 0 {
// make sure we're not just shutting down.
if s.transports.m != nil {
log.Error("you have no transports configured")
}
return nil
}

for _, p := range protocols {
transport, ok := s.transports.m[p.Code]
if !ok {
continue
}
if transport.Proxy() {
return transport
if isRelayAddr(a) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like this logic here. Instead, we could have transports reject /p2p-circuit address. wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not change this just yet since that would involve changing every transport. Let's revisit after discussing multiformats/multiaddr#140

return s.transports.m[ma.P_CIRCUIT]
}
for _, t := range s.transports.m {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general this needs to go the other way. We should be interpreting this right to left.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. We're iterating over a map[int]transport.Transport here. Inverting the order of iteration shouldn't have any effect, since map iterations are (pseudo-) randomized anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood. I thought we were iterating of the multiaddr components. I see now that we're iterating over all registered transports.

I think this isn't ideal because:

  • we have to make sure every registered transport doesn't intersect with any other transport. It would be tricky behavior if two transports return CanDial -> true for the same a since we would randomly pick one of the two. I think simply a guaranteed ordering here would make it better.
  • This means the transport owns the interpretation of the full multiaddr. This doesn't compose well. Imagine we have a percentencode component and a lenprefixencode component, we don't want every transport to have to know how to read the encoded data from the multiaddr. Instead they should be given the decoded data from something else and deal with their part of the multiaddr. This relates to Interpreting multiaddrs multiformats/multiaddr#140.

I'll draft a patch of what I'm thinking here and how it relates to multiformats/multiaddr#140

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's concretely what I mean: https://github.com/libp2p/go-libp2p/compare/fix-dial-transport-selection...marco/layered-transports?expand=1#diff-1d9e588fd7c4fedee40f7f985be2346b02ba540389251f4603cbc13e8f8fc371R94

Notice this gives us the benefit of removing the "extract certhash" logic out of webtransport completely both in the Dial and the CanDial method (I didn't actually do this refactor, since this is a demo).

if t.CanDial(a) {
return t
}
}

return s.transports.m[protocols[len(protocols)-1].Code]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we have a TransportForComponent function that takes the component and the rest of the multiaddr (and maybe the list of registered transports) and returns the transport to use. This fits with the encapsulation idea of multiaddrs.

This function could be a method of an interface and components could implement this interface. The certhash component would simply return the next transport in the multiaddr along with the certhash parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a layer violation, wouldn't it?
Components are a multiaddr concept. Multiaddr doesn't know anything about transports.

return nil
}

// TransportForListening retrieves the appropriate transport for listening on
Expand Down