validation: run VerifyDB on all chainstates #21523

jamesob · 2021-03-24T16:32:11Z

This is part of the assumeutxo project (parent PR: #15606)

~~Pretty cut and dry; parameterizes CVerifyDB methods so that we can run the verify procedure on multiple chainstates.~~

Two minor tweaks to ensure that VerifyDB can be run on multiple chainstates and a corresponding rename.

jamesob · 2021-03-24T16:40:47Z

I guess after rebasing on top of 2bdf37f, most of this is just a name change. But there are still a few changes of substance and the rename is probably worth doing.

DrahtBot · 2021-03-24T21:40:34Z

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

Conflicts

No conflicts as of last run.

ryanofsky

Code review ACK cd93f82. This seems safe and straightforward. I think it would be good to put the renames in a separate commit from the two more substantive changes, but it's not very important.

re: #21523 (comment)

Two minor tweaks to ensure that VerifyDB can be run on multiple chainstates and a corresponding rename.

IIUC the two tweaks are:

Updating AppInitMain to call VerifyDB on all chainstates, not just the active one
Tweaking VerifyDB to treat a snapshot chainstate like a pruned chainstate and not fail trying to read missing blocks

And everything else is cleanups and renames

ryanofsky · 2021-04-08T20:31:45Z

src/validation.cpp

            break;
-        if (fPruneMode && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
+        if ((fPruneMode || g_chainman.IsSnapshotActive()) && !(pindex->nStatus & BLOCK_HAVE_DATA)) {


In commit "validation: parameterize VerifyDB by chainstate" (cd93f82)

Could comment below be updated to explain the g_chainman check?

Also could logprint below be updated to not mention pruning if block isn't pruned?

Also it would seem more direct and less likely to hide errors to check here if chainstate specifically is an unvalidated snapshot, instead of checking if there is just any snapshot. This would avoid hiding legitimate errors if the background chainstate is missing blocks.

This would avoid hiding legitimate errors if the background chainstate is missing blocks.

Good catch! Fixed.

fjahr

Code review ACK cd93f82

But I agree with @ryanofsky that the comment and log message look like they should be updated as mentioned in #21523 (comment).

src/validation.cpp

ariard

Code Review ACK cd93f82

I agree with other reviewers, would be better to split in its own commit renaming.

src/validation.cpp

jamesob · 2021-04-13T14:51:23Z

Thanks for the reviews so far @ryanofsky @fjahr @ariard. I've incorporated your feedback and split out the rename commit.

ariard · 2021-04-13T16:33:42Z

Code Review ACK c47a00d

ryanofsky

Code review ACK c47a00d with caveat that first commit currently doesn't compile, but problems are fixed after. Changes since last review: splitting commits, making the BLOCK_HAVE_DATA check a little broader so it doesn't expect blocks from the non-snapshot chainstate, dropping type cast, adding IsSnapshotActive comment

ryanofsky · 2021-04-13T18:45:30Z

src/validation.cpp

-        if (nCheckLevel >= 3 && (coins.DynamicMemoryUsage() + active_chainstate.CoinsTip().DynamicMemoryUsage()) <= active_chainstate.m_coinstip_cache_size_bytes) {
+        int64_t curr_coins_usage = coins.DynamicMemoryUsage() + chainstate.CoinsTip().DynamicMemoryUsage();
+
+        if (nCheckLevel >= 3 && static_cast<unsigned long>(curr_coins_usage) <= chainstate.m_coinstip_cache_size_bytes) {


In commit "refactor: rename active_chainstate in VerifyDB" (de83651)

It doesn't really matter and this is fixed in the next commit, but there are three different types here: int64_t for curr_coins_usage, unsigned long for the cast, and size_t for m_coinstip_cache_size_bytes, and it would be nice to use size_t for all three from the start.

fjahr · 2021-04-14T21:10:17Z

Code Review ACK c47a00d

To prepare VerifyDB semantics for multiple chainstate use.

Removes assumptions of use only on the active chainstate.

jamesob · 2021-04-23T19:14:21Z

Rebased and fixed compilation for the first commit. Thanks for the looks all.

fjahr · 2021-04-23T21:36:58Z

Code review re-ACK 844ad0e

maflcko

review ACK 844ad0e 🐥

Show signature and timestamp

Signature:

Timestamp of file with hash 9b4c4c61593ad7aa160c2fb769c97159e09b00e6c5ba6cc0c987f607bb34c988 -

maflcko · 2021-04-24T07:23:45Z

src/validation.cpp


-        if (nCheckLevel >= 3 && static_cast<unsigned long>(curr_coins_usage) <= chainstate.m_coinstip_cache_size_bytes) {


9b604c0: Maybe move this hunk to the previous commit?

maflcko · 2021-04-24T07:25:21Z

src/validation.cpp

@@ -4135,8 +4138,9 @@ bool CVerifyDB::VerifyDB(
        uiInterface.ShowProgress(_("Verifying blocks...").translated, percentageDone, false);
        if (pindex->nHeight <= chainstate.m_chain.Height()-nCheckDepth)
            break;
-        if (fPruneMode && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
-            // If pruning, only go back as far as we have data.
+        if ((fPruneMode || is_snapshot_cs) && !(pindex->nStatus & BLOCK_HAVE_DATA)) {


9b604c0: Wouldn't it be easier to remove the prune and snapshot condition and simply check HAVE_DATA? Performance shouldn't decrease and it would be less code, which is nice.

I guess your goal is to treat it as error when block data is missing, but pruning is off an no snapshot is loaded?

Right - I think we want to fail here if we're lacking block data when not pruning or operating from snapshot.

Summary: > refactor: rename active_chainstate in VerifyDB > > To prepare VerifyDB semantics for multiple chainstate use. > validation: prepare VerifyDB for assumeutxo > > Removes assumptions of use only on the active chainstate. > doc: IsSnapshotActive This is a backport of [[bitcoin/bitcoin#21523 | core#21523]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11334

jamesob force-pushed the 2021-03-au-verifydb branch from d758ad1 to cd93f82 Compare March 24, 2021 16:33

jamesob changed the title ~~validation: parameterize VerifyDB by chainstate~~ validation: run VerifyDB on all chainstates Mar 24, 2021

DrahtBot added RPC/REST/ZMQ Validation labels Mar 24, 2021

This was referenced Mar 24, 2021

doc: Address feedback from Transifex translator community #21463

Closed

[Bundle 5/n] Prune g_chainman usage in RPC modules #21391

Merged

ryanofsky approved these changes Apr 8, 2021

View reviewed changes

fjahr reviewed Apr 11, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

ariard reviewed Apr 12, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

jamesob force-pushed the 2021-03-au-verifydb branch 3 times, most recently from 54584a6 to c47a00d Compare April 13, 2021 14:50

ryanofsky approved these changes Apr 13, 2021

View reviewed changes

DrahtBot added the Needs rebase label Apr 17, 2021

jamesob added 3 commits April 23, 2021 15:02

refactor: rename active_chainstate in VerifyDB

7901647

To prepare VerifyDB semantics for multiple chainstate use.

validation: prepare VerifyDB for assumeutxo

9b604c0

Removes assumptions of use only on the active chainstate.

doc: IsSnapshotActive

844ad0e

jamesob force-pushed the 2021-03-au-verifydb branch from c47a00d to 844ad0e Compare April 23, 2021 19:13

DrahtBot removed the Needs rebase label Apr 23, 2021

maflcko approved these changes Apr 24, 2021

View reviewed changes

maflcko merged commit c6d6bc8 into bitcoin:master Apr 27, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 27, 2021

Merge bitcoin#21523: validation: run VerifyDB on all chainstates

645aae3

kwvg mentioned this pull request Oct 9, 2021

merge bitcoin#15948, #15976, #16194...: assumeutxo project backports (part 1) dashpay/dash#4498

Merged

kwvg mentioned this pull request Feb 25, 2022

merge bitcoin#16240...21526: assumeutxo project backports (part 2) dashpay/dash#4703

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge c6d6bc8 into merged_master (Bitcoin PR bitcoin/bitcoin#21523)

6f238f5

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022


		if (nCheckLevel >= 3 && static_cast<unsigned long>(curr_coins_usage) <= chainstate.m_coinstip_cache_size_bytes) {

validation: run VerifyDB on all chainstates #21523

validation: run VerifyDB on all chainstates #21523

Uh oh!

Conversation

jamesob commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob commented Mar 24, 2021

Uh oh!

DrahtBot commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

jamesob Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented Apr 13, 2021

Uh oh!

ariard commented Apr 13, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

fjahr commented Apr 14, 2021

Uh oh!

jamesob commented Apr 23, 2021

Uh oh!

fjahr commented Apr 23, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 24, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 24, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 24, 2021

Choose a reason for hiding this comment

Uh oh!

jamesob Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob commented Mar 24, 2021 •

edited

Loading

DrahtBot commented Mar 24, 2021 •

edited

Loading