fix(sdk)!: enforce a per-protocol-version minimum GroveDB proof envelope (V1 from v14) - #4701
Merged
Merged
Conversation
Reject legacy and unknown GroveDB proof envelopes at Platform client verification boundaries before handing their bytes to Drive. Validate nested identity and compacted proofs while preserving raw Merk sync proof handling.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (76)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change enforces GroveDB proof envelope version 1 across Rust and WASM verification paths. It adds version errors, validates nested proofs, routes proof bytes through shared helpers, updates compacted proof checks, and adjusts Tenderdash signature verification. ChangesGroveDB proof validation
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant WASMVerificationEntryPoint
participant current_grovedb_proof
participant Drive
WASMVerificationEntryPoint->>current_grovedb_proof: validate raw proof bytes
current_grovedb_proof-->>WASMVerificationEntryPoint: current proof or verification error
WASMVerificationEntryPoint->>Drive: invoke verification with current proof
Merge Risk: ⚪ Minimal · up to The proof-version enforcement is consistently applied across the changed verification paths, with no concrete merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 57.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 141 functions across 50 files. (26 skipped: 26 over the file limit.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
Collaborator
|
🕓 Queued for automated review — 1st in line, estimated start in ~30 min (commit 7b80d8d)
|
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The V1 envelope guards are present in the inspected direct verification paths, including both identity double-proof components, while raw Merk trunk proofs retain their separate handling. One non-blocking regression-coverage suggestion is confirmed; no supplied finding establishes an in-scope correctness defect. Validation was source-based; no tests were independently executed.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — The diff makes a large, cross-cutting change to cryptographic proof acceptance in packages/rs-drive-proof-verifier/src/verify.rs and proof.rs, enforcing V1 envelopes across native and WASM entry points while separately handling nested proofs and raw Merk signature-authentication paths. - Phase 1 reviewers: not run (skipped for throughput: 19 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— 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/wasm-drive-verify/src/utils/proof.rs`:
- [SUGGESTION] packages/wasm-drive-verify/src/utils/proof.rs:35-40: Test envelope enforcement through public verification entry points
The two new tests exercise only the private envelope discriminator, so removing a guard from native.rs or either component of the WASM identity double-proof entry point would leave them passing. Existing identity tests do not assert the new unsupported-envelope error, and the V1 helper test checks only a serialized version number rather than a verifiable proof. Add public-entry-point regressions asserting the specific V0 rejection, including V0 independently in each identity double-proof component, plus a valid V1 fixture that still verifies. This would protect the newly introduced security policy against both missing guards and over-rejection.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Align generated ES-module wrappers with wasm-bindgen exports — The package exports route consumers to dist modules generated by scripts/build-modules.sh. Its identity module imports snake_case names such as verify_full_identity_by_identity_id, but the Rust binding exports verifyFullIdentityByIdentityId through wasm-bindgen; its wrapper also omits the required is_proof_subset argument. These concrete mismatches prevent the generated identity module from importing and invoking the binding correctly. Both predate this PR, which changes neither the generator nor the exported signature.
- Follow-up: Track a separate fix synchronizing generated imports and argument lists with wasm-bindgen exports, with a JavaScript smoke test for the published package entry points.
Import the envelope helpers instead of spelling out crate paths at every call site in drive-proof-verifier and wasm-drive-verify. Add regressions that reach the policy through public entry points: the native wasm-drive-verify functions reject V0 and unknown envelopes for identity, contract, and document proofs while a V1 envelope reaches Drive; the identity double proof rejects a V0 outer proof as well as a V0 inner one; the WASM double-proof entry point gets the same two cases. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The offline rs-sdk vectors were recorded before GroveDB emitted V1 envelopes, so every replayed proof now fails the V1 requirement. Where a recorded V0 proof contains only Merk layers that the V1 verifier accepts, re-encode it as a V1 envelope with identical Merk layers; the signed root hash and every verified value are unchanged. That covers 70 files in 48 vector directories. Proofs that report an item in a KVValueHash node or a non-empty subtree without its child hash cannot be upgraded: V1 rejects exactly those shapes. Gate the 24 tests that replay them behind `offline-testing` ignores that say to regenerate against a running Platform (contested-name cases also need #3720), and skip the live-network prerequisite check in offline runs so the error-path cases that never needed a proof keep running. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
QuantumExplorer
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The V1 envelope gate is applied consistently across the reviewed public verification paths, and the newly added rejection tests address the previously identified guard-coverage gap. However, several existing WASM integration success tests still use the 0xAB mock proof, which now fails the envelope gate even when the requested result is empty; the public WASM suite also still lacks a genuinely verifiable V1 proof fixture.
🟡 2 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Update WASM success-test fixtures for the new envelope requirement
packages/wasm-drive-verify/tests/common/mod.rs:7-10
mock_proof still returns an all-0xAB buffer. The new public verification functions call current_grovedb_proof before Drive verification, so this buffer decodes as an unsupported envelope version rather than reaching the intended verification logic. In particular, test_verify_multiple_identities_empty_array, test_verify_token_balances_empty_identity_array, and test_verify_token_statuses_empty_array expect Ok for empty requests but now receive an envelope-version error. Make the shared fixture begin with the big-endian V1 envelope discriminator while preserving invalid-payload behavior, or replace the affected success fixtures with valid V1 proof fixtures.
pub fn mock_proof(size: usize) -> Vec<u8> {
let mut proof = vec![0xAB; size];
if proof.len() >= 4 {
proof[..4].copy_from_slice(&1u32.to_be_bytes());
}
proof
}
source: gpt-6-astra (phase2-reviewer: general, ffi-engineer, rust-quality, security-auditor)
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: ffi-engineer); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This is a large, intricate cross-cutting change to cryptographic proof verification and peer-facing response deserialization, including identity double proofs and compacted address proofs in drive-proof-verifier and wasm-drive-verify. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— ffi-engineer (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 11% left, 5h 100% left),glm-5.3-flash(zai below 15% reserve: 5h 99% left, weekly 13% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— 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/wasm-drive-verify/tests/common/mod.rs`:
- [SUGGESTION] packages/wasm-drive-verify/tests/common/mod.rs:7-10: Update WASM success-test fixtures for the new envelope requirement
`mock_proof` still returns an all-`0xAB` buffer. The new public verification functions call `current_grovedb_proof` before Drive verification, so this buffer decodes as an unsupported envelope version rather than reaching the intended verification logic. In particular, `test_verify_multiple_identities_empty_array`, `test_verify_token_balances_empty_identity_array`, and `test_verify_token_statuses_empty_array` expect `Ok` for empty requests but now receive an envelope-version error. Make the shared fixture begin with the big-endian V1 envelope discriminator while preserving invalid-payload behavior, or replace the affected success fixtures with valid V1 proof fixtures.
In `packages/wasm-drive-verify/tests/identity_tests.rs`:
- [SUGGESTION] packages/wasm-drive-verify/tests/identity_tests.rs:104-106: Test envelope enforcement through public verification entry points
The added WASM public-entry-point tests verify V0 rejection and that a truncated V1 envelope passes the policy gate, but they do not successfully verify a real V1 proof through a wasm-bindgen export. The recorded successful V1 vectors are exercised by `drive-proof-verifier`, not by these WASM exports. Consequently, a regression in copying the V1 payload from `Uint8Array` or in accepting a valid V1 proof after the discriminator would remain undetected. Add a real verifiable V1 fixture through at least one exported WASM verifier, while retaining the independent V0 rejection tests for both components of the identity double proof.
…oof in WASM rs-sdk-ffi replays the rs-sdk vector directories through the FFI mock handle, so the seven tests that hit the V0-only recordings get the same ignore reason as their rs-sdk counterparts. In wasm-drive-verify, the shared mock proof now starts with the bincode-encoded V1 envelope discriminant so the empty-request success tests still reach Drive, and a recorded V1 identity-balance proof from the drive-proof-verifier corpus is verified through the exported verifyIdentityBalanceForIdentityId, pinning its root hash and balance. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4701 +/- ##
============================================
- Coverage 82.13% 81.31% -0.83%
============================================
Files 2827 2827
Lines 387980 389862 +1882
============================================
- Hits 318684 316998 -1686
- Misses 69296 72864 +3568
🚀 New features to boost your workflow:
|
QuantumExplorer
changed the title
Replace the hard-coded "envelope must be V1" check with a per-protocol- version floor: SystemLimits::minimum_grovedb_proof_envelope_version is 0 for every shipped protocol version and 1 from v14, and the gates reject only envelopes below it. Newer envelopes pass the gate and fail in GroveDB's own decoder if the client does not know them. drive-proof-verifier reads the floor in supported_grovedb_proof_bytes and verify_tenderdash_proof (platform_version is now threaded through), wasm-drive-verify in supported_grovedb_proof for the JavaScript and native entry points, and Drive's compacted address proof v1 for its two nested proofs. The error now names the envelope version, the floor, and the protocol version that set it. Tests cover both sides of the boundary: v14 rejects V0, v13 still lets it through, and V1 and newer pass the gate. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Both sides appended fields to SystemLimits: the time-range TTL pair from #4581 and the GroveDB proof envelope floor. Keep both, base fields first. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A policy rejection is not a corrupted proof, so Drive gains two ProofError variants instead of reusing CorruptedProof: InvalidGroveDBProofEnvelope when the bytes carry no decodable discriminant, and UnsupportedGroveDBProofEnvelopeVersion when the envelope is below the protocol version's floor. Both name which proof was read (the proof, the predecessor proof, or the forward proof) so the compacted address verifier can say which half failed. wasm-drive-verify builds the same variants for its JavaScript and native entry points, and drive-proof-verifier maps the version variant onto its own Error::UnsupportedGroveDBProofVersion, which now carries the proof label too, so SDK callers see one variant for direct and nested rejections alike. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lope Gating the recorded V0 vectors took most of the FromProof paths out of the offline suite, so add a table-driven regression that feeds a V0 GroveDB envelope to each direct-proof FromProof implementation at the latest protocol version and expects the typed rejection. The provider panics if touched, so a path that lost its gate fails loudly rather than reaching Drive or the context provider. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A signed DAPI response could supply a GroveDB V0 proof envelope. V0's historical item binding can return different item bytes while preserving the authenticated root, so Tenderdash root authentication alone does not make the returned value safe.
Change
SystemLimits::minimum_grovedb_proof_envelope_version: the lowest GroveDB proof envelope a client accepts, 0 for every shipped protocol version and 1 from protocol version 14drive-proof-verifier, in everyFromProofand inverify_tenderdash_proofwasm-drive-verifyJavaScript and native entry pointsThe gate is
version < minimum, so envelopes newer than the client knows pass the gate and fail in GroveDB's own decoder. Rejections are typed: Drive gainsProofError::InvalidGroveDBProofEnvelopeandProofError::UnsupportedGroveDBProofEnvelopeVersion(each naming which proof was read: the proof, the predecessor proof, or the forward proof), anddrive-proof-verifiermaps the latter onto its ownError::UnsupportedGroveDBProofVersionso SDK callers see one variant for direct and nested rejections alike. The message readsunsupported GroveDB proof envelope version 0 in the proof: protocol version 14 requires at least version 1. A client verifying with protocol version 13 tables still accepts V0; one verifying with 14 does not. Protobuf request and responseVersion::V0remains supported because it is unrelated to the GroveDB proof-envelope version. GroveDB only emits V1 envelopes from grove version 3, which Platform v13 introduced, so current mainnet and testnet responses already satisfy the floor.Related: dashpay/grovedb#852
Recorded rs-sdk test vectors
The offline
rs-sdkvectors were recorded on devnets running protocol versions 4 to 12, so 72 of the 105 vector directories carried V0 envelopes and 92 offline tests failed against this policy.KVValueHashnode that carries an item, and a non-empty subtree reported without its child hash. Those are the identity/token/group reads and the contested-resource family. They are gated behindoffline-testingignores whose reason says to regenerate against a running Platform; the contested-name cases also depend on the prerequisite seeding tracked in Seed DPNS contested-name prerequisites via SDK_TEST_DATA (extend create_sdk_test_data) #3720. Their directories keep the original V0 recordings so the regeneration overwrites them in place.rs-sdk-ffireplays the same directories through the FFI mock handle; the 7 tests that hit the V0-only recordings carry the same ignore reason.Tests
drive-proof-verifierfeeds a V0 envelope to 43 direct-proofFromProofpaths (identity, contract, address, epoch, upgrade, path-element, credits, proposer, shielded, token, and group queries) at the latest protocol version and expects the typed rejection; the context provider panics if reached, so a path that loses its gate fails loudlydrive-proof-verifierregression corpus is verified through the exported WASMverifyIdentityBalanceForIdentityId, pinning the expected root hash and balancemock_prooffixture now starts with the bincode-encoded V1 envelope discriminant, so the existing empty-request success tests still reach Drive instead of stopping at the envelope gatewasm-drive-verifyentry points reject V0 for identity, contract, and document proofs at the latest protocol version, let it through at protocol version 13, and pass a V1 envelope through to Drivedrive-proof-verifier), with the same two cases on the WASM double-proof entry pointValidation
cargo test -p drive-proof-verifier --lib --locked(277 passed)cargo test -p wasm-drive-verify --lib --locked(24 passed)cargo test -p dash-sdk --test main(108 passed, 28 ignored, 0 failed)cargo test -p rs-sdk-ffi --test integration(27 passed, 7 ignored, 0 failed)cargo check -p wasm-drive-verify --tests --target wasm32-unknown-unknowncargo check -p drive --no-default-features --features verifycargo test -p drive verify_compacted_address_balance_changes --lib --locked(13 passed)cargo test -p dash-platform-queries --lib --locked(57 passed)cargo check -p dash-sdk --lockedcargo check -p wasm-sdk --lockedcargo clippy -p platform-version -p drive-proof-verifier -p wasm-drive-verify --all-targets --locked --no-deps -- -D warningscargo fmt --all -- --check🤖 Generated with Claude Code