init, index: disallow indexes when running reindex-chainstate

mzumsande commented at 3:56 pm on April 6, 2022: member

When started together with -reindex-chainstate, currently coinstatsindex gets corrupted and the blockfilterindex flatfiles duplicated. See the OP of #24630 for more a more detailed explanation on why this happens.

This is an alternative to #24630 which does not wipe and rebuild the indexes but returns an InitError when they are activated, thus requiring the user to deactivate them temporarily until the -reindex-chainstate run is finished.

This also disallows -reindex-chainstate in combination with -txindex, which is not leading to corruption, but currently still rebuilds the index unnecessarily and unexpectedly.

As a long-term goal, it would be desirable to have the indexes tolerate reindex-chainstate by ignoring their BlockConnected notifications (there is discussion in #24630 about this) or possibly move reindex-chainstate option into a bitcoin-chainstate executable, which could also solve the problem. But these would be larger projects - until then, it might be better to disallow the interaction than having corrupted indexes.

The first commit adjusts the -reindex doc to mention that this option does rebuild all active indexes.

doc: Add note that -reindex will rebuild optional indexes 62e14285f9

DrahtBot added the label Tests on Apr 6, 2022

DrahtBot commented at 6:59 pm on April 6, 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:

#24830 (init: Allow -proxy="" setting values by ryanofsky)
#22072 (Add reindex=auto flag to automatically reindex corrupt data by AaronDewes)

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.

laanwj commented at 11:20 am on April 7, 2022: member

Seems ok (with the noted provision that it is a temporary state of affairs). Theoretially -reindex-chainstate is supposed to be simply a faster -reindex that doesn’t rebuild the block index, and work exactly the same otherwise. But if it doesn’t work that way, it’s better to disable the combination for now than have corruption and errors.

luke-jr commented at 9:03 pm on April 12, 2022: member

Ok with this temporarily, but I think we should really fix the issue instead long-term.

in src/init.cpp:1034 in 93fa8f6512 outdated

1030@@ -1031,6 +1031,19 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
1031         return InitError(_("No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>."));
1032     }
1033 
1034+    if (args.IsArgSet("-reindex-chainstate")) {

ryanofsky commented at 0:15 am on April 20, 2022:

In commit “init: disallow reindex-chainstate with optional indexes” (93fa8f651242af5354ba92f7aaec18e878d4f840)

It would be good to replace args.IsArgSet("-reindex-chainstate") with args.GetBoolArg("-reindex-chainstate", false) because otherwise this will wrongly trigger for -noreindex-chainstate.

(In general, it’s good to avoid the IsArgSet function. It’s frequently misused and leads to minor bugs like this.)

mzumsande commented at 8:24 pm on April 24, 2022:

Done as suggested, good catch! Maybe there could be a warning about this in the doc of IsArgSet (system.h).

in src/init.cpp:1043 in 93fa8f6512 outdated

1038+        }
1039+        if (g_enabled_filter_types.count(BlockFilterType::BASIC)) {
1040+            return InitError(_("reindex-chainstate with active blockfilterindex is not possible. Please deactivate the index temporarily or perform a full -reindex, which will also rebuild the index."));
1041+        }
1042+        if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
1043+            return InitError(_("reindex-chainstate with active txindex is not possible. Please deactivate the index temporarily or perform a full -reindex, which will also rebuild the index."));

ryanofsky commented at 0:25 am on April 20, 2022:

In commit “init: disallow reindex-chainstate with optional indexes” (93fa8f651242af5354ba92f7aaec18e878d4f840)

Curious, but do you know how or whether txindex gets corrupted? #24630 only mentions the other two indexes. I guess even if the txindex doesn’t get corrupted, it is probably still good to prevent -reindex-chainstate and -txindex from being used together if the current implementation is wastefully slow and inefficient, until we properly make the options more compatible.

mzumsande commented at 8:12 pm on April 24, 2022:

I don’t think that it will get corrupted, because it doesn’t have any kind of running variable like the other two indexes. However, it would get “rebuilt” by processing BlockConnected() signals for the entire chain (with the result being identical to the previously saved index data), which is probably not be what the user expects from running a -reindex-chainstate command. That’s why I disabled it as well - I don’t think it is strictly necessary and can drop this, if you / others would prefer that.

in src/init.cpp:1037 in 93fa8f6512 outdated

1030@@ -1031,6 +1031,19 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
1031         return InitError(_("No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>."));
1032     }
1033 
1034+    if (args.IsArgSet("-reindex-chainstate")) {
1035+        // indexes that must be deactivated to prevent index corruption, see #24630
1036+        if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) {
1037+            return InitError(_("reindex-chainstate with active coinstatsindex is not possible. Please deactivate the index temporarily or perform a full -reindex, which will also rebuild the index."));

ryanofsky commented at 0:46 am on April 20, 2022:

In commit “init: disallow reindex-chainstate with optional indexes” (93fa8f651242af5354ba92f7aaec18e878d4f840)

Minor: Here and below I think you could make recommendations more explicit. I would say “-reindex-chainstate option is not compatible with -coinstatsindex. Please temporarily disable -coinstatsindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.” But current wording is also OK if you prefer it.

mzumsande commented at 8:17 pm on April 24, 2022:

Thanks, I took your suggestion (just didn’t use a “-” after “disable” because here the index is meant, not the command option).

ryanofsky approved

ryanofsky commented at 0:59 am on April 20, 2022: member

Code review ACK 93fa8f651242af5354ba92f7aaec18e878d4f840. This looks great, improves documentation and test coverage, and avoids a corruption bug.

I agree with Luke it would be better in longer term to improve the indexes. Ideally all the indexing options should straightforwardly work well together without duplication, waste, or corruption. But as long as they don’t work well together it is good to prevent using them together, and this PR does it in a helpful way, suggesting good workarounds.

It doesn’t seem like it should be too hard to follow up this PR with a more permanent fix that just makes indexes ignore blockconnected notifications that aren’t relevant or just delays starting them until reindex-chainstate finishes.

init: disallow reindex-chainstate with optional indexes

It currently leads to corruption (coinstatsindex) or
data duplication (blockfilterindex), so disable it.

dac44fc06f

mzumsande force-pushed on Apr 24, 2022

mzumsande commented at 8:30 pm on April 24, 2022: member

Addressed feedback by @ryanofsky

ryanofsky approved

ryanofsky commented at 4:39 pm on April 25, 2022: member

Code review ACK dac44fc06ff9938d070aa5221d106a7f31da9d81. Just fixed IsArgSet call and edited error messages since last review

fanquake merged this on Apr 26, 2022

fanquake closed this on Apr 26, 2022

sidhujag referenced this in commit f79795166e on Apr 26, 2022

in src/init.cpp:1035 in dac44fc06f

1030@@ -1031,6 +1031,19 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
1031         return InitError(_("No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>."));
1032     }
1033 
1034+    if (args.GetBoolArg("-reindex-chainstate", false)) {
1035+        // indexes that must be deactivated to prevent index corruption, see #24630

MarcoFalke commented at 7:05 am on April 27, 2022:

not sure how helpful it is to write #24630. Either this should be a full URL or even better a description for devs to explain which indexes must be deactivated and which ones do not.

If all need to be deactivated it would be better to write “All index” instead of “indexes that”.

in src/init.cpp:1043 in dac44fc06f

1038+        }
1039+        if (g_enabled_filter_types.count(BlockFilterType::BASIC)) {
1040+            return InitError(_("-reindex-chainstate option is not compatible with -blockfilterindex. Please temporarily disable blockfilterindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes."));
1041+        }
1042+        if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
1043+            return InitError(_("-reindex-chainstate option is not compatible with -txindex. Please temporarily disable txindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes."));

MarcoFalke commented at 7:06 am on April 27, 2022:

nit: Maybe this can be wrapped in a lambda that takes the index name?

MarcoFalke referenced this in commit becea48fe0 on Apr 30, 2022

mzumsande deleted the branch on May 19, 2022

ryanofsky referenced this in commit 4e8a7654f6 on May 17, 2023

DrahtBot locked this on May 19, 2023

init, index: disallow indexes when running reindex-chainstate #24789

Conflicts