-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Test: followups to #27823 #28612
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
Test: followups to #27823 #28612
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
@@ -137,8 +138,8 @@ def check_clean_start(): | |||
# Since the genesis block is not checked by -checkblocks, the | |||
# perturbation window must be chosen such that a higher block | |||
# in blk*.dat is affected. | |||
tf.seek(150) | |||
tf.write(b'1' * 200) | |||
tf.seek(randint (150, 15000)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 5ab6419: What is the rationale of the range 150~15000?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to pick a range large enough for randomization to be efficient, but the range itself is arbitrary and could be adjusted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more clarification @brunoerg the 150 starting point was chosen in the previous test to exclude the genesis block iiuc. The 15000 range should be large enough to provide efficient randomization but small enough to not end outside of the blockfile (200 blocks in the test with ~160 characters each). Could also shorten or enlarge the range ((200*160 = 32000 characters) but I thought 15000 would a good middle ground.
Let me know if you have any suggestions. Can also add a line of documentation if necessary.
Concept ACK, better if someone else does a full review though since part of the changes are from me |
Light ACK 5ab6419 The refactoring changes in the first commit look correct and randomizing offset/size of the perturbation data seems to be an improvement for discovering more issues. Can't say much about the concrete random ranges chosen though, it might make sense to have someone review this that has more insight into the file format or specific cases that were in mind to be triggered (on the other hand, perfect is the enemy of good...). |
lgtm ACK 5ab6419 |
ACK 5ab6419 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran this locally a couple 100 times and the test failed for me maybe 1% of the time - haven't done any deeper analysis though.
Fixes #28603
Added suggested simplifications and implemented randomization