Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Makefile.test.include
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ BITCOIN_TESTS =\
test/descriptor_tests.cpp \
test/dynamic_activation_thresholds_tests.cpp \
test/evo_assetlocks_tests.cpp \
test/evo_cbtx_tests.cpp \
test/evo_deterministicmns_tests.cpp \
test/evo_islock_tests.cpp \
test/evo_mnhf_tests.cpp \
Expand Down
18 changes: 14 additions & 4 deletions src/evo/specialtxman.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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");

Copy link
Copy Markdown
Author

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 string

Carried forward from the prior review at dc82f29959 and still valid at 84ff99fd4c. Commit 84ff99fd4c ("test: clarify CbTx chainlock diff ancestor guard") is not a pure test/comment change: it renames the production consensus-validation reject reason from bad-cbtx-cldiff to bad-cbtx-cldiff-ancestor in src/evo/specialtxman.cpp (the defensive GetAncestor() == nullptr branch). 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 titled test: ... that mutates a consensus-rejection string is misleading to future git log -- src/evo/specialtxman.cpp / git blame / git log -S 'bad-cbtx-cldiff-ancestor' readers. Recommend git rebase -i --autosquash to fold 84ff99fd4c into ebedd1ba5e, or at minimum reword its subject to something like fix: rename CbTx ancestor-guard reject reason so the permanent history accurately describes the production change.

source: ['claude', 'codex']

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: test:-prefixed commit c93049f also renames a production reject string

Verified at HEAD c93049f: the commit titled test: clarify CbTx chainlock diff ancestor guard is not a pure test/comment change. Its diff against src/evo/specialtxman.cpp renames the production consensus reject reason returned at line 155 from bad-cbtx-cldiff to bad-cbtx-cldiff-ancestor (plus a new explanatory comment). Functionally the new name is an improvement — it disambiguates the unreachable defensive branch from the actively-used range-check rejection on line 141, which helps log triage. The issue is purely commit hygiene: dashpay/dash preserves PR commits on merge to develop, so a future git log --grep=cldiff or git blame on this line will land on a commit whose test: subject hides the user-visible reject-string change. Either fold the production rename into the prior fix: commit c892349 and keep only the test/comment additions here, or re-title this commit (e.g. refactor: distinguish CbTx chainlock ancestor reject reason) so the subject reflects the production rename. No code-correctness implication.

source: ['claude']

}
Comment thread
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) {
Expand Down
6 changes: 6 additions & 0 deletions src/evo/specialtxman.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class BlockValidationState;
class CBlock;
class CBlockIndex;
class CCbTx;
class CChain;
class CCoinsViewCache;
class CCreditPoolManager;
class CDeterministicMNList;
Expand Down Expand Up @@ -93,6 +94,11 @@ class CSpecialTxProcessor
};


/** Validates the bestCLSignature / bestCLHeightDiff fields embedded in a CbTx payload. */
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 CheckProRegTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev,
CDeterministicMNManager& dmnman, const CCoinsViewCache& view, const ChainstateManager& chainman,
TxValidationState& state, bool check_sigs);
Expand Down
70 changes: 70 additions & 0 deletions src/test/evo_cbtx_tests.cpp
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");
}
Comment thread
thepastaclaw marked this conversation as resolved.

BOOST_AUTO_TEST_SUITE_END()
Loading