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.

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

gs1.1 pruning backoff #95

Merged
merged 2 commits into from
Jun 23, 2020
Merged

gs1.1 pruning backoff #95

merged 2 commits into from
Jun 23, 2020

Conversation

wemeetagain
Copy link
Member

@wemeetagain wemeetagain commented Jun 23, 2020

Implements https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.1.md#prune-backoff-and-peer-exchange prune backoff, not peer exchange

Follows the go-libp2p-pubsub implementation closely.

Extends ControlPrune protobuf message and interface to include "peer info"s and backoff. (For now, only backoff is used)
Adds backoff Map, a map of maps, indexed by topic, then peer, with a value of a timestamp. If a peer sends a graft for a topic while a backoff entry exists, immediately prune, re-extend the backoff period.
Adds a heartbeatTicks counter, will be used for opportunistic grafting in a future PR. Here it's used to clear old backoff entries once every 15 heartbeats.
Adds a _makePrune ControlPrune constructor. For now, the PX section is not there.

@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2020

Codecov Report

Merging #95 into gsv1.1 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           gsv1.1      #95   +/-   ##
=======================================
  Coverage   81.81%   81.81%           
=======================================
  Files           1        1           
  Lines          11       11           
=======================================
  Hits            9        9           
  Misses          2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12b4fa3...378e459. Read the comment docs.

Copy link
Collaborator

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

This looks good. Just a small detail with the 15

ts/index.ts Outdated
*/
_clearBackoff (): void {
// we only clear once every 15 ticks to avoid iterating over the maps too much
if (this.heartbeatTicks % 15 !== 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does the 15 come from? Should this be a constant/configuration option?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, should we reset it instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

15 is just pulled from go-libp2p-pubsub. I'll pull it into a constant.

We can't reset it, at least not here, because other features will be relying on the ticks in a similar way (eg: https://github.com/ChainSafe/js-libp2p-gossipsub/pull/96/files#diff-86c7c8f2b553297cee358fb131db4f7bR239)
In go-libp2p-pubsub, this counter is never reset, even on stop. Shouldn't be a problem for us though, even at a heartbeat every 100ms, we should have over 10 million years before getting out of safe integer range.

Copy link
Collaborator

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants