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

fix: deadlock on retrieving WebTransport addresses #9857

Merged
merged 3 commits into from
May 8, 2023
Merged

fix: deadlock on retrieving WebTransport addresses #9857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2ca293
fix: use libp2p.ListenAddrStrings option, apply addresses to mocknet
hacdias May 5, 2023
c5c8544
test: test kubo boot with webtransport
hacdias May 5, 2023
f83c8d1
chore: upgrade go-libp2p to v0.27.2
hacdias May 5, 2023
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
11 changes: 10 additions & 1 deletion core/mock/mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,16 @@ func NewMockNode() (*core.IpfsNode, error) {
}

func MockHostOption(mn mocknet.Mocknet) libp2p2.HostOption {
return func(id peer.ID, ps pstore.Peerstore, _ ...libp2p.Option) (host.Host, error) {
return func(id peer.ID, ps pstore.Peerstore, opts ...libp2p.Option) (host.Host, error) {
var cfg libp2p.Config
if err := cfg.Apply(opts...); err != nil {
return nil, err
}

// The mocknet does not use the provided libp2p.Option. This options include
// the listening addresses we want our peer listening on. Therefore, we have
// to manually parse the configuration and add them here.
ps.AddAddrs(id, cfg.ListenAddrs, pstore.PermanentAddrTTL)
hacdias marked this conversation as resolved.
Show resolved Hide resolved
return mn.AddPeerWithPeerstore(id, ps)
}
}
Expand Down
2 changes: 1 addition & 1 deletion core/node/groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ func LibP2P(bcfg *BuildCfg, cfg *config.Config, userResourceOverrides rcmgr.Part
fx.Provide(libp2p.RelayTransport(enableRelayTransport)),
fx.Provide(libp2p.RelayService(enableRelayService, cfg.Swarm.RelayService)),
fx.Provide(libp2p.Transports(cfg.Swarm.Transports)),
fx.Invoke(libp2p.StartListening(cfg.Addresses.Swarm)),
fx.Provide(libp2p.ListenOn(cfg.Addresses.Swarm)),
fx.Invoke(libp2p.SetupDiscovery(cfg.Discovery.MDNS.Enabled)),
fx.Provide(libp2p.ForceReachability(cfg.Internal.Libp2pForceReachability)),
fx.Provide(libp2p.HolePunching(cfg.Swarm.EnableHolePunching, enableRelayClient)),
Expand Down
38 changes: 6 additions & 32 deletions core/node/libp2p/addrs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"fmt"

"github.com/libp2p/go-libp2p"
"github.com/libp2p/go-libp2p/core/host"
p2pbhost "github.com/libp2p/go-libp2p/p2p/host/basic"
ma "github.com/multiformats/go-multiaddr"
mamask "github.com/whyrusleeping/multiaddr-filter"
Expand Down Expand Up @@ -99,37 +98,12 @@ func AddrsFactory(announce []string, appendAnnouce []string, noAnnounce []string
}
}

func listenAddresses(addresses []string) ([]ma.Multiaddr, error) {
listen := make([]ma.Multiaddr, len(addresses))
for i, addr := range addresses {
maddr, err := ma.NewMultiaddr(addr)
if err != nil {
return nil, fmt.Errorf("failure to parse config.Addresses.Swarm: %s", addresses)
}
listen[i] = maddr
}

return listen, nil
}

func StartListening(addresses []string) func(host host.Host) error {
return func(host host.Host) error {
listenAddrs, err := listenAddresses(addresses)
if err != nil {
return err
}

// Actually start listening:
if err := host.Network().Listen(listenAddrs...); err != nil {
return err
}

// list out our addresses
addrs, err := host.Network().InterfaceListenAddresses()
if err != nil {
return err
func ListenOn(addresses []string) interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we're using the libp2p constructor now to pass listen addresses. However, I'm a little nervous about there being ancient race conditions that emerge here 1a3752b.

Hopefully this shouldn't be a problem any more given that it's highly unintuitive behavior and that if people used those code paths without copying kubo code they'd unlikely do the same thing as was done here.

@Jorropo you've been looking through the Bitswap code more recently than most any thoughts on if those race conditions are likely still present?

Note: lotus probably has a similar issue here https://github.com/filecoin-project/lotus/blob/e6c8072f505f724f869861ca0430101fc87286c0/node/modules/lp2p/addrs.go#L84 that should be resolved when they upgrade go-libp2p in filecoin-project/lotus#10671.

return func() (opts Libp2pOpts) {
return Libp2pOpts{
Opts: []libp2p.Option{
libp2p.ListenAddrStrings(addresses...),
},
}
log.Infof("Swarm listening at: %s", addrs)
Copy link
Member Author

@hacdias hacdias May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is this important? We already print Swarm listening on in daemon.go:769. Usage of log.Infof instead of fmt.Println seems to be the difference here, and this wouldn't be printed by default unless the user has logging enabled. If there is a reason to keep this, I suggest the following:

diff --git a/core/node/libp2p/hostopt.go b/core/node/libp2p/hostopt.go
index 74d6e5723..f8bdbed30 100644
--- a/core/node/libp2p/hostopt.go
+++ b/core/node/libp2p/hostopt.go
@@ -20,5 +20,11 @@ func constructPeerHost(id peer.ID, ps peerstore.Peerstore, options ...libp2p.Opt
 		return nil, fmt.Errorf("missing private key for node ID: %s", id.Pretty())
 	}
 	options = append([]libp2p.Option{libp2p.Identity(pkey), libp2p.Peerstore(ps)}, options...)
-	return libp2p.New(options...)
+	h, err := libp2p.New(options...)
+	if err != nil {
+		return nil, err
+	}
+
+	log.Infof("Swarm listening at: %s", h.Network().ListenAddresses())
+	return h, nil
 }

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can tell we shouldn't need this log info any more given we print it out on daemon run, unless there is code only reading from the log. From what I can tell this writing the listen addresses twice has been there for a very long time with no note indicating this was intentional in #3948.

I think if we want to keep the log there putting it where @hacdias suggested is reasonable, or we could move it next to the Printf so that it's more obvious what's going on.

return nil
}
}
2 changes: 1 addition & 1 deletion docs/examples/kubo-as-a-library/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ replace github.com/ipfs/kubo => ./../../..
require (
github.com/ipfs/boxo v0.8.2-0.20230503105907-8059f183d866
github.com/ipfs/kubo v0.0.0-00010101000000-000000000000
github.com/libp2p/go-libp2p v0.27.1
github.com/libp2p/go-libp2p v0.27.2
github.com/multiformats/go-multiaddr v0.9.0
)

Expand Down
4 changes: 2 additions & 2 deletions docs/examples/kubo-as-a-library/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,8 +489,8 @@ github.com/libp2p/go-flow-metrics v0.0.1/go.mod h1:Iv1GH0sG8DtYN3SVJ2eG221wMiNpZ
github.com/libp2p/go-flow-metrics v0.0.3/go.mod h1:HeoSNUrOJVK1jEpDqVEiUOIXqhbnS27omG0uWU5slZs=
github.com/libp2p/go-flow-metrics v0.1.0 h1:0iPhMI8PskQwzh57jB9WxIuIOQ0r+15PChFGkx3Q3WM=
github.com/libp2p/go-flow-metrics v0.1.0/go.mod h1:4Xi8MX8wj5aWNDAZttg6UPmc0ZrnFNsMtpsYUClFtro=
github.com/libp2p/go-libp2p v0.27.1 h1:k1u6RHsX3hqKnslDjsSgLNURxJ3O1atIZCY4gpMbbus=
github.com/libp2p/go-libp2p v0.27.1/go.mod h1:FAvvfQa/YOShUYdiSS03IR9OXzkcJXwcNA2FUCh9ImE=
github.com/libp2p/go-libp2p v0.27.2 h1:I1fxqxdm/O0TFoAZKje8wSMu9tfLlLdzTQvgT3HA6v0=
github.com/libp2p/go-libp2p v0.27.2/go.mod h1:FAvvfQa/YOShUYdiSS03IR9OXzkcJXwcNA2FUCh9ImE=
github.com/libp2p/go-libp2p-asn-util v0.3.0 h1:gMDcMyYiZKkocGXDQ5nsUQyquC9+H+iLEQHwOCZ7s8s=
github.com/libp2p/go-libp2p-asn-util v0.3.0/go.mod h1:B1mcOrKUE35Xq/ASTmQ4tN3LNzVVaMNmq2NACuqyB9w=
github.com/libp2p/go-libp2p-core v0.2.4/go.mod h1:STh4fdfa5vDYr0/SzYYeqnt+E6KfEV5VxfIrm0bcI0g=
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ require (
github.com/jbenet/goprocess v0.1.4
github.com/julienschmidt/httprouter v1.3.0
github.com/libp2p/go-doh-resolver v0.4.0
github.com/libp2p/go-libp2p v0.27.1
github.com/libp2p/go-libp2p v0.27.2
github.com/libp2p/go-libp2p-http v0.5.0
github.com/libp2p/go-libp2p-kad-dht v0.23.0
github.com/libp2p/go-libp2p-kbucket v0.5.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,8 +540,8 @@ github.com/libp2p/go-flow-metrics v0.0.1/go.mod h1:Iv1GH0sG8DtYN3SVJ2eG221wMiNpZ
github.com/libp2p/go-flow-metrics v0.0.3/go.mod h1:HeoSNUrOJVK1jEpDqVEiUOIXqhbnS27omG0uWU5slZs=
github.com/libp2p/go-flow-metrics v0.1.0 h1:0iPhMI8PskQwzh57jB9WxIuIOQ0r+15PChFGkx3Q3WM=
github.com/libp2p/go-flow-metrics v0.1.0/go.mod h1:4Xi8MX8wj5aWNDAZttg6UPmc0ZrnFNsMtpsYUClFtro=
github.com/libp2p/go-libp2p v0.27.1 h1:k1u6RHsX3hqKnslDjsSgLNURxJ3O1atIZCY4gpMbbus=
github.com/libp2p/go-libp2p v0.27.1/go.mod h1:FAvvfQa/YOShUYdiSS03IR9OXzkcJXwcNA2FUCh9ImE=
github.com/libp2p/go-libp2p v0.27.2 h1:I1fxqxdm/O0TFoAZKje8wSMu9tfLlLdzTQvgT3HA6v0=
github.com/libp2p/go-libp2p v0.27.2/go.mod h1:FAvvfQa/YOShUYdiSS03IR9OXzkcJXwcNA2FUCh9ImE=
github.com/libp2p/go-libp2p-asn-util v0.3.0 h1:gMDcMyYiZKkocGXDQ5nsUQyquC9+H+iLEQHwOCZ7s8s=
github.com/libp2p/go-libp2p-asn-util v0.3.0/go.mod h1:B1mcOrKUE35Xq/ASTmQ4tN3LNzVVaMNmq2NACuqyB9w=
github.com/libp2p/go-libp2p-core v0.2.4/go.mod h1:STh4fdfa5vDYr0/SzYYeqnt+E6KfEV5VxfIrm0bcI0g=
Expand Down
4 changes: 2 additions & 2 deletions test/cli/harness/peering.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ type Peering struct {
To int
}

func newRandPort() int {
func NewRandPort() int {
n := rand.Int()
return 3000 + (n % 1000)
}
Expand All @@ -24,7 +24,7 @@ func CreatePeerNodes(t *testing.T, n int, peerings []Peering) (*Harness, Nodes)
nodes.ForEachPar(func(node *Node) {
node.UpdateConfig(func(cfg *config.Config) {
cfg.Routing.Type = config.NewOptionalString("none")
cfg.Addresses.Swarm = []string{fmt.Sprintf("/ip4/127.0.0.1/tcp/%d", newRandPort())}
cfg.Addresses.Swarm = []string{fmt.Sprintf("/ip4/127.0.0.1/tcp/%d", NewRandPort())}
})

})
Expand Down
23 changes: 23 additions & 0 deletions test/cli/transports_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cli

import (
"fmt"
"os"
"path/filepath"
"testing"
Expand Down Expand Up @@ -124,4 +125,26 @@ func TestTransports(t *testing.T) {
runTests(nodes)
})

t.Run("QUIC connects with non-dialable transports", func(t *testing.T) {
// This test targets specific Kubo internals which may change later. This checks
// if we can announce an address we do not listen on, and then are able to connect
// via a different address that is available.
t.Parallel()
nodes := harness.NewT(t).NewNodes(5).Init()
nodes.ForEachPar(func(n *harness.Node) {
n.UpdateConfig(func(cfg *config.Config) {
// We need a specific port to announce so we first generate a random port.
// We can't use 0 here to automatically assign an available port because
// that would only work with Swarm, but not for the announcing.
port := harness.NewRandPort()
quicAddr := fmt.Sprintf("/ip4/127.0.0.1/udp/%d/quic-v1", port)
cfg.Addresses.Swarm = []string{quicAddr}
cfg.Addresses.Announce = []string{quicAddr, quicAddr + "/webtransport"}
})
})
disableRouting(nodes)
nodes.StartDaemons().Connect()
runTests(nodes)
})

}