Skip to content

fix(platform)!: decode external input with untrusted bincode decoders - #4625

Open
QuantumExplorer wants to merge 17 commits into
build/grovedb-6-0-0-bincode-2-1-0from
codex/grovedb-untrusted-bincode
Open

fix(platform)!: decode external input with untrusted bincode decoders#4625
QuantumExplorer wants to merge 17 commits into
build/grovedb-6-0-0-bincode-2-1-0from
codex/grovedb-untrusted-bincode

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Sep 8, 2026

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

Builds on #4635, which advances GroveDB to 6.0.0 and moves the workspace to the published grovedb-bincode 2.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 Decode implementations, so Platform's external input paths must opt in throughout their type graphs.

What was done?

  • Make PlatformDeserialize use 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, importing DecodeUntrusted next to Decode and Encode rather 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 derive DecodeUntrusted like everything else while their fields, variants and encoding attributes stay guarded. Local mock formats explicitly opt into ordinary decoding with platform_serialize(..., trusted).
  • Use untrusted decoding for proof verification, SDK request cursors and address synchronization, WASM asset-lock proofs, FFI proof input, and wallet persistence. Audited Serde graphs opt in explicitly; wallet blob decoding requires BlobDecode. Custom Value collections 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-features and cargo fmt --all --check.
  • Strict Clippy (--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.
  • DPP serialization tests: 188 passed. New DPP/Value untrusted_decode integration 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.
  • Wallet blob tests: six passed; persistence and reconstruction integration tests: 42 passed.
  • New FFI payload rejection and backward-reference rendering tests, plus WASM element rendering regression test.

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 PlatformDeserialize types need untrusted decoding implementations, and custom wallet blob types must explicitly implement BlobDecode after reviewing their deserialization graph.

Existing bincode encoding and the C ABI are unchanged. Ordinary Decode and the separate PlatformVersionedDecode APIs retain their existing contracts.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 2594dbf6-51a5-41c1-bc1c-8f44cf20e280

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.14925% with 167 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.56%. Comparing base (6b18e24) to head (bdd3492).

Files with missing lines Patch % Lines
...etual_distribution/distribution_function/encode.rs 83.22% 25 Missing ⚠️
packages/rs-dpp/src/address_funds/witness.rs 66.66% 11 Missing ⚠️
packages/rs-dpp/src/block/epoch/mod.rs 42.85% 8 Missing ⚠️
...ontract/associated_token/token_distribution_key.rs 0.00% 7 Missing ⚠️
...pp/src/data_contract/document_type/property/mod.rs 0.00% 7 Missing ⚠️
...-dpp/src/data_contract/errors/json_schema_error.rs 0.00% 7 Missing ⚠️
...act/associated_token/token_configuration/v0/mod.rs 0.00% 6 Missing ⚠️
...ckages/rs-dpp/src/data_contract/errors/contract.rs 0.00% 6 Missing ⚠️
...act/storage_requirements/keys_for_document_type.rs 0.00% 6 Missing ⚠️
packages/rs-dpp/src/block/block_info/mod.rs 0.00% 5 Missing ⚠️
... and 44 more
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     
Components Coverage Δ
dpp 77.26% <50.14%> (-6.87%) ⬇️
drive 84.39% <ø> (-0.30%) ⬇️
drive-abci 86.58% <ø> (-0.42%) ⬇️
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 88.11% <ø> (-4.81%) ⬇️
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 40.64% <ø> (-0.80%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thepastaclaw

thepastaclaw commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit f0c41bc) · triage: critical · Phase 2 only (queue backlog)

QuantumExplorer and others added 9 commits September 8, 2026 23:58
… 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 QuantumExplorer left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/rs-drive/Cargo.toml
QuantumExplorer and others added 3 commits September 9, 2026 05:18
…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.
@QuantumExplorer QuantumExplorer changed the title fix(platform)!: adopt GroveDB dev and untrusted bincode decoding fix(platform)!: decode external input with untrusted bincode decoders Sep 8, 2026
@QuantumExplorer
QuantumExplorer changed the base branch from v4.2-dev to build/grovedb-6-0-0-bincode-2-1-0 September 8, 2026 23:22

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: critical by gpt-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; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-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>
@QuantumExplorer

Copy link
Copy Markdown
Member Author

Addressed the automated review's test-coverage suggestion in 0cd0e03: tests/sqlite_blob_untrusted_allocation.rs decodes Vec<u32> and Vec<u8> blobs whose length prefix claims megabytes but carries one element, under an observing global allocator. blob::decode must fail as truncated with every allocation under 16 KiB, and a control run of ordinary bincode-serde on the same bytes asserts it reserves at least that much, so the test demonstrably detects a regression to ordinary decoding.

🤖 Addressed by Claude Code

…ntrusted-bincode

Brings the shared-layer cursor merge fix and the paged vote test from #4635.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: critical by gpt-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; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants