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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
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)) {
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.
IsSnapshotActive() in ChainstateManager doesn’t have a comment though part of the public interface.
This would avoid hiding legitimate errors if the background chainstate is missing blocks.
Good catch! Fixed.
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
VerifyDBcan be run on multiple chainstates and a corresponding rename.
IIUC the two tweaks are:
AppInitMain to call VerifyDB on all chainstates, not just the active oneVerifyDB to treat a snapshot chainstate like a pruned chainstate and not fail trying to read missing blocksAnd everything else is cleanups and renames
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();
size_t here you can drop the cast below.
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).
Code Review ACK cd93f827
I agree with other reviewers, would be better to split in its own commit renaming.
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) {
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.
To prepare VerifyDB semantics for multiple
chainstate use.
Removes assumptions of use only on the active chainstate.
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) {
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)) {
review ACK 844ad0eccaa1dbfefc30d19804d95d67c3d5f06d 🐥
Signature:
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3review ACK 844ad0eccaa1dbfefc30d19804d95d67c3d5f06d 🐥
4-----BEGIN PGP SIGNATURE-----
5
6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
7pUicJAv9ExcrfQAEyZXbnZ9PJ6yiL++S/RdO9zGQpraFXR3geg2Jzez5Lmnj+KYr
8+xHKFZVGVFTL1CdmHFivTAe8HRLMoyaGmzQDfwJl8WBm96EKVWg+Gk9gtvWp+6Pl
9Qv9M2uyKuKbSCAS/dKBhPzcsK6o41Ji2zrqmc9VhaS3uWA16VxNeaRAAmf0cBymL
10pOBb/EbsmgpuxeyefrTgR+bvT/jQ4t15r9TC3QBVVKH6X/bNfB0Tzb1vKv34DmX0
11rtGprvutdAgmHwxnsFhyeRT9ZIrasO2/0bMWEuaH/xqbjcDLa98bf7RV/0acPvsp
12HQR/iC0xXoGWa4Zj4ydrbGr18HrG9PYc6sAqxG5LQ/KIFgyUco/0PDEDpO35wYEr
133Cv7scuEEoeVBH1tzjss8dp7sCRArnSISzQMm45X9ZqjmsBjjvUtPGzkHgRh8xz/
14mgfhIg/V1MNOdCRXxCLnQ8gsw98LiwTMvLXPGSbB66qUACqvBaqCS9bWpdcQByd2
15AIGXoVA5
16=gUOg
17-----END PGP SIGNATURE-----
Timestamp of file with hash 9b4c4c61593ad7aa160c2fb769c97159e09b00e6c5ba6cc0c987f607bb34c988 -