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

Bandwidth estimate as a parameter #941

Merged
merged 3 commits into from
Aug 14, 2023
Merged

Bandwidth estimate as a parameter #941

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0abb75c
Bandwidth estimate as a parameter
diegomrsantos Aug 8, 2023
ba5ba98
Zero bandwidth estimate disables flood-limiting completely
diegomrsantos Aug 10, 2023
a4e8833
Use the param in bps
diegomrsantos Aug 10, 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: 3 additions & 2 deletions libp2p/protocols/pubsub/gossipsub.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ proc init*(_: type[GossipSubParams]): GossipSubParams =
behaviourPenaltyWeight: -1.0,
behaviourPenaltyDecay: 0.999,
disconnectBadPeers: false,
enablePX: false
enablePX: false,
bandwidthEstimateMbps: 100
Copy link
Contributor

Choose a reason for hiding this comment

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

0 should disable flood-limiting completely

Copy link
Contributor

Choose a reason for hiding this comment

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

also, I think it would make more sense for the parameter to be expressed in bps - this way, users can implement a parser that understands G, M, k, etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also, I think it would make more sense for the parameter to be expressed in bps - this way, users can implement a parser that understands G, M, k, etc

Is it common to specify bandwidth in a unit different than Mb? Maybe in Gb sometimes.

Copy link
Contributor

@arnetheduck arnetheduck Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I find it more flexible and easier when API:s use basic units rather than pre-baked multiples - there's more flexibility / future-proofing this way for API users (ie imagine working on an embedded system with more aggressive bandwidth constraints).

This is somewhat different from when working with "normal users" or end-user UX.

)

proc validateParameters*(parameters: GossipSubParams): Result[void, cstring] =
Expand Down Expand Up @@ -522,7 +523,7 @@ method publish*(g: GossipSub,
# but a peer's own messages will always be published to all known peers in the topic, limited
# to the amount of peers we can send it to in one heartbeat
let
bandwidth = 12_500_000 div 1000 # 100 Mbps or 12.5 MBps TODO replace with bandwidth estimate
bandwidth = (g.parameters.bandwidthEstimateMbps * 1_000_000) div 8 div 1000 # 100 Mbps or 12.5 MBps TODO replace with bandwidth estimate
msToTransmit = max(data.len div bandwidth, 1)
maxPeersToFlod =
max(g.parameters.heartbeatInterval.milliseconds div msToTransmit, g.parameters.dLow)
Expand Down
2 changes: 2 additions & 0 deletions libp2p/protocols/pubsub/gossipsub/types.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,8 @@ type
disconnectBadPeers*: bool
enablePX*: bool

bandwidthEstimateMbps*: int

BackoffTable* = Table[string, Table[PeerId, Moment]]
ValidationSeenTable* = Table[MessageId, HashSet[PubSubPeer]]

Expand Down
Loading