Repository navigation
feat(sdk): verify Core quorum proof chains (DIP 175) - #5521
Conversation
Add dash-core-proof, which verifies the DASHNC02 quorum proof chains and bootstrap envelopes Dash Core's getquorumproofchain RPC returns, from a trusted checkpoint. It accepts what Core's own verifier accepts: coinbase payload v4 (Core v24 merkleRootAssetUnlocks), checkpoints from the v20 height, and BLS signature encodings as Core deserializes them. Tests use unmodified getquorumproofchain results from mainnet and testnet, including links mined below the certified block. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fying Core resets a decoded identity element and then rejects it as malleable, so the point at infinity is not a valid quorum or ChainLock signature encoding. A bootstrap whose target is below the caller's minimum height is now refused before any signature work, and decoding uses Core's seed and transaction limits. Synthetic chains signed with known keys cover the rejections real chain data cannot reach: a repeated signer, a non-ChainLock quorum, a v4 coinbase end to end, and the commitment and envelope limits. A zero-link testnet proof covers the common recent-checkpoint case. The trust model now says the relay chooses the path, so every ChainLock quorum from the checkpoint on is trusted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Share one Merkle node hash between verification and its tests, read the CompactSize-prefixed byte strings in transactions through one helper, and import the crate's items at the top of the integration test. No behaviour change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
✅ Final review complete — no blockers (commit ab2b6b4) · triage: critical |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Proof data contracts and primitives packages/rs-core-proof/Cargo.toml, packages/rs-core-proof/src/{wire,state,merkle,lib}.rs, packages/rs-core-proof/README.md |
The crate adds bounded wire utilities, network and state types, Merkle paths, public APIs, error categories, and package documentation. |
BLS, commitment, and transaction validation packages/rs-core-proof/src/{bls,transaction}.rs, packages/rs-core-proof/tests/synthetic.rs |
The crate validates BLS keys and signatures, quorum commitments, mined commitments, and coinbase payloads. Tests cover accepted and rejected encodings and transaction shapes. |
Proof-chain encoding and verification packages/rs-core-proof/src/chain.rs, packages/rs-core-proof/tests/synthetic.rs |
The crate adds bounded proof-chain encoding and decoding. Verification checks the trusted checkpoint, quorum links, ChainLock certificates, and transaction inclusion. Synthetic tests cover linked quorums, continuity, and proof-size limits. |
Bootstrap envelope verification packages/rs-core-proof/src/bootstrap.rs, packages/rs-core-proof/tests/data/*, packages/rs-core-proof/tests/{synthetic,verification}.rs |
The verifier checks proof chains and bootstrap record openings against target-state roots. Mainnet and testnet fixtures and tests cover valid proofs, bootstrap records, and invalid or modified inputs. |
Workspace, build, and test integration .github/package-filters/*, .github/workflows/tests*.yml, Cargo.toml, Dockerfile |
The workspace, Docker stages, package filters, and Rust test workflows include dash-core-proof. The doctest-change detector also checks changed package READMEs included by src/lib.rs. |
Priority:
Estimated code review effort: 4 (Complex) | ~60 minutes
Change: Feature
Sequence Diagram(s)
sequenceDiagram
participant Caller
participant Bootstrap as bootstrap::verify
participant QuorumProofChain
participant MerklePath
Caller->>Bootstrap: provide envelope, trusted State, and minimum height
Bootstrap->>QuorumProofChain: decode and verify proof chain
QuorumProofChain-->>Bootstrap: return authenticated target State
Bootstrap->>MerklePath: verify record leaves against target-state roots
Bootstrap-->>Caller: return verified State and records
Merge Risk: 
ab2b6
-
This PR adds a standalone verifier crate with CI integration and no change to existing users. No actionable merge-blocking risk remains after the review.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 36.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 107 functions across 10 files. (12 skippe… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: adding SDK support to verify Core quorum proof chains under DIP 175. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 36.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 107 functions across 10 files. (12 skipped: 12 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The verifier has strong bounded decoding and cryptographic checks, but its documented trust boundary omits historical quorums that verification can admit; that security-contract mismatch needs correction before approval. Seven nonblocking findings cover caller guidance, encoder bounds, Core compatibility, upstream ownership, API maintainability, and test coverage. Current CI reports successful Rust workspace tests and Kotlin checks; PR Hygiene remains pending. This review was static, with no local builds or tests run.
🔴 1 blocking | 🟡 7 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 9: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate new verifier directly implements cryptographic signature verification and untrusted proof parsing in packages/rs-core-proof/src/bls.rs, chain.rs, merkle.rs and transaction.rs, making proof soundness and canonical decoding critical review surfaces. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-core-proof/src/lib.rs`:
- [BLOCKING] packages/rs-core-proof/src/lib.rs:11-14: Trust also extends to quorums retired before the checkpoint
The documented assumption does not cover every quorum this verifier can trust. `Link::mined_commitment` requires the derived mining height to be positive, but not above the checkpoint or associated with its active set. An honest checkpoint quorum can certify a block shortly after checkpoint S, while roughly 2,000 ancestor headers authenticate a commitment mined at S−2,000. That fits the 4,096-header budget and can reach a mainnet quorum already retired before S: LLMQ_400_60 retains four active quorums with a 288-block DKG interval. Possession of that older quorum's compromised key then permits signing an arbitrary target and roots without compromising any quorum covered by the stated assumption. Core's verifier permits the same handoff, so compatibility alone does not establish the narrower security promise. Extend the crate docs and README to cover authenticated historical quorums already retired before the checkpoint, and add a regression demonstrating this handoff. If the narrower boundary is intended, enforce an appropriate restriction in coordination with Core/DIP.
- [SUGGESTION] packages/rs-core-proof/src/lib.rs:44-46: Expose typed rejection categories instead of requiring string matching
The public error exposes only a static string. Callers that distinguish policy outcomes such as `stale target` or `untrusted snapshot` from integrity failures must therefore match message text, as the tests already do through `.0`. Message wording becomes an accidental machine-readable API, while adding structured context or variants later requires changing the public tuple field. Before consumers depend on this new crate, expose a typed error enum or a stable error-kind accessor and keep human-readable wording in `Display`. This is a nonblocking API-maintainability issue, not a verification bypass.
In `packages/rs-core-proof/README.md`:
- [SUGGESTION] packages/rs-core-proof/README.md:13-17: Make the quorum-key example actually bind the requested quorum
The comment requires checking record identity, but the function has no expected quorum type/hash parameters and returns the first successfully parsed commitment's key. `bootstrap::verify` authenticates membership under the target roots, not correspondence to the caller's request. An untrusted relay can therefore substitute a different valid quorum opening and this example will return that other key successfully. Make the example accept the expected type and hash, select a `RecordKind::Quorum` record, and compare the parsed commitment's identity before returning its key. Selecting only the record kind is insufficient because multiple different quorums can have valid openings.
In `packages/rs-core-proof/src/chain.rs`:
- [SUGGESTION] packages/rs-core-proof/src/chain.rs:283-285: Enforce encoder size budgets before copying data
`encode()` checks the one-megabyte output budget only after serializing the entire chain. Publicly constructed values can contain 4,095 individually permitted 100,000-byte mining transactions, causing approximately 410 MB of output allocation before returning `proof size limit`. The seed also bypasses `MAX_COMMITMENT_BYTES`: a 1,025-byte seed can encode successfully even though `decode()` immediately rejects it. Decoded relay input is already bounded, so this is nonblocking hardening of the public construction/encoding API rather than a remote decoding vulnerability. Preflight encoded length with checked arithmetic or use a budgeted writer that checks before each copy, and enforce the seed limit before writing it. Add coverage for oversized in-memory chains and a seed just above its limit.
In `packages/rs-core-proof/tests/synthetic.rs`:
- [SUGGESTION] packages/rs-core-proof/tests/synthetic.rs:364-366: Make the stale-target test enforce rejection before signature verification
This test uses a cryptographically valid chain, so it still passes if the minimum-height check moves below `chain.verify(trusted)`. The fixture-based stale-target test has the same limitation. Corrupt the target signature and continue expecting `stale target`, then verify that an admissible minimum height reaches signature rejection. This protects the explicitly documented early-rejection guarantee without timing assertions.
In `packages/rs-core-proof/src/transaction.rs`:
- [SUGGESTION] packages/rs-core-proof/src/transaction.rs:36-47: Track upstream ownership of the Core proof verifier
This crate independently maintains Core quorum parameters, commitment and coinbase wire decoding, and the ChainLock signing preimage. Both the pinned rust-dashcore revision and its supplied default-branch revision already provide `LLMQType::params`, `QuorumEntry`, `CoinbasePayload` including version 4, and `ChainLock::sign_id`, but neither provides a DASHNC02 proof-chain verifier. The standalone placement introduces no blocking Platform layering violation, but it fills a missing Core-library capability and creates rules that must be synchronized manually. Link an upstream issue or PR in dashpay/rust-dashcore for a transport-free, wasm-compatible verifier module or sibling crate that shares the existing Core primitives while preserving the proof-specific canonicality checks. The linked C++ Core work defines the format but does not track Rust ownership; no matching Rust ownership follow-up was found in the supplied discussion or upstream searches.
- [SUGGESTION] packages/rs-core-proof/src/transaction.rs:212: Match Core's null fallback for coinbase ChainLock signatures
This applies strict Basic-scheme point validation to `bestCLSignature`, but Core's BLS deserializer can accept a canonical opposite-scheme encoding and reset it to an invalid/null signature. Core's `CheckCbTxBestChainlock` permits that null value with height difference zero when the previous coinbase has no non-null ChainLock; the prohibition on dropping an existing ChainLock does not cover this initial-null case. Core's proof verifier also does not independently validate this signature field. Consequently, the comments and README assertion that these bytes cannot occur in a consensus-valid block is too strong, and this parser can reject a proof Core accepts. Reproduce Core's fallback/null semantics for this field or explicitly document this additional compatibility restriction and its initial-null case. Keep strict validation for certificate signatures and canonical commitment encodings.
In `.github/workflows/tests-rs-workspace.yml`:
- [SUGGESTION] .github/workflows/tests-rs-workspace.yml:459: Add dash-core-proof to the transport-free leak guard
The new crate is included in nextest, but the existing transport-free feature-cut step checks only `drive-proof-verifier` and `dash-platform-queries` as standalone native graphs. `dash-core-proof` is also an embedder/browser-facing verifier, and nothing consumes it yet, so the existing SDK graph checks do not protect its dependency boundary. Its current dependencies are transport-free; this is a missing regression guard, not a present dependency leak. Add a standalone locked check for the crate and include it in the native `cargo tree` loop so a future transport dependency is caught when introduced.
… errors A link's ancestor headers can reach a ChainLock quorum mined up to 4,096 blocks before the checkpoint, even one retired by then, so the trust model now names that boundary, and a synthetic chain pins the handoff. Errors gain kind(): StaleTarget, UntrustedCheckpoint or Invalid, so callers stop matching message text. encode() enforces Core's seed limit and checks the size budget per link, as Core does. The README example now returns only the quorum that was asked for and is compiled as a doctest, the coinbase ChainLock signature strictness is documented precisely, and the stale-target test proves no signature is checked. CI's transport-free check covers the new crate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of f1c413e confirms seven prior findings are fixed, while upstream ownership tracking remains a nonblocking follow-up. The revised security documentation introduces a separate omission: checkpoint-root seed quorums can be older than the stated 4,096-block honesty window. No builds or tests were run; current CI still has package detection and Rust-runner selection pending.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to cryptographic proof verification, BLS signatures, chain locks, Merkle commitments, transaction parsing, and peer-facing binary deserialization in the new dash-core-proof crate. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-core-proof/src/lib.rs`:
- [BLOCKING] packages/rs-core-proof/src/lib.rs:11-15: Include checkpoint-root seed quorums regardless of mining age
The 4,096-header budget bounds ancestor-authenticated handoffs, not the initial seed. `QuorumProofChain::verify` authenticates that seed solely against the checkpoint's quorum root (chain.rs:202–204), with no mining-age restriction. Core's `GetMinedAndActiveCommitmentsUntilBlock` retains non-rotating quorums by count, and `GetMinedCommitmentsUntilBlock` scans back toward height zero; null commitments neither enter that index nor replace existing entries in the coinbase quorum root. After sufficiently many failed DKG sessions, a valid checkpoint can therefore contain a ChainLock seed mined more than 4,096 blocks earlier. Compromising its key permits a zero-link proof authenticating invented target roots while every quorum covered by the documented window remains honest. In both the crate documentation and README, state the union of all ChainLock seed quorums represented under the checkpoint root, regardless of age, and the historical/post-checkpoint window already described. This repairs the security assumption without changing Core-compatible verification.
In `packages/rs-core-proof/src/transaction.rs`:
- [SUGGESTION] packages/rs-core-proof/src/transaction.rs:35-46: Track upstream ownership of the Core proof verifier
(existing thread: https://github.com/dashpay/platform/pull/5521#discussion_r4340258367)
The minimal dependency graph and proof-specific canonicality checks justify keeping this implementation standalone rather than importing today's full `dashcore` graph. The remaining maintenance concern is coordination of the duplicated Core quorum parameters, commitment/coinbase layouts, and signing preimage: both supplied rust-dashcore revisions provide those underlying primitives but lack a DASHNC02 verifier. A transport-free sibling crate maintained in `dashpay/rust-dashcore` could preserve this implementation and dependency budget without using its general-purpose decoders. No corresponding tracking link appears in the supplied discussion, and targeted upstream issue/PR searches found none. Link an upstream issue for adopting/extracting the verifier or explicitly synchronizing these shared rules so this follow-up is tracked; no dependency expansion or migration is required in this PR.
…st model The seed quorum is authenticated only against the checkpoint's quorum root, with no limit on when it was mined, so the trust model also covers every ChainLock quorum committed there, however old. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at ae92a26 found no blocking defect. Eight prior findings are fixed; three nonblocking suggestions remain concerning upstream ownership tracking, README doctest selection, and hash-decoding allocation. No builds or tests were run, and Rust CI remains pending behind changed-package detection and runner selection.
🟡 3 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large new verifier directly implements cryptographic proof acceptance, including BLS signature verification in packages/rs-core-proof/src/bls.rs and ChainLock proof-chain validation in packages/rs-core-proof/src/chain.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-core-proof/src/lib.rs`:
- [SUGGESTION] packages/rs-core-proof/src/lib.rs:97-99: Include the README doctest in CI's change detection
Including the README makes its example available to rustdoc, but ordinary PR CI does not select it. The detector in .github/workflows/tests.yml:169-184 examines only added or removed Rust doc-comment lines containing code fences; neither this include_str! attribute nor README edits match. The complete PR's Rust diff contains no matching fence, and the added nextest package entry does not execute doctests. Extend the detector to recognize the included README and its inclusion, or explicitly run this crate's doctests, so the example is compiled on relevant PRs rather than depending on a manual workflow dispatch or an unrelated doctest change.
In `packages/rs-core-proof/src/state.rs`:
- [SUGGESTION] packages/rs-core-proof/src/state.rs:113-116: Reject oversized RPC hashes before hex-decoding
from_rpc_hex decodes the entire supplied string into a Vec before checking whether the result contains exactly 32 bytes. State's JSON deserializer calls this helper without a preceding length check, so an oversized hash string causes an unnecessary input-proportional allocation and decoding pass before returning an error. Check for exactly 64 characters first, or decode directly into a fixed 32-byte array. This is nonblocking input hardening of the new public parsing API, not a bypass of the bounded binary proof decoder.
In `packages/rs-core-proof/src/transaction.rs`:
- [SUGGESTION] packages/rs-core-proof/src/transaction.rs:35-46: Track upstream ownership of the Core proof verifier
(existing thread: https://github.com/dashpay/platform/pull/5521#discussion_r4340258367)
The standalone dependency boundary and proof-specific canonicality checks are justified; this does not require importing dashcore or replacing these parsers. The remaining concern is synchronization: quorum parameters, commitment and coinbase layouts, and the ChainLock signing preimage now have another Rust implementation alongside rust-dashcore's LLMQType::params, QuorumEntry, CoinbasePayload, and ChainLock::sign_id. Inspection of both supplied upstream revisions confirms those primitives exist but no DASHNC02 verifier does, and focused upstream searches found no matching follow-up. Link a dashpay/rust-dashcore issue or PR establishing ownership and synchronization of this transport-free verifier, potentially as a sibling crate retaining the current dependencies and stricter checks. Tracking is sufficient here; relocation or a larger dependency graph is not requested.
from_rpc_hex decodes straight into a 32-byte array, so other lengths fail before any allocation. CI's doctest detection now also fires when a crate that compiles its README as doctests changes that README. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification at ab2b6b4 found no blocking defect and confirmed ten prior fixes; no builds or tests were run locally. One nonblocking upstream-ownership tracking suggestion remains. Current Rust workspace and Docker/JS build checks are pending, with no failed code checks observed; the base's cryptography migration does not supersede this crate, and the supplied merge evidence reports no conflicts.
🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to cryptographic proof verification, BLS signature and key handling, peer-facing binary deserialization, Merkle proofs, and chain trust validation in functions such as QuorumProofChain::verify and bootstrap::verify. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-core-proof/src/transaction.rs`:
- [SUGGESTION] packages/rs-core-proof/src/transaction.rs:35-46: Track upstream ownership of the Core proof verifier
(existing thread: https://github.com/dashpay/platform/pull/5521#discussion_r4340258367)
The minimal dependency graph and proof-specific canonicality checks justify keeping this verifier independent of the full dashcore crate. They also fit a lightweight sibling crate in the rust-dashcore ecosystem: both inspected rust-dashcore revisions already maintain the corresponding LLMQ parameters, commitment and coinbase representations, and ChainLock signing preimage, but neither implements DASHNC02 verification. This addition therefore fills a missing Core-library capability while introducing another implementation of rules that must remain synchronized. Link a dashpay/rust-dashcore issue or PR tracking ownership and synchronization of a transport-free, WASM-compatible verifier, explicitly preserving the strict parsers and minimal dependency graph. No dependency expansion or code migration is required in this PR; the upstream follow-up needs a tracking reference before it can be treated as deferred.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
/self-reviewed |
|
Ready for review — |
github-actions
Bot
added
ready-for-human
github-actions
Bot
removed
the
bot-review-skipped
|
Approved; this is new code / new crate to support future sdk functionality; I have tested it, it is working. |
#5521 committed the lockfile with dash-core-proof at 5.0.0-rc.4, but the crate takes the workspace version, 5.0.1. `cargo --locked` refuses the stale entry, so v5.1-dev fails before Clippy runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Issue being fixed or feature implemented
Every Platform proof is checked against a quorum public key, and SDK clients that don't run an SPV client (browsers using wasm-sdk / evo-sdk, and any Rust client without Core) get those keys from the HTTP quorum service on trust. Native wallets are moving to SPV-verified keys (#5425, #3417), but a browser cannot run SPV.
Dash Core v24 adds
getquorumproofchain(dashpay/dash#7107, #7727, #7752; DIP 175). From a checkpoint the client trusts, it returns a compactDASHNC02proof: ChainLock certificates through each ChainLock quorum rotation, ending in a ChainLocked block whose coinbase commits to the current masternode and quorum Merkle roots, plus Merkle openings of the requested Platform quorum under them. With it, a client can authenticate a quorum key from a relay it doesn't trust.This PR adds only the verifier. A follow-up adds an opt-in context provider that fetches proofs through the
fetch_quorum_public_keyhook from #5313; nothing changes for existing users here.What was done?
New crate
dash-core-proof(packages/rs-core-proof). It has no Platform dependencies:blst,sha2,rs-x11-hash,serdeandhexare all already in the SDK's dependency tree.QuorumProofChain::{decode, verify, encode}mirrors Core'sQuorumProofChain(same names, limits and checks). It checks:merkleRootAssetUnlocks.bootstrap::verify(bytes, &checkpoint, minimum_height)verifies the envelope Core returns inbootstrap_hex: the chain plus record openings under the target's roots. Records are returned only after everything has verified, and a target belowminimum_heightis refused before any signature work.parse_commitmentreturns a commitment's type, hash and validated public key.State(the checkpoint) serializes to the same JSON Core uses forProofState, so agetquorumproofchaintargetcan be used as a checkpoint file.Trust model, also in the crate docs: the checkpoint, plus the assumption that no ChainLock quorum active at the checkpoint or mined after it ever signs dishonestly. The relay chooses the path, and, as in Core, nothing checks that a signer was the quorum selected for its height. Applications should refresh checkpoints with releases.
How Has This Been Tested?
cargo test -p dash-core-proofruns 27 tests:Real proofs. The fixtures are unmodified
getquorumproofchainresults from mainnet and testnet nodes running Core with the RPC:Each one verifies to exactly the
targetCore reported and re-encodes to the same bytes. The requested quorum opens, every truncation is rejected, sampled byte flips across the whole envelope are rejected, and tampered links fail with the expected error.Synthetic chains signed with known keys. These reach the cases real data cannot:
Unit tests for CompactSize canonicality, Merkle duplicate-leaf handling, the checkpoint era boundary (
>=the v20 height, as in Core), JSON byte order, and BLS rejection of infinity, torsion and noncanonical points.Also run:
cargo clippy -p dash-core-proof --all-features --all-targets -- -D warningsandcargo check -p dash-core-proof --target wasm32-unknown-unknown.Breaking Changes
None. This is a new crate and nothing uses it yet.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
ab2b6b4github(.github/package-filters/rs-packages-direct.yml,.github/package-filters/rs-packages-no-workflows.yml,.github/package-filters/rs-packages.ymland 2 more) — ktechmidas or shumkovCargo.lock,Cargo.toml,Dockerfileand 15 more) — QuantumExplorer or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit
New Features
Tests