init: disallow reindex-chainstate when pruning #24626

mzumsande commented at 12:38 PM on March 21, 2022: member

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

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

in src/init.cpp:862 in 5e71d72229 outdated

 858 |      if (args.GetIntArg("-prune", 0)) {
 859 |          if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX))
 860 |              return InitError(_("Prune mode is incompatible with -txindex."));
 861 |          if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX))
 862 |              return InitError(_("Prune mode is incompatible with -coinstatsindex."));
 863 | +        if (args.GetBoolArg("-reindex-chainstate", false))

MarcoFalke commented at 12:45 PM on March 21, 2022:

nit for new code

        if (args.GetBoolArg("-reindex-chainstate", false)) {

mzumsande commented at 3:15 PM on March 21, 2022:

done

MarcoFalke approved

MarcoFalke commented at 12:46 PM on March 21, 2022: member

ACK. Thx

in src/init.cpp:862 in 5e71d72229 outdated

 859 |          if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX))
 860 |              return InitError(_("Prune mode is incompatible with -txindex."));
 861 |          if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX))
 862 |              return InitError(_("Prune mode is incompatible with -coinstatsindex."));
 863 | +        if (args.GetBoolArg("-reindex-chainstate", false))
 864 | +            return InitError(_("Prune mode is incompatible with -reindex-chainstate. Use full -reindex instead."));

jonatack commented at 12:50 PM on March 21, 2022:

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

mzumsande commented at 1:54 PM on March 21, 2022:

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?

jonatack commented at 2:20 PM on March 21, 2022:

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.

mzumsande commented at 3:15 PM on March 21, 2022:

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.

jonatack commented at 12:51 PM on March 21, 2022: member

Approach ACK

DrahtBot added the label Tests on Mar 21, 2022

mzumsande force-pushed on Mar 21, 2022

MarcoFalke commented at 3:16 PM on March 21, 2022: member

cr ACK 4be1e1dab94bcbf30a7ffcfb036be0fe62872f82

mzumsande commented at 3:16 PM on March 21, 2022: member

Pushed an update (style fix).

in src/init.cpp:856 in 4be1e1dab9 outdated

 852 | @@ -853,12 +853,15 @@ bool AppInitParameterInteraction(const ArgsManager& args)
 853 |          nLocalServices = ServiceFlags(nLocalServices | NODE_COMPACT_FILTERS);
 854 |      }
 855 |  
 856 | -    // if using block pruning, then disallow txindex and coinstatsindex
 857 | +    // if using block pruning, then disallow txindex and coinstatsindex and reindex-chainstate

MarcoFalke commented at 3:18 PM on March 21, 2022:

unrelated nit: I think the code is self-explanatory and if you re-touch, I think the comment can be removed.

MarcoFalke commented at 10:12 AM on March 24, 2022:

Actually I think it could make sense to remove it here. Looks like #24539 forgot to update the comment, so one option would be to go through the pain of updating #24539 (and causing more merge conflicts), or the other option to just remove it? wdyt?

mzumsande commented at 12:05 PM on March 24, 2022:

Makes sense, I removed it - I agree that it is self-explanatory and only leads to merge conflicts.

DrahtBot commented at 7:37 PM on March 21, 2022: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21726 (Improve Indices on pruned nodes via prune blockers by fjahr)

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.

init: disallow reindex-chainstate when pruning

This fixes a bug where the node would be stuck in an
endless loop when combining these parameters.

b2813980b8

mzumsande force-pushed on Mar 24, 2022

MarcoFalke commented at 12:05 PM on March 24, 2022: member

cr ACK b2813980b81034ff9b40bd45080fa67dea475d39

MarcoFalke merged this on Mar 24, 2022

MarcoFalke closed this on Mar 24, 2022

sidhujag referenced this in commit 8a2503d02e on Apr 2, 2022

mzumsande deleted the branch on Apr 6, 2022

DrahtBot locked this on Apr 6, 2023