Merge #6656: perf: use tiny cache for best chainlock in CbTx instead running heavy ReadBlockFromDisk

18044c9 perf: use tiny cache for best chainlock in CbTx instead running heavy ReadBlockFromDisk (Konstantin Akimov)
66c9592 refactor: use more strict scope v20 instead dip0003 (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Validation of chainlocks requires to read previous block from disk for every new block.

  ## What was done?
  Added CL from CbTx of tip of the chain.

  ## How Has This Been Tested?
  develop:
  <img width="754" alt="image" src="https://github.com/user-attachments/assets/7e1a5ab4-9634-4fc7-910e-9124d2d742d0" />
  ```
  2025-05-04T18:36:29Z [bench]         - CheckCbTxBestChainlock: 3.34ms [18.68s]
  2025-05-04T18:36:29Z [bench]   - Connect total: 54.40ms [94.48s (16.14ms/blk)]
  ```

  PR:
  <img width="754" alt="image" src="https://github.com/user-attachments/assets/462df0f1-358d-46dd-8a08-f3a0324bb282" />
  ```
  2025-05-04T18:53:18Z [bench]         - CheckCbTxBestChainlock: 3.15ms [16.41s]
  2025-05-04T18:53:18Z [bench]   - Connect total: 52.89ms [90.75s (15.51ms/blk)]
  ```

  `perf` looks a bit confusing (like cpu spent for CheckCbTxBestChainlock is close to zero which contradicts to logs time (16seconds vs 18seconds). It happened due to missing data for bls / gmp - somehow they are not attached to the caller, but shown as a root nodes without proper stacktrace). Detailed break-down is:
  ```
  2025-05-04T19:50:37Z [bench]         - CheckCbTxBestChainlock::payload: 0.22ms [1.34s]
  2025-05-04T19:50:37Z [bench]         - CheckCbTxBestChainlock::get-cb-cl: 0.52ms [2.50s]
  2025-05-04T19:50:37Z [bench]         - CheckCbTxBestChainlock::validate: 2.63ms [14.94s] <--- missing in perf
  2025-05-04T19:50:37Z [bench]         - CheckCbTxBestChainlock: 3.38ms [18.87s]
  ```

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 18044c9
  PastaPastaPasta:
    utACK 18044c9

Tree-SHA512: 21508505e4b3cec9cba7f3835e2b73a81461d7b626f97efb15d442767fa0862f6ba92ed1101e00b04dbbddab1b7c56da796b15c89d80c95ce9b0246bbbbe02f2

Author: PastaPastaPasta

src/chainparams.cpp

@@ -921,6 +921,7 @@ class CRegTestParams : public CChainParams {
         UpdateLLMQTestParametersFromArgs(args, Consensus::LLMQType::LLMQ_TEST_INSTANTSEND);
         UpdateLLMQTestParametersFromArgs(args, Consensus::LLMQType::LLMQ_TEST_PLATFORM);
         UpdateLLMQInstantSendDIP0024FromArgs(args);
+        assert(consensus.V20Height >= consensus.DIP0003Height);
     }
 
     /**

src/evo/cbtx.cpp

@@ -331,13 +331,26 @@ bool CheckCbTxBestChainlock(const CBlock& block, const CBlockIndex* pindex,
         return true;
     }
 
+    static Mutex cached_mutex;
+    static const CBlockIndex* cached_pindex GUARDED_BY(cached_mutex){nullptr};
+    static std::optional<std::pair<CBLSSignature, uint32_t>> cached_chainlock GUARDED_BY(cached_mutex){std::nullopt};
+
     auto best_clsig = chainlock_handler.GetBestChainLock();
     if (best_clsig.getHeight() == pindex->nHeight - 1 && cbTx.bestCLHeightDiff == 0 && cbTx.bestCLSignature == best_clsig.getSig()) {
         // matches our best clsig which still hold values for the previous block
+        LOCK(cached_mutex);
+        cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff);
+        cached_pindex = pindex;
         return true;
     }
 
-    const auto prevBlockCoinbaseChainlock = GetNonNullCoinbaseChainlock(pindex->pprev);
+    std::optional<std::pair<CBLSSignature, uint32_t>> prevBlockCoinbaseChainlock{std::nullopt};
+    if (LOCK(cached_mutex); cached_pindex == pindex->pprev) {
+        prevBlockCoinbaseChainlock = cached_chainlock;
+    }
+    if (!prevBlockCoinbaseChainlock.has_value()) {
+        prevBlockCoinbaseChainlock = GetNonNullCoinbaseChainlock(pindex->pprev);
+    }
     // If std::optional prevBlockCoinbaseChainlock is empty, then up to the previous block, coinbase Chainlock is null.
     if (prevBlockCoinbaseChainlock.has_value()) {
         // Previous block Coinbase has a non-null Chainlock: current block's Chainlock must be non-null and at least as new as the previous one
@@ -355,12 +368,18 @@ bool CheckCbTxBestChainlock(const CBlock& block, const CBlockIndex* pindex,
         int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(cbTx.bestCLHeightDiff) - 1;
         if (best_clsig.getHeight() == curBlockCoinbaseCLHeight && best_clsig.getSig() == cbTx.bestCLSignature) {
             // matches our best (but outdated) clsig, no need to verify it again
+            LOCK(cached_mutex);
+            cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff);
+            cached_pindex = pindex;
             return true;
         }
         uint256 curBlockCoinbaseCLBlockHash = pindex->GetAncestor(curBlockCoinbaseCLHeight)->GetBlockHash();
         if (chainlock_handler.VerifyChainLock(llmq::CChainLockSig(curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, cbTx.bestCLSignature)) != llmq::VerifyRecSigStatus::Valid) {
             return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-invalid-clsig");
         }
+        LOCK(cached_mutex);
+        cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff);
+        cached_pindex = pindex;
     } else if (cbTx.bestCLHeightDiff != 0) {
         // Null bestCLSignature is allowed only with bestCLHeightDiff = 0
         return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff");
@@ -437,8 +456,8 @@ std::optional<std::pair<CBLSSignature, uint32_t>> GetNonNullCoinbaseChainlock(co
         return std::nullopt;
     }
 
-    // There's no CbTx before DIP0003 activation
-    if (!DeploymentActiveAt(*pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_DIP0003)) {
+    // There's no CL in CbTx before v20 activation
+    if (!DeploymentActiveAt(*pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V20)) {
         return std::nullopt;
     }
 
@@ -454,14 +473,9 @@ std::optional<std::pair<CBLSSignature, uint32_t>> GetNonNullCoinbaseChainlock(co
         return std::nullopt;
     }
 
-    const CCbTx& cbtx = opt_cbtx.value();
-    if (cbtx.nVersion < CCbTx::Version::CLSIG_AND_BALANCE) {
-        return std::nullopt;
-    }
-
-    if (!cbtx.bestCLSignature.IsValid()) {
+    if (!opt_cbtx->bestCLSignature.IsValid()) {
         return std::nullopt;
     }
 
-    return std::make_pair(cbtx.bestCLSignature, cbtx.bestCLHeightDiff);
+    return std::make_pair(opt_cbtx->bestCLSignature, opt_cbtx->bestCLHeightDiff);
 }

test/functional/feature_assumevalid.py

@@ -62,7 +62,7 @@ class AssumeValidTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 3
-        self.extra_args = ["-dip3params=9000:9000", "-checkblockindex=0"]
+        self.extra_args = ["-dip3params=9000:9000", '-testactivationheight=v20@9000', "-checkblockindex=0"]
         self.rpc_timeout = 120
 
     def setup_network(self):

test/functional/feature_csv_activation.py

@@ -100,6 +100,7 @@ def set_test_params(self):
             '-peertimeout=999999',  # bump because mocktime might cause a disconnect otherwise
             '[email protected]',
             '-dip3params=2000:2000',
+            '-testactivationheight=v20@2000',
             f'-testactivationheight=csv@{CSV_ACTIVATION_HEIGHT}',
             '-par=1',  # Use only one script thread to get the exact reject reason for testing
         ]]