script, assumeutxo: Enhance validations in utxo_snapshot.sh #28852
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 |
Sjors
reviewed
Nov 13, 2023
|
|
||
| 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.
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.
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
|
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 |
BrandonOdiwuor
approved these changes
Nov 21, 2023
There was a problem hiding this comment.
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
ryanofsky
approved these changes
Dec 4, 2023
| # 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.
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 |
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Oct 24, 2024
…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
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Oct 24, 2024
…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
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Oct 24, 2024
…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
PastaPastaPasta
added a commit
to dashpay/dash
that referenced
this pull request
Oct 24, 2024
8bf1d06 Merge bitcoin#29308: doc: update `BroadcastTransaction` comment (glozow) 2a77808 Merge bitcoin-core/gui#789: Avoid non-self-contained Windows header (Hennadii Stepanov) da371b8 Merge bitcoin#28870: depends: Include `config.guess` and `config.sub` into `meta_depends` (fanquake) 2e41562 Merge bitcoin#29219: fuzz: Improve fuzzing stability for ellswift_roundtrip harness (fanquake) b091329 Merge bitcoin#29211: fuzz: fix `connman` initialization (Ava Chow) df42d41 Merge bitcoin#29200: net: create I2P sessions using both ECIES-X25519 and ElGamal encryption (fanquake) 4cdd1a8 Merge bitcoin#29172: fuzz: set `nMaxOutboundLimit` in connman target (fanquake) 97012ea Merge bitcoin#28962: doc: Rework guix docs after 1.4 release (fanquake) c70ff5d Merge bitcoin#28844: contrib: drop GCC MAX_VERSION to 4.3.0 in symbol-check (fanquake) e6f19e7 Merge bitcoin#29068: test: Actually fail when a python unit test fails (fanquake) 75e0334 Merge bitcoin#28989: test: Fix test by checking the actual exception instance (Andrew Chow) 8cd85d3 Merge bitcoin#28852: script, assumeutxo: Enhance validations in utxo_snapshot.sh (Ryan Ofsky) fd2e88d Merge bitcoin#26077: guix: switch from `guix environment` to `guix shell` (fanquake) 02741a7 Merge bitcoin#28913: coins: make sure PoolAllocator uses the correct alignment (fanquake) dfd53da Merge bitcoin#28902: doc: Simplify guix install doc, after 1.4 release (fanquake) Pull request description: ## Issue being fixed or feature implemented Batch of trivial backports ## What was done? See commits ## How Has This Been Tested? built locally; large combined merge passed tests locally ## Breaking Changes Should be none ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 8bf1d06 knst: utACK 8bf1d06 Tree-SHA512: 506273e5a188f9ca74edf656e3cd338992192e6e97f68c89fc43e34be20fb7f211b48e4dfa8693727839a7920da8284509413c722f55774a428939c296dad517
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
reconsiderblockcommand 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.