init: Return ChainstateLoadStatus::INTERRUPTED when verification was interrupted. #27157
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
…terrupted This also avoids a misleading block index loadtime log entry in init. Co-authored-by: Ryan Ofsky <[email protected]>
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
DrahtBot
changed the title
Member
|
It might be good to explain and motivate the behaviour change. Calling it "25574 followups" is less useful than, for example, mentioning if the exit code is changed, if a warning will be printed, or if this turns an operation into an error? |
mzumsande
changed the title
Contributor
Author
I changed the title and expanded the explanation. |
Member
|
thanks lgtm ACK c5825e1 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Feb 28, 2023
…hen verification was interrupted. c5825e1 doc: add explanation for fail_on_insufficient_dbcache (Ryan Ofsky) 7dff7da init: Return more fitting ChainStateLoadStatus if verification was interrupted (Martin Zumsande) Pull request description: This addresses two outstanding comments by ryanofsky from bitcoin#25574: * return `ChainstateLoadStatus::INTERRUPTED` instead of `ChainstateLoadStatus::SUCCESS` if verification was stopped by an interrupt. This would coincide with straightforward expectation, and it avoids a misleading [log entry](https://github.com/mzumsande/bitcoin/blob/c5825e14f8999a8c5f5121027af9e07ac51ab42e/src/init.cpp#L1526) in `init` for the block index load time (because that would include the verificiation, which didn't complete). It shouldn't affect node behavior otherwise because the shutdown signal would be caught in init anyway. In test, this would lead to an assert ([link](https://github.com/mzumsande/bitcoin/blob/c5825e14f8999a8c5f5121027af9e07ac51ab42e/src/test/util/setup_common.cpp#L230)), which also makes more sense because benign interrupts are not expected there during init. This can be tested by setting a large value for `-checkblocks`, interrupting the node during block verification and observing the log. bitcoin#25574 (comment) * add documentation for `require_full_verification` bitcoin#25574 (comment) ACKs for top commit: MarcoFalke: thanks lgtm ACK c5825e1 Tree-SHA512: ca1c71a1b046d30083337dd9ef6d52e66fa1ac8c4ecd807716e4aa6a894179a81df41caee916fa30997fd6e0b284412a3c8f2919d19c29d826fb580ffb89fd73
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Dec 7, 2023
…terrupted Summary: This means that the verifydb RPC will now return false if it cannot finish due to the node being shutdown. This also avoids a misleading block index loadtime log entry in init. This is a partial backport of [[bitcoin/bitcoin#25574 | core#25574]] and [[bitcoin/bitcoin#27157 | core#27157]] bitcoin/bitcoin@d6f781f bitcoin/bitcoin@7dff7da Depends on D14926 Test Plan: Co-authored-by: Ryan Ofsky <[email protected]> `src/bitcoind checkblocks` Press Ctrl-C, check that it still exits gracefully and does not print a success message for the block verification or block loading. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D14927
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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 addresses two outstanding comments by ryanofsky from #25574:
ChainstateLoadStatus::INTERRUPTEDinstead ofChainstateLoadStatus::SUCCESSif verification was stopped by an interrupt. This would coincide with straightforward expectation, and it avoids a misleading log entry ininitfor the block index load time (because that would include the verificiation, which didn't complete). It shouldn't affect node behavior otherwise because the shutdown signal would be caught in init anyway. In test, this would lead to an assert (link), which also makes more sense because benign interrupts are not expected there during init.This can be tested by setting a large value for
-checkblocks, interrupting the node during block verification and observing the log.validation: Improve error handling when VerifyDB dosn't finish successfully #25574 (comment)
require_full_verificationvalidation: Improve error handling when VerifyDB dosn't finish successfully #25574 (comment)