Skip to content

fix(sync-v2): Stop streaming if current block becomes voided #810

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

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

msbrogli
Copy link
Member

@msbrogli msbrogli commented Oct 13, 2023

Motivation

Fix one of the conditions that makes the tests/simulation/test_simulator.py::SyncV2RandomSimulatorTestCase::test_many_miners_since_beginning flaky.

If we do not stop the streaming, the later call to get_next_block_best_chain() will raise an assertion error since the next block will be voided.

Acceptance Criteria

  1. Stop streaming blocks if the current block gets voided.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@msbrogli msbrogli requested review from jansegre and glevco October 13, 2023 05:31
@msbrogli msbrogli self-assigned this Oct 13, 2023
Copy link
Member

@jansegre jansegre left a comment

Choose a reason for hiding this comment

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

Does this really improve it? The test matrix failed in all but one job (at least on the first run). Also I think we've discussed this before, we previously had this behavior (stopping a stream when it reaches a voided block) but we changed it, and I don't remember why.

@glevco
Copy link
Contributor

glevco commented Oct 13, 2023

@jansegre maybe it helped with one test, but others failed in the run. One way to check is this adding, for example, @flaky(min_passes=20, max_runs=20) to force the supposedly fixed test to run 20 times and pass all of them. We can run this locally just to check if all runs are passing (but keep it out of the PR).

@msbrogli msbrogli merged commit 45c7ba5 into master Oct 17, 2023
@msbrogli msbrogli deleted the fix/sync-v2-streamer-voided-block branch October 17, 2023 18:31
This was referenced Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants