-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: validate CbTx chainlock height diff #7363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -93,9 +93,9 @@ static bool SetStateVersion(CDeterministicMNState& state_mn, uint16_t nVersion, | |
| return true; | ||
| } | ||
|
|
||
| static bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex, const Consensus::Params& consensus_params, | ||
| const CChain& chain, const llmq::CQuorumManager& qman, | ||
| const chainlock::Chainlocks& chainlocks, BlockValidationState& state) | ||
| bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex, const Consensus::Params& consensus_params, | ||
| const CChain& chain, const llmq::CQuorumManager& qman, | ||
| const chainlock::Chainlocks& chainlocks, BlockValidationState& state) | ||
| { | ||
| if (cbTx.nVersion < CCbTx::Version::CLSIG_AND_BALANCE) { | ||
| return true; | ||
|
|
@@ -136,6 +136,10 @@ static bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex, | |
|
|
||
| // IsNull() doesn't exist for CBLSSignature: we assume that a valid BLS sig is non-null | ||
| if (cbTx.bestCLSignature.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) { | ||
| // matches our best (but outdated) clsig, no need to verify it again | ||
|
|
@@ -144,7 +148,13 @@ static bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex, | |
| cached_pindex = pindex; | ||
| return true; | ||
| } | ||
| uint256 curBlockCoinbaseCLBlockHash = pindex->GetAncestor(curBlockCoinbaseCLHeight)->GetBlockHash(); | ||
| const CBlockIndex* pAncestor = pindex->GetAncestor(curBlockCoinbaseCLHeight); | ||
| if (pAncestor == nullptr) { | ||
| // Defense-in-depth: the range check above keeps curBlockCoinbaseCLHeight in | ||
| // [0, pindex->nHeight - 1], so GetAncestor() should never return nullptr here. | ||
| return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff-ancestor"); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 Nitpick: Verified at HEAD c93049f: the commit titled source: ['claude'] |
||
| } | ||
|
thepastaclaw marked this conversation as resolved.
|
||
| uint256 curBlockCoinbaseCLBlockHash = pAncestor->GetBlockHash(); | ||
| chainlock::ChainLockSig clsig{curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, cbTx.bestCLSignature}; | ||
| llmq::VerifyRecSigStatus ret = chainlock::VerifyChainLock(consensus_params, chain, qman, clsig); | ||
| if (ret != llmq::VerifyRecSigStatus::Valid) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| // Copyright (c) 2026 The Dash Core developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #include <test/util/setup_common.h> | ||
|
|
||
| #include <bls/bls.h> | ||
| #include <chain.h> | ||
| #include <chainlock/chainlock.h> | ||
| #include <chainparams.h> | ||
| #include <consensus/validation.h> | ||
| #include <evo/cbtx.h> | ||
| #include <evo/specialtxman.h> | ||
| #include <llmq/context.h> | ||
| #include <llmq/quorumsman.h> | ||
| #include <uint256.h> | ||
| #include <validation.h> | ||
|
|
||
| #include <cstdint> | ||
| #include <limits> | ||
|
|
||
| #include <boost/test/unit_test.hpp> | ||
|
|
||
| BOOST_AUTO_TEST_SUITE(evo_cbtx_tests) | ||
|
|
||
| // Out-of-range bestCLHeightDiff (>= pindex->nHeight) must be rejected with | ||
| // "bad-cbtx-cldiff" so that the subsequent GetAncestor() call sees a valid height. | ||
| // | ||
| // The defensive nullptr branch after GetAncestor() returns "bad-cbtx-cldiff-ancestor". | ||
| // That branch is unreachable in practice (the range check guarantees the requested | ||
| // ancestor height is in [0, pindex->nHeight - 1], for which GetAncestor() never returns | ||
| // nullptr) and cannot be exercised from a unit test: a fake CBlockIndex with no pprev | ||
| // would trip GetAncestor()'s `assert(pprev)` while walking, not return nullptr. | ||
| BOOST_FIXTURE_TEST_CASE(check_cbtx_best_chainlock_rejects_excessive_height_diff, RegTestingSetup) | ||
| { | ||
| const auto& consensus_params = Params().GetConsensus(); | ||
| const auto& chain = m_node.chainman->ActiveChain(); | ||
| auto& qman = *Assert(m_node.llmq_ctx)->qman; | ||
| auto& chainlocks = *Assert(m_node.chainlocks); | ||
|
|
||
| // Standalone fake block index with no predecessor, so the prevBlockCoinbaseChainlock | ||
| // branch is skipped and the validation path under test is reached directly. | ||
| CBlockIndex pindex; | ||
| pindex.nHeight = 5; | ||
|
|
||
| // A structurally-valid BLS signature is required for the IsValid() guard. | ||
| CBLSSecretKey sk; | ||
| sk.MakeNewKey(); | ||
| const bool legacy_scheme = bls::bls_legacy_scheme.load(); | ||
| CBLSSignature valid_sig = sk.Sign(uint256::ONE, legacy_scheme); | ||
| BOOST_REQUIRE(valid_sig.IsValid()); | ||
|
|
||
| CCbTx cbTx; | ||
| cbTx.nVersion = CCbTx::Version::CLSIG_AND_BALANCE; | ||
| cbTx.bestCLSignature = valid_sig; | ||
|
|
||
| // bestCLHeightDiff == nHeight: lower boundary of the rejected range. | ||
| cbTx.bestCLHeightDiff = static_cast<uint32_t>(pindex.nHeight); | ||
| BlockValidationState state; | ||
| BOOST_CHECK(!CheckCbTxBestChainlock(cbTx, &pindex, consensus_params, chain, qman, chainlocks, state)); | ||
| BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-cbtx-cldiff"); | ||
|
|
||
| // Upper boundary: uint32_t max. | ||
| cbTx.bestCLHeightDiff = std::numeric_limits<uint32_t>::max(); | ||
| BlockValidationState state_big; | ||
| BOOST_CHECK(!CheckCbTxBestChainlock(cbTx, &pindex, consensus_params, chain, qman, chainlocks, state_big)); | ||
| BOOST_CHECK_EQUAL(state_big.GetRejectReason(), "bad-cbtx-cldiff"); | ||
| } | ||
|
thepastaclaw marked this conversation as resolved.
|
||
|
|
||
| BOOST_AUTO_TEST_SUITE_END() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 Nitpick: Follow-up commit titled
test:actually renames a production reject stringCarried forward from the prior review at
dc82f29959and still valid at84ff99fd4c. Commit84ff99fd4c("test: clarify CbTx chainlock diff ancestor guard") is not a pure test/comment change: it renames the production consensus-validation reject reason frombad-cbtx-cldifftobad-cbtx-cldiff-ancestorinsrc/evo/specialtxman.cpp(the defensiveGetAncestor() == nullptrbranch). Functionally the result is fine and the differentiated reject reasons are actually clearer than collapsing them — the issue is purely commit hygiene: Dash Core preserves PR commits on merge, so a permanent commit titledtest: ...that mutates a consensus-rejection string is misleading to futuregit log -- src/evo/specialtxman.cpp/git blame/git log -S 'bad-cbtx-cldiff-ancestor'readers. Recommendgit rebase -i --autosquashto fold84ff99fd4cintoebedd1ba5e, or at minimum reword its subject to something likefix: rename CbTx ancestor-guard reject reasonso the permanent history accurately describes the production change.source: ['claude', 'codex']