feat(llmq): add trustless quorum proof chain generation and verification #7107
Conversation
This implements a trustless mechanism for verifying quorum public keys without requiring the full blockchain history. A verifier with a known checkpoint (trusted chainlock quorum public keys) can verify any subsequent quorum's authenticity through a chain of cryptographic proofs. Key components: - Chainlock indexing: Store chainlocks from cbtx during block processing - Merkle proof generation: Build proofs linking commitments to blocks - Iterative proof chain building: Handle bridging scenarios where intermediate quorums must be proven before the target - Proof verification: Validate the chain starting from checkpoint New RPCs: - getchainlockbyheight: Retrieve indexed chainlock at a specific height - getquorumproofchain: Generate proof chain from checkpoint to target - verifyquorumproofchain: Verify a proof chain and extract public key Security features: - DoS protection with MAX_PROOF_CHAIN_LENGTH (50 quorums max) - Cycle detection to prevent infinite loops - Header chain continuity verification - BLS signature verification against known quorum keys Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
WalkthroughAdds a new LLMQ quorum-proof subsystem: public headers and implementation (src/llmq/quorumproofs.{h,cpp} and src/llmq/quorumproofdata.h) implementing CQuorumProofManager with chainlock indexing, Merkle proof construction/verification, proof-chain build/verify APIs, EvoDB persistence and migrations. Wires the manager into LLMQContext, CQuorumBlockProcessor, CSpecialTxProcessor/CChainstateHelper, and init migration logic. Exposes RPCs (getchainlockbyheight, getquorumproofchain, verifyquorumproofchain), new quorum-scanning/selection helpers, fast-path mined-commitment access, and adds unit, regression, and functional tests plus test runner entries. Sequence Diagram(s)sequenceDiagram
participant Client as RPC Client
participant RPC as getquorumproofchain
participant ProofMgr as CQuorumProofManager
participant EvoDB as CEvoDB
participant QBProc as CQuorumBlockProcessor
participant Chain as CChain
Client->>RPC: Call getquorumproofchain(checkpoint, target)
RPC->>ProofMgr: BuildProofChain(checkpoint, target, qman, chain, block_man)
ProofMgr->>EvoDB: Read stored quorum/coinbase proof data
ProofMgr->>QBProc: Fetch mined commitments / block metadata
ProofMgr->>Chain: Traverse headers between checkpoint and targets
ProofMgr->>ProofMgr: Construct Merkle & coinbase proofs per step
ProofMgr-->>RPC: Return QuorumProofChain (JSON + hex)
RPC-->>Client: Respond with proof
sequenceDiagram
participant Client as RPC Client
participant RPC as verifyquorumproofchain
participant ProofMgr as CQuorumProofManager
participant EvoDB as CEvoDB
participant QBProc as CQuorumBlockProcessor
participant Crypto as BLS Crypto
Client->>RPC: Call verifyquorumproofchain(checkpoint, proof, expected)
RPC->>ProofMgr: VerifyProofChain(checkpoint, proof, expected_llmq, expected_quorumHash)
ProofMgr->>ProofMgr: Check header continuity, sizes, limits
loop per proof element
ProofMgr->>EvoDB: Optionally validate chainlock index entries
ProofMgr->>QBProc: Verify commitments and Merkle roots against block data
ProofMgr->>Crypto: Verify chainlock/quorum signatures and public keys
end
ProofMgr-->>RPC: Return QuorumProofVerifyResult (valid/error)
RPC-->>Client: Respond with verification result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/llmq/quorumproofs.cpp`:
- Around line 494-499: The code uses
step.quorum->m_quorum_base_block_index->GetAncestor(step.chainlockHeight) which
fails when step.chainlockHeight is ahead of the base block height; replace this
lookup with active_chain[step.chainlockHeight] (using the active_chain
parameter) to safely access the block at chainlockHeight with bounds checking;
update the block lookup in the function where FindChainlockCoveringBlock results
are used so it references active_chain[step.chainlockHeight] instead of
GetAncestor on m_quorum_base_block_index.
In `@src/llmq/quorumproofs.h`:
- Around line 1-6: Run the project's clang-format on the changed header to
resolve CI formatting failures: apply clang-format (or clang-format-diff) to
src/llmq/quorumproofs.h and reformat the file so it matches the repository style
(fix whitespace, alignment, include ordering, and brace/indent rules) and
re-stage the changes; the header guard BITCOIN_LLMQ_QUORUMPROOFS_H can be used
to locate the file and verify the corrected formatting.
🧹 Nitpick comments (7)
src/rpc/quorums.cpp (1)
1311-1330: Validate input object structure in ParseCheckpointFromRPC.The helper function directly accesses keys like
checkpointObj["block_hash"]andcheckpointObj["chainlock_quorums"]without first verifying they exist. If a user provides a malformed checkpoint object missing required keys, this will throw a less informative exception.Consider adding existence checks or using
.find()with appropriate error messages for better RPC error handling.♻️ Suggested improvement
static llmq::QuorumCheckpoint ParseCheckpointFromRPC(const UniValue& checkpointObj) { + if (!checkpointObj.exists("block_hash") || !checkpointObj.exists("height") || + !checkpointObj.exists("chainlock_quorums")) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Checkpoint must contain block_hash, height, and chainlock_quorums"); + } + llmq::QuorumCheckpoint checkpoint; checkpoint.blockHash = ParseHashV(checkpointObj["block_hash"], "block_hash"); // ... rest unchangedtest/functional/feature_quorum_proof_chain.py (2)
49-78: Consider catching specific JSONRPCException instead of broad Exception.The broad
except Exceptioncatches can mask unexpected failures. For RPC error handling in tests, catchingJSONRPCExceptionspecifically would be more precise and help detect actual test failures vs expected "not found" responses.♻️ Suggested improvement
+from test_framework.authproxy import JSONRPCException + # In test_chainlock_index: for h in range(tip_height, 200, -1): try: cl_info = self.nodes[0].getchainlockbyheight(h) # ... success handling - except Exception: + except JSONRPCException: continue
115-142: Consider removing or using thebuild_checkpointhelper.The
build_checkpointmethod is defined but never called in the test. If it's intended for future use withgetquorumproofchain/verifyquorumproofchaintests, consider either:
- Adding tests that exercise these RPCs using this helper, or
- Adding a TODO comment explaining the intended future use
Currently, the test only covers
getchainlockbyheightbut not the proof chain generation/verification RPCs.Would you like me to help draft additional test cases for
getquorumproofchainandverifyquorumproofchainRPCs?src/test/quorum_proofs_tests.cpp (1)
199-224: Consider adding FromJson roundtrip verification.The test verifies
ToJsonoutput structure but doesn't complete the roundtrip by parsing withFromJson. Consider adding a full roundtrip test to ensure JSON serialization is bidirectional.💡 Suggested enhancement
BOOST_CHECK_EQUAL(json["height"].getInt<int>(), 1000); + + // Verify FromJson roundtrip + llmq::QuorumCheckpoint parsed = llmq::QuorumCheckpoint::FromJson(json); + BOOST_CHECK(parsed.blockHash == checkpoint.blockHash); + BOOST_CHECK_EQUAL(parsed.height, checkpoint.height); + BOOST_CHECK_EQUAL(parsed.chainlockQuorums.size(), checkpoint.chainlockQuorums.size()); }src/llmq/quorumproofs.cpp (2)
158-178: Consider adding JSON field existence validation.
FromJsondirectly accesses JSON fields without checking existence first. If a caller provides malformed JSON missing required fields, the error message may be unclear. Consider validating field presence.💡 Suggested improvement
QuorumCheckpoint QuorumCheckpoint::FromJson(const UniValue& obj) { QuorumCheckpoint checkpoint; + if (!obj.exists("blockHash") || !obj.exists("height") || !obj.exists("chainlockQuorums")) { + throw std::runtime_error("Missing required fields in checkpoint JSON"); + } + checkpoint.blockHash = uint256S(obj["blockHash"].get_str()); checkpoint.height = obj["height"].getInt<int32_t>();
230-253: Consider consolidating duplicated merkle proof verification logic.The static
VerifyMerkleProoffunction duplicates the logic inQuorumMerkleProof::Verify. Consider having one call the other to reduce code duplication.💡 Suggested refactor
static bool VerifyMerkleProof(const uint256& leafHash, const std::vector<uint256>& merklePath, const std::vector<bool>& merklePathSide, const uint256& expectedRoot) { - if (merklePath.size() != merklePathSide.size()) { - return false; - } - - if (merklePath.size() > MAX_MERKLE_PATH_LENGTH) { - return false; - } - - uint256 current = leafHash; - for (size_t i = 0; i < merklePath.size(); ++i) { - if (merklePathSide[i]) { - current = Hash(current, merklePath[i]); - } else { - current = Hash(merklePath[i], current); - } - } - - return current == expectedRoot; + QuorumMerkleProof proof; + proof.merklePath = merklePath; + proof.merklePathSide = merklePathSide; + return proof.Verify(leafHash, expectedRoot); }src/llmq/quorumproofs.h (1)
209-222: MoveDB_CHAINLOCK_BY_HEIGHTto an anonymous namespace or make itinline.The
static const std::stringin a header creates a separate copy in each translation unit that includes this header. For a string constant used as a DB key, this wastes memory. Consider usinginline constexpr(C++17) or moving to an anonymous namespace in the .cpp file.💡 Suggested fix
Move to the .cpp file within an anonymous namespace:
// In quorumproofs.cpp namespace { const std::string DB_CHAINLOCK_BY_HEIGHT = "q_clh"; } // anonymous namespaceOr if it must remain in the header (C++17):
-static const std::string DB_CHAINLOCK_BY_HEIGHT = "q_clh"; +inline constexpr std::string_view DB_CHAINLOCK_BY_HEIGHT = "q_clh";
| // Copyright (c) 2025 The Dash Core developers | ||
| // Distributed under the MIT/X11 software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #ifndef BITCOIN_LLMQ_QUORUMPROOFS_H | ||
| #define BITCOIN_LLMQ_QUORUMPROOFS_H |
There was a problem hiding this comment.
Fix clang-format issues as indicated by CI pipeline.
The CI pipeline reports clang-format differences. Run clang-format-diff to fix formatting.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 1-1: Clang format differences found in src/llmq/quorumproofs.h. Run the clang-format-diff tool to fix formatting.
🤖 Prompt for AI Agents
In `@src/llmq/quorumproofs.h` around lines 1 - 6, Run the project's clang-format
on the changed header to resolve CI formatting failures: apply clang-format (or
clang-format-diff) to src/llmq/quorumproofs.h and reformat the file so it
matches the repository style (fix whitespace, alignment, include ordering, and
brace/indent rules) and re-stage the changes; the header guard
BITCOIN_LLMQ_QUORUMPROOFS_H can be used to locate the file and verify the
corrected formatting.
- Add trivial test case to quorum_proofs_tests to ensure suite has at least one test in all build configurations (fixes nowallet build) - Replace std::to_string with strprintf to avoid locale-dependent functions (fixes lint-locale-dependence) - Move regression tests to separate file quorum_proofs_regression_tests.cpp to comply with one-suite-per-file naming convention (fixes lint-tests) - Set executable permission on feature_quorum_proof_chain.py functional test (fixes lint-files) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/llmq/quorumproofs.cpp`:
- Around line 582-670: The header-continuity check against proof.headers assumes
consecutive blocks but BuildProofChain only supplies commitment blocks, and the
code does not tie each quorum proof to the chainlock-signed block; update the
proof verification so each QuorumCommitmentProof is anchored to its chainlock:
for each qProof, require chainlock.blockHash == header.GetHash() (using the
header selected for that qProof) instead of enforcing proof.headers are strictly
consecutive, or alternatively modify BuildProofChain to include all intermediate
headers up to chainlock.nHeight and keep the continuity check; ensure you change
the loop that checks proof.headers continuity and the place that fetches const
CBlockHeader& header = proof.headers[proofIdx] so it selects the header matching
chainlock.blockHash (or add headers in BuildProofChain) and verify
chainlock.blockHash equality before any merkle/signature checks.
In `@src/test/quorum_proofs_regression_tests.cpp`:
- Around line 147-188: The test adds two headers (chain.headers) but only one
quorum proof (chain.quorumProofs), causing VerifyProofChain to abort on a
headers/ proofs count mismatch; add a second llmq::QuorumCommitmentProof for
header2 so counts match. Create another qProof (copying the first
llmq::QuorumCommitmentProof setup used for qProof), give it a distinct
commitment.quorumHash (e.g., uint256::THREE or similar), set
qProof.chainlockIndex consistent with existing clEntry usage, assign a
coinbaseTx (CMutableTransaction mtx like before) and push_back this second
qProof into chain.quorumProofs so chain.headers.size() ==
chain.quorumProofs.size().
♻️ Duplicate comments (1)
src/llmq/quorumproofs.cpp (1)
496-499: Chainlock block lookup can return nullptr when the chainlock height is ahead of the base block.Line 496-499 uses
GetAncestor(...), which only walks backward; for chainlock heights greater than the quorum base block height this yields nullptr. Prefer looking up by height onactive_chain(as already flagged).
🧹 Nitpick comments (2)
src/llmq/quorumproofs.cpp (1)
59-83: Avoid duplicate merkle-proof verification logic.Line 59-83 and Line 231-254 implement the same hashing loop/DoS checks. Consider delegating to a single helper to prevent drift.
Also applies to: 231-254
test/functional/feature_quorum_proof_chain.py (1)
55-76: Avoid swallowing unexpected RPC errors.Bare
except Exceptionmasks real failures in the scan loops; catchingJSONRPCExceptionkeeps intent while preserving unexpected errors.♻️ Suggested refinement (apply similarly to other loops)
-from test_framework.util import assert_equal, assert_raises_rpc_error +from test_framework.util import assert_equal, assert_raises_rpc_error +from test_framework.authproxy import JSONRPCException @@ - except Exception: - continue + except JSONRPCException as e: + if e.error.get("code") != -5: + raise + self.log.debug(f"Height {h} not chainlocked yet: {e}") + continue @@ - except Exception: - continue + except JSONRPCException as e: + if e.error.get("code") != -5: + raise + self.log.debug(f"Height {h} not chainlocked yet: {e}") + continue @@ - try: - cl_quorums = self.nodes[0].quorum("list", llmq_type) - except Exception: - # If quorum list fails, try with different type - cl_quorums = [] + try: + cl_quorums = self.nodes[0].quorum("list", llmq_type) + except JSONRPCException as e: + self.log.debug(f"quorum list failed for type {llmq_type}: {e}") + cl_quorums = [] @@ - except Exception: - continue + except JSONRPCException as e: + self.log.debug(f"quorum info failed for {qhash}: {e}") + continueAlso applies to: 86-95, 120-136
| // Create proof chain with DISCONTINUOUS headers | ||
| llmq::QuorumProofChain chain; | ||
|
|
||
| CBlockHeader header1; | ||
| header1.nVersion = 1; | ||
| header1.hashPrevBlock = uint256::ZERO; | ||
| header1.hashMerkleRoot = uint256::ONE; | ||
| header1.nTime = 1234567890; | ||
| header1.nBits = 0x1d00ffff; | ||
| header1.nNonce = 1; | ||
|
|
||
| CBlockHeader header2; | ||
| header2.nVersion = 1; | ||
| // BUG TRIGGER: prevBlockHash does NOT match header1.GetHash() | ||
| header2.hashPrevBlock = uint256::TWO; // Should be header1.GetHash() | ||
| header2.hashMerkleRoot = uint256::TWO; | ||
| header2.nTime = 1234567891; | ||
| header2.nBits = 0x1d00ffff; | ||
| header2.nNonce = 2; | ||
|
|
||
| chain.headers.push_back(header1); | ||
| chain.headers.push_back(header2); | ||
|
|
||
| // Add chainlock | ||
| llmq::ChainlockProofEntry clEntry; | ||
| clEntry.nHeight = 100; | ||
| clEntry.blockHash = header1.GetHash(); | ||
| clEntry.signature = sk.Sign(clEntry.blockHash, false); | ||
| chain.chainlocks.push_back(clEntry); | ||
|
|
||
| // Add quorum proof | ||
| llmq::QuorumCommitmentProof qProof; | ||
| qProof.commitment.llmqType = Consensus::LLMQType::LLMQ_TEST; | ||
| qProof.commitment.quorumHash = uint256::TWO; | ||
| qProof.chainlockIndex = 0; | ||
|
|
||
| CMutableTransaction mtx; | ||
| mtx.nVersion = 3; | ||
| mtx.nType = TRANSACTION_COINBASE; | ||
| qProof.coinbaseTx = MakeTransactionRef(mtx); | ||
| chain.quorumProofs.push_back(qProof); | ||
|
|
There was a problem hiding this comment.
Header-continuity test currently fails for header/proof count mismatch.
Line 167-188 adds 2 headers but only 1 quorum proof, so VerifyProofChain exits early with “Headers count does not match...” and the continuity check isn’t exercised.
🧪 Proposed fix to align header/proof counts
qProof.coinbaseTx = MakeTransactionRef(mtx);
chain.quorumProofs.push_back(qProof);
+ chain.quorumProofs.push_back(qProof); // keep size in sync with headers📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create proof chain with DISCONTINUOUS headers | |
| llmq::QuorumProofChain chain; | |
| CBlockHeader header1; | |
| header1.nVersion = 1; | |
| header1.hashPrevBlock = uint256::ZERO; | |
| header1.hashMerkleRoot = uint256::ONE; | |
| header1.nTime = 1234567890; | |
| header1.nBits = 0x1d00ffff; | |
| header1.nNonce = 1; | |
| CBlockHeader header2; | |
| header2.nVersion = 1; | |
| // BUG TRIGGER: prevBlockHash does NOT match header1.GetHash() | |
| header2.hashPrevBlock = uint256::TWO; // Should be header1.GetHash() | |
| header2.hashMerkleRoot = uint256::TWO; | |
| header2.nTime = 1234567891; | |
| header2.nBits = 0x1d00ffff; | |
| header2.nNonce = 2; | |
| chain.headers.push_back(header1); | |
| chain.headers.push_back(header2); | |
| // Add chainlock | |
| llmq::ChainlockProofEntry clEntry; | |
| clEntry.nHeight = 100; | |
| clEntry.blockHash = header1.GetHash(); | |
| clEntry.signature = sk.Sign(clEntry.blockHash, false); | |
| chain.chainlocks.push_back(clEntry); | |
| // Add quorum proof | |
| llmq::QuorumCommitmentProof qProof; | |
| qProof.commitment.llmqType = Consensus::LLMQType::LLMQ_TEST; | |
| qProof.commitment.quorumHash = uint256::TWO; | |
| qProof.chainlockIndex = 0; | |
| CMutableTransaction mtx; | |
| mtx.nVersion = 3; | |
| mtx.nType = TRANSACTION_COINBASE; | |
| qProof.coinbaseTx = MakeTransactionRef(mtx); | |
| chain.quorumProofs.push_back(qProof); | |
| // Create proof chain with DISCONTINUOUS headers | |
| llmq::QuorumProofChain chain; | |
| CBlockHeader header1; | |
| header1.nVersion = 1; | |
| header1.hashPrevBlock = uint256::ZERO; | |
| header1.hashMerkleRoot = uint256::ONE; | |
| header1.nTime = 1234567890; | |
| header1.nBits = 0x1d00ffff; | |
| header1.nNonce = 1; | |
| CBlockHeader header2; | |
| header2.nVersion = 1; | |
| // BUG TRIGGER: prevBlockHash does NOT match header1.GetHash() | |
| header2.hashPrevBlock = uint256::TWO; // Should be header1.GetHash() | |
| header2.hashMerkleRoot = uint256::TWO; | |
| header2.nTime = 1234567891; | |
| header2.nBits = 0x1d00ffff; | |
| header2.nNonce = 2; | |
| chain.headers.push_back(header1); | |
| chain.headers.push_back(header2); | |
| // Add chainlock | |
| llmq::ChainlockProofEntry clEntry; | |
| clEntry.nHeight = 100; | |
| clEntry.blockHash = header1.GetHash(); | |
| clEntry.signature = sk.Sign(clEntry.blockHash, false); | |
| chain.chainlocks.push_back(clEntry); | |
| // Add quorum proof | |
| llmq::QuorumCommitmentProof qProof; | |
| qProof.commitment.llmqType = Consensus::LLMQType::LLMQ_TEST; | |
| qProof.commitment.quorumHash = uint256::TWO; | |
| qProof.chainlockIndex = 0; | |
| CMutableTransaction mtx; | |
| mtx.nVersion = 3; | |
| mtx.nType = TRANSACTION_COINBASE; | |
| qProof.coinbaseTx = MakeTransactionRef(mtx); | |
| chain.quorumProofs.push_back(qProof); | |
| chain.quorumProofs.push_back(qProof); // keep size in sync with headers |
🤖 Prompt for AI Agents
In `@src/test/quorum_proofs_regression_tests.cpp` around lines 147 - 188, The test
adds two headers (chain.headers) but only one quorum proof (chain.quorumProofs),
causing VerifyProofChain to abort on a headers/ proofs count mismatch; add a
second llmq::QuorumCommitmentProof for header2 so counts match. Create another
qProof (copying the first llmq::QuorumCommitmentProof setup used for qProof),
give it a distinct commitment.quorumHash (e.g., uint256::THREE or similar), set
qProof.chainlockIndex consistent with existing clEntry usage, assign a
coinbaseTx (CMutableTransaction mtx like before) and push_back this second
qProof into chain.quorumProofs so chain.headers.size() ==
chain.quorumProofs.size().
Key fixes: - Use mined block (minedBlockHash) instead of formation block (m_quorum_base_block_index) when looking up commitment proofs - Fix BuildQuorumMerkleProof to match CalcCbTxMerkleRootQuorums logic by using pindex->pprev and scanning current block transactions - Fix GetAncestor issue by using active_chain[] for chainlock blocks since chainlock height can be >= mined block height - Add FindChainlockSignedByKnownQuorum for direct path optimization to reduce proof chain length when checkpoint quorum is still active - Add MigrateChainlockIndex to build chainlock index from historical blocks on first run after upgrade - Increase MAX_PROOF_CHAIN_LENGTH from 50 to 500 for long proofs - Add comprehensive debug logging for troubleshooting Benchmark results (single checkpoint quorum): - 30 hours: 1 step, 1.3 KB, 0.14s - 7 days: 5 steps, 6.8 KB, 0.34s - 30 days: 26 steps, 36 KB, 1.4s - 6 months: 150 steps, 208 KB, 14s - 12 months: 292 steps, 402 KB, 26s Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This optimization significantly reduces the proof size and generation time for long quorum proof chains by intelligently selecting blocks to prove. Key improvements: - Smart Block Selection: Searches the active quorum window (up to 100 blocks) instead of relying solely on the mined block. - Direct Bridging: Prioritizes blocks signed by known quorums to eliminate intermediate steps. - Proof Size Reduction: Preferentially selects non-superblock blocks to minimize coinbase transaction size. - Efficient Bridging: When no direct bridge exists, selects the oldest signing quorum to maximize the backward jump. Benchmark Results: - ~30 days: Steps reduced by 38% (26 -> 16), Size reduced by 38% (36KB -> 22KB). - ~6 months: Steps reduced by 40% (150 -> 90), Size reduced by 40% (207KB -> 123KB). Time improved by ~24%. - ~12 months: Steps reduced by 37% (292 -> 185), Size reduced by 37% (402KB -> 252KB). Time improved by ~20%.
Removed the 'superblock avoidance' heuristic which was causing unnecessary search overhead. The algorithm now terminates immediately upon finding a block signed by a known quorum (direct bridge). Benchmark Results (~12 months): - Time: Reduced from 20.6s to 15.3s (~25% faster) - Steps/Size: Identical
Avoid unnecessary DB lookups in chainlock search loop by pruning uninteresting signers early. Eliminate redundant block reads by passing block pointer to BuildQuorumMerkleProof. Cache ChainlockIndexEntry in ProofStep to prevent re-reading during proof construction. These changes reduce proof generation time for long chains (e.g., 12 months) by ~12-13%.
- Implement GetMinedCommitmentTxHash and GetMinedCommitmentBlockHash in CQuorumBlockProcessor to avoid expensive BLS deserialization. - Add ScanCommitments and SelectCommitmentForSigning to CQuorumManager to avoid building full CQuorum objects and MN lists during scans. - Update CQuorumProofManager to utilize these optimized methods for BuildQuorumMerkleProof and BuildProofChain. - Significantly reduces CPU usage and time for generating long proof chains.
Reduce proof chain generation time by 90%+ for long chains through: 1. CachedCommitmentInfo struct: Lightweight struct containing only the fields needed for signer selection (quorumHash, publicKey, quorumIndex, llmqType, pMinedBlock), avoiding repeated full CFinalCommitment deserialization. 2. ComputeSigningCommitmentIndex function: Computes commitment selection using cached data without any database reads, replacing repeated calls to DetermineChainlockSigningCommitment -> SelectCommitmentForSigning -> ScanCommitments. 3. Per-step commitment caching: Fetches active commitments once at the start of each proof step's search window instead of per height, reducing DB reads from O(heights × commitments) to O(commitments). 4. CommitmentHashCache: Cross-step caching of commitment hashes in BuildQuorumMerkleProof, avoiding repeated GetMinedCommitmentTxHash calls for commitments shared between consecutive proof steps. Benchmark results vs baseline: - 30 hours: 0.282s -> 0.136s (52% faster) - 7 days: 0.495s -> 0.159s (68% faster) - 30 days: 2.304s -> 0.203s (91% faster) - 6 months: 10.083s -> 0.572s (94% faster) - 12 months: 15.286s -> 1.090s (93% faster) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… function Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ration Add a database index (DB_QUORUM_PROOF_DATA) that stores pre-computed proof components for each quorum commitment: - Merkle proof within merkleRootQuorums - Coinbase transaction and merkle proof - Block header This avoids expensive ReadBlockFromDisk() and merkle proof computation at query time by: - Storing proof data when commitments are mined (ProcessBlock) - Removing proof data on reorg (UndoBlock) - Migrating historical commitments on first startup BuildProofChain() now uses the cached data when available, falling back to on-the-fly computation for backwards compatibility. Performance improvement: ~20% reduction in proof generation time (1.13s -> 0.89s for 12-month range). Further optimization of Phase 2 (chain building) is needed for larger gains. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
current performance of proof generation: ==============================================
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llmq/blockprocessor.cpp (1)
1-3: Fix clang-format diffs reported by CI.The clang-format diff check is failing; please run the repo’s clang-format (or clang-format-diff) on the touched hunks and re‑stage.
🤖 Fix all issues with AI agents
In `@src/llmq/quorumproofs.cpp`:
- Around line 703-704: The ProofStep being pushed uses pProofBlock for the
mined-block pointer but documentation and fallback logic expect
ProofStep::pMinedBlockIndex to point to the block where the commitment was
mined; change the construction passed to proofSteps.push_back to use the mined
block pointer (the variable that represents the mined block for
currentCommitment) instead of pProofBlock so the fallback merkle proof builder
reads the correct block when cached data is missing (update the arguments around
proofSteps.push_back/currentCommitment to supply the mined-block index rather
than pProofBlock).
- Around line 53-85: ComputeSigningCommitmentIndex currently silently returns 0
when a rotated quorum's signer (computed from selectionHash and
llmq_params.signingActiveQuorumCount) is not found in commitments, which can
mis-attribute signers; update the rotated branch in
ComputeSigningCommitmentIndex to treat a missing quorumIndex as an explicit
failure: after computing signer, if no commitments[i].quorumIndex matches,
either throw a descriptive exception (e.g., std::runtime_error) or use an
explicit error return (e.g., return SIZE_MAX) and document that callers must
handle this error, and update any callers of ComputeSigningCommitmentIndex to
handle the new failure path; reference symbols: ComputeSigningCommitmentIndex,
llmq_params.useRotation, signingActiveQuorumCount, selectionHash,
commitments[i].quorumIndex.
In `@src/rpc/quorums.cpp`:
- Around line 1497-1499: The file src/rpc/quorums.cpp is failing clang-format;
run the formatter (e.g. clang-format-diff.py -p1 or clang-format) on the file
and apply the changes so the RPC registration lines for "evo" entries (functions
getchainlockbyheight, getquorumproofchain, verifyquorumproofchain) match the
project's style; update the file with the formatted whitespace/commas/alignment
and re-run CI to ensure clang-format diffs are resolved.
- Around line 1406-1470: The handler verifyquorumproofchain currently parses
expectedType and calls llmq_ctx.quorum_proof_manager->VerifyProofChain without
validating the LLMQ type; add a guard after parsing expectedType (the value
produced by static_cast<Consensus::LLMQType>(request.params[3].getInt<int>()))
to ensure it is a known/defined LLMQ type and return a clear RPC error
(valid=false with an explanatory message or throw RPC_INVALID_PARAMETER) if it
is not, before calling VerifyProofChain on proofChain/checkpoint.
♻️ Duplicate comments (1)
src/llmq/quorumproofs.cpp (1)
851-910: Header continuity check conflicts with proof layout; chainlocks aren’t anchored to headers.The headers here are the commitment-mined blocks, which are typically not consecutive, so the strict
prevBlockHashchain will reject multi-step proofs. Also, the chainlock signature isn’t tied to any header hash, so an unrelated header chain could still satisfy the merkle proofs. Consider either including intermediate headers up to the chainlock block, or anchoring each proof by requiringchainlock.blockHash == header.GetHash()and adjusting generation/verification accordingly.
🧹 Nitpick comments (2)
src/llmq/blockprocessor.cpp (1)
167-211: Consider consolidating the merkle-path helper.This helper is duplicated in
src/llmq/quorumproofs.cpp. Extracting a single shared implementation will reduce the risk of subtle divergence later.src/llmq/quorumproofs.h (1)
8-18: Make the header self-contained forUniValue/std::map/std::string.These types are used directly but the header doesn’t include their declarations. If they aren’t pulled transitively, this header won’t compile on its own. Consider adding explicit includes (or a
UniValueforward declaration if you prefer to keep the include light).🛠️ Proposed fix
`#include` <llmq/types.h> `#include` <primitives/block.h> `#include` <primitives/transaction.h> `#include` <serialize.h> `#include` <uint256.h> +#include <univalue.h> +#include <map> +#include <string> `#include` <set> `#include` <vector>
Move QuorumMerkleProof and QuorumProofData structs to a new header file (quorumproofdata.h) to break circular dependencies between llmq/blockprocessor, llmq/quorumproofs, evo/cbtx, and llmq/quorumsman. Also fixes trailing whitespace in quorumproofs.cpp. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
Some nits, haven't reviewed logic yet
src/evo/specialtxman.cpp
Outdated
| // This prevents indexing chainlocks from blocks during a reorg | ||
| if (!fJustCheck && opt_cbTx->bestCLSignature.IsValid() && | ||
| m_chainman.ActiveChain().Contains(pindex)) { | ||
| int32_t chainlockedHeight = pindex->nHeight - static_cast<int32_t>(opt_cbTx->bestCLHeightDiff) - 1; |
There was a problem hiding this comment.
nit: replace int32_t to int which is used all-over-codebase for height and height calculation
There was a problem hiding this comment.
m_chainman.ActiveChain().Contains(pindex)) {
Is it relevant check? I think this if should be revised
src/evo/specialtxman.cpp
Outdated
| // Remove chainlock index for this block's cbtx | ||
| if (block.vtx.size() > 0 && block.vtx[0]->nType == TRANSACTION_COINBASE) { | ||
| if (const auto opt_cbTx = GetTxPayload<CCbTx>(*block.vtx[0]); opt_cbTx && opt_cbTx->bestCLSignature.IsValid()) { | ||
| int32_t chainlockedHeight = pindex->nHeight - static_cast<int32_t>(opt_cbTx->bestCLHeightDiff) - 1; |
| @@ -0,0 +1,694 @@ | |||
| // Copyright (c) 2025 The Dash Core developers | |||
|
|
||
| class QuorumProofChainTest(DashTestFramework): | ||
| def set_test_params(self): | ||
| self.set_dash_test_params(5, 4) |
There was a problem hiding this comment.
I believe here should be 5, 3
3 masternodes and 2 regular nodes to test proof.
I also think, that this functional test should include a test with disconnected node to teset proof.
| class QuorumProofChainTest(DashTestFramework): | ||
| def set_test_params(self): | ||
| self.set_dash_test_params(5, 4) | ||
| self.delay_v20_and_mn_rr(height=200) |
There was a problem hiding this comment.
why? I believe it's no needed
| # Connect all nodes to node1 so that we always have the whole network connected | ||
| # Otherwise only masternode connections will be established between nodes | ||
| for i in range(2, len(self.nodes)): | ||
| self.connect_nodes(i, 1) |
There was a problem hiding this comment.
why? I believe it's no needed
src/rpc/quorums.cpp
Outdated
| {RPCResult::Type::STR, "error", /* optional */ true, "Error message (if invalid)"}, | ||
| }}, | ||
| RPCExamples{ | ||
| HelpExampleCli("verifyquorumproofchain", "'{...}' \"proof_hex\" \"quorum_hash\" 104") |
There was a problem hiding this comment.
'{...}' "proof_hex" "quorum_hash" 10
Looks like placeholder instead RPC name method
src/rpc/quorums.cpp
Outdated
| RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::BOOL, "valid", "Whether the proof is valid"}, | ||
| {RPCResult::Type::STR_HEX, "quorum_public_key", /* optional */ true, "Verified public key (if valid)"}, |
There was a problem hiding this comment.
| {RPCResult::Type::STR_HEX, "quorum_public_key", /* optional */ true, "Verified public key (if valid)"}, | |
| {RPCResult::Type::STR_HEX, "quorum_public_key", /*optional*=/ true, "Verified public key (if valid)"}, |
same for other instances
src/rpc/quorums.cpp
Outdated
| const UniValue& q = quorumsArr[i]; | ||
| llmq::QuorumCheckpoint::QuorumEntry entry; | ||
| entry.quorumHash = ParseHashV(q["quorum_hash"], "quorum_hash"); | ||
| entry.quorumType = static_cast<Consensus::LLMQType>(q["quorum_type"].getInt<int>()); |
There was a problem hiding this comment.
| entry.quorumType = static_cast<Consensus::LLMQType>(q["quorum_type"].getInt<int>()); | |
| entry.quorumType = static_cast<Consensus::LLMQType>(q["quorum_type"].getInt<uint8_t>()); |
because enum class LLMQType : uint8_t
src/rpc/quorums.cpp
Outdated
| { | ||
| LOCK(cs_main); | ||
| const CBlockIndex* pindex = chainman.ActiveChain()[height]; | ||
| if (pindex == nullptr) { | ||
| throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found at height"); | ||
| } | ||
| blockHash = pindex->GetBlockHash(); | ||
| } |
There was a problem hiding this comment.
| { | |
| LOCK(cs_main); | |
| const CBlockIndex* pindex = chainman.ActiveChain()[height]; | |
| if (pindex == nullptr) { | |
| throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found at height"); | |
| } | |
| blockHash = pindex->GetBlockHash(); | |
| } | |
| const CBlockIndex* pindex = WITH_LOCK(::cs_main, return chainman.ActiveChain()[height]); | |
| if (pindex == nullptr) { | |
| throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found at height"); | |
| } | |
| blockHash = pindex->GetBlockHash(); |
no need to keep cs_main for extra checks; pindex doesn't require cs_main to be kept
- Change ComputeSigningCommitmentIndex to return std::optional<size_t> to avoid silent fallback that could mis-attribute signers - Remove unnecessary fallback path in BuildProofChain (migration ensures all historical commitments are indexed) - Remove legacy header continuity check that incorrectly assumed consecutive blocks (headers are from commitment blocks spaced by DKG intervals) - Add LLMQ type validation in verifyquorumproofchain RPC - Use uint8_t for LLMQ type cast (matches enum class : uint8_t) - Reduce cs_main lock scope using WITH_LOCK - Fix /*optional=*/ syntax and RPC example placeholder - Change int32_t to int for chainlockedHeight (style consistency) - Update regression test for count mismatch validation - Fix functional test params (5,3) and remove unnecessary delay Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/llmq/quorumproofs.cpp`:
- Around line 867-897: Ensure the header at proof.headers[proofIdx] actually
matches the chainlock's signed block hash by checking header.GetHash() ==
chainlock.blockHash before using it for merkle verification; if the check fails,
set result.error (e.g. "Header does not match chainlock block hash in proof %d")
and return result. Insert this validation immediately after obtaining const
CBlockHeader& header = proof.headers[proofIdx] and prior to any merkle proof
verifications (coinbase and quorum commitment).
🧹 Nitpick comments (7)
src/llmq/quorumproofs.cpp (3)
1-1: Nit: Copyright year should be 2026.As noted in a past review comment, the current date is January 2026, but the copyright header says 2025.
🔧 Suggested fix
-// Copyright (c) 2025 The Dash Core developers +// Copyright (c) 2026 The Dash Core developers
294-338: Consider extractingBuildMerkleProofPathto a shared utility.This function is duplicated verbatim from
src/llmq/blockprocessor.cpp(lines 170-210). Consider moving it to a shared header (e.g.,src/consensus/merkle.hor a newsrc/llmq/merkle_utils.h) to avoid duplication and ensure both implementations stay in sync.
1156-1160: Progress percentage calculation is a rough estimate.The progress calculation
indexed_count / 10is a rough estimate that may not reflect actual progress. For example, if there are 500 total quorums, progress would cap at ~50% before completing. Consider tracking the total count upfront for accurate progress display, or document that this is an approximate indicator.test/functional/feature_quorum_proof_chain.py (3)
114-141:build_checkpointhelper is defined but never called.The
build_checkpointmethod is implemented but not used inrun_test. If this is intended for future test expansion (e.g., testinggetquorumproofchain/verifyquorumproofchain), consider adding a TODO comment. Otherwise, it could be removed to avoid dead code.
55-77: Consider catching specificJSONRPCExceptioninstead of bareException.The try-except-continue pattern is common in test iteration, but catching a specific exception type would be more precise and avoid masking unexpected errors:
🔧 Suggested improvement
+from test_framework.authproxy import JSONRPCException ... for h in range(tip_height, 0, -1): try: cl_info = self.nodes[0].getchainlockbyheight(h) ... return - except Exception: + except JSONRPCException: continue
43-46: Consider adding tests forgetquorumproofchainandverifyquorumproofchainRPCs.The functional test covers
getchainlockbyheightbut not the proof chain generation/verification RPCs. Thebuild_checkpointhelper suggests these were planned. Adding coverage would validate the end-to-end proof chain workflow.Would you like me to help draft additional test cases for these RPCs?
src/rpc/quorums.cpp (1)
1376-1381: Inconsistent LLMQ type parsing between RPCs.
getquorumproofchainusesgetInt<int>()(line 1377) whileverifyquorumproofchainusesgetInt<uint8_t>()(line 1462) for parsing the LLMQ type. SinceLLMQTypeisenum class LLMQType : uint8_t, consider usinguint8_tconsistently:🔧 Suggested fix
- const Consensus::LLMQType targetType = static_cast<Consensus::LLMQType>(request.params[2].getInt<int>()); + const Consensus::LLMQType targetType = static_cast<Consensus::LLMQType>(request.params[2].getInt<uint8_t>());
| // Get the corresponding header for this quorum proof | ||
| const CBlockHeader& header = proof.headers[proofIdx]; | ||
|
|
||
| // Verify coinbase tx is in the block via merkle proof | ||
| if (!qProof.coinbaseTx) { | ||
| result.error = strprintf("Missing coinbase transaction in proof %d", proofIdx); | ||
| return result; | ||
| } | ||
|
|
||
| const uint256 coinbaseTxHash = qProof.coinbaseTx->GetHash(); | ||
| QuorumMerkleProof coinbaseMerkleProof{qProof.coinbaseMerklePath, qProof.coinbaseMerklePathSide}; | ||
| if (!coinbaseMerkleProof.Verify(coinbaseTxHash, header.hashMerkleRoot)) { | ||
| result.error = strprintf("Coinbase merkle proof verification failed in proof %d", proofIdx); | ||
| return result; | ||
| } | ||
|
|
||
| // Extract merkleRootQuorums from cbtx | ||
| auto opt_cbtx = GetTxPayload<CCbTx>(*qProof.coinbaseTx); | ||
| if (!opt_cbtx.has_value()) { | ||
| result.error = strprintf("Invalid coinbase transaction payload in proof %d", proofIdx); | ||
| return result; | ||
| } | ||
|
|
||
| const CCbTx& cbtx = opt_cbtx.value(); | ||
|
|
||
| // Verify the quorum commitment merkle proof against merkleRootQuorums | ||
| uint256 commitmentHash = ::SerializeHash(qProof.commitment); | ||
| if (!qProof.quorumMerkleProof.Verify(commitmentHash, cbtx.merkleRootQuorums)) { | ||
| result.error = strprintf("Quorum commitment merkle proof verification failed in proof %d", proofIdx); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full VerifyProofChain function to understand the validation flow
cd src/llmq && wc -l quorumproofs.cppRepository: dashpay/dash
Length of output: 77
🏁 Script executed:
# Get the full VerifyProofChain function to understand the structure
rg -n 'VerifyProofChain' src/llmq/quorumproofs.cpp | head -5Repository: dashpay/dash
Length of output: 122
🏁 Script executed:
# Look at the section around lines 867-897 and the full function context
sed -n '800,920p' src/llmq/quorumproofs.cppRepository: dashpay/dash
Length of output: 5279
🏁 Script executed:
# Search for where chainlock.blockHash is used/validated
rg -n 'chainlock.*blockHash|blockHash.*chainlock' src/llmq/quorumproofs.cppRepository: dashpay/dash
Length of output: 402
🏁 Script executed:
# Search for header.GetHash() usage
rg -n 'header\.GetHash\(\)' src/llmq/quorumproofs.cppRepository: dashpay/dash
Length of output: 117
🏁 Script executed:
# Search for any comparison between header hash and chainlock blockHash
rg -n 'GetHash.*blockHash|blockHash.*GetHash' src/llmq/quorumproofs.cppRepository: dashpay/dash
Length of output: 38
Verify that the header used for merkle proof verification corresponds to the chainlock's signed block hash.
The code verifies the chainlock signature against chainlock.blockHash (line 853-854) but does not validate that the header retrieved at proof.headers[proofIdx] actually corresponds to that block. An attacker could provide a valid chainlock signature for one block while substituting a different header for merkle verification, as the chainlock signature proof only verifies the signature validity, not that the header matches.
Add a validation that header.GetHash() == chainlock.blockHash before using the header for merkle proof verification.
🤖 Prompt for AI Agents
In `@src/llmq/quorumproofs.cpp` around lines 867 - 897, Ensure the header at
proof.headers[proofIdx] actually matches the chainlock's signed block hash by
checking header.GetHash() == chainlock.blockHash before using it for merkle
verification; if the check fails, set result.error (e.g. "Header does not match
chainlock block hash in proof %d") and return result. Insert this validation
immediately after obtaining const CBlockHeader& header = proof.headers[proofIdx]
and prior to any merkle proof verifications (coinbase and quorum commitment).
| return ret; | ||
| } | ||
|
|
||
| uint256 CQuorumBlockProcessor::GetMinedCommitmentTxHash(Consensus::LLMQType llmqType, const uint256& quorumHash) const |
There was a problem hiding this comment.
the name is confusing, it's not a txhash, it's a hash of a serialized commitment message
|
|
||
| // Read block from disk | ||
| CBlock block; | ||
| if (!ReadBlockFromDisk(block, pindex, chainparams.GetConsensus())) { |
There was a problem hiding this comment.
This should be a critical error on non-pruned nodes. User must reindex.
|
|
||
| // Try to extract CCbTx from coinbase | ||
| auto opt_cbtx = GetTxPayload<CCbTx>(*block.vtx[0]); | ||
| if (!opt_cbtx.has_value()) { |
| int32_t chainlockedHeight = pindex->nHeight - static_cast<int32_t>(cbtx.bestCLHeightDiff) - 1; | ||
| const CBlockIndex* pChainlockedBlock = pindex->GetAncestor(chainlockedHeight); | ||
|
|
||
| if (pChainlockedBlock) { |
| // CQuorumProofManager implementation | ||
| // | ||
|
|
||
| void CQuorumProofManager::IndexChainlock(int32_t chainlockedHeight, const uint256& blockHash, |
| { | ||
| {"checkpoint", RPCArg::Type::OBJ, RPCArg::Optional::NO, "Checkpoint data", | ||
| { | ||
| {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "Checkpoint block hash"}, |
There was a problem hiding this comment.
we should be able to calculate checkpoint from block_hash internally, this would simplify rpc params a lot
| { | ||
| {"checkpoint", RPCArg::Type::OBJ, RPCArg::Optional::NO, "Checkpoint data", | ||
| { | ||
| {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "Checkpoint block hash"}, |
| } | ||
|
|
||
|
|
||
| static RPCHelpMan getchainlockbyheight() |
There was a problem hiding this comment.
let's maybe split this RPC (and chainlock index) into its own PR?
There was a problem hiding this comment.
I guess we could; but we need the chain lock index to support this behavior
- Change llmq_type from 104 to 100 (LLMQ_TEST for chainlocks)
- Use quorum("list") without type argument and get "llmq_test" key
- Add optional height parameter for checkpoint at specific height
- Add set_dash_llmq_test_params(3, 2) for proper quorum configuration
- Switch from mine_cycle_quorum() to mine_quorum() for type 100
- Remove unnecessary activate_v20() call
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add helper method to tamper with serialized proof hex at specified byte offsets. This will be used to test that verification correctly rejects corrupted proofs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The chainlock indexing code incorrectly checked if the block was in the active chain before indexing. This check always failed because ProcessSpecialTxsInBlock is called during ConnectBlock, but the chain tip is only updated AFTER ConnectBlock returns (via m_chain.SetTip()). This fix removes the ActiveChain().Contains() check since it's not needed - the fJustCheck flag already prevents indexing during validation only (as opposed to actual block connection). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add test_getquorumproofchain_single_step() test method - Fix V20 activation (required for chainlock signatures in cbtx) - Add debug logging for cbtx version and chainlock signature - Fix assertion key names (use camelCase 'quorumProofs' to match RPC) - Add explicit block generation after chainlocks for proper indexing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…C++ bug) Add the test method for proof chain verification, but skip it due to a bug discovered in BuildProofChain (quorumproofs.cpp:655-696). The bug: BuildProofChain incorrectly identifies which quorum signed a chainlock when quorum rotation has occurred. It uses cached commitments from the mined block height, but the actual chainlock at a later height may have been signed by different active commitments. See activity.md for full bug details. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…signature verification Two bugs prevented proof chain verification from working: 1. BuildProofChain cached chainlock signing commitments at a fixed reference height (pMinedBlock->nHeight - SIGN_HEIGHT_OFFSET) and used them for all heights in the search window. Due to quorum rotation, the active commitments at a later chainlock height can differ. Fix: use DetermineChainlockSigningCommitment() for each height, which correctly looks at commitments at (h - SIGN_HEIGHT_OFFSET). 2. VerifyProofChain used blockHash directly as the verification message, but chainlocks are signed using SignHash(llmqType, quorumHash, requestId, blockHash). Fix: add signingQuorumHash and signingQuorumType to ChainlockProofEntry (matching the DIP spec), and use proper SignHash construction for verification. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Test that tampered proof chains are correctly rejected by verifyquorumproofchain. Uses tamper_proof_hex() helper to modify a byte in the middle of a valid proof, then verifies the verification returns valid=false with an error message. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Test that verifyquorumproofchain correctly rejects proofs when the claimed target quorum hash doesn't match the proof's actual target. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Tests that proof chain verification rejects proofs when the checkpoint contains a wrong public key. Uses a different quorum's public key to ensure valid BLS format while testing the signature verification logic. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Test getquorumproofchain RPC error handling: - Invalid LLMQ type returns -8 error - Non-existent quorum hash returns -5 error Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implements multi-step proof chain generation test that: - Builds a checkpoint with current quorums - Mines 3 additional quorums to make checkpoint quorums inactive - Generates a multi-step proof from checkpoint to new quorum - Verifies the proof structure Known issue: Multi-step proof verification currently fails with 'Quorum commitment merkle proof verification failed in proof 0' which indicates a bug in the underlying C++ code. The test documents this behavior while still validating that proof generation works. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The previous implementation used cached QuorumProofData computed when the quorum was mined (at height M). When building a multi-step proof chain, chainlocks reference blocks at different heights (H) where the quorum is still active. This caused merkle proof verification to fail because the cached proofs were for the wrong block. Fix by calling ComputeQuorumProofData() for the actual chainlock block instead of using GetQuorumProofData() which returns cached data for the mined block. Also fix tests to use proper assertions instead of silent skips when test preconditions aren't met. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove unnecessary fallback to legacy BLS scheme in proof chain verification. All modern chainlocks (post-v19) use the non-legacy scheme, and compact quorum proofs will only be used with modern checkpoints. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove duplicate BuildMerkleProofPath function from blockprocessor.cpp, now shared from quorumproofs.cpp - Consolidate redundant variables in BuildProofChain - Use std::map for direct chainlock index lookup instead of set + linear search - Use aggregate initialization for proof entry structs - Remove unused knownQuorumPubKeys set in VerifyProofChain - Simplify RPC code with range-based loops and std::move Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add to .gitignore to prevent development notes from being tracked. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/rpc/quorums.cpp`:
- Around line 1350-1358: The RPC help entries in the RPCResult construction use
snake_case keys (e.g., "quorum_proofs", "proof_hex") that don't match the actual
JSON output (camelCase like quorumProofs, proofHex, quorumPublicKey); update the
RPCResult key strings inside the RPC response description to use the camelCase
names emitted by the implementation (adjust entries such as
"quorum_proofs"->"quorumProofs", "proof_hex"->"proofHex", and any similar keys
like "quorum_public_key"->"quorumPublicKey"), and make the same changes for the
other occurrence mentioned (the block around the later entries referenced) so
the help text matches the real JSON field names.
In `@test/functional/feature_quorum_proof_chain.py`:
- Line 364: The log call uses an unnecessary f-string prefix in the message;
update the call to self.log.info by removing the leading "f" so the literal
string "Mining blocks to embed chainlock signatures..." is passed (locate the
self.log.info(...) invocation in the
test/functional/feature_quorum_proof_chain.py file and replace the f-string with
a plain string).
- Around line 329-337: Adjust the indentation of the two assert_raises_rpc_error
calls so they align correctly with the surrounding code to satisfy flake8 E128;
locate the lines calling self.nodes[0].getquorumproofchain with parameters
(checkpoint, checkpoint['chainlock_quorums'][0]['quorum_hash'], 999) and
(checkpoint, fake_hash, llmq_type) and re-indent the continued argument lines to
align under the first argument of each function call (keeping the same
arguments: checkpoint, quorum hash / fake_hash, llmq_type) so the wrapped
parameters are vertically aligned.
- Around line 12-14: Replace broad except Exception handlers used around RPC
"scan" calls with a specific except JSONRPCException to only catch the expected
"not found" RPC error; import JSONRPCException from test_framework.authproxy (or
the project's authproxy module) and in those except blocks bind the exception
(e.g., except JSONRPCException as e:) to assert or check the error message,
while re-raising or letting other unexpected exceptions propagate. Apply this
change to the handlers referenced (the import area and the try/except blocks
around the scan RPC in the ranges shown: the import block near the top and the
try/except blocks currently at 94-115 and 125-134), ensuring unexpected
exceptions are not swallowed.
♻️ Duplicate comments (1)
src/llmq/quorumproofs.cpp (1)
759-772: Bind the header to the chainlock’s signed block hash.Right now the proof can pair a valid chainlock signature for block A with an unrelated header/merkle root for block B. Validate the header hash matches
chainlock.blockHashbefore merkle proof checks.🛠️ Suggested guard
const CBlockHeader& header = proof.headers[proofIdx]; + if (header.GetHash() != chainlock.blockHash) { + result.error = strprintf("Header does not match chainlock block hash in proof %d", proofIdx); + return result; + }
🧹 Nitpick comments (1)
src/llmq/blockprocessor.cpp (1)
606-634: Fast-path hash assumes SER_DISK == SER_GETHASH — add a guard/test.If
CFinalCommitmentserialization ever diverges, this path would silently compute a different hash thanSerializeHashand poison proofs. Consider a debug-only assertion or a unit test to lock the invariant. If you use the assert, add<cassert>if it's not already included.🛠️ Suggested debug guard
- return Hash(MakeByteSpan(ssValue).first(ssValue.size() - 32)); + const uint256 fast_hash = Hash(MakeByteSpan(ssValue).first(ssValue.size() - 32)); +#ifdef DEBUG + auto [commitment, _] = GetMinedCommitment(llmqType, quorumHash); + if (!commitment.IsNull()) { + assert(fast_hash == ::SerializeHash(commitment)); + } +#endif + return fast_hash;
| RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::ARR, "headers", "Block headers in the proof chain", | ||
| {{RPCResult::Type::OBJ, "", false, "Header object"}}}, | ||
| {RPCResult::Type::ARR, "chainlocks", "Chainlock proofs", | ||
| {{RPCResult::Type::OBJ, "", false, "Chainlock entry"}}}, | ||
| {RPCResult::Type::ARR, "quorum_proofs", "Quorum commitment proofs", | ||
| {{RPCResult::Type::OBJ, "", false, "Quorum proof entry"}}}, | ||
| {RPCResult::Type::STR_HEX, "proof_hex", "Serialized proof (hex)"}, |
There was a problem hiding this comment.
RPC help keys should match actual JSON output.
The response builders emit camelCase (quorumProofs, quorumPublicKey), but the help strings show snake_case. Update the help docs to avoid confusing callers.
🛠️ Suggested doc fix
- {RPCResult::Type::ARR, "quorum_proofs", "Quorum commitment proofs",
+ {RPCResult::Type::ARR, "quorumProofs", "Quorum commitment proofs",
{{RPCResult::Type::OBJ, "", false, "Quorum proof entry"}}},
@@
- {RPCResult::Type::STR_HEX, "quorum_public_key", /*optional=*/ true, "Verified public key (if valid)"},
+ {RPCResult::Type::STR_HEX, "quorumPublicKey", /*optional=*/ true, "Verified public key (if valid)"},Also applies to: 1424-1429
🤖 Prompt for AI Agents
In `@src/rpc/quorums.cpp` around lines 1350 - 1358, The RPC help entries in the
RPCResult construction use snake_case keys (e.g., "quorum_proofs", "proof_hex")
that don't match the actual JSON output (camelCase like quorumProofs, proofHex,
quorumPublicKey); update the RPCResult key strings inside the RPC response
description to use the camelCase names emitted by the implementation (adjust
entries such as "quorum_proofs"->"quorumProofs", "proof_hex"->"proofHex", and
any similar keys like "quorum_public_key"->"quorumPublicKey"), and make the same
changes for the other occurrence mentioned (the block around the later entries
referenced) so the help text matches the real JSON field names.
| from test_framework.test_framework import DashTestFramework | ||
| from test_framework.util import assert_equal, assert_raises_rpc_error | ||
|
|
There was a problem hiding this comment.
Narrow the RPC scan exception handling.
Catching Exception hides unexpected failures and triggers Ruff BLE001/S112. Prefer JSONRPCException for the expected “not found” case and log/raise anything else.
🛠️ Suggested update
-from test_framework.test_framework import DashTestFramework
-from test_framework.util import assert_equal, assert_raises_rpc_error
+from test_framework.test_framework import DashTestFramework
+from test_framework.authproxy import JSONRPCException
+from test_framework.util import assert_equal, assert_raises_rpc_error
@@
- except Exception:
- continue
+ except JSONRPCException:
+ continue
+ except Exception as e:
+ self.log.error(f"Unexpected error while scanning chainlocks: {e}")
+ raise
@@
- except Exception:
- continue
+ except JSONRPCException:
+ continue
+ except Exception as e:
+ self.log.error(f"Unexpected error while scanning chainlocks: {e}")
+ raiseAlso applies to: 94-115, 125-134
🤖 Prompt for AI Agents
In `@test/functional/feature_quorum_proof_chain.py` around lines 12 - 14, Replace
broad except Exception handlers used around RPC "scan" calls with a specific
except JSONRPCException to only catch the expected "not found" RPC error; import
JSONRPCException from test_framework.authproxy (or the project's authproxy
module) and in those except blocks bind the exception (e.g., except
JSONRPCException as e:) to assert or check the error message, while re-raising
or letting other unexpected exceptions propagate. Apply this change to the
handlers referenced (the import area and the try/except blocks around the scan
RPC in the ranges shown: the import block near the top and the try/except blocks
currently at 94-115 and 125-134), ensuring unexpected exceptions are not
swallowed.
| # Test invalid LLMQ type | ||
| assert_raises_rpc_error(-8, "Invalid LLMQ type", | ||
| self.nodes[0].getquorumproofchain, checkpoint, | ||
| checkpoint['chainlock_quorums'][0]['quorum_hash'], 999) | ||
|
|
||
| # Test non-existent quorum hash | ||
| fake_hash = "0" * 64 | ||
| assert_raises_rpc_error(-5, None, | ||
| self.nodes[0].getquorumproofchain, checkpoint, fake_hash, llmq_type) |
There was a problem hiding this comment.
Fix indentation to satisfy flake8 E128.
🧹 Suggested formatting fix
- assert_raises_rpc_error(-8, "Invalid LLMQ type",
- self.nodes[0].getquorumproofchain, checkpoint,
- checkpoint['chainlock_quorums'][0]['quorum_hash'], 999)
+ assert_raises_rpc_error(-8, "Invalid LLMQ type",
+ self.nodes[0].getquorumproofchain, checkpoint,
+ checkpoint['chainlock_quorums'][0]['quorum_hash'], 999)
@@
- assert_raises_rpc_error(-5, None,
- self.nodes[0].getquorumproofchain, checkpoint, fake_hash, llmq_type)
+ assert_raises_rpc_error(-5, None,
+ self.nodes[0].getquorumproofchain, checkpoint, fake_hash, llmq_type)🧰 Tools
🪛 Flake8 (7.3.0)
[error] 331-331: continuation line under-indented for visual indent
(E128)
[error] 337-337: continuation line under-indented for visual indent
(E128)
🤖 Prompt for AI Agents
In `@test/functional/feature_quorum_proof_chain.py` around lines 329 - 337, Adjust
the indentation of the two assert_raises_rpc_error calls so they align correctly
with the surrounding code to satisfy flake8 E128; locate the lines calling
self.nodes[0].getquorumproofchain with parameters (checkpoint,
checkpoint['chainlock_quorums'][0]['quorum_hash'], 999) and (checkpoint,
fake_hash, llmq_type) and re-indent the continued argument lines to align under
the first argument of each function call (keeping the same arguments:
checkpoint, quorum hash / fake_hash, llmq_type) so the wrapped parameters are
vertically aligned.
|
|
||
| # Mine extra blocks to embed chainlock signatures in cbtx | ||
| # This ensures the chainlock index has entries we can use for proof chains | ||
| self.log.info(f"Mining blocks to embed chainlock signatures...") |
There was a problem hiding this comment.
Remove stray f-string prefix.
🧹 Suggested formatting fix
- self.log.info(f"Mining blocks to embed chainlock signatures...")
+ self.log.info("Mining blocks to embed chainlock signatures...")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.log.info(f"Mining blocks to embed chainlock signatures...") | |
| self.log.info("Mining blocks to embed chainlock signatures...") |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 364-364: f-string is missing placeholders
(F541)
🪛 Ruff (0.14.13)
364-364: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
In `@test/functional/feature_quorum_proof_chain.py` at line 364, The log call uses
an unnecessary f-string prefix in the message; update the call to self.log.info
by removing the leading "f" so the literal string "Mining blocks to embed
chainlock signatures..." is passed (locate the self.log.info(...) invocation in
the test/functional/feature_quorum_proof_chain.py file and replace the f-string
with a plain string).
3 participants
NOTE: vibe coded; untested by human. Implements this DIP dashpay/dips#175
Issue being fixed or feature implemented
This implements a trustless mechanism for verifying quorum public keys without requiring the full blockchain history. This is needed by Platform SDK to verify quorum authenticity starting from a known checkpoint (trusted chainlock quorum public keys).
The core problem solved: When a light client has a checkpoint with known chainlock quorum public keys, it needs to verify newer quorums that weren't in the checkpoint. This requires "bridging" - proving intermediate quorums in a chain until reaching the target.
What was done?
Core Implementation (
src/llmq/quorumproofs.cpp,src/llmq/quorumproofs.h)merkleRootQuorumsSelectQuorumForSigninglogicNew RPCs (
src/rpc/quorums.cpp)getchainlockbyheight: Retrieve indexed chainlock at a specific heightgetquorumproofchain: Generate proof chain from checkpoint to target quorumverifyquorumproofchain: Verify a proof chain and extract the target quorum's public keySecurity Features
MAX_PROOF_CHAIN_LENGTH(50 quorums max)Integration
CSpecialTxProcessor::ProcessSpecialTxsInBlockUndoSpecialTxsInBlockfor reorg handlingCQuorumProofManageradded toLLMQContextHow Has This Been Tested?
Unit Tests (
src/test/quorum_proofs_tests.cpp)forged_chainlock_signature_rejected: Verifies BLS signature validation catches forged signaturesdiscontinuous_headers_rejected: Verifies header chain continuity check catches spliced headersFunctional Tests (
test/functional/feature_quorum_proof_chain.py)getchainlockbyheightRPC error handlingAll 12 unit tests pass.
Breaking Changes
None. This is a new feature addition with new RPCs. No existing functionality is modified.
Checklist:
🤖 Generated with Claude Code