init: disallow reindex-chainstate when pruning #24626
Conversation
src/init.cpp
Outdated
There was a problem hiding this comment.
Might be good to update -prune as well
- argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks, and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex and -coinstatsindex. "
+ argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks, and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex, -coinstatsindex, and -reindex-chainstate. "There was a problem hiding this comment.
Hmm, can you imagine a situation where this would help anyone? In contrast to the index options, -reindex-chainstate doesn't enable some additional optional feature, but is a one-time measure to recover from a corrupted chainstate. So I would think at the point where you choose to enable pruning, you probably don't care or need to know about reindex-chainstate at all. Or is the general strategy to list all interactions for completeness?
There was a problem hiding this comment.
Agree but if the user can get an error, then according to the principle of least surprise it probably should be in the doc that mentions these interactions, it can't hurt to mention it rather than guessing what users will do or not do, and it might avoid someone filing an issue.
There was a problem hiding this comment.
Agree but if the user can get an error then should perhaps be in the doc that mentions these interactions, can't hurt to mention it, and might avoid someone filing an issue. Not a blocker though.
ok, tbh I'd prefer not to add it: It is in the doc of -reindex-chainstate which is what I would expect users to consult if they plan to reindex. It's more of a technicality either way because when pruning I need to download the entire chain / rebuild the block index anyway, negating the difference between the -reindex and -reindex-chainstate. On the other hand, this doesn't extend the already long -prune doc.
But it's not super important to me, so if others also prefer to mention it, I will.
|
Approach ACK |
mzumsande
force-pushed
the
202203_reindex_cs_prune
branch
from
5e71d72 to
4be1e1d
Compare
|
cr ACK 4be1e1dab94bcbf30a7ffcfb036be0fe62872f82 |
|
Pushed an update (style fix). |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
This fixes a bug where the node would be stuck in an endless loop when combining these parameters.
mzumsande
force-pushed
the
202203_reindex_cs_prune
branch
from
4be1e1d to
b281398
Compare
|
cr ACK b281398 |
b281398 init: disallow reindex-chainstate when pruning (Martin Zumsande) Pull request description: The combination of `-reindex-chainstate` and `-prune` currently makes the node stuck in an endless loop: - `LoadChainstate()` will wipe the existing chainstate (so we have no genesis block anymore). It won't clean up unusable block files by calling `CleanupBlockRevFiles()` as for full `-reindex`. - `ThreadImport()` has [logic](https://github.com/bitcoin/bitcoin/blob/91d12344b1e51809c1ef6b630b631a6da00267c3/src/node/blockstorage.cpp#L855) of reloading Genesis after reindexing. This is what makes full `-reindex` work with `-prune` but it's not executed for `-reindex-chainstate`. - Since we still don't have a genesis block, init will wait for it forever in an endless loop ([code](https://github.com/bitcoin/bitcoin/blob/91d12344b1e51809c1ef6b630b631a6da00267c3/src/init.cpp#L1630-L1640)). Fix this by disallowing `-reindex-chainstate` together with `-prune`. This is discouraged in the help for `-reindex-chainstate` anyway ("When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.") but wasn't enforced. Fixes bitcoin#24242 ACKs for top commit: MarcoFalke: cr ACK b281398 Tree-SHA512: 7220842daaf9a4f972d82b13b81fdeac2833bf5e665c5b0f8eaf6a4bcd0725c8e97d19ec956ca4b730065a983475bb3a2732713d338f4caf8666ccbf63d4d988
4 participants
The combination of
-reindex-chainstateand-prunecurrently makes the node stuck in an endless loop:LoadChainstate()will wipe the existing chainstate (so we have no genesis block anymore). It won't clean up unusable block files by callingCleanupBlockRevFiles()as for full-reindex.ThreadImport()has logic of reloading Genesis after reindexing. This is what makes full-reindexwork with-prunebut it's not executed for-reindex-chainstate.Fix this by disallowing
-reindex-chainstatetogether with-prune. This is discouraged in the help for-reindex-chainstateanyway ("When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.") but wasn't enforced.Fixes #24242