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

Lr/vote collector #3615

Merged
merged 9 commits into from
Sep 3, 2024
Merged

Lr/vote collector #3615

merged 9 commits into from
Sep 3, 2024

Conversation

lukaszrzasik
Copy link
Contributor

@lukaszrzasik lukaszrzasik commented Aug 27, 2024

Closes #3564

This PR:

  • Adds a dishonest_voting test that shows the original issue.
  • Adds a wrapper NetworkEventTaskStateModifier which can change NetworkEventTaskState's behaviour for testing purposes.
  • Old consensus, new consensus, DA and Upgrade tasks now store a BTreeMap of vote collector tasks. When a vote with a higher view number arrives, it doesn't overwrite the existing vote collector for a different view. Those collectors can accumulate votes for different views in parallel. When the threshold is reached the collector is removed and all the collector for the older views are removed as well.
  • Creates a helper function handle_vote to avoid code duplication

This PR does not:

Does not change vote collecting for ViewSync tasks as these differ in the vote collecting logic. As far as I understand they are not susceptible to the original issue as well.

Key places to review:

Changes in the crates/task-impls/src/vote_collection.rs file. Especially the garbage collecting logic. I'm not sure about it.
crates/task-impls/src/network.rs file for the new testing wrapper.

How to test this PR:

All CI tests should pass, including the new test.

@lukaszrzasik lukaszrzasik marked this pull request as ready for review August 28, 2024 16:31
Copy link
Contributor

@lukeiannucci lukeiannucci left a comment

Choose a reason for hiding this comment

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

looks good to me

@lukaszrzasik lukaszrzasik merged commit 8299d6a into main Sep 3, 2024
36 checks passed
@lukaszrzasik lukaszrzasik deleted the lr/vote-collector branch September 3, 2024 18:46
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.

[AUDIT] - Old Consensus - fix vote collector spawning
3 participants