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

Fix typo incrementing instead of decrementing #595

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

clostao
Copy link

@clostao clostao commented Dec 5, 2024

There is an error in MostSeen::remove method.

The issue is that for determining the label of the network it's used a voting-like system in which when nodes start sending telemetry logs also send its network information (genesis hash, network name, etc.). For each genesis hash the network name that has been sent the most times is the one to be shown.

The issue was that when a node lost connection with telemetry instead of decreasing the number of votes from the network name the node voted the service was increasing the number of votes, leading to a corruption of the voting. With the current fix, as long as the number of nodes sending the correct network name remains higher from those sending a wrong name, the correct label should be shown.

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Dec 5, 2024

User @clostao, please sign the CLA here.

@jsdw
Copy link
Collaborator

jsdw commented Dec 5, 2024

This tracks with the comment too; thanks for your fix @clostao

This bug requires a specific set of circumstances to trigger
and become visible:
1. decrement a label that is non-zero but not first
2. repeat the decrement at least best_count()/2 times
3. increment that label
4. check the best count
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Dec 5, 2024

User @teor2345, please sign the CLA here.

@teor2345
Copy link

teor2345 commented Dec 5, 2024

This bug got through because the current tests don’t check the value of a label that isn’t the best, which is removed multiple times, then inserted again. I’ve added a test case which would trigger this bug.

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

nice catch, just sign the CLA bot stuff then it should be ready to go

@teor2345
Copy link

teor2345 commented Dec 9, 2024

nice catch, just sign the CLA bot stuff then it should be ready to go

Done! Just took us a few days to get work approval 🙂

@niklasad1 niklasad1 requested a review from a team December 9, 2024 07:40
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.

4 participants