validation: run VerifyDB on all chainstates #21523

pull jamesob wants to merge 3 commits into bitcoin:master from jamesob:2021-03-au-verifydb changing 4 files +41 −26
  1. jamesob commented at 4:32 PM on March 24, 2021: member

    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.

  2. jamesob force-pushed on Mar 24, 2021
  3. jamesob commented at 4:40 PM on March 24, 2021: member

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

  4. jamesob renamed this:
    validation: parameterize VerifyDB by chainstate
    validation: run VerifyDB on all chainstates
    on Mar 24, 2021
  5. DrahtBot added the label RPC/REST/ZMQ on Mar 24, 2021
  6. DrahtBot added the label Validation on Mar 24, 2021
  7. DrahtBot commented at 9:40 PM on March 24, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  8. DrahtBot cross-referenced this on Mar 24, 2021 from issue doc: Address feedback from Transifex translator community by hebasto
  9. DrahtBot cross-referenced this on Mar 24, 2021 from issue [Bundle 5/n] Prune g_chainman usage in RPC modules by dongcarl
  10. fanquake added this to the "Blockers" column in a project
  11. fanquake added this to the "In progress" column in a project
  12. in src/validation.cpp:4284 in cd93f827eb outdated 
    4288 |          uiInterface.ShowProgress(_("Verifying blocks...").translated, percentageDone, false);
4289 | -        if (pindex->nHeight <= active_chainstate.m_chain.Height()-nCheckDepth)
4290 | +        if (pindex->nHeight <= chainstate.m_chain.Height()-nCheckDepth)
4291 |              break;
4292 | -        if (fPruneMode && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
4293 | +        if ((fPruneMode || g_chainman.IsSnapshotActive()) && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
    ryanofsky commented at 8:31 PM on April 8, 2021:

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

    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.

    ariard commented at 7:31 PM on April 12, 2021:

    nit: IsSnapshotActive() in ChainstateManager doesn't have a comment though part of the public interface.

    jamesob commented at 2:42 PM on April 13, 2021:

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

    Good catch! Fixed.

  13. ryanofsky approved
  14. ryanofsky commented at 8:39 PM on April 8, 2021: contributor

    Code review ACK cd93f827eb1178c3b001b45af5c062bf0be2cf05. 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#issue-599846568

    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

  15. in src/validation.cpp:4311 in cd93f827eb outdated 
    4302 | @@ -4300,9 +4303,11 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CChainState& active_ch
4303 |              }
4304 |          }
4305 |          // check level 3: check for inconsistencies during memory-only disconnect of tip blocks
4306 | -        if (nCheckLevel >= 3 && (coins.DynamicMemoryUsage() + active_chainstate.CoinsTip().DynamicMemoryUsage()) <= active_chainstate.m_coinstip_cache_size_bytes) {
4307 | +        int64_t curr_coins_usage = coins.DynamicMemoryUsage() + chainstate.CoinsTip().DynamicMemoryUsage();
    fjahr commented at 8:28 PM on April 11, 2021:

    nit: I think if you make this size_t here you can drop the cast below.

  16. fjahr commented at 8:42 PM on April 11, 2021: contributor

    Code review ACK cd93f827eb1178c3b001b45af5c062bf0be2cf05

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

  17. ariard commented at 7:50 PM on April 12, 2021: member

    Code Review ACK cd93f827

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

  18. jamesob force-pushed on Apr 13, 2021
  19. jamesob force-pushed on Apr 13, 2021
  20. jamesob force-pushed on Apr 13, 2021
  21. jamesob commented at 2:51 PM on April 13, 2021: member

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

  22. ariard commented at 4:33 PM on April 13, 2021: member

    Code Review ACK c47a00d3

  23. in src/validation.cpp:4310 in de836511d8 outdated 
    4304 | @@ -4301,9 +4305,11 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CChainState& active_ch
4305 |              }
4306 |          }
4307 |          // check level 3: check for inconsistencies during memory-only disconnect of tip blocks
4308 | -        if (nCheckLevel >= 3 && (coins.DynamicMemoryUsage() + active_chainstate.CoinsTip().DynamicMemoryUsage()) <= active_chainstate.m_coinstip_cache_size_bytes) {
4309 | +        int64_t curr_coins_usage = coins.DynamicMemoryUsage() + chainstate.CoinsTip().DynamicMemoryUsage();
4310 | +
4311 | +        if (nCheckLevel >= 3 && static_cast<unsigned long>(curr_coins_usage) <= chainstate.m_coinstip_cache_size_bytes) {
    ryanofsky commented at 6:45 PM on April 13, 2021:

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

    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.

  24. ryanofsky approved
  25. ryanofsky commented at 6:56 PM on April 13, 2021: contributor

    Code review ACK c47a00d3de4ac3e62d3eee215f6180d4f3170dfb 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

  26. fjahr commented at 9:10 PM on April 14, 2021: contributor

    Code Review ACK c47a00d3de4ac3e62d3eee215f6180d4f3170dfb

  27. DrahtBot added the label Needs rebase on Apr 17, 2021
  28. refactor: rename active_chainstate in VerifyDB
    To prepare VerifyDB semantics for multiple
chainstate use.
    7901647d72
  29. validation: prepare VerifyDB for assumeutxo
    Removes assumptions of use only on the active chainstate.
    9b604c0207
  30. doc: IsSnapshotActive 844ad0ecca
  31. jamesob force-pushed on Apr 23, 2021
  32. jamesob commented at 7:14 PM on April 23, 2021: member

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

  33. DrahtBot removed the label Needs rebase on Apr 23, 2021
  34. fjahr commented at 9:36 PM on April 23, 2021: contributor

    Code review re-ACK 844ad0eccaa1dbfefc30d19804d95d67c3d5f06d

  35. in src/validation.cpp:4163 in 9b604c0207 outdated 
    4161 | @@ -4158,9 +4162,9 @@ bool CVerifyDB::VerifyDB(
4162 |              }
4163 |          }
4164 |          // check level 3: check for inconsistencies during memory-only disconnect of tip blocks
4165 | -        int64_t curr_coins_usage = coins.DynamicMemoryUsage() + chainstate.CoinsTip().DynamicMemoryUsage();
4166 | +        size_t curr_coins_usage = coins.DynamicMemoryUsage() + chainstate.CoinsTip().DynamicMemoryUsage();
4167 |  
4168 | -        if (nCheckLevel >= 3 && static_cast<unsigned long>(curr_coins_usage) <= chainstate.m_coinstip_cache_size_bytes) {
    MarcoFalke commented at 7:23 AM on April 24, 2021:

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

  36. in src/validation.cpp:4141 in 9b604c0207 outdated 
    4137 | @@ -4135,8 +4138,9 @@ bool CVerifyDB::VerifyDB(
4138 |          uiInterface.ShowProgress(_("Verifying blocks...").translated, percentageDone, false);
4139 |          if (pindex->nHeight <= chainstate.m_chain.Height()-nCheckDepth)
4140 |              break;
4141 | -        if (fPruneMode && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
4142 | -            // If pruning, only go back as far as we have data.
4143 | +        if ((fPruneMode || is_snapshot_cs) && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
    MarcoFalke commented at 7:25 AM on April 24, 2021:

    9b604c0207b526c008617cdca210f35db5e344db: 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.

    MarcoFalke commented at 8:23 AM on April 24, 2021:

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

    jamesob commented at 5:09 PM on April 26, 2021:

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

  37. MarcoFalke approved
  38. MarcoFalke commented at 7:26 AM on April 24, 2021: member

    review ACK 844ad0eccaa1dbfefc30d19804d95d67c3d5f06d 🐥

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 844ad0eccaa1dbfefc30d19804d95d67c3d5f06d 🐥
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUicJAv9ExcrfQAEyZXbnZ9PJ6yiL++S/RdO9zGQpraFXR3geg2Jzez5Lmnj+KYr
+xHKFZVGVFTL1CdmHFivTAe8HRLMoyaGmzQDfwJl8WBm96EKVWg+Gk9gtvWp+6Pl
Qv9M2uyKuKbSCAS/dKBhPzcsK6o41Ji2zrqmc9VhaS3uWA16VxNeaRAAmf0cBymL
pOBb/EbsmgpuxeyefrTgR+bvT/jQ4t15r9TC3QBVVKH6X/bNfB0Tzb1vKv34DmX0
rtGprvutdAgmHwxnsFhyeRT9ZIrasO2/0bMWEuaH/xqbjcDLa98bf7RV/0acPvsp
HQR/iC0xXoGWa4Zj4ydrbGr18HrG9PYc6sAqxG5LQ/KIFgyUco/0PDEDpO35wYEr
3Cv7scuEEoeVBH1tzjss8dp7sCRArnSISzQMm45X9ZqjmsBjjvUtPGzkHgRh8xz/
mgfhIg/V1MNOdCRXxCLnQ8gsw98LiwTMvLXPGSbB66qUACqvBaqCS9bWpdcQByd2
AIGXoVA5
=gUOg
-----END PGP SIGNATURE-----

    Timestamp of file with hash 9b4c4c61593ad7aa160c2fb769c97159e09b00e6c5ba6cc0c987f607bb34c988 -

    </details>

  39. MarcoFalke merged this on Apr 27, 2021
  40. MarcoFalke closed this on Apr 27, 2021
  41. sidhujag referenced this in commit 645aae3bfb on Apr 27, 2021
  42. fanquake removed this from the "Blockers" column in a project
  43. fanquake moved this from the "In progress" to the "Done" column in a project
  44. kwvg cross-referenced this on Oct 9, 2021 from issue merge bitcoin#15948, #15976, #16194...: assumeutxo project backports (part 1) by kwvg
  45. kwvg cross-referenced this on Feb 25, 2022 from issue merge bitcoin#16240...21526: assumeutxo project backports (part 2) by kwvg
  46. Fabcien referenced this in commit 3ffb61f603 on Apr 13, 2022
  47. gwillen referenced this in commit 6f238f5ad9 on Jun 1, 2022
  48. bitcoin locked this on Aug 18, 2022
Contributors
jamesobDrahtBotryanofskyariardfjahrMarcoFalke
Labels
RPC/REST/ZMQValidation
Linked (view graph)
#15606 assumeutxo
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC