fix: reject commitments with no quorum members and cover genesis end-to-end

GetAllQuorumMembers() can legitimately return an empty member set (disabled LLMQ type, out-of-range quorumIndex, and now a parentless quorum base). CFinalCommitment::VerifySignatureAsync indexed members[0] unconditionally on the is_single_member() branch, so an empty set is an out-of-bounds read. No built-in LLMQ type has size == 1, but regtest -llmqtestparams and devnet -llmqdevnetparams both allow it. Reject an empty member set explicitly instead.

Add llmq_commitment_tests/commitment_genesis_quorum_hash_rejected_test, which drives the real consensus entry point CheckLLMQCommitment with a mined commitment naming the genesis block. Verified that removing the IsQuorumTypeEnabled null guard makes this test abort the binary (EXIT=134), which the previous direct-call test did not. Also correct the stale reproduction comment in evo_utils_tests: the nullptr literal there was a compile error pre-fix, not the abort site.

Author: PastaPastaPasta

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)) {

src/test/evo_utils_tests.cpp

@@ -71,18 +71,17 @@ BOOST_FIXTURE_TEST_CASE(utils_IsQuorumTypeEnabled_tests_mainnet, TestingSetup)
 }
 
 // Regression: a genesis quorum base has pprev == nullptr.
-// Pre-fix that pointer was fed to IsQuorumTypeEnabled's gsl::not_null
-// parameter and aborted the process (not catchable by std::exception handlers).
+// 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.
 //
-// Verified pre-fix failure (unpatched code, debug build, aarch64-apple-darwin):
-//   ./src/test/test_dash --run_test=evo_utils_tests/genesis_quorum_base_null_pprev_safe
-//   ERROR: error detected null not_null detected at test/evo_utils_tests.cpp:100:81:test_method
-//   libc++abi: terminating
-//   fatal error: ... signal: SIGABRT (application abort requested)
-//   EXIT=134
-// Post-fix: IsQuorumTypeEnabled returns false and GetAllQuorumMembers returns
-// empty members instead of aborting. Covers the V019 GetAllQuorumMembers path
-// and the V016/U034 sink pattern used by NetDKG.
+// 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());

src/test/llmq_commitment_tests.cpp

@@ -17,6 +17,7 @@
 #include <streams.h>
 #include <util/check.h>
 #include <util/strencodings.h>
+#include <validation.h>
 
 #include <boost/test/unit_test.hpp>
 
@@ -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