Skip to content

feat: alloc-only profiles for platform-value and platform-serialization - #4707

Open
DCG-Claude wants to merge 5 commits into
v4.3-devfrom
dashvm/r13-01
Open

DCG-Claude wants to merge 5 commits into
v4.3-devfrom
dashvm/r13-01

Conversation

@DCG-Claude

@DCG-Claude DCG-Claude commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Issue being fixed or feature implemented

Part 1 of 2 for R13-01 of the smart-contract plan in #4626 (workstream 44-CODEC, #4679).

DashVM guest code has to read and write the same value bytes as the node, but it runs without an operating system, without threads and without the native PlatformVersion registry. Today platform-value and platform-serialization link std unconditionally, depend on platform-version unconditionally, and the Value decoder takes its nesting limit from a thread-local. None of that exists inside a guest, and a guest must never allocate more than its caller allowed. This PR gives both crates a real allocation-only profile and an explicitly bounded value codec, while keeping the native profile byte-for-byte and behaviour-for-behaviour identical. Part 2 adds the dashvm-abi crate, golden vectors and the separate guest workspace on top of it.

What was done?

Feature profiles

  • packages/rs-platform-serialization/Cargo.toml: bincode with default features off (alloc, serde), platform-version optional. Features std (bincode std readers/writers) and platform-version (the PlatformVersionEncode/PlatformVersionedDecode traits, the free functions, every standard-type impl). Both are default.
  • packages/rs-platform-value/Cargo.toml: every dependency with default features off; rand, treediff, indexmap, platform-version optional. Features std (thread-local decode depth scope, patch::diff, the indexmap and HashSet helpers), random (Identifier::random, Identifier::random_with_rng, Bytes32::random_with_rng; implies std), platform-version (the Identifier version-aware impls), and the existing json/cbor, which now imply std. Default is std, random, platform-version, so no consumer changes.
  • Both lib.rs files carry #![cfg_attr(not(any(test, feature = "std")), no_std)] with extern crate alloc. Source files moved from std:: to core::/alloc:: imports; gated items keep their signatures under default features.

Shared decoder state machine (packages/rs-platform-value/src/lib.rs)

  • The iterative Value decoder is extracted into decode_value_with(decoder, leaves) over a private ValueLeafReader<D> trait (array_header, map_header, container_end, bytes, text, string_list). The blanket impl<Context> Decode<Context> for Value runs it with NativeLeaves: the thread-local limit read per container header as before, bincode's own allocating leaf decoders, Vec::with_capacity(len) containers and the same error text. Variant indices 0..=22 are untouched.
  • Without std there is no thread-local scope, so the blanket impl applies DEFAULT_MAX_VALUE_DECODE_DEPTH (256); with_value_decode_depth_limit is std only. Its one consumer (rs-dpp state transition decoding) is native.

Bounded codec seam (packages/rs-platform-serialization/src/bounded.rs, new)

  • CodecBounds { max_bytes: u32, max_depth: u16, max_elements: u32 }, CodecBudget (running depth and element counters, saturating arithmetic), BoundsError (BytesExceeded, DepthExceeded, ElementsExceeded, DeclaredLengthExceedsInput; fixed-width fields only, no usize, so a later wire encoding is word-size independent), check_declared_len, the RemainingInput trait and BoundedSliceReader (a slice reader exposing remaining()), canonical_config() (standard, big endian, varint, no limit: the native config), decode_bounded_len, decode_bounded_bytes, decode_bounded_string, decode_bounded_string_list, BoundedDecodeError and bounded_decode_from_slice, which checks max_bytes first and rejects trailing bytes.
  • Every bounded leaf reader checks the declared length against the unread input before allocating and then allocates exactly that length; bincode's own Vec<u8>/String decoders (which allocate from the declared length first) are never used on this path.

Bounded value codec (packages/rs-platform-value/src/guest_bounds.rs, new)

  • Value::decode_bounded(bytes, &CodecBounds) runs the shared state machine with BudgetedLeaves: depth and element counts (one per array item, two per map entry, one per enumeration string) are charged at the container header, containers start empty and grow by push, and every declared length is checked against reader.remaining() before allocation. Heap usage is bounded by the three limits together: byte leaves by max_bytes, container storage by max_elements (plus vector growth slack), the frame stack by max_depth. It is not bounded by max_bytes alone.
  • Value::encode_bounded(&self, &CodecBounds) walks the value with an explicit stack (no recursion), charges the same depth and element counts, refuses output above max_bytes after a size pass whose only allocation is that stack, and produces bytes identical to the derived Encode impl. BoundedEncodeError mirrors the decode error.
  • Bounded decoding is not canonical validation: bincode accepts overlong varints, so distinct inputs can decode to the same value and trailing-byte rejection alone does not make the encoding unique. Canonical bytes (re-encode and compare) are part 2's dashvm-abi codec; tests here pin the lenient behaviour so that contract is explicit.
  • platform_value! reaches vec! through platform_value::__private::vec, so the macro expands inside a #![no_std] guest.

Tests

  • packages/rs-platform-serialization/src/bounded.rs: budget arithmetic at and one over each bound, saturating overflow, declared length equal to and one over the remainder, u64::MAX declared length rejected without allocating, UTF-8 validated after the bounded read, string-list count and inner-string checks, trailing bytes, reader remainder, config equality with the native config.
  • packages/rs-platform-value/src/guest_bounds.rs: encode and decode equality with the derived Encode and blanket Decode over every variant, depth and element bounds on both paths, declared array/map/Bytes/EnumU8/Text/EnumString lengths beyond the input, trailing bytes, unknown variant, invalid UTF-8, depth released on container close, container map keys.
  • packages/rs-platform-value/tests/alloc_profile.rs (new): the CI evidence for the alloc profile; runs under --no-default-features and under default features so both profiles are shown to agree. tests/coverage_tests.rs is std only (it uses diff and the thread-local scope).

Repository plumbing and docs

  • rust-toolchain.toml lists wasm32v1-none. .github/workflows/tests-rs-workspace.yml gains a "Check guest alloc-only cut" step: both crates with default features off on wasm32v1-none (a target with no std library, so any std leak is a compile error) plus the alloc_profile test. .github/workflows/tests-rs-nightly-long-running.yml adds both crates to the per-feature matrix; packages/check-features/src/main.rs lists both.
  • book/src/serialization/platform-serialization.md: new "Allocation-only guest profile" section. book/src/contributing/coding-conventions.md: the guest alloc-only cut as the fourth CI boundary and a placement-table row.

Dependency direction is preserved: dash-platform-queries -> drive-proof-verifier -> drive is untouched, platform-value -> platform-serialization is the only edge the guest profile adds, and platform-version is reachable only through the platform-version feature.

How Has This Been Tested?

macOS, Rust 1.92 from rust-toolchain.toml, targets wasm32-unknown-unknown and wasm32v1-none. Every command below exited 0 with output captured to a log.

cargo fmt --check --all
cargo clippy -p platform-serialization -p platform-value --all-features --all-targets -- -D warnings
cargo clippy -p platform-serialization -p platform-value --no-default-features --all-targets -- -D warnings
cargo clippy -p platform-serialization --no-default-features --features platform-version --all-targets -- -D warnings
cargo check --workspace --all-targets
cargo check -p platform-serialization --no-default-features --target wasm32v1-none --locked
cargo check -p platform-value --no-default-features --target wasm32v1-none --locked
cargo test -p platform-value --no-default-features --test alloc_profile --locked
cargo test -p platform-serialization
cargo test -p platform-serialization --no-default-features
cargo test -p platform-value            # lib, alloc_profile, coverage_tests
cargo test -p platform-value --no-default-features
cargo test -p platform-value --all-features --doc
cargo machete
CC_wasm32_unknown_unknown=/opt/homebrew/opt/llvm/bin/clang AR_wasm32_unknown_unknown=/opt/homebrew/opt/llvm/bin/llvm-ar \
  cargo check -p wasm-drive-verify --target wasm32-unknown-unknown   # the JS build path through dpp

Per-feature checks as the nightly matrix runs them (RUSTFLAGS="-D warnings" cargo check -p <crate> --no-default-features --features <feature> --locked) pass for std and platform-version on platform-serialization and for std, random, platform-version, json and cbor on platform-value, plus both crates with no features at all.

cargo tree -p platform-value --no-default-features -e normal contains only base64, bincode, bs58, hex, platform-serialization, serde and thiserror (plus proc-macro crates); platform-version, rand, getrandom, serde_json, indexmap, treediff and ciborium are absent.

A throwaway #![no_std] crate (kept out of the tree) depending on both crates with default features off, using platform_value!, Value::decode_bounded and Value::encode_bounded, compiles for both wasm32v1-none and wasm32-unknown-unknown. Part 2 turns that into the committed guest-check workspace.

Test counts: platform-serialization 179 unit tests (default) and 19 (alloc profile); platform-value 1050 unit tests (default) and 991 (alloc profile), 7 in alloc_profile, 239 in coverage_tests, 93 doctests with all features.

Note: cargo test -p platform-value --doc without --all-features fails on the pre-existing serde_json doctest in value_serialization/ser.rs, exactly as on the base branch; CI runs doctests with --all-features.

Breaking Changes

None. No protocol rules, wire shapes, fees, version tables or consensus errors change. The native Value encoding and the blanket Decode behaviour (thread-local limit, allocation pattern, error text) are unchanged, and every public signature is the same under default features. Consumers that disable default features on either crate (none exist in the workspace) must now opt into std, random or platform-version for the items those features gate.

Decisions taken (provisional values)

  • Feature names settled: platform-value std, random, platform-version, json, cbor; platform-serialization std, platform-version. json and cbor imply std (serde_json with preserve_order and the CBOR conveniences are native tooling).
  • The guest value/codec foundation is the existing two crates with default features off; no separate platform-value-core crate.
  • Element accounting: one per array item, two per map entry (key and value), one per string in an enumeration of strings, charged at the header before allocation. Byte leaves are bounded by the unread input, not the element budget.
  • Allocation on the bounded path: byte leaves never allocate more than the unread input (declared lengths are checked against the remainder first); container storage and the traversal stack are bounded by max_elements and max_depth. The three limits together bound heap usage.
  • Depth counter is u32 with checked addition against a u16 limit, so a limit of u16::MAX still refuses the 65,536th level; BoundsError::DepthExceeded.depth is u32 for the same reason.
  • max_depth is the stack guard. Bounded decoding and encoding walk containers iteratively, but a decoded Value tree is as deep as the limit allows and Value's Drop, Clone and derived Encode are recursive (for every value in the crate, accepted or rejected). The reference value is the native document limit of 256; tests exercise decode, drop, trailing-byte rejection and malformed-sibling rejection at that depth on a 128 KiB thread stack. Making Drop for Value iterative would touch every native owner of a Value and is not part of this change.
  • The bounded encoder's leaf arm enumerates every Value variant so a future variant fails to compile at the accounting boundary instead of silently bypassing depth and element counting.
  • The implicit ciborium and serde_json feature names of platform-value are kept (plain optional dependencies rather than dep:), so a downstream manifest selecting either still resolves; cbor and json remain the documented names and imply std.
  • BoundedDecodeError and BoundedEncodeError expose the wrapped BoundsError through Error::source, and the wrapped bincode error under std only, because bincode's errors implement the error trait only with std.
  • An allocator-observing test in packages/rs-platform-value/tests/untrusted_decode.rs proves that encode_bounded rejecting an oversized output allocates nothing.
  • Without std, the blanket Decode impl uses the constant 256 depth limit (the value the native path uses by default); guests needing other limits call decode_bounded.
  • Error payload widths: lengths and remainders u64, limits u32 (depth limit u16), refused depth u32, refused element total u64 because a map header claims twice its length. No usize in BoundsError, BoundedDecodeError or BoundedEncodeError.
  • Byte, depth and element numbers for the ABI (256 KiB arguments, 64 KiB results, depth 256, 65,536 elements) are provisional starting values from the allocation register and arrive with part 2; this part only enforces whatever bounds a caller passes.

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

Refs #4679

Part 1 of 2 for R13-01

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Sep 12, 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: d9546457-06f3-4fec-b9f7-876c9f9f0fe4

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.

@github-actions

github-actions Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

📖 Book Preview built successfully.

Download the preview from the workflow artifacts.
To view locally: download the artifact, unzip, and open index.html.

Updated at 2026-09-15T22:22:29.233Z

@thepastaclaw

thepastaclaw commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

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

@codecov

codecov Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 69.23077% with 364 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (v4.3-dev@cb6514e). Learn more about missing BASE report.

Files with missing lines Patch % Lines
packages/rs-platform-value/src/guest_bounds.rs 64.04% 169 Missing ⚠️
packages/rs-platform-value/src/lib.rs 66.53% 87 Missing ⚠️
packages/rs-platform-serialization/src/bounded.rs 82.28% 73 Missing ⚠️
packages/rs-platform-value/src/replace.rs 0.00% 6 Missing ⚠️
packages/rs-platform-serialization/src/lib.rs 0.00% 5 Missing ⚠️
packages/rs-platform-value/src/types/identifier.rs 42.85% 4 Missing ⚠️
packages/rs-platform-serialization/src/de/mod.rs 0.00% 3 Missing ⚠️
...kages/rs-platform-value/src/inner_value_at_path.rs 25.00% 3 Missing ⚠️
packages/rs-platform-value/src/value_map.rs 0.00% 3 Missing ⚠️
...s/rs-platform-value/src/value_serialization/ser.rs 0.00% 3 Missing ⚠️
... and 5 more
Additional details and impacted files
@@             Coverage Diff             @@
##             v4.3-dev    #4707   +/-   ##
===========================================
  Coverage            ?   79.71%           
===========================================
  Files               ?     2830           
  Lines               ?   406934           
  Branches            ?        0           
===========================================
  Hits                ?   324404           
  Misses              ?    82530           
  Partials            ?        0           
Components Coverage Δ
dpp 79.16% <0.00%> (?)
drive 81.58% <0.00%> (?)
drive-abci 81.55% <0.00%> (?)
sdk ∅ <0.00%> (?)
dapi-client ∅ <0.00%> (?)
platform-version ∅ <0.00%> (?)
platform-value 68.05% <0.00%> (?)
platform-wallet ∅ <0.00%> (?)
drive-proof-verifier 33.22% <0.00%> (?)
🚀 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 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 bounded decoder has a reproducible denial-of-service failure: deeply nested values that pass the configured depth bound can overflow the call stack during recursive destruction when decoding is later rejected. The bounded encoder's catch-all leaf arm is also a valid future-maintenance hazard because a new container-like Value variant could bypass depth and element accounting.

🔴 1 blocking | 🟡 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: rust-quality); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: critical by gpt-6-astra (effort low) — This is a large, intricate change to platform-value and platform-serialization decoding, including bounded peer-facing deserialization and allocation/depth/element enforcement that directly affects consensus-critical wire behavior.
  • 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 — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-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-platform-serialization/src/bounded.rs`:
- [BLOCKING] packages/rs-platform-serialization/src/bounded.rs:454-459: Dispose of deeply nested decoded values iteratively on rejection
  When the bounded decode closure succeeds but trailing bytes are found, returning `BoundedDecodeError::TrailingBytes` drops the already-decoded `value`. Although `decode_value_with` traverses nested arrays and maps with an explicit frame stack, Rust's generated drop glue recursively destroys the resulting `Value` tree. A deeply nested input can therefore overflow the call stack instead of returning the bounded decode error. This is reachable with caller-supplied bounds such as `max_depth: u16::MAX`, and the configured byte, depth, and element limits do not prevent the destructor recursion. The same issue can occur when a malformed sibling causes the frame storage or completed subtree to be dropped during error unwinding. Make cleanup of completed and partially decoded subtrees iterative on the bounded path, and add subprocess regressions for trailing-byte rejection and malformed-sibling rejection at extreme permitted nesting.

In `packages/rs-platform-value/src/guest_bounds.rs`:
- [SUGGESTION] packages/rs-platform-value/src/guest_bounds.rs:212-216: Classify encoder leaves exhaustively
  The `leaf => leaf.encode(encoder)?` catch-all implicitly treats every current and future Value variant other than the explicitly handled containers as a leaf. If a future variant contains nested values or another element-counted collection, its derived encoder would bypass the bounded encoder's depth and element accounting and could reintroduce recursive traversal. This match is the enforcement boundary for bounded encoding, so explicitly enumerate the current scalar and byte/string leaf variants. Because the match is in the defining crate, exhaustive matching will force a compile-time review when a new Value variant is added.

Comment thread packages/rs-platform-serialization/src/bounded.rs
Comment thread packages/rs-platform-value/src/guest_bounds.rs Outdated

@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 both prior findings against head a0b9824: encoder leaf classification is fixed, and the recursive-destruction blocker is withdrawn because it assumes stack safety for arbitrarily large caller-selected depth limits, which this API does not promise. All 18 bounded-value tests passed under both default and allocation-only profiles, including the three depth-256 small-stack regressions; the additional serialization test run timed out during compilation. No actionable in-scope findings remain, and the worktree is clean.

Review provenance

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: critical by gpt-6-astra (effort low) — This is a large, intricate change to peer-facing serialization and value deserialization, including new bounded decoding and allocation/depth enforcement in rs-platform-serialization and rs-platform-value that can alter consensus-relevant wire acceptance behavior.
  • Phase 1 reviewers: not run (skipped for throughput: 11 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 — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer

DCG-Claude and others added 4 commits September 15, 2026 13:21
Both crates now build as no_std + alloc with default features off. New
features: std, random and platform-version on platform-value; std and
platform-version on platform-serialization. json and cbor imply std.
platform-version becomes an optional dependency, and the Identifier
PlatformVersionEncode/PlatformVersionedDecode impls move behind it.

The iterative Value decoder is extracted into one state machine
(decode_value_with) with a leaf-reader trait. The blanket Decode impl
keeps the historical native behaviour byte for byte: thread-local depth
limit under std, bincode's allocating leaf decoders and pre-sized
containers. A new Value::decode_bounded/encode_bounded pair takes explicit
CodecBounds (max bytes, depth, elements) and never allocates from an
untrusted length: every declared length is checked against the unread
input before allocation, containers start empty and grow by push.

platform_serialization::bounded adds CodecBounds, CodecBudget,
BoundsError, BoundedSliceReader with remaining(), canonical_config,
bounded leaf readers and bounded_decode_from_slice, which rejects
trailing bytes. Thread-local depth scope, patch diffing, the indexmap
and HashSet helpers and the random constructors are std or random only.
The platform_value! macro reaches vec! through a crate re-export so it
expands in a no_std guest.

tests/alloc_profile.rs is the integration test for the allocation-only
profile and also runs under default features.

Refs #4679

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Lists wasm32v1-none in rust-toolchain.toml, adds a CI step that checks
platform-serialization and platform-value with default features off on
that target and runs the alloc_profile integration test, adds both
crates to the nightly per-feature matrix and to check-features, and
documents the allocation-only profile in the serialization chapter and
as the fourth CI boundary in the coding conventions.

Refs #4679

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…tees precisely

CodecBudget counted depth in a u16 with saturating addition, so with
max_depth = u16::MAX the 65,536th enter_container succeeded and every
deeper level followed. The counter is now u32 with checked addition
against the u16 limit and BoundsError::DepthExceeded.depth is u32, so one
past the widest limit is still representable and refused. A regression
walks exactly u16::MAX levels and rejects the next.

The docs claimed no allocation exceeds the unread input and that
trailing-byte rejection yields one accepted encoding per value. Neither
is true as stated: container storage and the traversal stack are bounded
by max_elements and max_depth rather than max_bytes, and bincode accepts
overlong varints so distinct inputs decode to the same value. The module,
method and book docs now say what each limit bounds and that canonical
validation (re-encode and compare) belongs to the ABI layer; tests pin
the lenient varint behaviour, and the book example asserts a value round
trip on encoder-produced bytes.

Refs #4679

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…th contract

The bounded encoder's leaf arm was a catch-all, so a future Value variant
that nests or holds a counted collection would have bypassed depth and
element accounting silently. Every leaf variant is now listed, and a new
variant fails to compile at this match until its accounting is decided.

Bounded decoding never recurses, but the returned tree is as deep as
max_depth allows and Value's Drop, Clone and derived Encode walk it
recursively, on success and on the partially built value discarded on
rejection alike. That is the same contract every other path in this
crate lives with, and max_depth is the stack guard; the CodecBounds and
decode_bounded docs now say so and point at the native document limit
of 256 as the reference value. Three tests run decode, drop, trailing
byte rejection and malformed sibling rejection at depth 256 on a 128 KiB
thread stack so a regression in that contract fails loudly.

Refs #4679

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@DCG-Claude

Copy link
Copy Markdown
Collaborator Author

Rebased onto the current v4.3-dev (four commits, none dropped; git range-diff shows the two later fix commits unchanged and the first two changed only where upstream moved).

Two upstream changes landed on the seam this PR introduces, and the rebase makes one design decision on top of them:

  • GroveDB 6.0 / grovedb-bincode 2.1.0. Upstream now pins bincode as grovedb-bincode = 2.1.0 through [workspace.dependencies]. A workspace = true entry cannot switch the inherited default features off, and the guest profile needs std off, so platform-value and platform-serialization repeat the same package and exact pin with default-features = false. A comment above each dependency line says why the pin is duplicated. Cargo.lock still resolves to a single grovedb-bincode 2.1.0.
  • Untrusted bincode decoding. Upstream added a DecodeUntrusted impl for Value by duplicating the iterative decoder in a macro, with one flag controlling try_reserve before each push and empty initial container storage. This PR had already extracted that decoder into decode_value_with over a ValueLeafReader, so the rebase keeps the single state machine and expresses the untrusted path as a second leaf reader: UntrustedLeaves starts containers empty, reserves one element at a time through a new reserve_element hook on the trait (a no-op for the native and bounded readers), and reads byte, text and string-list leaves through bincode's own untrusted decoders. Upstream's untrusted_decode.rs tests pass unchanged against this shape, as do this PR's alloc-profile and bounded-codec tests. The DecodeUntrusted derives upstream added to BinaryData, Bytes20, Bytes32, Bytes36 and Identifier are kept, with the core/alloc imports this PR needs around them.
  • The toolchain file follows upstream to Rust 1.98.1 and keeps this PR's wasm32v1-none target entry.

Local gate on the rebased tree: cargo fmt --check, clippy with all targets and warnings as errors on both crates, cargo check --workspace --all-targets --locked, the guest alloc-only cut (both crates on wasm32v1-none with default features off, plus the alloc_profile test), and the full unit and integration tests of both crates all pass. The platform-value doctest that imports serde_json fails without the json feature on v4.3-dev as well as here, so it is not something this PR introduced; it passes with --features json.


🤖 Posted autonomously by DashVM (Claude Fable 5.1) on behalf of pasta.

@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.

Re-review — Final validation — Phase 2 only (queue backlog)

The bounded codec and alloc-only split are implemented consistently, and the previously reported encoder-exhaustiveness issue is fixed. Two non-blocking quality improvements remain around preserving standard error-chain diagnostics and directly testing the encoder's no-output-allocation guarantee; the Cargo feature rename also removes existing implicit feature names and should be addressed for compatibility.

🟡 3 suggestion(s)

Review provenance

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (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: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: critical by gpt-6-astra (effort low) — The intricate refactor of Value decoding into decode_value_with in packages/rs-platform-value/src/lib.rs changes the shared native and untrusted protocol deserialization path, including container allocation, depth enforcement and byte accounting, alongside substantial new bounded codec logic.
  • Phase 1 reviewers: not run (skipped for throughput: 23 PRs queued, above the 10 limit)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • 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 — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — platform-versioning (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, gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — platform-versioning (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
🤖 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-value/Cargo.toml`:
- [SUGGESTION] packages/rs-platform-value/Cargo.toml:57-58: Preserve the existing Cargo feature names
  Changing these entries to `dep:ciborium` and `dep:serde_json` removes the previously exposed implicit `ciborium` and `serde_json` Cargo features. Existing downstream manifests selecting either name now fail dependency resolution even with default features enabled, unlike the intentional no-default-features migration described in the PR. Retain compatibility aliases in this feature table, or explicitly classify and document their removal as a breaking crate API change in the release metadata.

In `packages/rs-platform-serialization/src/bounded.rs`:
- [SUGGESTION] packages/rs-platform-serialization/src/bounded.rs:370: Expose wrapped errors through Error::source
  The empty `Error` implementation returns `None` from `source()`, including for `Bounds` and `Decode` variants that retain an underlying error. Generic error reporters and callers traversing an error chain therefore cannot reach the underlying `BoundsError` or `DecodeError` after the outer error is type-erased. Return the wrapped error from `source()`, leaving `TrailingBytes` without a source, and apply the same treatment to `BoundedEncodeError` in `packages/rs-platform-value/src/guest_bounds.rs`.

In `packages/rs-platform-value/src/guest_bounds.rs`:
- [SUGGESTION] packages/rs-platform-value/src/guest_bounds.rs:461-469: Measure allocation behavior in the oversized-output regression
  This test promises rejection without allocating the output, but it only checks the eventual `BytesExceeded` error. Moving the size check after allocating or filling the output buffer would still pass, despite violating a central bounded-encoding guarantee. Add an allocator-observing regression around `encode_bounded`, keeping construction of the input `Value` outside the observation window; for this byte-leaf case, the size pass needs no traversal frames, so the test can directly verify that rejection does not allocate an output buffer.

Comment thread packages/rs-platform-value/Cargo.toml Outdated
Comment thread packages/rs-platform-serialization/src/bounded.rs Outdated
Comment thread packages/rs-platform-value/src/guest_bounds.rs
…s, measure encoder rejection

The alloc-only split had switched the cbor and json features to dep:
syntax, which silently removed the implicit ciborium and serde_json
feature names that existed before. Both are back as plain optional
dependencies so a manifest selecting either name still resolves; cbor and
json keep implying std.

BoundedDecodeError and BoundedEncodeError now return the wrapped
BoundsError from Error::source, and the wrapped bincode error under std
(bincode's errors implement the error trait only there), so type-erased
reporters can walk the chain. TrailingBytes has no source.

The oversized-output regression only checked the returned error; a new
allocator-observing test in tests/untrusted_decode.rs proves that
rejecting a 202 byte encoding under a 128 byte bound allocates nothing.

Refs #4679

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot added this to the v4.3.0 milestone Sep 15, 2026

@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.

Re-review — Final validation — Phase 2 only (queue backlog)

Reviewed the complete diff at e6bb212 and found no remaining actionable in-scope defects. Four prior suggestions are fixed; the recursive-destruction blocker is withdrawn because it assumed stack safety beyond the documented caller-selected depth contract. Independent validation passed native and alloc-only unit/integration suites, all-feature tests and doctests, individual feature builds including compatibility names, and both wasm32v1-none checks; the worktree remains clean.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (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: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: critical by gpt-6-astra (effort low) — The intricate refactor of Value decoding in packages/rs-platform-value/src/lib.rs replaces the shared Decode/DecodeUntrusted state machine used for incoming protocol values, directly changing peer-facing deserialization alongside new allocation, element, and depth budgeting.
  • Phase 1 reviewers: not run (skipped for throughput: 14 PRs queued, above the 10 limit)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • 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 — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — platform-versioning (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, gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — platform-versioning (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
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

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