Bump libp2p for gossipsub improvements #5229
Conversation
|
per vacp2p/nim-libp2p#911 (comment), we should add a (hidden) command-line option for controlling the bandwidth thing (or disabling limiting altogether) in case it turns out to have some unintended side effect |
diegomrsantos
force-pushed
the
bumplibp2pp
branch
from
2b0b4e7
to
6ca12ba
Compare
beacon_chain/conf_light_client.nim
Outdated
| @@ -145,6 +145,11 @@ type LightClientConf* = object | |||
| desc: "A file containing the hex-encoded 256 bit secret key to be used for verifying/generating JWT tokens" | |||
| name: "jwt-secret" .}: Option[InputFile] | |||
|
|
|||
| bandwidthEstimate* {. | |||
| hidden | |||
| desc: "Bandwidth estimate for the node (bytes per second)" | |||
There was a problem hiding this comment.
beacon_chain/conf.nim
Outdated
| bandwidthEstimate* {. | ||
| hidden | ||
| desc: "Bandwidth estimate for the node (bits per second)" | ||
| name: "bandwidth-estimate" .}: Option[Natural] |
There was a problem hiding this comment.
Hmm, should we indicate the unit (bits per second) somehow in the argument name?
From usage, the unit is not obvious, as the default fallback also shows: config.bandwidthEstimate.get(100_000_000)
There was a problem hiding this comment.
I imagine we should in some distant future add a parser for M etc - but this is a hidden parameter that we'll use as an escape hatch in case of problems, ergo it doesn't really matter for now
There was a problem hiding this comment.
Worth explicitly making a --debug parameter, with the associated support (non)guarantees, or is this meant to be an actual end-user escape meant to live indefinitely in its current form, just not documented/readily found?
There was a problem hiding this comment.
we can prefix it with debug-, yeah cc @diegomrsantos
There was a problem hiding this comment.
Likewise the LC version of this command-line option?
There was a problem hiding this comment.
Should it be debug-bandwithEstimate? I can also make it debug-bandwithEstimatebps. I didn't add the unit in the name before cause it's already in the description.
There was a problem hiding this comment.
debug-bandwidth-estimate - we use dash style for arguments throughout, this is for the "name" field
There was a problem hiding this comment.
the variable name is good as is
|
this looks good to go but we should probably not include this in the next release but rather the release after so that it gets some siginificant testing time in the fleet |
Notably includes gossipsub improvements:
Also includes:
ProveFieldwarning incrypto.initvacp2p/nim-libp2p#915