The linearize-data contrib script does not quite work correctly in the case of a chain reorg, and it fully-truncates files with an incorrect open mode.
This small patch clips off incorrect trailing data and corrects file open mode type.
(small patch being passed around which I did not write but thought would be helpful to the community)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31888.
See the guideline for information on the review process. A summary of reviews will appear here.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/37309076255
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Lint is failing on the formatting of the commit message:
0[10:12:47.126] The subject line of commit hash 0cf4a6245d2bc8d57f91dd2b4f5bd5a0eed18ed4 is followed by a non-empty line. Subject lines should always be followed by a blank line.
1[10:12:47.126] ^^^
2[10:12:47.126]
3[10:12:47.126] ^---- ⚠️ Failure generated from lint check 'commit_msg' (Check that commit messages have a new line before the body or no body at all.)!
It also looks like the changes somehow break the feature_loadblock.py test, which uses the linearize script
this clips off unwanted final data in event of reorg and fixes the file open
mode to prevent the truncation of files reopened
small patch being passed around which I did not write but thought would be
helpful to the community at large
The changes function to make linearize “less-perturb” on-disk data. Altogether, in the event of a block reorganization, the reorg’d blocks are sometimes shorter than prior blocks’ data in individual output files when split on e.g. size. When the writer potentially moves on to the next blk data file therefore, the rest of the block data that pre-existed the reorganization might need to be truncated off the end of that file before moving on to the next one.
For the “wb” Python open mode, all pre-existing block files as it writes out blk* files are destroyed and re-created by an open with “wb”, which in python will auto-truncate any pre-existing file to 0 bytes—for many large files, something pulling data from the source will always see rapidly-changing source dataset all the time.
With the change, the attempt to verify that the file doesn’t pre-exist and then re-write in-place from 0 will be much less likely to break anything trying to read the files, most of them will effectively never change, and instead we just get the (much less likely) chance of a reorg write racing a potential midstream read, plus the much less-likely truncate-eliminating-a-bit-of-end-file data.
It’s not really perfect when considered in conjunction with other tools, but it’s much better than the current behaviour. Probably some form of atomic file modification would be “better” in the sense of guaranteeing on-disk state consistency, or, perhaps a trigger to external tools to identify “current” consistent state, but in the meanwhile I can’t really see this change hurting because of how shallow reorgs usually are and the inordinate size of the dataset we have to manipulate.
The use case is for people who repeatedly re-run linearize and make use of the results on an ongoing basis.
Consider the following bash script
while true;do ./linearize-hashes.py linearize.cfg > hashlist.txt; ./linearize-data.py linearize.cfg; sleep 120; done
This will continuously write out the bootstrap files, without this patch the files will continuously be truncated to zero length and then re-written, that behavior is undesirable.
My reading from #31888 (comment) seems to give an example of why this pull is needed. However, I haven’t looked closely and reviewed/tested this myself
Huh, yes, I did read that the wrong way around. Will take a look myself then :)
The use case described in that comment is precisely demonstrating a logical outcome of the regressive, drive-destroying behaviour for anyone who wants to use this script more than once, so, I’d thought it was a good example of what the goal of the diff is and didn’t need comment. Does it need further comment?
To be clear, I agree it is a desirable thing to patch and the examples in the discussion here clarify that. I’m just saying that this should be clearly described in the pull request description and the commit message.
Sure, I can flesh it out more if you like and make it more explicit.
Yes, that’s all that’s required to make progress here.
