From 4e86bda4dc024c3b91e4c5f3e4d70cf0c15a77be Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 28 Jun 2024 20:37:47 +0300 Subject: [PATCH 1/2] feat: stricter bestCLHeightDiff checks --- src/evo/cbtx.cpp | 26 +++++++++++++------------- src/evo/cbtx.h | 2 +- src/evo/specialtxman.cpp | 2 +- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index 7c4201bfbd..6f4c171b21 100644 --- a/src/evo/cbtx.cpp +++ b/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(prevBlockCoinbaseChainlock.value().second) - 1; - int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast(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"); + } } } diff --git a/src/evo/cbtx.h b/src/evo/cbtx.h index 5e4d443edf..dccfb56212 100644 --- a/src/evo/cbtx.h +++ b/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); diff --git a/src/evo/specialtxman.cpp b/src/evo/specialtxman.cpp index 3bee881d5e..9f3c61070e 100644 --- a/src/evo/specialtxman.cpp +++ b/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; } From 6c5246803d90623b8d2a7d76945ff62eea5229d3 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 28 Jun 2024 22:13:04 +0300 Subject: [PATCH 2/2] test: add tests for both current and future behaviour --- test/functional/feature_llmq_chainlocks.py | 80 +++++++++++++++++++++- 1 file changed, 79 insertions(+), 1 deletion(-) diff --git a/test/functional/feature_llmq_chainlocks.py b/test/functional/feature_llmq_chainlocks.py index abc3dd6af7..a74920e062 100755 --- a/test/functional/feature_llmq_chainlocks.py +++ b/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()