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. |
| # 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.
In 5ab6419: What is the rationale of the range 150~15000?
There was a problem hiding this comment.
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.
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 |
mzumsande
left a comment
mzumsande
left a comment
There was a problem hiding this comment.
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.
8 participants
Fixes #28603
Added suggested simplifications and implemented randomization