script: utxo_snapshot.sh error handling

hazeycode commented at 4:49 pm on June 9, 2023: none

There are 2 parts to this patchset, both concern the operation of ./contrib/devtools/utxo_snapshot.sh:

Error handling is added so that the chainstate can be automatically recovered if an error occurs
Early exit if a file at OUTPUT_PATH already exists

The easiest way to test this is to take only the 1st part and follow these steps:

Create a dummy file for the purpose of the test e.g. echo "some bytes so file is not empty" > ./utxo-788440.dat
Run bitcoind and wait for it to sync
Run utxosnapshot.sh with some blockheight and a filepath that already exists e.g. ./contrib/devtools/utxo_snapshot.sh 788440 ./utxo-788440.dat ./src/bitcoin-cli
Wait for chain rewind to happen, snapshot to be generated and write to disk fail
Observe that the pivot block is now reconsidered and the chain is restored to the original height

Note 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

DrahtBot commented at 4:49 pm on June 9, 2023: contributor

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	Sjors
Concept ACK	pablomartin4btc
Stale ACK	jamesob

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Consensus on Jun 9, 2023

hazeycode force-pushed on Jun 9, 2023

DrahtBot added the label CI failed on Jun 9, 2023

hazeycode force-pushed on Jun 9, 2023

jamesob commented at 5:09 pm on June 9, 2023: member

Thanks for working on this! crACK

jamesob commented at 5:11 pm on June 9, 2023: member

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

hazeycode force-pushed on Jun 9, 2023

DrahtBot removed the label CI failed on Jun 9, 2023

jamesob commented at 2:46 pm on June 29, 2023: member

ACK https://github.com/bitcoin/bitcoin/pull/27845/commits/39aa958b2049bc50819d645052c0b90cf19f306e

But I think you can just squash these two commits down into one.

Add error handling to utxo_snapshot.sh

Closes #27841

10d9bef004

hazeycode force-pushed on Jul 3, 2023

achow101 requested review from ryanofsky on Sep 20, 2023

achow101 requested review from Sjors on Sep 20, 2023

achow101 requested review from theStack on Sep 20, 2023

Sjors commented at 9:53 am on September 21, 2023: member

Lightly tested ACK 10d9bef0045398641fc723bd73b55ab9c512dac1

DrahtBot removed review request from Sjors on Sep 21, 2023

DrahtBot requested review from jamesob on Sep 21, 2023

in contrib/devtools/utxo_snapshot.sh:34 in 10d9bef004

25@@ -26,9 +26,24 @@ OUTPUT_PATH="${1}"; shift;
26 # Most of the calls we make take a while to run, so pad with a lengthy timeout.
27 BITCOIN_CLI_CALL="${*} -rpcclienttimeout=9999999"
28 
29+# Early exit if file at OUTPUT_PATH already exists
30+if [[ -f "$OUTPUT_PATH" ]]; then
31+  (>&2 echo "$OUTPUT_PATH already exists.")
32+  exit
33+fi
34+

pablomartin4btc commented at 3:41 pm on October 13, 2023:

This feature is valuable because it allows users to prevent resource-intensive tasks when they are unnecessary.

in contrib/devtools/utxo_snapshot.sh:46 in 10d9bef004

41+err_handler() {
42+  (>&2 echo "Restoring chain to original height; this may take a while")
43+  ${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}"
44+  exit "$1"
45+}
46+

pablomartin4btc commented at 4:45 pm on October 13, 2023:

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.

 0diff --git a/contrib/devtools/utxo_snapshot.sh b/contrib/devtools/utxo_snapshot.sh
 1index dee25ff67..0ec555cc8 100755
 2--- a/contrib/devtools/utxo_snapshot.sh
 3+++ b/contrib/devtools/utxo_snapshot.sh
 4@@ -26,6 +26,21 @@ OUTPUT_PATH="${1}"; shift;
 5 # Most of the calls we make take a while to run, so pad with a lengthy timeout.
 6 BITCOIN_CLI_CALL="${*} -rpcclienttimeout=9999999"
 7 
 8+function cleanup {
 9+  (>&2 echo "Restoring chain to original height; this may take a while")
10+  ${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}"
11+}
12+
13+function early_exit {
14+  (>&2 echo "Exiting due to Ctrl-C")
15+  cleanup
16+  exit 1
17+}
18+
19+# Trap for normal exit and Ctrl-C
20+trap cleanup EXIT
21+trap early_exit INT
22+
23 # Block we'll invalidate/reconsider to rewind/fast-forward the chain.
24 PIVOT_BLOCKHASH=$($BITCOIN_CLI_CALL getblockhash $(( GENERATE_AT_HEIGHT + 1 )) )
25 
26@@ -39,6 +54,3 @@ else
27   (>&2 echo "Generating UTXO snapshot...")
28   ${BITCOIN_CLI_CALL} dumptxoutset "${OUTPUT_PATH}"
29 fi
30-
31-(>&2 echo "Restoring chain to original height; this may take a while")
32-${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}"

in contrib/devtools/utxo_snapshot.sh:47 in 10d9bef004

42+  (>&2 echo "Restoring chain to original height; this may take a while")
43+  ${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}"
44+  exit "$1"
45+}
46+
47 (>&2 echo "Rewinding chain back to height ${GENERATE_AT_HEIGHT} (by invalidating ${PIVOT_BLOCKHASH}); this may take a while")

pablomartin4btc commented at 4:58 pm on October 13, 2023:

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.

 0diff --git a/contrib/devtools/utxo_snapshot.sh b/contrib/devtools/utxo_snapshot.sh
 1index 0ec555cc8..2bf2d9cd5 100755
 2--- a/contrib/devtools/utxo_snapshot.sh
 3+++ b/contrib/devtools/utxo_snapshot.sh
 4@@ -8,6 +8,8 @@ export LC_ALL=C
 5 
 6 set -ueo pipefail
 7 
 8+NETWORK_DISABLED=false
 9+
10 if (( $# < 3 )); then
11   echo 'Usage: utxo_snapshot.sh <generate-at-height> <snapshot-out-path> <bitcoin-cli-call ...>'
12   echo
13@@ -29,6 +31,11 @@ BITCOIN_CLI_CALL="${*} -rpcclienttimeout=9999999"
14 function cleanup {
15   (>&2 echo "Restoring chain to original height; this may take a while")
16   ${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}"
17+
18+  if $DISABLE_NETWORK; then
19+    (>&2 echo "Restoring network activity")
20+    ${BITCOIN_CLI_CALL} setnetworkactive true
21+  fi
22 }
23 
24 function early_exit {
25@@ -41,6 +48,17 @@ function early_exit {
26 trap cleanup EXIT
27 trap early_exit INT
28 
29+# Prompt the user to disable network activity
30+read -p "Do you want to disable network activity (setnetworkactive false) before running invalidateblock? (Y/n): " -r
31+if [[ "$REPLY" =~ ^[Yy]*$ || -z "$REPLY" ]]; then
32+  # User input is "Y", "y", or Enter key, proceed with the action
33+  NETWORK_DISABLED=true
34+  (>&2 echo "Disabling network activity")
35+  ${BITCOIN_CLI_CALL} setnetworkactive false
36+else
37+  (>&2 echo "Network activity remains enabled")
38+fi
39+
40 # Block we'll invalidate/reconsider to rewind/fast-forward the chain.
41 PIVOT_BLOCKHASH=$($BITCOIN_CLI_CALL getblockhash $(( GENERATE_AT_HEIGHT + 1 )) )

pablomartin4btc commented at 5:09 pm on October 13, 2023: member

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.

DrahtBot added the label CI failed on Oct 25, 2023

pablomartin4btc commented at 1:11 pm on November 10, 2023: member

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

hazeycode commented at 2:06 pm on November 10, 2023: none

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.

pablomartin4btc commented at 2:20 pm on November 10, 2023: member

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!

fanquake closed this on Nov 10, 2023

script: utxo_snapshot.sh error handling #27845

Code Coverage

Reviews