Merge bitcoin/bitcoin#24403: Avoid implicit-integer-sign-change in VerifyLoadedChainstate

fa7991601c Fixup style of VerifyDB (MarcoFalke)
fa462ea787 Avoid implicit-integer-sign-change in VerifyLoadedChainstate (MarcoFalke)

Pull request description:

  This happens when checking all blocks (`-1`).

  To test:

  ```
  ./configure CC=clang CXX=clang++ --with-sanitizers=undefined,integer
  make
  UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./test/functional/rpc_blockchain.py

ACKs for top commit:
  theStack:
    Code-review ACK fa7991601c
  brunoerg:
    crACK fa7991601c

Tree-SHA512: bcbe6becf2fbedd21bbde83a544122e79465937346802039532143b2e4165784905a8852c0ccb088b964874df5e5550931fdde3629cbcee3ae237f2f63c43a8e

src/node/chainstate.cpp

@@ -129,8 +129,8 @@ std::optional<ChainstateLoadVerifyError> VerifyLoadedChainstate(ChainstateManage
                                                                 bool fReset,
                                                                 bool fReindexChainState,
                                                                 const Consensus::Params& consensus_params,
-                                                                unsigned int check_blocks,
+                                                                int check_blocks,
-                                                                unsigned int check_level,
+                                                                int check_level,
                                                                 std::function<int64_t()> get_unix_time_seconds)
 {
     auto is_coinsview_empty = [&](CChainState* chainstate) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {

src/node/chainstate.h

@@ -79,8 +79,8 @@ std::optional<ChainstateLoadVerifyError> VerifyLoadedChainstate(ChainstateManage
                                                                 bool fReset,
                                                                 bool fReindexChainState,
                                                                 const Consensus::Params& consensus_params,
-                                                                unsigned int check_blocks,
+                                                                int check_blocks,
-                                                                unsigned int check_level,
+                                                                int check_level,
                                                                 std::function<int64_t()> get_unix_time_seconds);
 } // namespace node
 

src/validation.cpp

@@ -3843,12 +3843,14 @@ bool CVerifyDB::VerifyDB(
 {
     AssertLockHeld(cs_main);
 
-    if (chainstate.m_chain.Tip() == nullptr || chainstate.m_chain.Tip()->pprev == nullptr)
+    if (chainstate.m_chain.Tip() == nullptr || chainstate.m_chain.Tip()->pprev == nullptr) {
         return true;
+    }
 
     // Verify blocks in the best chain
-    if (nCheckDepth <= 0 || nCheckDepth > chainstate.m_chain.Height())
+    if (nCheckDepth <= 0 || nCheckDepth > chainstate.m_chain.Height()) {
         nCheckDepth = chainstate.m_chain.Height();
+    }
     nCheckLevel = std::max(0, std::min(4, nCheckLevel));
     LogPrintf("Verifying last %i blocks at level %i\n", nCheckDepth, nCheckLevel);
     CCoinsViewCache coins(&coinsview);
@@ -3863,14 +3865,15 @@ bool CVerifyDB::VerifyDB(
 
     for (pindex = chainstate.m_chain.Tip(); pindex && pindex->pprev; pindex = pindex->pprev) {
         const int percentageDone = std::max(1, std::min(99, (int)(((double)(chainstate.m_chain.Height() - pindex->nHeight)) / (double)nCheckDepth * (nCheckLevel >= 4 ? 50 : 100))));
-        if (reportDone < percentageDone/10) {
+        if (reportDone < percentageDone / 10) {
             // report every 10% step
             LogPrintf("[%d%%]...", percentageDone); /* Continued */
-            reportDone = percentageDone/10;
+            reportDone = percentageDone / 10;
         }
         uiInterface.ShowProgress(_("Verifying blocks…").translated, percentageDone, false);
-        if (pindex->nHeight <= chainstate.m_chain.Height()-nCheckDepth)
+        if (pindex->nHeight <= chainstate.m_chain.Height() - nCheckDepth) {
             break;
+        }
         if ((fPruneMode || is_snapshot_cs) && !(pindex->nStatus & BLOCK_HAVE_DATA)) {
             // If pruning or running under an assumeutxo snapshot, only go
             // back as far as we have data.
@@ -3879,12 +3882,14 @@ bool CVerifyDB::VerifyDB(
         }
         CBlock block;
         // check level 0: read from disk
-        if (!ReadBlockFromDisk(block, pindex, consensus_params))
+        if (!ReadBlockFromDisk(block, pindex, consensus_params)) {
             return error("VerifyDB(): *** ReadBlockFromDisk failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
+        }
         // check level 1: verify block validity
-        if (nCheckLevel >= 1 && !CheckBlock(block, state, consensus_params))
+        if (nCheckLevel >= 1 && !CheckBlock(block, state, consensus_params)) {
             return error("%s: *** found bad block at %d, hash=%s (%s)\n", __func__,
                          pindex->nHeight, pindex->GetBlockHash().ToString(), state.ToString());
+        }
         // check level 2: verify undo validity
         if (nCheckLevel >= 2 && pindex) {
             CBlockUndo undo;
@@ -3912,8 +3917,9 @@ bool CVerifyDB::VerifyDB(
         }
         if (ShutdownRequested()) return true;
     }
-    if (pindexFailure)
+    if (pindexFailure) {
         return error("VerifyDB(): *** coin database inconsistencies found (last %i blocks, %i good transactions before that)\n", chainstate.m_chain.Height() - pindexFailure->nHeight + 1, nGoodTransactions);
+    }
 
     // store block count as we move pindex at check level >= 4
     int block_count = chainstate.m_chain.Height() - pindex->nHeight;
@@ -3922,10 +3928,10 @@ bool CVerifyDB::VerifyDB(
     if (nCheckLevel >= 4) {
         while (pindex != chainstate.m_chain.Tip()) {
             const int percentageDone = std::max(1, std::min(99, 100 - (int)(((double)(chainstate.m_chain.Height() - pindex->nHeight)) / (double)nCheckDepth * 50)));
-            if (reportDone < percentageDone/10) {
+            if (reportDone < percentageDone / 10) {
                 // report every 10% step
                 LogPrintf("[%d%%]...", percentageDone); /* Continued */
-                reportDone = percentageDone/10;
+                reportDone = percentageDone / 10;
             }
             uiInterface.ShowProgress(_("Verifying blocks…").translated, percentageDone, false);
             pindex = chainstate.m_chain.Next(pindex);

test/functional/rpc_blockchain.py

@@ -69,7 +69,14 @@ class BlockchainTest(BitcoinTestFramework):
         self.wallet = MiniWallet(self.nodes[0])
         self.mine_chain()
         self._test_max_future_block_time()
-        self.restart_node(0, extra_args=['-stopatheight=207', '-prune=1'])  # Set extra args with pruning after rescan is complete
+        self.restart_node(
+            0,
+            extra_args=[
+                "-stopatheight=207",
+                "-checkblocks=-1",  # Check all blocks
+                "-prune=1",  # Set pruning after rescan is complete
+            ],
+        )
 
         self._test_getblockchaininfo()
         self._test_getchaintxstats()