Remove libp2p multiaddress #8683
Conversation
694abe9 to
00b8194
Compare
ackintosh
left a comment
There was a problem hiding this comment.
Thanks @jxs for the cleanup! ✨ Looks good to me.
One small thought: how about having a deprecation period for the --libp2p-nodes flag to preserve CLI compatibility? We usually do this for CLI changes -- keeping the old flag as an alias, emitting a warning when used ( e.g. the --logfile deprecation in #7723).
pawanjay176
left a comment
There was a problem hiding this comment.
This looks clean. I had no idea we accepted multiaddrs in boot-nodes as well.
Agree with @ackintosh about the deprecation instead of delete. I think some existing infra might depend on the flag.
…overy disabled - Filter multiaddrs to only those with UDP and P2P protocols required for discv5 - Add warning when discovery is disabled but discv5-eligible boot-node multiaddrs are provided
|
@jxs @pawanjay176 Also, I've filed a PR to improve the UX for the case where a user specifies a UDP multiaddr with |
…tosh Filter boot-node multiaddrs for discv5 eligibility
jxs
left a comment
There was a problem hiding this comment.
addressed Akihito's suggestion and re-introduced the flag with deprecation notice, PTAL
83fd1cd to
f1b1b81
Compare
Co-authored-by: Akihito Nakano <sora.akatsuki@gmail.com>
Co-authored-by: Akihito Nakano <sora.akatsuki@gmail.com>
Co-authored-by: Akihito Nakano <sora.akatsuki@gmail.com>
|
Some required checks have failed. Could you please take a look @jxs? 🙏 |
Merge Queue StatusRule:
This pull request spent 13 minutes 18 seconds in the queue, with no time running CI. Required conditions to merge
ReasonPull request #8683 has been dequeued by a HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
|
@mergify dequeue |
✅ The pull request has been removed from the queue |
|
Just quickly dequeueing so I can merge another PR which bypasses mergify: Looks like CI for the book might also be failing? |
yup, thanks Michael. Addressed |
|
@mergify queue |
✅ The pull request has been merged automaticallyDetailsThe pull request has been merged automatically at 0c9f97f |
Merge Queue StatusRule:
This pull request spent 32 minutes 49 seconds in the queue, including 30 minutes 57 seconds running CI. Required conditions to merge
|
After a user reported on Discord reported having problems starting Lighthouse with discovery disabled and providing
--boot-nodesI noticed that when a bootnode is provided as ENR lighthouse connects to it directly using libp2p, when a bootnode is a multiaddress it needs to be a UDP multiaddress so that Lighthouse uses Discovery to fetch that node's ENR.
But then later when bootstrapping libp2p, Lighthouse tries to dial those multiaddresses directly from libp2p if they are TCP, thing is, they won't be, as that was not allowed when the config was parsed.
Lighthouse also supports
libp2p-nodesconfig flag that dials the addresses provided in the same way as when providing a validENR.This PR basically removes the
--libp2p-nodesflag and uniforms onboot-nodesflag allowing users to pass TCP multiaddresses with the--boot-nodesflag