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

Fix unfair IWANT penalty #166

Merged
merged 7 commits into from
Dec 2, 2020
Merged

Fix unfair IWANT penalty #166

merged 7 commits into from
Dec 2, 2020

Conversation

Nashatyrev
Copy link
Collaborator

@Nashatyrev Nashatyrev commented Dec 1, 2020 •
edited
Loading

Fix the following scenario:

  • Node receives unseen IHAVE for message M from peer P1
  • Node sends IWANT M to P1
  • Node receives message M from peer P2 notify the client and waits for its validation
  • While still in process of validation the M is received from P1 (as a response to IWANT)
  • notifyAnyValidMessage is not called for the latter inbound and pending entry is not removed from iWantRequests
  • Peer P1 is penalized for IWANT response timeout

The iWantRequests entry need to be cleared on any message. If a message occurs invalid the peer would be penalized in another way

Also add the test verifying that a long period of Gossip IGNORE (i.e. not publishing any messages) doesn't underscore the node with Eth2 params

ajsutton
ajsutton approved these changes Dec 1, 2020
View reviewed changes
Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

LGTM.

src/test/kotlin/io/libp2p/pubsub/gossip/GossipPubsubRouterTest.kt Outdated Show resolved Hide resolved
src/test/kotlin/io/libp2p/pubsub/gossip/Eth2GossipParams.kt
val AttestTopicPrefix = "/eth2/00000000/beacon_attestation_"
val AttestTopicSuffix = "/ssz_snappy"

val Eth2DefaultGossipParams = GossipParams(
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't find the source for these params to confirm they are all set correctly. But as I understand it at this point they're only used for the test - we'd have to specifically chose to use them on the Teku side before scoring is activated right?

We just need to be cautious about how we enable peer scoring - probably should have a CLI option to toggle it for a while so we can test it really carefully in real world conditions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The source was some draft (I created this file a couple of months ago) from the group investigating Gossip params for Eth2.
Right this is just for testing purposes for now and the precise numbers are not too important here.
Yes, when enabling scoring in Teku we would need to revisit them for sure.

@Nashatyrev @ajsutton
Fix wording
f88afa8 
Co-authored-by: Adrian Sutton <adrian.sutton@consensys.net>
@Nashatyrev Nashatyrev merged commit c514862 into develop Dec 2, 2020
@Nashatyrev Nashatyrev mentioned this pull request Dec 4, 2020
Merged
Nashatyrev added a commit that referenced this pull request Dec 4, 2020
@Nashatyrev
Merge pull request #169 from libp2p/0.6.3
a891d03 
Release 0.6.3
* Get negotiated Gossip protocol version from either inbound or outbound stream (#160)
* Fix unfair IWANT penalty (#166)
* Fix 2 flaky tests, clean up build (#168)
@Nashatyrev Nashatyrev deleted the fix/incorrect-missing-iwant-penalty branch July 4, 2022 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajsutton ajsutton ajsutton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Nashatyrev @ajsutton