validation: Skip VerifyDB checks of level >=3 if dbcache is too small #27009

mzumsande · 2023-01-31T18:39:13Z

This is the first two commits from #25574, leaving out all changes to -verifychain error-handling :

The Problem of 25563 is that when we skip blocks at level 3 due to an insufficient dbcache (skipping some DisconnectBlock() calls), we would still attempt the level 4 checks, attempting to reconnect a block that was never disconnected, leading to an assert in ConnectBlock().
Fix this by not attempting level 4 checks in this case.
Logging of verification progress is now split over multiple lines. This is more verbose, but now each update has its own timestamp, and other threads logging concurrently will no longer lead to mangled output.

This can be tested with a small dbcache value, for example:
bitcoind -signet -dbcache=10
bitcoin-cli -signet verifychain 4 1000

Fixes #25563

The previous behavior, skipping some L3 DisconnectBlock calls, but still attempting to reconnect these blocks at L4, makes ConnectBlock assert. The variable skipped_l3_checks is introduced because even with an insufficient cache for the L3 checks, the L1/L2 checks in the same loop should still be completed. Fixes bitcoin#25563.

This allows to log a timestamp for each entry, and avoids potential interference with other threads that could log concurrently.

DrahtBot · 2023-01-31T18:39:16Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke, john-moffett

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

maflcko · 2023-02-03T13:51:08Z

This removes a crashing check and turns it into a logprint to explain why it was skipped.

Given that no one reported an issue about this in the past, it seems unlikely to affect anyone. However, in the future the log can be displayed more verbosely, for example as an InitWarning in init or as a json-dict-key {"warnings":[...]} in the rpc.

review ACK fe683f3 🗄

Show signature

Signature:

john-moffett

ACK fe683f3

I'm definitely uncomfortable with:

src/bitcoin-cli -signet verifychain 4 10000
true

when it didn't fully verify at the requested level. However, I think it's preferable to a SIGABRT, especially given that we're already potentially silently failing with level 3, so I won't stand in the way of an improvement. If this needs to be a two-PR process, I'm in favor of that over no action.

To be clear, I prefer #25574 in its current form, despite the RPC behavior change.

john-moffett · 2023-02-03T18:12:27Z

src/validation.cpp


-    LogPrintf("[DONE].\n");
-    LogPrintf("No coin database inconsistencies in last %i blocks (%i transactions)\n", block_count, nGoodTransactions);
+    LogPrintf("Verification: No coin database inconsistencies in last %i blocks (%i transactions)\n", block_count, nGoodTransactions);


Nano-Nit: Would prefer "Verification complete: "

… dbcach…

maflcko · 2023-02-06T08:52:17Z

when it didn't fully verify at the requested level. However, I think it's preferable to a SIGABRT, especially given that we're already potentially silently failing with level 3, so I won't stand in the way of an improvement. If this needs to be a two-PR process, I'm in favor of that over no action.

How is logging a warning for the not-enough-memory case different from logging a warning for the not-enough-storage case (pruning)? I think the warning can be displayed more prominently in a follow-up, but I think doing the same for similar cases seems fine.

To be clear, I prefer #25574 in its current form, despite the RPC behavior change.

That pull changes the not-enough-memory case to an error and leaves the not-enough-storage case (pruning) a silent pass with log warning. I have no objection to merging 25574, but I think it may not be the ideal solution either.

john-moffett · 2023-02-06T14:11:28Z

How is logging a warning for the not-enough-memory case different from logging a warning for the not-enough-storage case (pruning)?

I agree that they're not different, but my main concern isn't about the warning log. It's about the output from the RPC.

That pull changes the not-enough-memory case to an error and leaves the not-enough-storage case (pruning) a silent pass with log warning.

True, it's not ideal, because I think that the latter case should also change the RPC behavior to return false (by adding another new VerifyDBResult value).

mzumsande · 2023-02-06T21:12:44Z

Let's continue this discussion in #25574.

… finish successfully 0af16e7 doc: add release note for #25574 (Martin Zumsande) 57ef2a4 validation: report if pruning prevents completion of verification (Martin Zumsande) 0c7785b init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache (Martin Zumsande) d6f781f validation: return VerifyDBResult::INTERRUPTED if verification was interrupted (Martin Zumsande) 6360b53 validation: Change return value of VerifyDB to enum type (Martin Zumsande) Pull request description: `VerifyDB()` can fail to complete due to insufficient dbcache at the level 3 checks. This PR improves the error handling in this case in the following ways: - The rpc `-verifychain` now returns false if the check can't be completed due to insufficient cache - During init, we only log a warning if the default values for `-checkblocks` and `-checklevel` are taken and the check doesn't complete. However, if the user actively specifies one of these args, we return with an InitError if we can't complete the check. This PR also changes `-verifychain` RPC to return `false` if the verification didn't finish due to missing block data (pruning) or due to being interrupted by the node being shutdown. Previously, this PR also included a fix for a possible assert during verification - this was done in #27009 (now merged). ACKs for top commit: achow101: ACK 0af16e7 ryanofsky: Code review ACK 0af16e7. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups john-moffett: ACK 0af16e7 MarcoFalke: lgtm re-ACK 0af16e7 🎚 Tree-SHA512: 84b4f767cf9bfbafef362312757c9bf765b41ae3977f4ece840e40c52a2266b1457832df0cdf70440be0aac2168d9b58fc817238630b0b6812f3836ca950bc0e

Summary: The previous behavior, skipping some L3 DisconnectBlock calls, but still attempting to reconnect these blocks at L4, makes ConnectBlock assert. The variable skipped_l3_checks is introduced because even with an insufficient cache for the L3 checks, the L1/L2 checks in the same loop should still be completed. This is a partial backport of [[bitcoin/bitcoin#27009 | core#27009]] bitcoin/bitcoin@61431e3 Fixes [[bitcoin/bitcoin#25563 | core#25563]]. Test Plan: `src/bitcoind -dbcache=10` `src/bitcoin-cli verifychain 4 10000` This produces the following output in the bitcoind terminal: ``` 2023-12-06T12:57:54Z Verifying last 10000 blocks at level 4 2023-12-06T12:57:54Z [0%]...[10%]...[20%]...[30%]...[40%]...[50%]...Skipped verification of level >=3 (insufficient database cache size). Consider increasing -dbcache. 2023-12-06T12:57:57Z [DONE]. 2023-12-06T12:57:57Z No coin database inconsistencies in last 10000 blocks (9992 transactions) ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D14921

Summary: This allows to log a timestamp for each entry, and avoids potential interference with other threads that could log concurrently. This concludes backport of [[bitcoin/bitcoin#27009 | core#27009]] bitcoin/bitcoin@fe683f3 Depends on D14921 Test Plan: `ninja && src/bitcoind` `src/bitcoin-cli verifychain 4 10000` Check the output of the `bitcoind` process. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D14922

mzumsande added 2 commits January 31, 2023 10:43

log: Log VerifyDB Progress over multiple lines

fe683f3

This allows to log a timestamp for each entry, and avoids potential interference with other threads that could log concurrently.

DrahtBot added the Validation label Jan 31, 2023

mzumsande mentioned this pull request Jan 31, 2023

validation: Improve error handling when VerifyDB dosn't finish successfully #25574

Merged

maflcko approved these changes Feb 3, 2023

View reviewed changes

fanquake requested a review from john-moffett February 3, 2023 16:27

john-moffett approved these changes Feb 3, 2023

View reviewed changes

fanquake merged commit 8f4ae65 into bitcoin:master Feb 5, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 5, 2023

Merge bitcoin#27009: validation: Skip VerifyDB checks of level >=3 if…

27c3bef

… dbcach…

mzumsande deleted the 202301_verifychain_assertfix branch February 6, 2023 19:17

bitcoin locked and limited conversation to collaborators Feb 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

validation: Skip VerifyDB checks of level >=3 if dbcache is too small #27009

validation: Skip VerifyDB checks of level >=3 if dbcache is too small #27009

Uh oh!

mzumsande commented Jan 31, 2023 •

edited

Loading

Uh oh!

DrahtBot commented Jan 31, 2023 •

edited

Loading

Uh oh!

maflcko commented Feb 3, 2023

Uh oh!

john-moffett left a comment

Uh oh!

john-moffett Feb 3, 2023

Uh oh!

maflcko commented Feb 6, 2023

Uh oh!

john-moffett commented Feb 6, 2023

Uh oh!

mzumsande commented Feb 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

validation: Skip VerifyDB checks of level >=3 if dbcache is too small #27009

validation: Skip VerifyDB checks of level >=3 if dbcache is too small #27009

Uh oh!

Conversation

mzumsande commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

maflcko commented Feb 3, 2023

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

john-moffett Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 6, 2023

Uh oh!

john-moffett commented Feb 6, 2023

Uh oh!

mzumsande commented Feb 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mzumsande commented Jan 31, 2023 •

edited

Loading

DrahtBot commented Jan 31, 2023 •

edited

Loading