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

Bump libp2p for gossipsub improvements #5229

Merged
merged 10 commits into from
Aug 24, 2023
Merged

Bump libp2p for gossipsub improvements #5229

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a7fcec8
Bump libp2p
Menduist Jul 31, 2023
ed08683
Merge branch 'unstable' into bumplibp2pp
arnetheduck Aug 9, 2023
dabf606
Merge branch 'unstable' into bumplibp2pp
diegomrsantos Aug 11, 2023
6ca12ba
Add bandwidthEstimate param
diegomrsantos Aug 14, 2023
d3be909
Merge branch 'unstable' into bumplibp2pp
diegomrsantos Aug 14, 2023
a58552f
Fix typo and type
diegomrsantos Aug 14, 2023
396dd7b
Add config for light client
diegomrsantos Aug 14, 2023
da9d554
Fix unit
diegomrsantos Aug 15, 2023
ea27c56
Add debug prefix
diegomrsantos Aug 15, 2023
23e00cb
Merge branch 'unstable' into bumplibp2pp
diegomrsantos Aug 15, 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
5 changes: 5 additions & 0 deletions beacon_chain/conf.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,11 @@ type
desc: "Experimental, debug option; could disappear at any time without warning"
name: "temporary-debug-trusted-setup-file" .}: Option[string]

bandwidthEstimate* {.
hidden
desc: "Bandwidth estimate for the node (bytes per second)"
name: "bandwidth-estimate" .}: Option[Natural]
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

@arnetheduck arnetheduck Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we can prefix it with debug-, yeah cc @diegomrsantos

Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise the LC version of this command-line option?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

debug-bandwidth-estimate - we use dash style for arguments throughout, this is for the "name" field

Copy link
Member

Choose a reason for hiding this comment

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

the variable name is good as is

Copy link
Contributor

@diegomrsantos diegomrsantos Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Done.


of BNStartUpCmd.wallets:
case walletsCmd* {.command.}: WalletsCmd
of WalletsCmd.create:
Expand Down
5 changes: 5 additions & 0 deletions beacon_chain/conf_light_client.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)"
Copy link
Member

Choose a reason for hiding this comment

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

bytes or bits? https://github.com/status-im/nim-libp2p/pull/941/files#diff-abbf987f19a24c9940cd01bde79a1b1e0819f1c3b6d7ba2efa04ad680d022c78R528 suggests bits

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

name: "bandwidth-estimate" .}: Option[Natural]

# Testing
stopAtEpoch* {.
hidden
Expand Down
2 changes: 2 additions & 0 deletions beacon_chain/networking/eth2_network.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2385,6 +2385,8 @@ proc createEth2Node*(rng: ref HmacDrbgContext,
res.mgetOrPut(peerId, @[]).add(maddress)
info "Adding priviledged direct peer", peerId, address = maddress
res
,
bandwidthEstimatebps: config.bandwidthEstimate.get(100_000_000)
)
pubsub = GossipSub.init(
switch = switch,
Expand Down
2 changes: 1 addition & 1 deletion vendor/nim-libp2p
Open in desktop
Submodule nim-libp2p updated from 224f92 to f80ce3
Loading