Skip to content

Conversation

L0laL33tz
Copy link
Contributor

Fixes #28603

Added suggested simplifications and implemented randomization

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 8, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theStack, maflcko, achow101
Concept ACK fjahr

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))
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@fjahr
Copy link
Contributor

fjahr commented Oct 10, 2023

Concept ACK, better if someone else does a full review though since part of the changes are from me

@fanquake fanquake requested a review from theStack November 1, 2023 10:41
@theStack
Copy link
Contributor

theStack commented Nov 6, 2023

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...).

@DrahtBot DrahtBot requested review from fjahr and removed request for theStack November 6, 2023 15:09
@maflcko
Copy link
Member

maflcko commented Nov 6, 2023

lgtm ACK 5ab6419

@achow101
Copy link
Member

achow101 commented Nov 6, 2023

ACK 5ab6419

@achow101 achow101 merged commit 0387ca0 into bitcoin:master Nov 6, 2023
Copy link
Contributor

@mzumsande mzumsande left a 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.

@bitcoin bitcoin locked and limited conversation to collaborators Nov 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend feature_init.py file perturbations
8 participants