perf: deserialize CbTx bestCLSignature lazily and avoid verifying of duplicated chainlocks

src/chainlock/handler.cpp

@@ -110,7 +110,7 @@ MessageProcessingResult ChainlockHandler::ProcessCoinbaseChainLock(const CBlock&
     }
 
     const auto cbtx = GetTxPayload<CCbTx>(*block.vtx.front(), /*assert_type=*/false);
-    if (!cbtx || cbtx->nVersion < CCbTx::Version::CLSIG_AND_BALANCE || !cbtx->bestCLSignature.IsValid()) {
+    if (!cbtx || cbtx->nVersion < CCbTx::Version::CLSIG_AND_BALANCE || !cbtx->bestCLSignature.Get().IsValid()) {
         return {};
     }
 
@@ -126,7 +126,7 @@ MessageProcessingResult ChainlockHandler::ProcessCoinbaseChainLock(const CBlock&
     if (ancestor == nullptr) {
         return {};
     }
-    const ChainLockSig clsig{height, ancestor->GetBlockHash(), cbtx->bestCLSignature};
+    const ChainLockSig clsig{height, ancestor->GetBlockHash(), cbtx->bestCLSignature.Get()};
     return ProcessNewChainLock(/*from=*/-1, clsig, qman, ::SerializeHash(clsig));
 }
 

src/evo/cbtx.cpp

@@ -51,6 +51,23 @@ bool CheckCbTx(const CCbTx& cbTx, const CBlockIndex* pindexPrev, bool is_v24_act
         }
     }
 
+    // The signature is deserialized lazily. Get() decodes it in the active scheme only and
+    // resets the wrapper on failure, which is what the eager parser did with bytes encoded
+    // in the opposite scheme; bytes that decode in neither scheme made it throw, so they
+    // are rejected here.
+    if (cbTx.nVersion >= CCbTx::Version::CLSIG_AND_BALANCE) {
+        CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
+        ss << cbTx.bestCLSignature;
+        if (!cbTx.bestCLSignature.Get().IsValid()) {
+            try {
+                CBLSSignature sig;
+                ss >> sig;
+            } catch (const std::ios_base::failure&) {
+                return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-payload");
+            }
+        }
+    }
+
     return true;
 }
 
@@ -203,8 +220,8 @@ std::optional<std::pair<CBLSSignature, uint32_t>> GetNonNullCoinbaseChainlock(co
     }
     const auto payload = GetTxPayload<CCbTx>(*block.vtx[0]);
     if (!payload || payload->nVersion < CCbTx::Version::CLSIG_AND_BALANCE || payload->nHeight != height ||
-        !payload->bestCLSignature.IsValid() || payload->bestCLHeightDiff >= uint32_t(height)) {
+        !payload->bestCLSignature.Get().IsValid() || payload->bestCLHeightDiff >= uint32_t(height)) {
         return std::nullopt;
     }
-    return std::make_pair(payload->bestCLSignature, payload->bestCLHeightDiff);
+    return std::make_pair(payload->bestCLSignature.Get(), payload->bestCLHeightDiff);
 }

src/evo/cbtx.h

@@ -44,7 +44,7 @@ class CCbTx
     uint256 merkleRootMNList;
     uint256 merkleRootQuorums;
     uint32_t bestCLHeightDiff{0};
-    CBLSSignature bestCLSignature;
+    CBLSLazySignature bestCLSignature;
     CAmount creditPoolBalance{0};
     /** Merkle root over the instance hashes of the block's version 2+ asset unlock transactions
      *  (block order; null when there are none). Their txids exclude the quorum signing info, so

src/evo/specialtxman.cpp

@@ -163,32 +163,40 @@ bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex, const
     if (cbTx.nVersion < CCbTx::Version::CLSIG_AND_BALANCE) {
         return true;
     }
-
+    const CBLSSignature& cbtx_sig{cbTx.bestCLSignature.Get()};
+
+    struct CachedChainlock {
+        const CBlockIndex* pindex{nullptr};
+        CBLSSignature sig;
+        uint32_t height_diff{0};
+        //! Passed VerifyChainLock in this process (or was carried over from such a block) rather than
+        //! merely matched the chainlock handler's best lock, whose verification scheme is unknown here
+        bool verified_here{false};
+    };
     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};
+    static std::optional<CachedChainlock> cached GUARDED_BY(cached_mutex);
 
     auto best_clsig = chainlocks.GetBestChainLock();
     if (best_clsig.getHeight() == pindex->nHeight - 1 && cbTx.bestCLHeightDiff == 0 &&
-        cbTx.bestCLSignature == best_clsig.getSig()) {
+        cbtx_sig == 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;
+        WITH_LOCK(cached_mutex, cached.emplace(pindex, cbtx_sig, cbTx.bestCLHeightDiff, /*verified_here=*/false));
         return true;
     }
 
     std::optional<std::pair<CBLSSignature, uint32_t>> prevBlockCoinbaseChainlock{std::nullopt};
-    if (LOCK(cached_mutex); cached_pindex == pindex->pprev) {
-        prevBlockCoinbaseChainlock = cached_chainlock;
+    bool prev_verified_here{false};
+    if (LOCK(cached_mutex); cached && cached->pindex == pindex->pprev) {
+        prevBlockCoinbaseChainlock = std::make_pair(cached->sig, cached->height_diff);
+        prev_verified_here = cached->verified_here;
     }
     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
-        if (!cbTx.bestCLSignature.IsValid()) {
+        if (!cbtx_sig.IsValid()) {
             // IsNull() doesn't exist for CBLSSignature: we assume that a non valid BLS sig is null
             return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-null-clsig");
         }
@@ -198,17 +206,23 @@ bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex, const
     }
 
     // IsNull() doesn't exist for CBLSSignature: we assume that a valid BLS sig is non-null
-    if (cbTx.bestCLSignature.IsValid()) {
+    if (cbtx_sig.IsValid()) {
         // Reject out-of-range bestCLHeightDiff that would yield a pre-genesis ancestor height.
         if (cbTx.bestCLHeightDiff >= static_cast<uint32_t>(pindex->nHeight)) {
             return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff");
         }
         int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(cbTx.bestCLHeightDiff) - 1;
-        if (best_clsig.getHeight() == curBlockCoinbaseCLHeight && best_clsig.getSig() == cbTx.bestCLSignature) {
+        if (best_clsig.getHeight() == curBlockCoinbaseCLHeight && best_clsig.getSig() == cbtx_sig) {
             // 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;
+            WITH_LOCK(cached_mutex, cached.emplace(pindex, cbtx_sig, cbTx.bestCLHeightDiff, /*verified_here=*/false));
+            return true;
+        }
+        // The previous block's coinbase chainlock was verified by this process; carried into this
+        // block with the offset grown by one it names the same height, block and signature.
+        if (prev_verified_here && prevBlockCoinbaseChainlock.has_value() &&
+            cbTx.bestCLHeightDiff == prevBlockCoinbaseChainlock->second + 1 &&
+            cbtx_sig == prevBlockCoinbaseChainlock->first) {
+            WITH_LOCK(cached_mutex, cached.emplace(pindex, cbtx_sig, cbTx.bestCLHeightDiff, /*verified_here=*/true));
             return true;
         }
         const CBlockIndex* pAncestor = pindex->GetAncestor(curBlockCoinbaseCLHeight);
@@ -218,14 +232,12 @@ bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex, const
             return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff-ancestor");
         }
         uint256 curBlockCoinbaseCLBlockHash = pAncestor->GetBlockHash();
-        chainlock::ChainLockSig clsig{curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, cbTx.bestCLSignature};
+        chainlock::ChainLockSig clsig{curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, cbtx_sig};
         llmq::VerifyRecSigStatus ret = chainlock::VerifyChainLock(consensus_params, chain, qman, clsig);
         if (ret != 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;
+        WITH_LOCK(cached_mutex, cached.emplace(pindex, cbtx_sig, cbTx.bestCLHeightDiff, /*verified_here=*/true));
     } else if (cbTx.bestCLHeightDiff != 0) {
         // Null bestCLSignature is allowed only with bestCLHeightDiff = 0
         return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff");

src/kernel/chainparams.cpp

@@ -1031,6 +1031,9 @@ class CRegTestParams : public CChainParams {
         assert(consensus.V20Height >= consensus.DIP0003Height);
         // MN_RR reallocate part of reward to CreditPool which exits since V20
         assert(consensus.MN_RRHeight >= consensus.V20Height);
+        // Coinbase chainlocks (V20) are verified once and reused by the next block, so the BLS
+        // scheme must not change after V20 activation
+        assert(consensus.V20Height >= consensus.V19Height);
     }
 
 private:

src/node/miner.cpp

@@ -354,7 +354,10 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
                 throw std::runtime_error(strprintf("%s: CalcCbTxMerkleRootQuorums failed: %s", __func__, state.ToString()));
             }
             if (fV20Active_context) {
-                if (CalcCbTxBestChainlock(m_chainlocks, pindexPrev, cbTx.bestCLHeightDiff, cbTx.bestCLSignature)) {
+                CBLSSignature best_cl_sig;
+                const bool has_best_cl{CalcCbTxBestChainlock(m_chainlocks, pindexPrev, cbTx.bestCLHeightDiff, best_cl_sig)};
+                cbTx.bestCLSignature.Set(best_cl_sig, bls::bls_legacy_scheme.load());
+                if (has_best_cl) {
                     LogPrintf("CreateNewBlock() h[%d] CbTx bestCLHeightDiff[%d] CLSig[%s]\n", nHeight, cbTx.bestCLHeightDiff, cbTx.bestCLSignature.ToString());
                 } else {
                     // not an error

src/test/evo_cbtx_tests.cpp

@@ -22,9 +22,11 @@
 #include <llmq/params.h>
 #include <primitives/block.h>
 #include <primitives/transaction.h>
+#include <streams.h>
 #include <uint256.h>
 #include <validation.h>
 
+#include <algorithm>
 #include <cstdint>
 #include <limits>
 #include <utility>
@@ -66,7 +68,7 @@ BOOST_FIXTURE_TEST_CASE(check_cbtx_best_chainlock_rejects_excessive_height_diff,
 
     CCbTx cbTx;
     cbTx.nVersion = CCbTx::Version::CLSIG_AND_BALANCE;
-    cbTx.bestCLSignature = valid_sig;
+    cbTx.bestCLSignature.Set(valid_sig, legacy_scheme);
 
     // bestCLHeightDiff == nHeight: lower boundary of the rejected range.
     cbTx.bestCLHeightDiff = static_cast<uint32_t>(pindex.nHeight);
@@ -79,6 +81,20 @@ BOOST_FIXTURE_TEST_CASE(check_cbtx_best_chainlock_rejects_excessive_height_diff,
     BlockValidationState state_big;
     BOOST_CHECK(!CheckCbTxBestChainlock(cbTx, &pindex, consensus_params, chain, qman, chainlocks, state_big));
     BOOST_CHECK_EQUAL(state_big.GetRejectReason(), "bad-cbtx-cldiff");
+
+    // The signature is deserialized lazily, so the payload parser accepts any bytes in its
+    // place and CheckCbTx rejects an encoding that is not a signature; the valid signature
+    // above and the null encoding pass.
+    TxValidationState tx_state;
+    BOOST_CHECK(CheckCbTx(cbTx, /*pindexPrev=*/nullptr, /*is_v24_active=*/false, tx_state));
+    std::vector<uint8_t> sig_bytes(CBLSSignature::SerSize, 0xff);
+    CDataStream(sig_bytes, SER_NETWORK, PROTOCOL_VERSION) >> cbTx.bestCLSignature;
+    BOOST_CHECK(!CheckCbTx(cbTx, /*pindexPrev=*/nullptr, /*is_v24_active=*/false, tx_state));
+    BOOST_CHECK_EQUAL(tx_state.GetRejectReason(), "bad-cbtx-payload");
+    std::fill(sig_bytes.begin(), sig_bytes.end(), 0);
+    CDataStream(sig_bytes, SER_NETWORK, PROTOCOL_VERSION) >> cbTx.bestCLSignature;
+    TxValidationState null_state;
+    BOOST_CHECK(CheckCbTx(cbTx, /*pindexPrev=*/nullptr, /*is_v24_active=*/false, null_state));
 }
 
 namespace {

src/test/llmq_chainlock_tests.cpp

@@ -71,7 +71,7 @@ BOOST_FIXTURE_TEST_CASE(historical_coinbase_lookup_from_disk, RegTestingSetup)
             } else {
                 certified = activation + 16999;
             }
-            payload.bestCLSignature = signature;
+            payload.bestCLSignature.Set(signature, bls::bls_legacy_scheme.load());
             payload.bestCLHeightDiff = height - certified - 1;
         }
         CMutableTransaction coinbase;
@@ -415,7 +415,7 @@ BOOST_FIXTURE_TEST_CASE(coinbase_chainlock_processing, RegTestingSetup)
 
     CCbTx cbtx;
     cbtx.nVersion = CCbTx::Version::CLSIG_AND_BALANCE;
-    cbtx.bestCLSignature = CreateRandomBLSSignature();
+    cbtx.bestCLSignature.Set(CreateRandomBLSSignature(), bls::bls_legacy_scheme.load());
     CMutableTransaction tx;
     tx.nVersion = CTransaction::SPECIAL_VERSION;
     tx.nType = TRANSACTION_COINBASE;
@@ -439,7 +439,7 @@ BOOST_FIXTURE_TEST_CASE(coinbase_chainlock_processing, RegTestingSetup)
         set_payload();
         process(&indexes.back());
         const int32_t height = 499 - offset;
-        const ChainLockSig expected{height, hashes[height], cbtx.bestCLSignature};
+        const ChainLockSig expected{height, hashes[height], cbtx.bestCLSignature.Get()};
         BOOST_CHECK(m_node.clhandler->AlreadyHave(CInv{MSG_CLSIG, ::SerializeHash(expected)}));
     }
     const auto seen = m_node.clhandler->SeenChainLockCacheSizeForTesting();
@@ -464,7 +464,7 @@ BOOST_FIXTURE_TEST_CASE(coinbase_chainlock_processing, RegTestingSetup)
     process(&indexes.back());
     BOOST_REQUIRE(m_node.sporkman->UpdateSpork(SPORK_19_CHAINLOCKS_ENABLED, 0).has_value());
 
-    cbtx.bestCLSignature = {};
+    cbtx.bestCLSignature = CBLSLazySignature{};
     set_payload();
     process(&indexes.back());
     cbtx.nVersion = CCbTx::Version::MERKLE_ROOT_QUORUMS;

src/test/quorum_proofs_tests.cpp

@@ -163,11 +163,12 @@ struct QuorumProofGenerationSetup : TestingSetup {
                 });
                 BOOST_REQUIRE(match != commitments.end());
                 const size_t key = size_t(match - commitments.begin());
-                payload.bestCLSignature = keys[key].Sign(llmq::SignHash{params.type, signer->quorumHash,
-                                                                        chainlock::GenSigRequestId(height - 1),
-                                                                        previous->GetBlockHash()}
-                                                             .Get(),
-                                                         false);
+                payload.bestCLSignature.Set(keys[key].Sign(llmq::SignHash{params.type, signer->quorumHash,
+                                                                           chainlock::GenSigRequestId(height - 1),
+                                                                           previous->GetBlockHash()}
+                                                                .Get(),
+                                                            false),
+                                            bls::bls_legacy_scheme.load());
             }
             const bool mining = height == checkpoint_height - params.dkgInterval || height == checkpoint_height ||
                                 (mine_nonstandard_handoff && height == handoff_height);

test/functional/feature_protx_version.py

@@ -38,6 +38,8 @@ def set_test_params(self):
             f'-vbparams=v24:{self.mocktime}:999999999999:350:10:8:6:5:0',
         ]] * 2
         self.set_dash_test_params(2, 1, evo_count=2, extra_args=self.extra_args)
+        # regtest requires v20 not earlier than v19
+        self.delay_v20_and_mn_rr(200)
 
     def get_peer_ids(self, node_idx):
         return {peer['id'] for peer in self.nodes[node_idx].getpeerinfo()}