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

Introduce a cap to reputation score increase #4230

Merged
merged 6 commits into from
Aug 10, 2022

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Aug 8, 2022

Signed-off-by: Fabio Di Fabio fabio.difabio@consensys.net

PR description

#4130 introduced an way to increase reputation in case the peer is doing good, but there is no cap to the reputation score the peer can have.

The log shows peers with very high reputation, for example:

FullSyncTargetManager","message":"Caught up to best peer: PeerId 0xb3e02a84270c590f562e2c8b8274930c54b0a00ae001d44bd790b55c3bbc578c70738e6d4a2d32df78b6cf27a071fe54a068f3192c42ac29bd81a6663fe4fa0f, reputation PeerReputation 561, validated? true, disconnected? false, client: Geth/v1.10.21-stable-67109427/linux-amd64/go1.18.5, connection 1312721736, enode enode://b3e02a84270c590f562e2c8b8274930c54b0a00ae001d44bd790b55c3bbc578c70738e6d4a2d32df78b6cf27a071fe54a068f3192c42ac29bd81a6663fe4fa0f@34.92.61.128:29888, chain state: ChainState{estimatedHeight=7369229, bestBlock=BestBlock{totalDifficulty=0x0000000000000000000000000000000000000000000000000000000000a45b44, blockHash=0xb7c2ba396d3f4561fe619796e7ef399080823161f4b831c5b3f48445e7bf13a1, number=7369228}}. Current peers: 3","throwable":""}

since there is no cap to how much the reputation can increase.

I think make sense to have an upper bound, a peer can increase its repution until it reaches the default max again, otherwise a peer that is good for a long time, and start to be useless, will take a long time before its reputation goes below other more recent good peers

Fixed Issue(s)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

having an upper limit makes sense. it would make sense though to have the default score less than the max

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 requested a review from macfarla August 9, 2022 13:17
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

LGTM

@fab-10 fab-10 merged commit 9d476ea into hyperledger:main Aug 10, 2022
@fab-10 fab-10 deleted the peer-reputation-cap branch August 10, 2022 12:04
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants