Conversation
jxs
force-pushed
the
remove-libp2p-multiaddress
branch
from
694abe9 to
00b8194
Compare
ackintosh
left a comment
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
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
jxs
left a comment
There was a problem hiding this comment.
addressed Akihito's suggestion and re-introduced the flag with deprecation notice, PTAL
jxs
force-pushed
the
remove-libp2p-multiaddress
branch
from
83fd1cd to
f1b1b81
Compare
|
|
||
| // DEPRECATED: can be removed in v8.2.0./v9.0.0 | ||
| if let Some(libp2p_addresses_str) = cli_args.get_one::<String>("libp2p-addresses") { | ||
| warn!("The --libp2p-nodes flag is deprecated and replaced by --boot-nodes"); |
There was a problem hiding this comment.
| warn!("The --libp2p-nodes flag is deprecated and replaced by --boot-nodes"); | |
| warn!("The --libp2p-addresses flag is deprecated and replaced by --boot-nodes"); |
| .value_name("MULTIADDR") | ||
| .help("One or more comma-delimited multiaddrs to manually connect to a libp2p peer \ | ||
| without an ENR.") | ||
| without an ENR. DEPRECATED. The --libp2p-nodes flag is deprecated and replaced by --boot-nodes") |
There was a problem hiding this comment.
| without an ENR. DEPRECATED. The --libp2p-nodes flag is deprecated and replaced by --boot-nodes") | |
| without an ENR. DEPRECATED. The --libp2p-addresses flag is deprecated and replaced by --boot-nodes") |
| --libp2p-addresses <MULTIADDR> | ||
| One or more comma-delimited multiaddrs to manually connect to a libp2p | ||
| peer without an ENR. | ||
| peer without an ENR. DEPRECATED. The --libp2p-nodes flag is deprecated |
There was a problem hiding this comment.
| peer without an ENR. DEPRECATED. The --libp2p-nodes flag is deprecated | |
| peer without an ENR. DEPRECATED. The --libp2p-addresses flag is deprecated |
3 participants
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