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
script, assumeutxo: Enhance validations in utxo_snapshot.sh #28852
script, assumeutxo: Enhance validations in utxo_snapshot.sh #28852
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. |
e8a0e63
to
682595f
Compare
682595f
to
9d7a643
Compare
156cd0c
to
579fcae
Compare
579fcae
to
3cb390f
Compare
Concept ACK |
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.
A few suggestions after testing and reviewing 3cb390f.
|
||
function cleanup { | ||
(>&2 echo "Restoring chain to original height; this may take a while") | ||
${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}" |
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.
Note that once invalidateblock
has been called, this won't stop it. It will go all the way back and then forward again.
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.
Same as in the previous PR #27845. We could add more context in the output message and perhaps we could update the documentation as well:
bitcoin/doc/design/assumeutxo.md
Lines 42 to 45 in 5800c55
The utility script | |
`./contrib/devtools/utxo_snapshot.sh` may be of use. | |
By the way, at some point when a bash script gets very complicated, it may be better to convert it to Python. |
I do agree, but so far I think it's manageable. I'm happy to convert it if more reviewers see other issues there. |
- Ensure that the snapshot height is higher than the pruned block height when the node is pruned. - Validate the correctness of the file path and check if the file already exists. - Make network activity disablement optional for the user. - Ensure the reconsiderblock command is triggered on exit, even in the case of user interruption (Ctrl-C). Co-authored-by: Chris Heyes <22148308+hazeycode@users.noreply.github.com> Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
3cb390f
to
11b7269
Compare
Addressed @Sjors's feedback. cc @jamesob as he reviewed #27845, @ryanofsky & @theStack |
tACK 11b7269 |
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.
Concept ACK
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.
tested ACk 11b7269
- Successfully tested error handling added and chainstate can be automatically recovered if an error occurs e.g when the snapshot file already exists or user interruption by
ctrl + c
- Successfully tested the option to enable or disable network activity
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.
Code review ACK 11b7269
# Distributed under the MIT software license, see the accompanying | ||
# file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
# | ||
export LC_ALL=C | ||
|
||
set -ueo pipefail | ||
|
||
NETWORK_DISABLED=false |
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 commit "script: Enhance validations in utxo_snapshot.sh" (11b7269)
Assignment seems a little out of place here, would suggest moving it below PRUNE=...
with other variable assignments.
Note: this actually had 3 acks. DrahtBot didn't count #28852 (review) due to capitalization |
This PR resolves #27841 and some more:
Ensure that the snapshot height is higher than the pruned block height when the node is pruned (Suggested by @Sjors here).
Validate the correctness of the file path and check if the file already exists (@hazeycode's #27845).
Make network activity disablement optional for the user (Suggested by @Sjors here and here).
Ensure the
reconsiderblock
command is triggered on exit (@hazeycode's same PR as above), even in the case of user interruption (Ctrl-C).In order to perform some testing please follow the instructions in the description of previous @hazeycode's PR #27845.