Merge #6087: feat: stricter bestCLHeightDiff checks

6c5246803d test: add tests for both current and future behaviour (UdjinM6)
4e86bda4dc feat: stricter bestCLHeightDiff checks (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  Current `bestCLHeightDiff` checks are too relaxed

  ## What was done?
  Make`bestCLHeightDiff` checks stricter

  ## How Has This Been Tested?
  Run a node on mainnet/testet, run tests

  ## Breaking Changes
  Old nodes aren't aware of this new logic so it's activated via `mn_rr` hardfork

  ## 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 _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 6c5246803d

Tree-SHA512: 2028d0ceb00a2270c92831ef38488d009d0bac47be4fc6a23ac93efdcf74847f1b9e99a529863fb4e14c65f120adda4e12c5b9e084d0f667d5f0fbaf80e3701d

src/evo/cbtx.cpp

@@ -330,7 +330,7 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre
 }
 
 bool CheckCbTxBestChainlock(const CBlock& block, const CBlockIndex* pindex,
-                            const llmq::CChainLocksHandler& chainlock_handler, BlockValidationState& state)
+                            const llmq::CChainLocksHandler& chainlock_handler, BlockValidationState& state, const bool check_clhdiff)
 {
     if (block.vtx[0]->nType != TRANSACTION_COINBASE) {
         return true;
@@ -351,18 +351,18 @@ bool CheckCbTxBestChainlock(const CBlock& block, const CBlockIndex* pindex,
         return true;
     }
 
-    auto prevBlockCoinbaseChainlock = GetNonNullCoinbaseChainlock(pindex);
-    // 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 (!opt_cbTx->bestCLSignature.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");
-        }
-        int prevBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(prevBlockCoinbaseChainlock.value().second) - 1;
-        int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(opt_cbTx->bestCLHeightDiff) - 1;
-        if (curBlockCoinbaseCLHeight < prevBlockCoinbaseCLHeight) {
-            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-older-clsig");
+    if (check_clhdiff) {
+        auto 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 (!opt_cbTx->bestCLSignature.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");
+            }
+            if (opt_cbTx->bestCLHeightDiff > prevBlockCoinbaseChainlock.value().second + 1) {
+                return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-older-clsig");
+            }
         }
     }
 

src/evo/cbtx.h

@@ -96,7 +96,7 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre
                                BlockValidationState& state);
 
 bool CheckCbTxBestChainlock(const CBlock& block, const CBlockIndex* pindexPrev,
-                            const llmq::CChainLocksHandler& chainlock_handler, BlockValidationState& state);
+                            const llmq::CChainLocksHandler& chainlock_handler, BlockValidationState& state, const bool check_clhdiff);
 bool CalcCbTxBestChainlock(const llmq::CChainLocksHandler& chainlock_handler, const CBlockIndex* pindexPrev,
                            uint32_t& bestCLHeightDiff, CBLSSignature& bestCLSignature);
 

src/evo/specialtxman.cpp

@@ -200,7 +200,7 @@ bool CSpecialTxProcessor::ProcessSpecialTxsInBlock(const CBlock& block, const CB
         nTimeMerkle += nTime5 - nTime4;
         LogPrint(BCLog::BENCHMARK, "        - CheckCbTxMerkleRoots: %.2fms [%.2fs]\n", 0.001 * (nTime5 - nTime4), nTimeMerkle * 0.000001);
 
-        if (fCheckCbTxMerkleRoots && !CheckCbTxBestChainlock(block, pindex, m_clhandler, state)) {
+        if (fCheckCbTxMerkleRoots && !CheckCbTxBestChainlock(block, pindex, m_clhandler, state, DeploymentActiveAt(*pindex, m_consensus_params, Consensus::DEPLOYMENT_MN_RR))) {
             // pass the state returned by the function above
             return false;
         }

test/functional/feature_llmq_chainlocks.py

@@ -11,9 +11,11 @@ Checks LLMQs based ChainLocks
 '''
 
 import time
+from io import BytesIO
 
+from test_framework.messages import CBlock, CCbTx
 from test_framework.test_framework import DashTestFramework
-from test_framework.util import force_finish_mnsync, assert_equal, assert_raises_rpc_error
+from test_framework.util import assert_equal, assert_raises_rpc_error, force_finish_mnsync, hex_str_to_bytes, softfork_active
 
 
 class LLMQChainLocksTest(DashTestFramework):
@@ -243,6 +245,15 @@ class LLMQChainLocksTest(DashTestFramework):
             assert_equal(tip_1['cbTx']['bestCLSignature'], tip_0['cbTx']['bestCLSignature'])
             assert_equal(tip_1['cbTx']['bestCLHeightDiff'], tip_0['cbTx']['bestCLHeightDiff'] + 1)
 
+        self.log.info("Test that bestCLHeightDiff conditions are relaxed before mn_rr")
+        self.test_bestCLHeightDiff(False)
+
+        self.activate_mn_rr()
+        self.log.info("Activated mn_rr at height:" + str(self.nodes[0].getblockcount()))
+
+        self.log.info("Test that bestCLHeightDiff conditions are stricter after mn_rr")
+        self.test_bestCLHeightDiff(True)
+
     def create_chained_txs(self, node, amount):
         txid = node.sendtoaddress(node.getnewaddress(), amount)
         tx = node.getrawtransaction(txid, 1)
@@ -284,6 +295,73 @@ class LLMQChainLocksTest(DashTestFramework):
         else:
             assert "bestCLHeightDiff" not in cbtx and "bestCLSignature" not in cbtx
 
+    def test_bestCLHeightDiff(self, mn_rr_active):
+        # We need 2 blocks we can grab clsigs from
+        for _ in range(2):
+            self.wait_for_chainlocked_block_all_nodes(self.nodes[0].generate(1)[0])
+        assert_equal(softfork_active(self.nodes[1], "mn_rr"), mn_rr_active)
+        tip1_hash = self.nodes[1].getbestblockhash()
+
+        self.isolate_node(1)
+        tip0_hash = self.nodes[0].generate(1)[0]
+        block_hex = self.nodes[0].getblock(tip0_hash, 0)
+        mal_block = CBlock()
+        mal_block.deserialize(BytesIO(hex_str_to_bytes(block_hex)))
+        cbtx = CCbTx()
+        cbtx.deserialize(BytesIO(mal_block.vtx[0].vExtraPayload))
+        assert_equal(cbtx.bestCLHeightDiff, 0)
+
+        cbtx.bestCLHeightDiff = 1
+        mal_block.vtx[0].vExtraPayload = cbtx.serialize()
+        mal_block.vtx[0].rehash()
+        mal_block.hashMerkleRoot = mal_block.calc_merkle_root()
+        mal_block.solve()
+        result = self.nodes[1].submitblock(mal_block.serialize().hex())
+        assert_equal(result, "bad-cbtx-invalid-clsig")
+        assert_equal(self.nodes[1].getbestblockhash(), tip1_hash)
+
+        # Update the sig too and it should pass now
+        cbtx.bestCLSignature = hex_str_to_bytes(self.nodes[1].getblock(tip1_hash, 2)["tx"][0]["cbTx"]["bestCLSignature"])
+        mal_block.vtx[0].vExtraPayload = cbtx.serialize()
+        mal_block.vtx[0].rehash()
+        mal_block.hashMerkleRoot = mal_block.calc_merkle_root()
+        mal_block.solve()
+        result = self.nodes[1].submitblock(mal_block.serialize().hex())
+        assert_equal(result, None)
+        assert not self.nodes[1].getbestblockhash() == tip1_hash
+
+        # Revert to test another use case
+        self.nodes[1].invalidateblock(self.nodes[1].getbestblockhash())
+        assert_equal(self.nodes[1].getbestblockhash(), tip1_hash)
+
+        # Now it's too old but fails because of another reason when mn_rr is active
+        cbtx.bestCLHeightDiff = 2
+        mal_block.vtx[0].vExtraPayload = cbtx.serialize()
+        mal_block.vtx[0].rehash()
+        mal_block.hashMerkleRoot = mal_block.calc_merkle_root()
+        mal_block.solve()
+        result = self.nodes[1].submitblock(mal_block.serialize().hex())
+        assert_equal(result, "bad-cbtx-older-clsig" if mn_rr_active else "bad-cbtx-invalid-clsig")
+        assert_equal(self.nodes[1].getbestblockhash(), tip1_hash)
+
+        # Update the sig too and it should pass now when mn_rr is not active and fail otherwise
+        old_blockhash = self.nodes[1].getblockhash(self.nodes[1].getblockcount() - 1)
+        cbtx.bestCLSignature = hex_str_to_bytes(self.nodes[1].getblock(old_blockhash, 2)["tx"][0]["cbTx"]["bestCLSignature"])
+        mal_block.vtx[0].vExtraPayload = cbtx.serialize()
+        mal_block.vtx[0].rehash()
+        mal_block.hashMerkleRoot = mal_block.calc_merkle_root()
+        mal_block.solve()
+        result = self.nodes[1].submitblock(mal_block.serialize().hex())
+        if mn_rr_active:
+            assert_equal(result, "bad-cbtx-older-clsig")
+            assert_equal(self.nodes[1].getbestblockhash(), tip1_hash)
+        else:
+            assert_equal(result, None)
+            assert not self.nodes[1].getbestblockhash() == tip1_hash
+
+        self.reconnect_isolated_node(1, 0)
+        self.sync_all()
+
 
 if __name__ == '__main__':
     LLMQChainLocksTest().main()