"panic: runtime error: invalid memory address or nil pointer dereference" in p2p/host/autorelay/relay_finder.go:209 if EnableAutoRelay() #1852
Comments
alx696
changed the title
|
Have you changed anything in the code? This is the standard code path, which should be covered quite well by our tests. When does the crash occur? Is it directly after you start the node, or does it take some time? |
|
@marten-seemann Code not changed. Error occurs after the node starts up for a while. This modification upgrades Go and libp2p, op/op.go adjusted according to the new api, other files just formated. |
|
@alx696 Can you please turn on autorelay debug logging? |
|
@marten-seemann I am using go-open-p2p as embedded. I do not know how to set environment variable that time. The error occurs when calling the method . |
Unfortunately, we won't be able to debug this any further without the autorelay log. You can just set the environment variable on your system: Alternatively, in your code: import logging "github.com/ipfs/go-log/v2"
func init() {
logging.SetLogLevel("autorelay", "debug")
} |
|
@marten-seemann Thanks for you reply. I have try run to op/op.go. It looks like there are only 2 more logs: electron: Android: Sorry for not providing useful information. Thanks very much. |
|
Hi there, I am currently facing the same issue. It appears that running with the log level set to debug yields an additional line, but nothing of interest. Setup code I used
func setupHost(ctx context.Context, cfg *P2PNetworkConfig) (host.Host, *dht.IpfsDHT, error) {
cm, err := connmgr.NewConnManager(100, 400)
if err != nil {
return nil, nil, err
}
privKey, err := DeriveIdentity(cfg.PrivateKey)
if err != nil {
return nil, nil, err
}
zap.S().Debug("identity derived")
// Setup libp2p host with custom options
// We will initialize a Kademlia DHT instance in the meantime
var kadDHT *dht.IpfsDHT
h, err := libp2p.New(
libp2p.Identity(privKey),
libp2p.ListenAddrStrings(cfg.ListenAddr),
// Security
// Transport
// Muxer
libp2p.ConnectionManager(cm),
libp2p.NATPortMap(),
libp2p.Routing(func(h host.Host) (routing.PeerRouting, error) {
kadDHT, err = dht.New(
ctx, h,
dht.Mode(dht.ModeServer),
dht.BootstrapPeers(
dht.GetDefaultBootstrapPeerAddrInfos()...,
),
)
return kadDHT, err
}),
// AutoRelay
libp2p.EnableAutoRelay(),
)
return h, kadDHT, err
}The same logic used to work with previous versions of the libp2p infrastructure (see peerchat for an example). There was a recent discussion on the libp2p forums featuring the same error, I'll leave this here in case it is related: https://discuss.libp2p.io/t/minimal-example-for-autonat-autorelay-with-pnet/1589. Even though there has been a huge improvement in the quality of the available documentation, I could not find a minimal example with AutoRelay, which would probably help a lot in this case. |
|
Yeah, it looks like |
|
I’m touching this code today, I can fix this. |
p-shahi
added
kind/bug
|
@marten-seemann this is actually this issue I brought up in Code review: #1587 (comment). We should at the very least panic in the constructor with a useful error. I don't think it's nice to nil pointer panic on users for not having deep knowledge with how autorelay works (it has auto in the name!). The docs around For the time being folks can see how this is used in Kubo You'll need to pass a static list of relays or a way to get relay peers. |
You're right. My bad. The best way to solve this would be to have an error returned by We could split the function into two options, one that requires a list of static relays and one that requires a peer source. Having options, some of which are required, is not a nice API pattern (and this is totally my fault, I wrote it like this). |
I think we should do this |
|
I'll make more explicit that EnableAutoRelay did not need any options originally. The right approach would have been to rename the method so that I get a big error when I build with a new libp2p version (not a panic on initialization!!), and then I can proceed to read the documentation and release notes about why a method is gone etc. |
This is very unhelpful... are we supposed to now have an autorelayFeeder and copy-paste all that code from Kubo? |
|
As a user, I want to have a private network with a variable number of peers where all of them have relay capabilities that are used automatically as needed. I'm thinking that perhaps not just the options changed, but the semantics. How did EnableAutoRelay() work before it required any options? Did it have logic to discover peers by itself? |
Per libp2p/go-libp2p#1852, the AutoRelay subsystem is now panicking on users. EnableAutoRelay must now be called with options, otherwise it seems to panic for some people. Disabling it is the best for now, given relays are enabled and a node must be able to connect to others on bootstrap, perhaps it does not need to re-discover new relays (every other node should be a relay). In any case we should revisit relay support and related services in Cluster, since semantics have changed a lot in libp2p, relayV2 is a thing, hole-punching is a thing etc. etc.
|
@hsanjuan You’re right, we should’ve broken the API when we refactored this. Better late then never, here’s the issue to split the
Previously, peers announced themselves as relays to the DHT, and we’d discover them from there. This was fine with a small number of relays, but obviously doesn’t scale to a network where (almost) all public nodes are relays (as they are with relay v2). |
Makes sense. I'm trying to think if EnableAutoRelay makes sense at all on a small network where all peers are relays though (EnableRelay() is set). Does not using EnableAutoRelay mean that peers will not attempt to use other peers as relays at all? Note that they should be connected to peers that are relays already, so they potentially know that they expose the relay protocol. |
Per libp2p/go-libp2p#1852, the AutoRelay subsystem is now panicking on users. EnableAutoRelay must now be called with options, otherwise it seems to panic for some people. Disabling it is the best for now, given relays are enabled and a node must be able to connect to others on bootstrap, perhaps it does not need to re-discover new relays (every other node should be a relay). In any case we should revisit relay support and related services in Cluster, since semantics have changed a lot in libp2p, relayV2 is a thing, hole-punching is a thing etc. etc.
After update to v0.23.2, an error will occur:
If comment out
libp2p.EnableAutoRelay(), no errors.Version Information
The text was updated successfully, but these errors were encountered: