From 706d3901818813d286b1bf0b8243a36fcd657d63 Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 3 Aug 2026 09:58:20 -0500 Subject: [PATCH] fix: handle null pprev in IsQuorumTypeEnabled instead of terminating IsQuorumTypeEnabled took gsl::not_null, but three call sites pass a pprev that is null when the quorum base index is genesis: GetAllQuorumMembers, the DKG message handler, and indirectly CFinalCommitment::Verify. The not_null converting constructor calls Expects(), which routes to a [[noreturn]] noexcept terminate, so this is a process abort that the try/catch in CheckSpecialTx cannot contain. It is reached before any signature check. A qfcommit from an unauthenticated peer reaches it while the node is below the DKG interval, i.e. during a fresh sync and trivially on regtest/devnet; the DKG path is gated only on a verified proRegTx, so any masternode operator can abort any node at any height. Accept a raw pointer and return false on null, with defence-in-depth null checks at the call sites. Also reject commitments with an empty member set, which closes an out-of-bounds read at members[0]. That check has no consensus implication: an empty member set already fails the validMembers/signers bitset loop, and an aggregate BLS verify over an empty pubkey vector can never return true. --- src/llmq/commitment.cpp | 8 ++++++ src/llmq/net_dkg.cpp | 6 ++++ src/llmq/utils.cpp | 4 ++- src/test/evo_utils_tests.cpp | 35 ++++++++++++++++++++++++ src/test/llmq_commitment_tests.cpp | 44 ++++++++++++++++++++++++++++++ src/validation.cpp | 7 ++++- src/validation.h | 3 +- 7 files changed, 104 insertions(+), 3 deletions(-) diff --git a/src/llmq/commitment.cpp b/src/llmq/commitment.cpp index 596c9f8599c8..8bfb10b5dd92 100644 --- a/src/llmq/commitment.cpp +++ b/src/llmq/commitment.cpp @@ -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; + } + 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)) { diff --git a/src/llmq/net_dkg.cpp b/src/llmq/net_dkg.cpp index e533aadf8777..d4d3895ba520 100644 --- a/src/llmq/net_dkg.cpp +++ b/src/llmq/net_dkg.cpp @@ -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); diff --git a/src/llmq/utils.cpp b/src/llmq/utils.cpp index 106af38c2c68..d81ff80946ae 100644 --- a/src/llmq/utils.cpp +++ b/src/llmq/utils.cpp @@ -637,7 +637,9 @@ QuorumMembers GetAllQuorumMembers(Consensus::LLMQType llmqType, const UtilParame static RecursiveMutex cs_indexed_members; static std::map, 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 {}; } diff --git a/src/test/evo_utils_tests.cpp b/src/test/evo_utils_tests.cpp index 3c892dda8128..4de10a0db5a3 100644 --- a/src/test/evo_utils_tests.cpp +++ b/src/test/evo_utils_tests.cpp @@ -5,7 +5,9 @@ #include #include +#include #include +#include #include #include @@ -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)); + + // 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() diff --git a/src/test/llmq_commitment_tests.cpp b/src/test/llmq_commitment_tests.cpp index d9bebee9cb87..a083b900e056 100644 --- a/src/test/llmq_commitment_tests.cpp +++ b/src/test/llmq_commitment_tests.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include @@ -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 diff --git a/src/validation.cpp b/src/validation.cpp index bd99df8bc8d6..2fbe014cb32f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5723,10 +5723,15 @@ bool ChainstateManager::IsSnapshotActive() const } bool ChainstateManager::IsQuorumTypeEnabled(const Consensus::LLMQType llmqType, - gsl::not_null pindexPrev, + const CBlockIndex* pindexPrev, std::optional optDIP0024IsActive, std::optional 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( diff --git a/src/validation.h b/src/validation.h index 3fb064739d83..c46694377cc8 100644 --- a/src/validation.h +++ b/src/validation.h @@ -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 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 optDIP0024IsActive = std::nullopt, std::optional optHaveDIP0024Quorums = std::nullopt) const;