fix(platform)!: decode external input with untrusted bincode decoders - #4625
fix(platform)!: decode external input with untrusted bincode decoders#4625QuantumExplorer wants to merge 17 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## build/grovedb-6-0-0-bincode-2-1-0 #4625 +/- ##
=====================================================================
- Coverage 84.67% 82.56% -2.12%
=====================================================================
Files 2793 2767 -26
Lines 374859 379792 +4933
=====================================================================
- Hits 317414 313574 -3840
- Misses 57445 66218 +8773
🚀 New features to boost your workflow:
|
|
✅ Final review complete — no blockers (commit f0c41bc) · triage: critical · Phase 2 only (queue backlog) |
… items Clippy's items_after_test_module lint (denied under -D warnings in CI) rejects the test module that sat between two exported FFI functions. Move it to the end of the file; the tests are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
librocksdb-sys 0.19.0+11.8.1 (pulled in by rocksdb 0.25) targets the RocksDB 11 C API, which dropped the deprecated in_range callback from rocksdb_slicetransform_create. The Docker image and the librocksdb GitHub action still built RocksDB 10.4.2 and 9.9.3, so bindgen produced the six-argument signature and the rocksdb crate failed to compile against it. Build 11.8.1 in both places to match the vendored version. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GroveDB #948 and rust-dashcore #1005 are merged. Point at the merge commits on grovedb develop (985ece65) and rust-dashcore dev (057e79cd) instead of the pull-request heads. The rust-dashcore tree is identical; the grovedb merge only adds the #947 release workflow change on top of the previously pinned tree. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… it in derives Every derive that spelled bincode::DecodeUntrusted now names DecodeUntrusted and imports it beside Decode and Encode, the way the other bincode derives are written. The two files that had no bincode import at all (credit operations and the compacted address balance proof) import Encode and Decode the same way instead of path-qualifying them. The Txid adapter in serialization/untrusted.rs keeps its fully qualified call: that module has no imports by design. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ng untrusted decoding CI diffs every @immutable and @append_only block against the base branch and rejects any change to the former and any deleted line in the latter. Adding DecodeUntrusted to those derive lists reflowed them, which read as a change or a deletion. BlockInfo is tagged immutable, so its derive list is restored verbatim and DecodeUntrusted is implemented by hand outside the block, reading the fields in declaration order like the derived Decode; a test pins the two decoders to each other. The four append-only enums keep their derive lists and carry the new derive as a separate attribute; a comment between the two keeps rustfmt from merging them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
GroveDB now refuses a V1 layer proof that is read in the other op family (grovedb #863): a layer proof is emitted entirely in the family of the direction its query walks. When a document query carries a startAt or startAfter cursor, the prover merges the one-key cursor lookup into the page proof, and since grovedb's merge needs every input to walk the same way, the cursor layer is emitted in the page's orderBy direction. The verifier rebuilt that lookup as a fresh ascending single-key query, so every proved descending page with a cursor failed with InvalidProof. Read the cursor layer in the page's direction when it is verified as a subset of the page proof. The direction comes from the same query builder the prover uses and never depends on the cursor document; a single key decodes identically either way. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Keep the workspace bincode alias and the GroveDB pin from this branch; take the crates.io versioned-feature-core from #4631.
cargo fmt --check fails on v4.2-dev since #4218 landed this test unformatted. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
QuantumExplorer
left a comment
There was a problem hiding this comment.
Reviewed commit 462faae and found one P1 regression, detailed inline. Before posting, I checked the new head, 5dd5d62: it aligns the cursor verification query with the page's direction and appears to address the finding. I have not rerun the four failing pagination tests against that fix, so this is a review comment rather than a request for changes on the new head.
Validation on the originally reviewed commit: 1,708 local tests passed across platform-value, platform-serialization, and the DPP untrusted-decoding regression suite. FFI inspection found no additional actionable issue; the FFI tests could not run because a dependency build attempted a download that failed. No live network upgrade or browser/device execution was performed.
…der and cursor The proved contestant-votes helper verified every proof with a fixed ascending, unlimited query regardless of the request it had just sent. GroveDB now refuses a layer proof read in the other op family, so the descending request failed with InvalidProof. Mirror the request's order, count and cursor into the verifier query, as the SDK's proof verifier does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The @immutable and @append_only check text-diffs a tagged block against the base branch, and the derive attribute sits inside that block, so adding a derive (which rustfmt reflows onto separate lines) read as a change to the structure. A derive list cannot change how the existing fields or variants serialize. Strip #[derive(...)] attributes, single- or multi-line, before diffing; fields, variants and the attributes that do shape the encoding (serde, repr, platform_serialize) stay compared verbatim. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
With the immutable structure check ignoring derive lists, BlockInfo and the four append-only enums derive DecodeUntrusted like every other type. This replaces the hand-written BlockInfo decoder and the separate derive attributes that only existed to keep those blocks byte-identical. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ntrusted-bincode The dependency bump and GroveDB adaptations now live in #4635; this branch keeps only the untrusted-decoding adoption on top of it.
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the supplied finding against exact head bdd3492 and retained one non-blocking test-coverage suggestion directly tied to the PR's allocation-safety goal. No blocking findings were supplied or established during this verification. The scoped diff passes git diff --check; this verification did not rerun Rust tests.
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)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — Changing default deserialization across consensus types, proof verification, SDK inputs, FFI boundaries, and wallet storage is a broad security-sensitive change whose mistakes could cause memory exhaustion, validation bypasses, wire incompatibility, or rejection of valid protocol and persisted data. - Phase 1 reviewers: not run (skipped for throughput: 20 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
🟡 1 suggestion(s)
1 finding(s) omitted from inline comments because GitHub refused the PR diff as too large; listed below.
1 unmapped finding(s)
1. [suggestion] Add allocation-sensitive coverage for the wallet's Serde decoding path
packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs:83-85
This switches wallet blobs to untrusted Serde decoding, but the six blob tests only cover round trips, trailing bytes, physically oversized input, and malformed outpoints. None checks allocation behavior, so they would not detect a regression to ordinary Serde decoding that reserves collection capacity before returning an error. The allocation-observation tests in platform-value exercise native Value decoding rather than UntrustedBlob and its Serde visitors. Add an allocation-observation regression through blob::decode for a truncated collection, asserting both a decoding error and bounded allocation requests. This directly protects the allocation guarantee introduced by this PR without implying that the current implementation is defective.
🤖 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-platform-wallet-storage/src/sqlite/schema/blob.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs:83-85: Add allocation-sensitive coverage for the wallet's Serde decoding path
This switches wallet blobs to untrusted Serde decoding, but the six blob tests only cover round trips, trailing bytes, physically oversized input, and malformed outpoints. None checks allocation behavior, so they would not detect a regression to ordinary Serde decoding that reserves collection capacity before returning an error. The allocation-observation tests in platform-value exercise native Value decoding rather than UntrustedBlob<T> and its Serde visitors. Add an allocation-observation regression through blob::decode for a truncated collection, asserting both a decoding error and bounded allocation requests. This directly protects the allocation guarantee introduced by this PR without implying that the current implementation is defective.
…uncated blob collections blob::decode runs bincode-serde in untrusted mode so a length prefix is never turned into capacity. The existing blob tests covered round trips, trailing bytes, oversized input and malformed outpoints, but none would notice a regression to ordinary Serde decoding, which reserves up to Serde's 1 MiB cap before failing on a truncated payload. Decode Vec<u32> and Vec<u8> blobs whose prefix claims megabytes but which carry one element, under an observing global allocator: the codec must fail as truncated (not oversized) with every allocation under 16 KiB. Each test first runs the ordinary decoder on the same bytes as a control and asserts it does reserve at least that much, so the ceiling is proven to tell the two apart. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Addressed the automated review's test-coverage suggestion in 0cd0e03: 🤖 Addressed by Claude Code |
…ntrusted-bincode Brings the shared-layer cursor merge fix and the paged vote test from #4635.
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The reviewed head consistently adopts untrusted bincode decoding across the external-input paths covered by this PR. The previously identified wallet persistence allocation gap is fixed by the new allocation-sensitive regression test, and no additional in-scope correctness, security, consensus, or maintainability issues were identified.
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)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — This is a very large security-sensitive change spanning 537 files and broad external deserialization paths across consensus types, proofs, SDK/FFI/WASM interfaces, wallet persistence, and storage, where incomplete or incorrect untrusted decoding could cause memory exhaustion, acceptance/rejection regressions, or consensus and compatibility failures. - Phase 1 reviewers: not run (skipped for throughput: 13 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
Issue being fixed or feature implemented
Builds on #4635, which advances GroveDB to 6.0.0 and moves the workspace to the published
grovedb-bincode2.1.0 and adapts to the GroveDB API changes. This PR is based on that branch until it merges and contains only the untrusted-decoding adoption.Adopt GroveDB #938 and its explicit untrusted decoding APIs. A short input containing an inflated collection length must fail without first reserving memory for the claimed payload. Updating the dependency alone does not change ordinary
Decodeimplementations, so Platform's external input paths must opt in throughout their type graphs.What was done?
PlatformDeserializeuse untrusted decoding by default, including its no-limit entry point. Add native untrusted derives across DPP and preserve custom wire schemas, validation, depth limits, and configured budgets. Most changed files only add the derive, importingDecodeUntrustednext toDecodeandEncoderather than path-qualifying it. The immutable-structure CI check now strips#[derive(...)]attributes from tagged blocks before diffing them, since a derive cannot change a structure's wire layout, so the tagged types deriveDecodeUntrustedlike everything else while their fields, variants and encoding attributes stay guarded. Local mock formats explicitly opt into ordinary decoding withplatform_serialize(..., trusted).BlobDecode. CustomValuecollections grow only after decoding entries. Ordinary-only foreign FFI payload/BLS-key decoders receive explicit budgets.How Has This Been Tested?
Local macOS Rust checks: full workspace compilation, nine untrusted-decoding regression tests, and 256 Drive proof-verification tests were rerun successfully. Implementation validation also includes:
cargo check --workspace --all-targets --all-featuresandcargo fmt --all --check.--all-targets --all-features -- -D warnings) for platform-value, platform-serialization-derive, dpp, drive, drive-proof-verifier, dash-sdk, and platform-wallet-storage.cargo test -p platform-value -p platform-serialization --all-features: 1,702 passed; two ignored documentation tests.untrusted_decodeintegration suites: nine passed, covering malformed lengths, observed allocation sizes, owned/borrowed decoding, wire compatibility, custom validation, and trusted fixture opt-in.cargo test -p drive --lib --all-features verify::: 256 passed. Drive ABCI cursor tests: 12 passed.After merging
v4.2-dev, the CI Rust test job's nextest package set (--all-features, same filter) was run locally in full: 14,864 tests passed. Native host compilation and targeted tests were run; device/browser execution and a live network upgrade were not exercised.Breaking Changes
Custom
PlatformDeserializetypes need untrusted decoding implementations, and custom wallet blob types must explicitly implementBlobDecodeafter reviewing their deserialization graph.Existing bincode encoding and the C ABI are unchanged. Ordinary
Decodeand the separatePlatformVersionedDecodeAPIs retain their existing contracts.Checklist:
For repository code-owners and collaborators only