script: utxo_snapshot.sh error handling #27845
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. |
eaa9335 to
4bdbd1a
Compare
4bdbd1a to
5e35de4
Compare
|
Thanks for working on this! crACK |
|
I see you've got some lint failures in CI; you can run linters (faithfully) locally using the container added here: https://github.com/bitcoin/bitcoin/pull/26906/files |
5e35de4 to
39aa958
Compare
|
ACK 39aa958 But I think you can just squash these two commits down into one. |
39aa958 to
10d9bef
Compare
|
Lightly tested ACK 10d9bef |
There was a problem hiding this comment.
cr ACK.
I was working on this file this week and when I was about to create a PR I found this thankfully, and thanks for working on this, I had the described issues with this script and it's important to fix it.
If you want to link this PR to the issue that it resolves please consider to add some of these keywords in the PR description following by the issue #, and you could remove the reference from the commit body.
I leave a couple of suggestions regarding the code.
| # Early exit if file at OUTPUT_PATH already exists | ||
| if [[ -f "$OUTPUT_PATH" ]]; then | ||
| (>&2 echo "$OUTPUT_PATH already exists.") | ||
| exit | ||
| fi | ||
|
|
There was a problem hiding this comment.
This feature is valuable because it allows users to prevent resource-intensive tasks when they are unnecessary.
| # Trap any errors that occur | ||
| trap 'err_handler $?' ERR | ||
|
|
||
| err_handler() { | ||
| (>&2 echo "Restoring chain to original height; this may take a while") | ||
| ${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}" | ||
| exit "$1" | ||
| } | ||
|
|
There was a problem hiding this comment.
Since we have set -ueo pipefail at the top, that line enables strict error handling, and the script will exit if any command returns a non-zero status (indicating an error). So I think that a better approach would be to trap the EXIT (and a trap INT for e user interruption - Ctrl-C) and call a cleanup function which will be triggered when the script exits for any reason, including both successful runs and errors. That way also we don't have to remove the trap at the end and even have the call to reconsiderblock there because will be in the cleanup function.
Please check an example here...
diff --git a/contrib/devtools/utxo_snapshot.sh b/contrib/devtools/utxo_snapshot.sh
index dee25ff67..0ec555cc8 100755
--- a/contrib/devtools/utxo_snapshot.sh
+++ b/contrib/devtools/utxo_snapshot.sh
@@ -26,6 +26,21 @@ OUTPUT_PATH="${1}"; shift;
# Most of the calls we make take a while to run, so pad with a lengthy timeout.
BITCOIN_CLI_CALL="${*} -rpcclienttimeout=9999999"
+function cleanup {
+ (>&2 echo "Restoring chain to original height; this may take a while")
+ ${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}"
+}
+
+function early_exit {
+ (>&2 echo "Exiting due to Ctrl-C")
+ cleanup
+ exit 1
+}
+
+# Trap for normal exit and Ctrl-C
+trap cleanup EXIT
+trap early_exit INT
+
# Block we'll invalidate/reconsider to rewind/fast-forward the chain.
PIVOT_BLOCKHASH=$($BITCOIN_CLI_CALL getblockhash $(( GENERATE_AT_HEIGHT + 1 )) )
@@ -39,6 +54,3 @@ else
(>&2 echo "Generating UTXO snapshot...")
${BITCOIN_CLI_CALL} dumptxoutset "${OUTPUT_PATH}"
fi
-
-(>&2 echo "Restoring chain to original height; this may take a while")
-${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}"| ${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}" | ||
| exit "$1" | ||
| } | ||
|
|
||
| (>&2 echo "Rewinding chain back to height ${GENERATE_AT_HEIGHT} (by invalidating ${PIVOT_BLOCKHASH}); this may take a while") |
There was a problem hiding this comment.
I think this can be done on a follow-up but since we are here perhaps we can include a suggestion from @Sjors made a couple of years ago (here and here) about disabling network activity during the process. In the following example I made it optional for the users to decide whether or not they wish it.
Please check the example mentioned above here...
diff --git a/contrib/devtools/utxo_snapshot.sh b/contrib/devtools/utxo_snapshot.sh
index 0ec555cc8..2bf2d9cd5 100755
--- a/contrib/devtools/utxo_snapshot.sh
+++ b/contrib/devtools/utxo_snapshot.sh
@@ -8,6 +8,8 @@ export LC_ALL=C
set -ueo pipefail
+NETWORK_DISABLED=false
+
if (( $# < 3 )); then
echo 'Usage: utxo_snapshot.sh <generate-at-height> <snapshot-out-path> <bitcoin-cli-call ...>'
echo
@@ -29,6 +31,11 @@ BITCOIN_CLI_CALL="${*} -rpcclienttimeout=9999999"
function cleanup {
(>&2 echo "Restoring chain to original height; this may take a while")
${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}"
+
+ if $DISABLE_NETWORK; then
+ (>&2 echo "Restoring network activity")
+ ${BITCOIN_CLI_CALL} setnetworkactive true
+ fi
}
function early_exit {
@@ -41,6 +48,17 @@ function early_exit {
trap cleanup EXIT
trap early_exit INT
+# Prompt the user to disable network activity
+read -p "Do you want to disable network activity (setnetworkactive false) before running invalidateblock? (Y/n): " -r
+if [[ "$REPLY" =~ ^[Yy]*$ || -z "$REPLY" ]]; then
+ # User input is "Y", "y", or Enter key, proceed with the action
+ NETWORK_DISABLED=true
+ (>&2 echo "Disabling network activity")
+ ${BITCOIN_CLI_CALL} setnetworkactive false
+else
+ (>&2 echo "Network activity remains enabled")
+fi
+
# Block we'll invalidate/reconsider to rewind/fast-forward the chain.
PIVOT_BLOCKHASH=$($BITCOIN_CLI_CALL getblockhash $(( GENERATE_AT_HEIGHT + 1 )) )|
@hazeycode, are you still interested in continuing to work on this PR? Please let me know, otherwise I'll pick it up. Btw, there's also another suggestion from @Sjors to improve the script with another validation that might be needed. |
|
Sorry for the delayed response @pablomartin4btc. Thanks for looking at this. Unfortunately, I don't have a time to work on this further at the moment, so feel free to take baton. |
Thanks @hazeycode, no worries, please close this PR, I'll create a new one and will add you as a co-author in the commit. Cheers! |
…in utxo_snapshot.sh 11b7269 script: Enhance validations in utxo_snapshot.sh (pablomartin4btc) Pull request description: 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](bitcoin/bitcoin#28553 (comment))). - Validate the correctness of the file path and check if the file already exists (@hazeycode's [#27845](bitcoin/bitcoin#27845)). - Make network activity disablement optional for the user (Suggested by @Sjors [here](bitcoin/bitcoin#16899 (comment)) and [here](bitcoin/bitcoin#16899 (comment))). - 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. ACKs for top commit: Sjors: tACK 11b7269 ryanofsky: Code review ACK 11b7269 Tree-SHA512: 2b699894c6f732ad5104f5a2bcf5dc86ed31edcc9d664690cab55b94a8ab00e2ca5bde901ee1d63acddca7ea80ad1734d8cfe78f9c02f8470f264fe93a2af759
…snapshot.sh 11b7269 script: Enhance validations in utxo_snapshot.sh (pablomartin4btc) Pull request description: This PR resolves bitcoin#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](bitcoin#28553 (comment))). - Validate the correctness of the file path and check if the file already exists (@hazeycode's [bitcoin#27845](bitcoin#27845)). - Make network activity disablement optional for the user (Suggested by @Sjors [here](bitcoin#16899 (comment)) and [here](bitcoin#16899 (comment))). - 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 bitcoin#27845. ACKs for top commit: Sjors: tACK 11b7269 ryanofsky: Code review ACK 11b7269 Tree-SHA512: 2b699894c6f732ad5104f5a2bcf5dc86ed31edcc9d664690cab55b94a8ab00e2ca5bde901ee1d63acddca7ea80ad1734d8cfe78f9c02f8470f264fe93a2af759
…snapshot.sh 11b7269 script: Enhance validations in utxo_snapshot.sh (pablomartin4btc) Pull request description: This PR resolves bitcoin#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](bitcoin#28553 (comment))). - Validate the correctness of the file path and check if the file already exists (@hazeycode's [bitcoin#27845](bitcoin#27845)). - Make network activity disablement optional for the user (Suggested by @Sjors [here](bitcoin#16899 (comment)) and [here](bitcoin#16899 (comment))). - 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 bitcoin#27845. ACKs for top commit: Sjors: tACK 11b7269 ryanofsky: Code review ACK 11b7269 Tree-SHA512: 2b699894c6f732ad5104f5a2bcf5dc86ed31edcc9d664690cab55b94a8ab00e2ca5bde901ee1d63acddca7ea80ad1734d8cfe78f9c02f8470f264fe93a2af759
…snapshot.sh 11b7269 script: Enhance validations in utxo_snapshot.sh (pablomartin4btc) Pull request description: This PR resolves bitcoin#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](bitcoin#28553 (comment))). - Validate the correctness of the file path and check if the file already exists (@hazeycode's [bitcoin#27845](bitcoin#27845)). - Make network activity disablement optional for the user (Suggested by @Sjors [here](bitcoin#16899 (comment)) and [here](bitcoin#16899 (comment))). - 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 bitcoin#27845. ACKs for top commit: Sjors: tACK 11b7269 ryanofsky: Code review ACK 11b7269 Tree-SHA512: 2b699894c6f732ad5104f5a2bcf5dc86ed31edcc9d664690cab55b94a8ab00e2ca5bde901ee1d63acddca7ea80ad1734d8cfe78f9c02f8470f264fe93a2af759
There are 2 parts to this patchset, both concern the operation of
./contrib/devtools/utxo_snapshot.sh:The easiest way to test this is to take only the 1st part and follow these steps:
echo "some bytes so file is not empty" > ./utxo-788440.dat./contrib/devtools/utxo_snapshot.sh 788440 ./utxo-788440.dat ./src/bitcoin-cliNote that the 2nd part precludes error handling in the case that the file already exists.
For more info see #27841
UPDATE: Squashed into a single commit