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

1. 
   mzumsande commented at 6:39 pm on January 31, 2023: contributor

   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

2. validation: Skip VerifyDB checks of level >=3 if dbcache is too small

   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 #25563.

   61431e3a57
3. log: Log VerifyDB Progress over multiple lines

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

   fe683f3524
4. 
   DrahtBot commented at 6:39 pm on January 31, 2023: contributor

   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.

5. DrahtBot added the label Validation on Jan 31, 2023
6. 
   maflcko commented at 1:51 pm on February 3, 2023: member

   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 fe683f352480245add0b27fe7efef5fef4c1e8c3 🗄

   Signature:

    0-----BEGIN PGP SIGNED MESSAGE-----
    1Hash: SHA512
    2
    3review ACK fe683f352480245add0b27fe7efef5fef4c1e8c3 🗄
    4-----BEGIN PGP SIGNATURE-----
    5
    6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    7pUgyFQwAhO+FMR6aO4RkWEgkOTsUA11ZyFae6aiDhoo2/4sQ4zW/+ssqZxGZxTkH
    8UwOc1MPXm1rPGvMhfNsHke9WWi6mfmp9G4MeXdyph9PiOFzoCYUWEaz8eTbba2ay
    9x+SuLJAlP0JTu6/kVCSp8GmoMTNoR+dt6kuRWO4DSloIK6yBRi+uUjdYFZAIvbsE
   10JMv9+Te6Yty2HeMNxbn7CBbigLfv+bppbeQKdrKy25EoDHMBVDbANkbipvUbShyw
   11Rc6pz9N1Uo9afdVazMLwzBvzy7OtADGvrAxP4cgyF6p5GKzDbXSvaxH88p2x4NSF
   12+3WBM8bHxTqU0YWWeuaKIz2XAc9lU3Ho72pQ1Hod7uDlSNiQck6rSlQALs8Szp/g
   13BLRBr2I6Qa06sbpyB3/rGdPt+nqcPF4X9CzJCOmuoi4+H44P4+5jDtXx4AY79Inp
   14jpZixdEFeru/sL/i8tSzoIDPG0A4uUHB1Z87FYJD9mH9gXrPwCiWPBc3lzfw6rLi
   15Go9R0uBP
   16=H0N/
   17-----END PGP SIGNATURE-----
   

7. maflcko approved
8. fanquake requested review from john-moffett on Feb 3, 2023
9. in src/validation.cpp:4177 in fe683f3524

   4173@@ -4167,8 +4174,7 @@ bool CVerifyDB::VerifyDB(
   4174         }
   4175     }
   4176 
   4177-    LogPrintf("[DONE].\n");
   4178-    LogPrintf("No coin database inconsistencies in last %i blocks (%i transactions)\n", block_count, nGoodTransactions);
   4179+    LogPrintf("Verification: No coin database inconsistencies in last %i blocks (%i transactions)\n", block_count, nGoodTransactions);
   

   ---

   ---

   

   john-moffett commented at 6:12 pm on February 3, 2023:

   Nano-Nit: Would prefer “Verification complete: "
10. john-moffett approved
11. 
    john-moffett commented at 7:21 pm on February 3, 2023: contributor

    ACK fe683f352480245add0b27fe7efef5fef4c1e8c3

    I’m definitely uncomfortable with:

    0src/bitcoin-cli -signet verifychain 4 10000
    1true
    

    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.

12. fanquake merged this on Feb 5, 2023
13. fanquake closed this on Feb 5, 2023

    ---

14. sidhujag referenced this in commit 27c3befb95 on Feb 5, 2023
15. 
    maflcko commented at 8:52 am on February 6, 2023: member

    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.

16. 
    john-moffett commented at 2:11 pm on February 6, 2023: contributor

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

17. mzumsande deleted the branch on Feb 6, 2023
18. 
    mzumsande commented at 9:12 pm on February 6, 2023: contributor

    Let’s continue this discussion in #25574.
19. achow101 referenced this in commit 832fa2d238 on Feb 22, 2023
20. Fabcien referenced this in commit 38a301820d on Dec 7, 2023
21. Fabcien referenced this in commit 294916caf9 on Dec 7, 2023
22. bitcoin locked this on Feb 6, 2024

Contributors
mzumsande DrahtBot maflcko john-moffett

Labels
Validation