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

feat: add iwant request tracking #107

Merged
merged 3 commits into from
Jul 6, 2020
Merged

Conversation

wemeetagain
Copy link
Member

Implements https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.1.md#spam-protection-measures bullet point 4

Follows go-libp2p-pubsub implementation closely

Suffers from issues related to #105
eg: in cases where a peer has signaled that they IHAVE a message, we send a IWANT, they send the message, but don't include a signature on a message when we have strict signing, the peer should be penalized twice, once for the invalid message, and once more if they fail to deliver on their promise to send the IWANTed message.
Right now, they only get the first penalty, from the invalid message.

@wemeetagain wemeetagain force-pushed the cayman/gs1.1-iwant-penalties branch from 2beb0c7 to b3942e4 Compare July 6, 2020 03:43
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           gsv1.1     #107   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files           2        2           
  Lines          24       24           
=======================================
  Hits           22       22           
  Misses          2        2           
Impacted Files Coverage Δ
test/utils/index.js 81.81% <ø> (ø)

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 24b4d1a...b63df8b. 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.

Overall this looks good to me. Just found some minor things that wanted to raise

ts/index.ts Show resolved Hide resolved

/**
* Someone delivered a message, stop tracking promises for it
* @param {string} p peer id
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that we are not using the peerID

Copy link
Member Author

@wemeetagain wemeetagain Jul 6, 2020

Choose a reason for hiding this comment

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

The idea is to have the same function signature as PeerScore#deliverMessage.
So that we might in the future abstract the different 'tracers' as in go-libp2p-pubsub
See https://github.com/libp2p/go-libp2p-pubsub/blob/master/pubsub.go#L972
Eg: they don't call each one individually:

this.score.deliverMessage(p, msg)
this.gossipTracer.deliverMessage(p, msg)
this.tagTracer.deliverMessage(p, msg)

They have some sort of registration for each of their tracers, then all tracers get called on a single call to deliverMessage.
Note the go-libp2p-pubsub tracer functionality is happening on the pubsub layer, not the gossipsub layer. Aggregating our "tracers" together would be a first step towards moving in that direction. Not saying thats what we should do, but aligning function signatures is a prereq.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, sounds good

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

@wemeetagain wemeetagain merged commit 355f1fe into gsv1.1 Jul 6, 2020
@wemeetagain wemeetagain deleted the cayman/gs1.1-iwant-penalties branch July 6, 2020 16:04
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.

3 participants