From 86105daab3a23069ffa01508d6bbef8be2630c19 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 20 Nov 2024 11:28:35 -0600 Subject: [PATCH] Merge #6408: refactor: removed pre-MN_RR logic of validation of CL 3f2e064b18bdb47bbf0100307d2b339ebe124998 refactor: set `const auto& cbTx` to avoid using optional throughout method (pasta) 0c0d91e49189739c4ba1eb3a36d7586e2499a619 fix: functional test feature_llmq_chainlocks.py should activate MN_RR instead v20 (Konstantin Akimov) af93e877f29828bb268ab8b472192a47186160e1 refactor: removed pre-MN_RR logic of validation of CL (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented The fork MN_RR is active awhile on testnet and mainnet and no more need legacy check ## What was done? Removes legacy version of checks for CL and related functional tests ## How Has This Been Tested? Run unit / functional test - DONE Reindex testnet - DONE Reindex mainnet - DONE ## Breaking Changes Removed pre-mn_rr version of checks for CL. It's no more relevant on mainnet and testnet. It affects behavior on new devnets and regtest for pre-mn_rr activation blocks. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK 3f2e064b18bdb47bbf0100307d2b339ebe124998 Tree-SHA512: 8b4b3a20a54602f4df9d98e17c79004214493b15c0bce9c08c68d667a5cba86b817947f008d646c48ef9f2f86676c02085c7d0ed36e83548ef5425b64faffb89 --- src/evo/cbtx.cpp | 39 +++++++++++----------- src/evo/cbtx.h | 2 +- src/evo/specialtxman.cpp | 2 +- test/functional/feature_llmq_chainlocks.py | 33 ++++++------------ 4 files changed, 32 insertions(+), 44 deletions(-) diff --git a/src/evo/cbtx.cpp b/src/evo/cbtx.cpp index a9a5abe762f8e..fc08bd5db57fd 100644 --- a/src/evo/cbtx.cpp +++ b/src/evo/cbtx.cpp @@ -332,7 +332,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 bool check_clhdiff) + const llmq::CChainLocksHandler& chainlock_handler, BlockValidationState& state) { if (block.vtx[0]->nType != TRANSACTION_COINBASE) { return true; @@ -342,44 +342,43 @@ bool CheckCbTxBestChainlock(const CBlock& block, const CBlockIndex* pindex, if (!opt_cbTx) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-payload"); } + const auto& cbTx = *opt_cbTx; - if (opt_cbTx->nVersion < CCbTx::Version::CLSIG_AND_BALANCE) { + if (cbTx.nVersion < CCbTx::Version::CLSIG_AND_BALANCE) { return true; } auto best_clsig = chainlock_handler.GetBestChainLock(); - if (best_clsig.getHeight() == pindex->nHeight - 1 && opt_cbTx->bestCLHeightDiff == 0 && opt_cbTx->bestCLSignature == best_clsig.getSig()) { + 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 return true; } - 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"); - } + const 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 (!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 (cbTx.bestCLHeightDiff > prevBlockCoinbaseChainlock.value().second + 1) { + return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-older-clsig"); } } // IsNull() doesn't exist for CBLSSignature: we assume that a valid BLS sig is non-null - if (opt_cbTx->bestCLSignature.IsValid()) { - int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast(opt_cbTx->bestCLHeightDiff) - 1; - if (best_clsig.getHeight() == curBlockCoinbaseCLHeight && best_clsig.getSig() == opt_cbTx->bestCLSignature) { + if (cbTx.bestCLSignature.IsValid()) { + int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast(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 return true; } uint256 curBlockCoinbaseCLBlockHash = pindex->GetAncestor(curBlockCoinbaseCLHeight)->GetBlockHash(); - if (chainlock_handler.VerifyChainLock(llmq::CChainLockSig(curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, opt_cbTx->bestCLSignature)) != llmq::VerifyRecSigStatus::Valid) { + if (chainlock_handler.VerifyChainLock(llmq::CChainLockSig(curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, cbTx.bestCLSignature)) != llmq::VerifyRecSigStatus::Valid) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-invalid-clsig"); } - } else if (opt_cbTx->bestCLHeightDiff != 0) { + } else if (cbTx.bestCLHeightDiff != 0) { // Null bestCLSignature is allowed only with bestCLHeightDiff = 0 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff"); } diff --git a/src/evo/cbtx.h b/src/evo/cbtx.h index cb8ab1f92cc4f..ad6b74d10fb90 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 bool check_clhdiff); + const llmq::CChainLocksHandler& chainlock_handler, BlockValidationState& state); 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 c8c84e84bed44..5ed1e08c10ba0 100644 --- a/src/evo/specialtxman.cpp +++ b/src/evo/specialtxman.cpp @@ -188,7 +188,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, DeploymentActiveAt(*pindex, m_consensus_params, Consensus::DEPLOYMENT_MN_RR))) { + if (fCheckCbTxMerkleRoots && !CheckCbTxBestChainlock(block, pindex, m_clhandler, state)) { // pass the state returned by the function above return false; } diff --git a/test/functional/feature_llmq_chainlocks.py b/test/functional/feature_llmq_chainlocks.py index 03807ddd2ba30..1714393939b00 100755 --- a/test/functional/feature_llmq_chainlocks.py +++ b/test/functional/feature_llmq_chainlocks.py @@ -15,12 +15,12 @@ from test_framework.messages import CBlock, CCbTx from test_framework.test_framework import DashTestFramework -from test_framework.util import assert_equal, assert_raises_rpc_error, force_finish_mnsync, softfork_active +from test_framework.util import assert_equal, assert_raises_rpc_error, force_finish_mnsync class LLMQChainLocksTest(DashTestFramework): def set_test_params(self): - self.set_dash_test_params(5, 4, [["-testactivationheight=mn_rr@1100"]] * 5) + self.set_dash_test_params(5, 4) def run_test(self): # Connect all nodes to node1 so that we always have the whole network connected @@ -31,8 +31,8 @@ def run_test(self): self.test_coinbase_best_cl(self.nodes[0], expected_cl_in_cb=False) - self.activate_v20(expected_activation_height=900) - self.log.info("Activated v20 at height:" + str(self.nodes[0].getblockcount())) + self.activate_mn_rr(expected_activation_height=900) + self.log.info("Activated MN_RR at height:" + str(self.nodes[0].getblockcount())) # v20 is active for the next block, not for the tip self.test_coinbase_best_cl(self.nodes[0], expected_cl_in_cb=False) @@ -243,14 +243,8 @@ def run_test(self): 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(expected_activation_height=1100) - 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) + self.log.info("Test bestCLHeightDiff restrictions") + self.test_bestCLHeightDiff() def create_chained_txs(self, node, amount): txid = node.sendtoaddress(node.getnewaddress(), amount) @@ -293,11 +287,10 @@ def test_coinbase_best_cl(self, node, expected_cl_in_cb=True, expected_null_cl=F else: assert "bestCLHeightDiff" not in cbtx and "bestCLSignature" not in cbtx - def test_bestCLHeightDiff(self, mn_rr_active): + def test_bestCLHeightDiff(self): # We need 2 blocks we can grab clsigs from for _ in range(2): self.wait_for_chainlocked_block_all_nodes(self.generate(self.nodes[0], 1, sync_fun=self.no_op)[0]) - assert_equal(softfork_active(self.nodes[1], "mn_rr"), mn_rr_active) tip1_hash = self.nodes[1].getbestblockhash() self.isolate_node(1) @@ -339,10 +332,10 @@ def test_bestCLHeightDiff(self, mn_rr_active): 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(result, "bad-cbtx-older-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 + # Update the sig too and it should fail old_blockhash = self.nodes[1].getblockhash(self.nodes[1].getblockcount() - 1) cbtx.bestCLSignature = bytes.fromhex(self.nodes[1].getblock(old_blockhash, 2)["tx"][0]["cbTx"]["bestCLSignature"]) mal_block.vtx[0].vExtraPayload = cbtx.serialize() @@ -350,12 +343,8 @@ def test_bestCLHeightDiff(self, mn_rr_active): 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 + assert_equal(result, "bad-cbtx-older-clsig") + assert_equal(self.nodes[1].getbestblockhash(), tip1_hash) self.reconnect_isolated_node(1, 0) self.sync_all()