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
8 changes: 8 additions & 0 deletions src/llmq/commitment.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,14 @@ bool CFinalCommitment::VerifySignatureAsync(const llmq::UtilParameters& util_par
LogPrint(BCLog::LLMQ, "CFinalCommitment::%s members[%s] quorumPublicKey[%s] commitmentHash[%s]\n", __func__,
ss3.str(), quorumPublicKey.ToString(), commitmentHash.ToString());
}
// GetAllQuorumMembers() legitimately returns an empty set (disabled LLMQ type, out-of-range
// quorumIndex, parentless quorum base). A commitment can never be valid without members, and
// the single-member branch below indexes members[0] unconditionally, so reject here.
if (members.empty()) {
LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] no quorum members\n", quorumHash.ToString());
return false;
Comment on lines +60 to +62

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Propagate the empty-member failure from queued verification

When an empty member set reaches block verification, this new return false does not reject the block because CQuorumBlockProcessor::ProcessBlock ignores VerifySignatureAsync's return value and then treats an empty check queue as successful. This is reachable with the accepted zero-threshold regtest/devnet quorum overrides: the count checks accept all-false member bitsets, while a genesis quorum base produces no members, so the commitment can be processed without either BLS signature being verified. Have the queued caller reject a false return (and cover that caller in the regression test) rather than relying solely on the return here.

AGENTS.md reference: AGENTS.md:L164-L164

Useful? React with 👍 / 👎.

}

if (llmq_params.is_single_member()) {
LogPrintf("pubkey operator: %s\n", members[0]->pdmnState->pubKeyOperator.Get().ToString());
if (!membersSig.VerifyInsecure(members[0]->pdmnState->pubKeyOperator.Get(), commitmentHash)) {
Expand Down
6 changes: 6 additions & 0 deletions src/llmq/net_dkg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,12 @@ void NetDKG::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStre
m_peer_manager->PeerMisbehaving(pfrom.GetId(), 10);
return;
}
// Genesis (and any parentless index) is never a valid quorum base.
if (pQuorumBaseBlockIndex->pprev == nullptr) {
LogPrintf("NetDKG -- invalid genesis/parentless quorumHash %s\n", quorumHash.ToString());
m_peer_manager->PeerMisbehaving(pfrom.GetId(), 100);
return;
}
if (!m_chainman.IsQuorumTypeEnabled(llmqType, pQuorumBaseBlockIndex->pprev)) {
LogPrintf("NetDKG -- llmqType [%d] quorums aren't active\n", std23::to_underlying(llmqType));
m_peer_manager->PeerMisbehaving(pfrom.GetId(), 100);
Expand Down
4 changes: 3 additions & 1 deletion src/llmq/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,9 @@ QuorumMembers GetAllQuorumMembers(Consensus::LLMQType llmqType, const UtilParame
static RecursiveMutex cs_indexed_members;
static std::map<Consensus::LLMQType, unordered_lru_cache<std::pair<uint256, int>, QuorumMembers, StaticSaltedHasher>> mapIndexedQuorumMembers GUARDED_BY(cs_indexed_members);

if (!util_params.m_chainman.IsQuorumTypeEnabled(llmqType, util_params.m_base_index->pprev)) {
// Genesis has pprev == nullptr and cannot host a quorum; treat as disabled.
if (util_params.m_base_index->pprev == nullptr ||
!util_params.m_chainman.IsQuorumTypeEnabled(llmqType, util_params.m_base_index->pprev)) {
return {};
}

Expand Down
35 changes: 35 additions & 0 deletions src/test/evo_utils_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
#include <test/util/setup_common.h>

#include <chainparams.h>
#include <llmq/context.h>
#include <llmq/options.h>
#include <llmq/utils.h>
#include <validation.h>

#include <boost/test/unit_test.hpp>
Expand Down Expand Up @@ -68,4 +70,37 @@ BOOST_FIXTURE_TEST_CASE(utils_IsQuorumTypeEnabled_tests_mainnet, TestingSetup)
Test(m_node);
}

// Regression: a genesis quorum base has pprev == nullptr.
// Pre-fix that pointer was fed to IsQuorumTypeEnabled's gsl::not_null parameter,
// whose converting constructor calls Expects() and so std::terminate()s the
// process. That is [[noreturn]] noexcept, not an exception, so no try/catch in
// the validation or net-processing stack could contain it.
//
// Note the two guards below are not equivalent. Passing a literal nullptr was a
// *compile* error pre-fix (not_null(std::nullptr_t) is deleted), so that line
// only pins the relaxed signature. The GetAllQuorumMembers() call is the one
// that reproduced the abort, since the null arrives through a runtime pointer.
// The end-to-end consensus path is covered by
// llmq_commitment_tests/commitment_genesis_quorum_hash_rejected_test.
BOOST_FIXTURE_TEST_CASE(genesis_quorum_base_null_pprev_safe, RegTestingSetup)
{
const CBlockIndex* genesis = WITH_LOCK(::cs_main, return m_node.chainman->ActiveTip());
BOOST_REQUIRE(genesis != nullptr);
BOOST_REQUIRE_EQUAL(genesis->nHeight, 0);
BOOST_REQUIRE(genesis->pprev == nullptr);

const auto llmq_type = Params().GetConsensus().llmqTypeChainLocks;

// IsQuorumTypeEnabled sink pattern (NetDKG passes pQuorumBaseBlockIndex->pprev).
BOOST_CHECK(!m_node.chainman->IsQuorumTypeEnabled(llmq_type, nullptr));

Comment on lines +95 to +96

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Blocking: Rewrite the non-buildable regression-test commit

Commit 78f6283a4ca adds this literal nullptr call while IsQuorumTypeEnabled() still accepts gsl::not_null<const CBlockIndex*>. The not_null(std::nullptr_t) constructor is explicitly deleted in src/gsl/pointers.h, so that commit fails at compilation rather than reproducing the claimed runtime termination. The later commit changes the API and makes the line compile, but it leaves a broken and misleading bisect point in the permanent series. Squash the test into the null-handling fix, or rewrite the test-first commit to pass a runtime pointer variable initialized from genesis->pprev so it compiles against the old signature and reaches the intended Expects() failure.

source: ['codex']

@thepastaclaw thepastaclaw Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Correction — Rewrite the non-buildable regression-test commit remains STILL VALID at 2204e009e3cbef16445ce799134cfd88a2bd2416.

The rebase preserved the same patch: commit 38804a2c75e still passes a literal nullptr while its parent API requires gsl::not_null<const CBlockIndex*>; the raw-pointer signature is introduced only by the following commit. The earlier auto-resolution text was generated incorrectly because the finding's category/hash changed between review rounds. The Codex verifier kept this as a blocking finding, and the conversation remains unresolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved in this update — Rewrite the non-buildable regression-test commit no longer present.

Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.

// GetAllQuorumMembers with m_base_index == genesis.
const llmq::UtilParameters util_params{*Assert(m_node.dmnman),
*Assert(m_node.llmq_ctx)->qsnapman,
*Assert(m_node.chainman),
genesis};
const auto members = llmq::utils::GetAllQuorumMembers(llmq_type, util_params);
BOOST_CHECK(members.empty());
}

BOOST_AUTO_TEST_SUITE_END()
44 changes: 44 additions & 0 deletions src/test/llmq_commitment_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <streams.h>
#include <util/check.h>
#include <util/strencodings.h>
#include <validation.h>

#include <boost/test/unit_test.hpp>

Expand Down Expand Up @@ -192,6 +193,49 @@ BOOST_FIXTURE_TEST_CASE(commitment_check_undersized_bitset_debug_log_test, RegTe
"unexpected v[0] in clamped log line: " + *it);
}

BOOST_FIXTURE_TEST_CASE(commitment_genesis_quorum_hash_rejected_test, RegTestingSetup)
{
// End-to-end regression: a mined TRANSACTION_QUORUM_COMMITMENT whose
// quorumHash names the genesis block. Genesis has pprev == nullptr, and
// CFinalCommitment::Verify -> GetAllQuorumMembers used to hand that null to
// ChainstateManager::IsQuorumTypeEnabled's gsl::not_null parameter, which
// std::terminate()s the node. This drives the real consensus entry point
// (CheckLLMQCommitment), so the whole chain of guards is exercised: without
// them this test aborts the test binary (SIGABRT) rather than failing.
const CBlockIndex* genesis = WITH_LOCK(::cs_main, return m_node.chainman->ActiveTip());
BOOST_REQUIRE(genesis != nullptr);
BOOST_REQUIRE_EQUAL(genesis->nHeight, 0);
BOOST_REQUIRE(genesis->pprev == nullptr);

CFinalCommitmentTxPayload payload;
payload.nVersion = CFinalCommitmentTxPayload::CURRENT_VERSION;
// CheckLLMQCommitment requires nHeight == m_base_index->nHeight + 1.
payload.nHeight = 1;
payload.commitment = CreateValidCommitment(TEST_PARAMS, genesis->GetBlockHash());
// Genesis is past regtest's V19Height (1) and TEST_PARAMS does not rotate.
payload.commitment.nVersion = CFinalCommitment::BASIC_BLS_NON_INDEXED_QUORUM_VERSION;
BOOST_REQUIRE(!payload.commitment.IsNull());

CMutableTransaction mtx;
mtx.nVersion = CTransaction::SPECIAL_VERSION;
mtx.nType = TRANSACTION_QUORUM_COMMITMENT;
SetTxPayload(mtx, payload);
const CTransaction tx{mtx};

const llmq::UtilParameters util_params{*Assert(m_node.dmnman),
*Assert(m_node.llmq_ctx)->qsnapman,
*Assert(m_node.chainman),
genesis};

TxValidationState state;
BOOST_CHECK(!llmq::CheckLLMQCommitment(util_params, tx, state));
BOOST_CHECK(state.IsInvalid());
// Reached Verify(), where the empty member set makes the validMembers bitset
// check reject. Any earlier reject reason would mean the test stopped short
// of the vulnerable code path.
BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-qc-invalid");
}

BOOST_AUTO_TEST_CASE(commitment_serialization_test)
{
// Test with valid commitment
Expand Down
7 changes: 6 additions & 1 deletion src/validation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5723,10 +5723,15 @@ bool ChainstateManager::IsSnapshotActive() const
}

bool ChainstateManager::IsQuorumTypeEnabled(const Consensus::LLMQType llmqType,
gsl::not_null<const CBlockIndex*> pindexPrev,
const CBlockIndex* pindexPrev,
std::optional<bool> optDIP0024IsActive,
std::optional<bool> optHaveDIP0024Quorums) const
{
// Null pindexPrev (genesis has no parent) means no prior height for an LLMQ type.
if (pindexPrev == nullptr) {
return false;
}

constexpr int TESTNET_LLMQ_25_67_ACTIVATION_HEIGHT = 847000;

const bool fDIP0024IsActive{optDIP0024IsActive.value_or(
Expand Down
3 changes: 2 additions & 1 deletion src/validation.h
Original file line number Diff line number Diff line change
Expand Up @@ -1091,7 +1091,8 @@ class ChainstateManager
//! ResizeCoinsCaches() as needed.
void MaybeRebalanceCaches() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

bool IsQuorumTypeEnabled(const Consensus::LLMQType llmqType, gsl::not_null<const CBlockIndex*> pindexPrev,
//! pindexPrev may be nullptr (e.g. genesis has no parent); null returns false.
bool IsQuorumTypeEnabled(const Consensus::LLMQType llmqType, const CBlockIndex* pindexPrev,
std::optional<bool> optDIP0024IsActive = std::nullopt,
std::optional<bool> optHaveDIP0024Quorums = std::nullopt) const;

Expand Down
Loading