script, assumeutxo: Enhance validations in utxo_snapshot.sh #28852

pull pablomartin4btc wants to merge 1 commits into bitcoin:master from pablomartin4btc:devtools-improve-utxo_snapshot changing 1 files +54 −4

pablomartin4btc commented at 4:11 PM on November 11, 2023: member
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.
DrahtBot commented at 4:11 PM on November 11, 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, ryanofsky

Concept ACK BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
pablomartin4btc force-pushed on Nov 11, 2023
DrahtBot added the label CI failed on Nov 11, 2023
pablomartin4btc force-pushed on Nov 11, 2023
pablomartin4btc marked this as a draft on Nov 11, 2023
pablomartin4btc force-pushed on Nov 11, 2023
pablomartin4btc force-pushed on Nov 11, 2023
pablomartin4btc marked this as ready for review on Nov 11, 2023
DrahtBot removed the label CI failed on Nov 11, 2023
pablomartin4btc force-pushed on Nov 12, 2023
Sjors commented at 6:16 AM on November 13, 2023: member

Concept ACK
in contrib/devtools/utxo_snapshot.sh:32 in 3cb390f3c5 outdated
```
  27 | @@ -26,6 +28,57 @@ OUTPUT_PATH="${1}"; shift;
  28 |  # Most of the calls we make take a while to run, so pad with a lengthy timeout.
  29 |  BITCOIN_CLI_CALL="${*} -rpcclienttimeout=9999999"
  30 |  
  31 | +# Check if the node is pruned and get the pruned block height
  32 | +PRUNED=$( ${BITCOIN_CLI_CALL} getblockchaininfo | grep 'pruneheight' | awk '{print $2}' | tr -d ',' )
```
Sjors commented at 12:35 PM on November 13, 2023:

On macOS 13.6 this silently exits with -1 when a node is not pruned (pruneheight is not set).

pablomartin4btc commented at 9:56 PM on November 13, 2023:

I'll update it with an alternative, as I don't have an instance with macOS, please let me know if that would work better now.

pablomartin4btc commented at 10:03 PM on November 13, 2023:

Done, pls let me know.
in contrib/devtools/utxo_snapshot.sh:68 in 3cb390f3c5 outdated
```
  67 | +# Trap for normal exit and Ctrl-C
  68 | +trap cleanup EXIT
  69 | +trap early_exit INT
  70 | +
  71 | +# Prompt the user to disable network activity
  72 | +read -p "Do you want to disable network activity (setnetworkactive false) before running invalidateblock? (Y/n): " -r
```
Sjors commented at 12:37 PM on November 13, 2023:

If I abort here with ctrl + c it tries to cleanup, so maybe move the trap calls down a bit?

pablomartin4btc commented at 9:57 PM on November 13, 2023:

Yes! I'll do, but the reconsiderblock won't do anything, the issue I see now is that PIVOT_BLOCKHASH wasn't initialised so perhaps it fails there.

pablomartin4btc commented at 10:02 PM on November 13, 2023:

Done.
in contrib/devtools/utxo_snapshot.sh:53 in 3cb390f3c5 outdated
```
  48 | +  exit 1
  49 | +fi
  50 | +
  51 | +function cleanup {
  52 | +  (>&2 echo "Restoring chain to original height; this may take a while")
  53 | +  ${BITCOIN_CLI_CALL} reconsiderblock "${PIVOT_BLOCKHASH}"
```
Sjors commented at 12:42 PM on November 13, 2023:

Note that once invalidateblock has been called, this won't stop it. It will go all the way back and then forward again.

pablomartin4btc commented at 9:57 PM on November 13, 2023:

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: https://github.com/bitcoin/bitcoin/blob/5800c558eb5efb4839ed00d6967e43306d68e1c3/doc/design/assumeutxo.md?plain=1#L42-L45
Sjors commented at 12:43 PM on November 13, 2023: member

A few suggestions after testing and reviewing 3cb390f3c59d3ed0475f1be0c886d46a044fa898.
Sjors commented at 12:44 PM on November 13, 2023: member

By the way, at some point when a bash script gets very complicated, it may be better to convert it to Python.
pablomartin4btc commented at 10:00 PM on November 13, 2023: member

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.

Type	Reviewers
ACK	Sjors, ryanofsky
Concept ACK	BrandonOdiwuor

script: Enhance validations in utxo_snapshot.sh

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

11b7269d83

pablomartin4btc force-pushed on Nov 13, 2023
pablomartin4btc commented at 10:07 PM on November 13, 2023: member

Addressed @Sjors's feedback.

cc @jamesob as he reviewed #27845, @ryanofsky & @theStack
Sjors commented at 5:15 AM on November 14, 2023: member

tACK 11b7269d83a56f919f9dddb7f7c72a96d428627f
BrandonOdiwuor commented at 3:39 PM on November 20, 2023: contributor

Concept ACK
BrandonOdiwuor approved
BrandonOdiwuor commented at 3:37 AM on November 21, 2023: contributor
tested ACk 11b7269d83a56f919f9dddb7f7c72a96d428627f
1. 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
2. Successfully tested the option to enable or disable network activity
in contrib/devtools/utxo_snapshot.sh:11 in 11b7269d83
```
   7 |  #
   8 |  export LC_ALL=C
   9 |  
  10 |  set -ueo pipefail
  11 |  
  12 | +NETWORK_DISABLED=false
```
ryanofsky commented at 10:00 PM on December 4, 2023:

In commit "script: Enhance validations in utxo_snapshot.sh" (11b7269d83a56f919f9dddb7f7c72a96d428627f)

Assignment seems a little out of place here, would suggest moving it below PRUNE=... with other variable assignments.
ryanofsky approved
ryanofsky commented at 10:03 PM on December 4, 2023: contributor

Code review ACK 11b7269d83a56f919f9dddb7f7c72a96d428627f
DrahtBot requested review from BrandonOdiwuor on Dec 4, 2023
DrahtBot removed review request from BrandonOdiwuor on Dec 4, 2023
DrahtBot requested review from BrandonOdiwuor on Dec 4, 2023
ryanofsky merged this on Dec 4, 2023
ryanofsky closed this on Dec 4, 2023
ryanofsky commented at 10:08 PM on December 4, 2023: contributor

Note: this actually had 3 acks. DrahtBot didn't count #28852#pullrequestreview-1741060818 due to capitalization
PastaPastaPasta referenced this in commit e89c513828 on Oct 24, 2024
PastaPastaPasta referenced this in commit 0aeb0b0be7 on Oct 24, 2024
PastaPastaPasta referenced this in commit 8cd85d311f on Oct 24, 2024
PastaPastaPasta referenced this in commit a67319351a on Oct 24, 2024
bitcoin locked this on Dec 3, 2024

Contributors

Review Requested
BrandonOdiwuor

Linked (view graph)

#16899 UTXO snapshot creation (dumptxoutset)#27841 Stuck chainstate when utxo_snapshot.sh fails #27845 script: utxo_snapshot.sh error handling #28553 validation: assumeutxo params mainnet

script, assumeutxo: Enhance validations in utxo_snapshot.sh #28852

Code Coverage

Reviews