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

Simulation branch with one committee fix #402

Merged
merged 3 commits into from
Sep 15, 2023

Conversation

bacv
Copy link
Member

@bacv bacv commented Sep 13, 2023

During research simulation runs tree and branch overlays behave as expected, the only issue was that branch overlay timed out without incrementing the view when it contained single committee. This was not the case for flat and tree overlays.

This change fixes the problem, but to be totally sure, I'd like to ask @0xFugue to rerun all tests with this version and check if branch overlay still correlate to tree overlay results with higher committee count.

@bacv bacv added this to the Simulation App milestone Sep 13, 2023
@bacv bacv self-assigned this Sep 13, 2023
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
consensus-engine/src/overlay/branch_overlay.rs 100.00%
consensus-engine/src/overlay/mod.rs 100.00%

📢 Thoughts on this report? Let us know!.

Copy link
Contributor

@al8n al8n left a comment

Choose a reason for hiding this comment

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

Thanks!

@0xFugue
Copy link

0xFugue commented Sep 13, 2023

Thanks, this does appear to fix the issue. Let me run it over a larger dataset and see if everything works as intended; it would take a day, cheers

@bacv bacv merged commit 70dc4f3 into master Sep 15, 2023
13 checks passed
@bacv bacv deleted the simulation-branch-one-committee branch September 15, 2023 06:10
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.

4 participants