Skip to content

feat(platform): add the dashvm-validation crate and the DashVM protocol table - #4712

Open
DCG-Claude wants to merge 10 commits into
v5.0-devfrom
dashvm/r08-01
Open

DCG-Claude wants to merge 10 commits into
v5.0-devfrom
dashvm/r08-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 3 for R08-01 of the smart-contract plan (#4626): the Dash runtime abstraction, protocol-versioned engine dispatch and deterministic configuration. This part delivers the two foundations the runtime (Part 2) and the named-module linker (Part 3) build on:

  1. the protocol table that selects DashVM behaviour per protocol version, so every engine profile, preparation generation, metering generation, limit and weight has one home in platform-version;
  2. the validation crate that turns submitted canonical WebAssembly into the prepared WebAssembly the engine compiles, deterministically and without an engine, so the node and the developer tooling share one answer to "will this bundle be admitted".

Refs #4683
Refs #4681

What was done?

Protocol table (packages/rs-platform-version):

  • New version/dashvm_versions/{mod.rs, v1.rs} with DashVmVersion { engine_profile, preparation, metering, limits: DashVmLimits, weights: DashVmMeteringWeights } and the first table DASHVM_VERSION_V1. The table follows the DriveAbciWithdrawalConstants precedent: a dedicated constants table for one domain, referenced from the platform version, so a reviewer looking for a DashVM number finds it in the version crate and nowhere else.
  • PlatformVersion gains dashvm: Option<DashVmVersion>. It is None on PLATFORM_V1 to PLATFORM_V14, on the two mocks and on the new placeholders 15 and 16 (the exact pre-feature value, so shipped versions are behaviour-preserving) and Some(DASHVM_VERSION_V1) on PLATFORM_V17, the 5.0 protocol version.
  • New v15.rs, v16.rs (struct-update placeholders for the 4.3 and 4.4 releases, as the allocation register reserves them) and v17.rs; LATEST_VERSION and LATEST_PLATFORM_VERSION move to 17. The sibling PR that introduces the smart-contract computation limits creates the same three files in the same shape; whichever lands second rebases and the add/add conflict is resolved by merging the two v17.rs overrides.
  • The table test checks the invariants the numbers must satisfy wherever the table exists (initial memory fits an instance, an instance fits the invocation reservation, a function fits the module operator cap, no zero cap, nothing before 17 carries a table) rather than restating the literal.
  • book/src/versioning/platform-version.md documents the new field and why 15 and 16 exist.

Validation crate (packages/rs-dashvm-validation, Cargo dashvm-validation, #![forbid(unsafe_code)], #![deny(missing_docs)], pinned to wasmparser =0.236.0 and wasm-encoder =0.236.0, the line Wasmtime 36 ships with):

  • profile.rs: PreparationProfile, built with TryFrom<&PlatformVersion> (fails with ProfileError::NotActive before the 5.0 version) or from_dashvm_version (fails with UnknownGeneration when the table names a generation the binary does not implement). Every limit in the crate is read from it.
  • wasm_features.rs: the explicit allowlist (admitted_features: mutable globals, sign extension, saturating float to int, multi-value, bulk memory, nullable funcref) and a classifier over every operator group of the pinned parser, so each rejection names the proposal it needs (ForbiddenFeature::{Simd, RelaxedSimd, Threads, SharedEverythingThreads, Memory64, MultiMemory, TailCall, ExtendedConst, FunctionReferences, Gc, ExternRef, Exceptions, LegacyExceptions, StackSwitching, WideArithmetic, MemoryControl, CustomPageSizes, ComponentModel}). A new operator group in a future parser is a compile error, not a hole.
  • structure.rs: one pass that runs the pinned validator payload by payload, classifies before validating, and measures against the profile's caps in binary order with the byte cap first (StructuralCap::{Functions, Types, Params, Results, Locals, Globals, Exports, OperatorsPerModule, OperatorsPerFunction, BasicBlocksPerFunction, NestingDepth, InitialMemoryPages, MemoryMaximumPages, TableElements, DataSegmentBytes}), records StructuralMeasurements and InitializationMeasurements, rejects start sections and multiple or imported memories and tables.
  • abi_validation.rs and abi_names.rs: the import allowlist (dash_host envelope functions with exact signatures, dash:<name> bundle bindings, dash_vm reserved and rejected), the export rules (memory, dash_alloc, at least one entry export of (i32, i32) -> i64, no table exports, no re-exported imports) and the ModuleInterface. Every ABI name lives in abi_names.rs so the ABI allocation changes one file.
  • instrumentation/{frame_cost, thunks, rewrite}.rs: the portable logical stack. Three imports are injected (dash_vm.stack_bytes, dash_vm.stack_depth, dash_vm.trap), every direct call to a defined function is wrapped with generated enter and leave helpers charging the callee's frame cost (32 + params + locals + operand slots) and one activation, and every function reachable through the table or an export gets a thunk so return inside bodies stays correct. Index shifting goes through the Reencode hooks; custom sections are stripped; the output carries no limit value (both counters count down from what the runtime sets).
  • admission.rs: two stage-aware validators. validate_submitted produces a SubmittedModule, the only input the instrumenter accepts. validate_prepared re-runs validation and measurement on the output and checks provenance against the submitted facts and the instrumenter's report: exact injected imports in exact positions, submitted imports carried unchanged, type and function counts, per-function operator counts, helper and thunk shapes, export redirection, unchanged memory, table and segments, no surviving custom section, no reference bypassing a thunk. Any drift is ModuleError::Internal, a node fault, never a paid rejection.
  • hashing.rs: CanonicalHash (over the submitted bytes, before any transform), PreparedHash (only over bytes that passed validate_prepared) and BundleDigest, all SHA-256 with distinct domain prefixes and length framing.
  • bundle.rs and bundle_preparation.rs: the descriptor types (ModuleName, FuncSignature, ModuleInterface, PreparedModule, BundleBinding, EntryRef, PreparedBundle) kept here until the ABI crate takes over the manifest shape, and validate_and_prepare_bundle(inputs, declared_bindings, entries, &profile): name rules, module count, bindings resolved against the target's exports with exact signatures and cross-checked against the declared list, Kahn's algorithm with canonical-name tie-break for initialization_order, entry checks, and the digest over generations, names, hashes, bindings and entries.
  • stack.rs documents the counter contract and the two trap codes the runtime distinguishes.

Lockfile: besides the new crate line (wasmparser, wasm-encoder, wat, wast, wasmprinter, termcolor, unicode-width, all at the versions Wasmtime 36 ships with), Cargo.lock bumps itertools and heck under bindgen, criterion and prost-build. That is the resolver re-visiting those graphs when a workspace member is added, not a cargo update: the base lock alone re-resolves to a zero diff, and the bumps reappear with cargo update --workspace --offline as soon as the member exists. cargo metadata --locked passes on the result.

CI: dashvm-validation is registered in both package filters (with the version crate as a trigger) and in the nextest --package list, so its tests run on every PR that touches it.

How Has This Been Tested?

Unit tests beside the implementation (76 in the crate, 1 ignored regenerator; 21 in the version crate), all with WAT fixtures through wat =1.236.0:

  • PreparationProfile::try_from(&PlatformVersion) fails on every version 1 to 16 and succeeds on PlatformVersion::latest(); an unknown preparation generation is refused.
  • Every forbidden proposal rejects with its typed feature (18 fixtures plus a component header), start sections reject, malformed bytes are Invalid not ForbiddenFeature.
  • Every structural cap admits at the cap and rejects at cap plus one; imported functions count against the function cap; memory page caps for initial and maximum; table element cap and required maximum; data segment byte cap; a frame larger than the logical stack.
  • Memory and table shape rules, the import allowlist including all three instrumentation names submitted by an author, the host envelope with all four functions and a signature mismatch, the export rules.
  • Custom sections: the canonical hash changes, the prepared hash and bytes do not; the name section is stripped.
  • Instrumentation golden: src/tests/fixtures/three_functions.wat prepares to the checked-in three_functions.prepared.wasm byte for byte, with the expected frame costs, 3 wrapped call sites and 4 thunks; exports and table entries are redirected to thunks and globals shift; the operator count of the output equals the submitted count plus a known amount per site and thunk; the output re-validates under the admitted features; a helper type is reused when present and added otherwise; prepared bytes are identical under two different logical-stack limits.
  • Provenance: three hand-drifted outputs (a fourth dash_vm import, swapped counters, renamed trap), two lying reports and the canonical bytes themselves all fail as Internal.
  • Bundles: two-module ordering and dependencies, canonical order among independents, a cycle, unknown target, missing export, signature mismatch, undeclared and overdeclared bindings, bad names, duplicates, module count at the cap and cap plus one, entry checks, a dash_vm import inside a bundle, digest stability and sensitivity to names and entries with per-module hashes independent of the bundle.
  • Hash domain separation and a pinned golden for the empty module.
  • Review follow-ups: an oversized function section (200,000 one-byte entries) and an oversized import section are refused at cap plus one before expansion; truncated or forbidden instrumenter output is Internal while the prepared byte cap stays PreparedTooLarge; a late mismatch in two 200,000-entry binding lists is diagnosed without quadratic scanning.
  • Second review round: a section of 200,000 submitted global imports is refused at the first entry (with the reserved dash_vm module refused ahead of the kind), the injected globals are accepted only in the prepared stage, and PreparationProfile only hands out the generation it implements.
  • Third review round: an element segment of 200,000 distinct out-of-range function references is refused at the first bad index (also ref.func in bodies and constant expressions), the defined-global cap admits at cap and refuses at cap plus one, and an extra defined global in the instrumenter's output fails provenance.
  • Fourth review round: dependencies_of sorts before deduplicating (a bundle with unsorted, interleaved bindings lists each target once), and the data-segment cap's doc comment now states that active and passive bytes are counted together, as the code always did.
  • Fifth review round: PreparedBundle::module is a linear scan instead of a binary search over the public modules list, so a bundle assembled out of canonical order still resolves every name (the field-order test reverses the list).

Local gate (exit codes captured to files):

cargo fmt --all -- --check                                                            # rc 0
cargo clippy -p dashvm-validation -p platform-version --all-features --all-targets -- -D warnings   # rc 0
CARGO_TARGET_DIR=/Users/dashvm/work/target-r08-01 cargo check --workspace --all-targets            # rc 0
cargo test -p platform-version --features mock-versions                               # rc 0
cargo test -p dashvm-validation                                                       # rc 0
cargo machete packages/rs-dashvm-validation                                           # rc 0
cargo metadata --locked                                                               # rc 0

Not run locally: the full dpp, drive and drive-abci suites (CI runs them). The version bump to 17 changes PlatformVersion::latest(), which those suites use for their newest-generation tests; the workspace check with all targets passes, and the sibling computation-limits PR on the same base has already exercised the bump in drive-abci.

Breaking Changes

None. No shipped protocol version changes behaviour: every PLATFORM_V1 to PLATFORM_V14 literal gains only dashvm: None, and nothing outside the new crate reads the table yet. Protocol version 17 is not activatable by any network until the 5.0 release.

PlatformVersion gains a required field, so any out-of-tree code that writes the struct literally must add dashvm. No code in this repository does so outside the version crate.

Decisions taken (provisional values)

  1. Every number in DASHVM_VERSION_V1 is the allocation register's provisional starting value (limits under A06, weights under A15, bundle shape under A08); measurement and revision belong to R03-04, FIX-05, R03-07 and R12-04. The one exception is max_globals_per_module (1,024), which the register does not list: it was added at review to bound instantiation work the memory and data caps do not cover, sized like the export cap, and is provisional on the same terms.
  2. The DashVM limits and weights live in a dedicated DashVmVersion constants table under the dashvm slot (the Option is the backfill), following the DriveAbciWithdrawalConstants precedent; the two block-level computation budgets that ABCI reads across crates live in SystemLimits under the sibling computation-limits PR.
  3. PlatformVersion.dashvm is Some first on protocol version 17, the register's 5.0 number; 15 and 16 are placeholders for 4.3 and 4.4. The activation gate for state transitions is separate (R08-11).
  4. Admitted feature set: exactly the proposals Rust 1.92 enables by default for wasm32-unknown-unknown (mutable globals, sign extension, saturating float to int, multi-value, bulk memory, reference types restricted to nullable funcref), plus scalar floats. Everything else, including externref, extended const, tail calls and typed function references, is rejected by name.
  5. Start sections are rejected outright in preparation generation 0 (the strict reading of "bounded starts" under A08); Rust guests do not emit one.
  6. Logical stack: two shared host globals (stack_bytes, stack_depth) that count down, one trap import, a frame cost of 32 bytes base plus 4 or 8 bytes per typed parameter and local plus 8 bytes per operand slot (an upper bound; the validator reports the operand height, not per-slot types), thunks for table and export references. Both limits provisional (A06, FIX-05).
  7. Host envelope and guest export names (dash_host.host_call/response_len/response_read/response_release, memory, dash_alloc, entry signature (i32, i32) -> i64) are placeholders until the ABI allocation (A07, R08-04); they are isolated in abi_names.rs.
  8. Bundle descriptor types live in dashvm-validation until R08-03 and R08-04 move the manifest-facing parts into the ABI crate.
  9. Bindings must be declared and must equal the imports the code establishes; the bundle digest covers generations, names, both hashes, bindings and entries.
  10. Provenance drift in the instrumenter's output is an internal error (node fault), never a paid rejection.
  11. The golden prepared fixture may only be regenerated together with a new preparation generation, because prepared hashes and compiled artifact keys derive from those bytes.

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

Part 1 of 3 for R08-01

🤖 Generated with Claude Code

Automated reviewer consensus (Fable 5.1 implementer, GPT-6 Astra reviewer)

Reviewer consensus

Plan Review consensus

  • R08-01-1 [blocker] Preparation rejects the imports that instrumentation injects -> resolved
    • at PLAN.md:88-90, 140
  • R08-01-2 [blocker] Historical replay cannot survive the proposed engine upgrade -> resolved
    • at PLAN.md:36-57, 104, 171
  • R08-01-3 [major] The planned A→B→A test is impossible under the planned DAG rule -> resolved
    • at PLAN.md:92, 108-112, 163
  • R08-01-4 [major] Consensus limits are placed outside the required SystemLimits tables -> withdrawn
    • at PLAN.md:59-82, 126-128
  • R08-01-5 [major] Nested calls do not actually share the outer resource scope -> resolved
    • at PLAN.md:118-120
  • R08-01-6 [major] Initialization charging contradicts the promised pre-instantiation failure -> resolved
    • at PLAN.md:111-112, 153
  • R08-01-7 [major] Persistent retention and restart recovery are not specified -> resolved
    • at PLAN.md:98-104, 150, 153
  • R08-01-8 [major] Consensus fuel equivalence is asserted without a complete operator proof -> resolved
    • at PLAN.md:69-70, 120, 153, 173-174
    • round 1 R08-01-1: accept: Real contradiction. PLAN.md section 3.3 now defines two stage-aware validators over one feature set: validate_submitted applies the user-import rules including the dash_vm rejection and returns a SubmittedModule proof type that is the only input the instrumenter accepts; validate_prepared re-runs wa
    • round 1 R08-01-2: accept: Correct: with one linked crate, profile v0 would be recompiled by the new compiler after an upgrade and a lost historical artifact could not be rebuilt under original behaviour. Fixed in sections 3.1 and 3.4: the engine crate is declared under a renamed dependency (wasmtime_v36 = { package = "wasmti
    • round 1 R08-01-3: partial: The observation is right: bindings are acyclic at preparation, so A -> B -> A within one invocation is impossible and the test as written could not exist. The suggested alternative of implementing cyclic linking is rejected: the DIP (dip-dashvm-engine-abi, Immutable bundle schema) states the propose
    • round 1 R08-01-4: reject: The conventions rule is that a number goes into SystemLimits or the relevant *_constants table and the versioned method reads it; a dedicated constants sub-table is the established form for domain numbers, see DriveAbciWithdrawalConstants (packages/rs-platform-version/src/version/drive_abci_versions
    • round 1 R08-01-5: accept: The earlier fresh-invoke-with-remaining-fuel design only shared computation. Section 3.6 now defines InvocationScope, an owned per-outer-invocation value holding computation, logical stack (bytes and depth behind the two host globals), the page reservation, host-call count, nesting depth (provisiona
    • round 1 R08-01-6: accept: Section 3.5 now reserves the whole closure's initialisation cost before instantiating anything: the linker sums instance base plus initialised-byte cost over every module in the selected closure using the InitializationMeasurements recorded at preparation, with checked arithmetic, compares against t
    • round 1 R08-01-7: partial: The gap is real: the plan had retention only in memory and said nothing about restart. The suggested fix of durable cache-side reference metadata is the wrong shape for this repository: which bundle versions are runnable or pending under which protocol version is on-chain state owned by Drive and AB
    • round 1 R08-01-8: accept: The plan asserted equivalence on fixtures only. I audited the pinned translation code (wasmtime-internal-cranelift-36.0.14/src/func_environ.rs, fuel_before_op, fuel_check, translate_loop_header) and section 3.6 now states the generation 0 schedule exactly: 1 unit per operator except nop, drop, block

Review consensus

Astra raised no findings.

DCG-Claude and others added 5 commits September 12, 2026 08:26
…5 to 17

Add `PlatformVersion::dashvm: Option<DashVmVersion>`, the table that selects
the smart-contract engine profile, the preparation generation and the
metering generation together with the numeric limits and metering weights
those generations read. It follows the `DriveAbciWithdrawalConstants`
precedent of a dedicated constants table per domain, so every DashVM number
has one home and the runtime crates only project it.

The slot is `None` on every shipped protocol version and both mocks, the
exact pre-feature value, and `Some(DASHVM_VERSION_V1)` on protocol version 17,
the 5.0 version. Versions 15 and 16 are struct-update placeholders for the
4.3 and 4.4 releases as the allocation register reserves them; a forward
merge of the real file is resolved by taking the incoming file. Every value in
`DASHVM_VERSION_V1` is the register's provisional starting value.

The table test checks the cross-field invariants the numbers must satisfy
wherever the table exists rather than restating the literal.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Add `packages/rs-dashvm-validation`, the deterministic transformation from
submitted canonical WebAssembly to the prepared WebAssembly the engine
compiles. It carries no engine, storage or state, so the node and the
developer tooling share one answer to "will this bundle be admitted".

Per module: the canonical byte cap before any decoding, full validation by
the pinned wasmparser under an explicit allowlist (mutable globals, sign
extension, saturating conversions, multi-value, bulk memory, nullable
funcref) with every other proposal classified to a typed rejection,
structural caps read from the protocol's DashVM table, the memory and table
shape rules, the host envelope import allowlist with `dash_vm` reserved,
required `memory` and `dash_alloc` exports plus at least one entry, start
sections rejected, custom sections stripped, and the portable logical-stack
instrumentation: two shared host globals and a trap import, a per-function
frame cost burned into every wrapped direct call, and thunks for functions
reachable through the table or an export. The output is re-validated and
checked for provenance against the submitted module; any drift is an
internal error, never a paid rejection. Canonical and prepared hashes are
domain separated.

Per bundle: validated names, bindings resolved against the target's exports
with exact signatures, a declared-binding cross-check, Kahn's algorithm with
canonical-name tie-break for the initialisation order, entry checks and a
bundle digest.

Every limit comes from `PreparationProfile`, the projection of
`PlatformVersion::dashvm`; the crate defines no number of its own.

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

The crate's test suite lives beside the implementation: every forbidden
proposal rejects with its typed feature, every structural cap admits at the
cap and rejects at cap plus one, memory and table shape rules, the import
allowlist including the reserved dash_vm module, the export rules, custom
section stripping (canonical hash changes, prepared hash does not), the
instrumentation golden output on a three-function fixture with its frame
costs and operator counts, thunk selection, provenance failures on three
hand-drifted outputs and two lying reports, bundle ordering, cycles, missing
targets and exports, signature mismatches, declared-binding cross-checks,
entry checks and digest stability.

The crate is registered in the CI package filters and the nextest package
list so its tests run, with a README describing the pipeline.

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

Clippy's large-error lint fires on `ModuleError` and `BundleError` because
the two variants that carry a pair of function signatures make every
`Result` in the crate wide. Boxing those payloads keeps the happy path small
without changing what a rejection reports.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The golden test asserted the hash against itself. It now pins the real
SHA-256 of the empty module under the canonical domain, so a change to the
domain prefix or the length framing fails here and is a deliberate new hash
domain rather than a silent drift.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@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: 43465f95-e26b-404c-bf31-2ec5cb2bf883

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-18T18:27:30.527Z

@thepastaclaw

thepastaclaw commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

⚠️ DEGRADED — Final review complete — no blockers (commit 144bf1f) · triage: normal · stand-in models (primary models out of quota)

@codecov

codecov Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.73190% with 169 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.52%. Comparing base (5f1e0cc) to head (144bf1f).

Files with missing lines Patch % Lines
packages/rs-dashvm-validation/src/structure.rs 92.61% 44 Missing ⚠️
...s-dashvm-validation/src/instrumentation/rewrite.rs 86.45% 42 Missing ⚠️
packages/rs-dashvm-validation/src/admission.rs 85.64% 29 Missing ⚠️
packages/rs-dashvm-validation/src/wasm_features.rs 90.14% 21 Missing ⚠️
packages/rs-dashvm-validation/src/hashing.rs 74.57% 15 Missing ⚠️
packages/rs-dashvm-validation/src/bundle.rs 94.36% 8 Missing ⚠️
...ackages/rs-dashvm-validation/src/abi_validation.rs 94.26% 7 Missing ⚠️
...ges/rs-dashvm-validation/src/bundle_preparation.rs 98.95% 2 Missing ⚠️
packages/rs-dashvm-validation/src/profile.rs 98.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           v5.0-dev    #4712      +/-   ##
============================================
+ Coverage     86.36%   86.52%   +0.15%     
============================================
  Files          2766     2809      +43     
  Lines        366105   371164    +5059     
============================================
+ Hits         316191   321148    +4957     
- Misses        49914    50016     +102     
Components Coverage Δ
dpp 87.56% <ø> (+0.27%) ⬆️
drive 84.69% <ø> (+0.44%) ⬆️
drive-abci 89.73% <ø> (+0.06%) ⬆️
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.92% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 49.78% <ø> (ø)
🚀 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)

Three in-scope suggestions are confirmed: function-section allocation precedes its cap check, prepared-output validation can return submission errors instead of internal faults, and binding-mismatch diagnostics perform quadratic searches. The proposed memory blocker is not supported: the code explicitly permits an omitted declared maximum and assigns execution-time memory limits to the runtime, which is outside this foundational PR. Verification was source-based; no tests were run.

🟡 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: 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: normal by gpt-6-astra (effort low) — This is a large, intricate addition spanning protocol-version dispatch, deterministic WebAssembly admission/preparation/instrumentation, and CI/build integration, but it does not itself change consensus-critical funds, cryptographic, key-handling, peer-deserialization, or storage-migration surfaces.
  • 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 high); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort high); 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-dashvm-validation/src/structure.rs`:
- [SUGGESTION] packages/rs-dashvm-validation/src/structure.rs:432-448: Enforce the function cap before expanding the entire section
  The function-section loop allocates a FunctionFact for every declared function before checking max_functions_per_module. measure calls this classifier before validator.payload, so the underlying validator cannot reject an excessive function count first. A submission below the 16 MiB byte cap can encode millions of one-byte references to an existing type, expanding into hundreds of MiB of function facts before rejection against the 50,000-function cap; valid function bodies are not needed to reach this allocation. Enforce the running imported-plus-defined count before each push, or preflight the section count with an explicitly chosen error precedence. Apply the same early function-count check to import_section, which also accumulates the entire section before checking its function cap, and add an oversized-section regression.

In `packages/rs-dashvm-validation/src/admission.rs`:
- [SUGGESTION] packages/rs-dashvm-validation/src/admission.rs:70: Translate prepared-output validation failures into internal errors
  The bare ? propagates failures from validating generated bytes as ordinary module rejections. For example, truncated instrumenter output returns Invalid, while an introduced forbidden instruction returns ForbiddenFeature. prepare_module reaches this call only after the submitted bytes passed admission and were instrumented, so these failures indicate an instrumenter fault, not an invalid submission. This contradicts the documented Internal-versus-paid-rejection boundary in admission.rs and errors.rs. Translate prepared-stage measurement failures to Internal while preserving PreparedTooLarge as the intentional output-size rejection, and add a malformed-output regression alongside the existing provenance tests.

In `packages/rs-dashvm-validation/src/bundle_preparation.rs`:
- [SUGGESTION] packages/rs-dashvm-validation/src/bundle_preparation.rs:193-194: Use the existing binding order for mismatch diagnostics
  Each find predicate performs a linear Vec::contains search for every binding it examines. When large declared and resolved lists differ only near the end, producing the rejection diagnostic takes quadratic time; both searches are evaluated before the match selects its message. The configured module, import, and export caps allow enough distinct bindings for this to require billions of comparisons. Both tuple vectors are already lexicographically sorted: declared is sorted here, and resolved preserves the ordering of the sorted BundleBinding list. Use binary_search to retain the current first-difference diagnostic without quadratic scanning, and add a large nearly-equal-list regression.

Comment thread packages/rs-dashvm-validation/src/structure.rs Outdated
Comment thread packages/rs-dashvm-validation/src/admission.rs Outdated
Comment thread packages/rs-dashvm-validation/src/bundle_preparation.rs Outdated
Three findings from the automated review, each real:

- The import and function sections recorded every entry before comparing
  the count against the function cap, so a small module declaring millions
  of one-byte function entries expanded into a fact per entry before being
  refused. Both sections now enforce the running count on each entry and
  refuse at cap plus one. A hand-encoded regression declares 200,000
  functions under a cap of 64.
- The measurement pass on the instrumenter's output propagated its
  refusals as the paid rejections the same bytes would earn on submission
  (Invalid, ForbiddenFeature), contradicting the documented boundary: the
  output comes from a module that already passed admission, so a refusal
  there is the instrumenter contradicting itself. Those refusals are now
  Internal; the prepared byte cap keeps its own variant. A regression
  covers truncated output, an inserted tail call and the byte cap.
- The declared-binding mismatch diagnostic searched each list with a linear
  scan per element. Both lists are sorted, so binary search finds the first
  difference without the quadratic cost. A regression diagnoses a late
  difference in two 200,000-entry lists.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot added this to the v5.0.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)

All three prior findings are fixed at 3e30f50. Three distinct findings remain: a blocking allocation issue when rejecting submitted global imports, an unchecked preparation-generation invariant, and repeated import counting during signature lookup. Targeted tests passed—68 dashvm-validation tests and 21 platform-version tests, with one intentional ignored regenerator—and git diff --check passed; the worktree is unchanged.

🔴 1 blocking | 🟡 2 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); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)

  • Triage: normal by gpt-6-astra (effort low) — This is a large, intricate addition spanning protocol-version dispatch, WebAssembly admission, deterministic preparation, instrumentation, and CI/build integration, but it does not itself change consensus rules, funds movement, cryptography, peer-facing deserialization, or storage migrations.
  • 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 high); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort high); agent phase2-reviewer, gpt-6-astra — platform-versioning (completed, effort high); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort high); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort high); 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-dashvm-validation/src/structure.rs`:
- [BLOCKING] packages/rs-dashvm-validation/src/structure.rs:393-399: Reject submitted global imports before accumulating their facts
  The submitted-stage classifier records global imports even though interface_of_submitted rejects them later. These entries do not increment imported_functions, so the function cap does not bound their accumulation into ImportFact values and owned names. Because measure classifies the entire section before calling validator.payload, the validator's limits cannot stop this allocation first. The canonical byte cap therefore permits substantial memory amplification for input that admission will never accept. This affects the new untrusted-module admission API itself, independently of future runtime integration. Reject global imports during submitted-stage classification, preserving the reserved-module diagnostic for dash_vm, while retaining the instrumentation globals in the prepared stage. Add coverage for early rejection and prepared-stage acceptance.
- [SUGGESTION] packages/rs-dashvm-validation/src/structure.rs:122-125: Reuse the measured import count during signature lookup
  function_signature calls this method for every lookup, including each generated thunk in Rewriter::parse_code_section and each function export during ABI validation. Even defined-function lookups therefore scan the full import list before performing otherwise indexed reads, adding O(imports × thunks) work during preparation. The measurement pass already maintains structure.imported_functions in both submitted and prepared stages, and these callers consume completed facts. Return that recorded count to remove the repeated scans without changing admission behavior.

In `packages/rs-dashvm-validation/src/profile.rs`:
- [SUGGESTION] packages/rs-dashvm-validation/src/profile.rs:18-21: Keep the supported-generation invariant inside PreparationProfile
  from_dashvm_version rejects unsupported preparation generations, but the public generation field allows callers to bypass that check through direct construction or mutation. validate_submitted and prepare_module always execute generation-0 behavior, while prepare_module and validate_and_prepare_bundle copy the supplied generation into their output metadata. The public API can consequently label generation-0 preparation as an unsupported generation instead of failing closed. Make generation private with a read-only accessor and checked construction, or enforce generation dispatch at the shared public entry points. Add coverage at that boundary. This is non-blocking for shipped protocols because the normal constructor checks the generation and the crate is not yet connected to an existing consensus execution path.

Comment thread packages/rs-dashvm-validation/src/structure.rs
Comment thread packages/rs-dashvm-validation/src/profile.rs Outdated
Comment thread packages/rs-dashvm-validation/src/structure.rs Outdated
Three findings from the second automated review round:

- A submitted global import was recorded as an import fact and refused only
  later by the interface rules, so the function cap did not bound how many
  such facts a module under the byte cap could accumulate. The submitted
  stage of the measurement pass now refuses the reserved dash_vm module and
  any global import at the entry where it appears, with the diagnostics the
  interface rules gave, while the prepared stage keeps accepting the two
  counters the instrumenter injects. Regressions: 200,000 global imports
  refused at the first one, a dash_vm import refused as reserved ahead of
  its kind, and the injected globals accepted only in the prepared stage.
- The preparation and metering generations of PreparationProfile were
  public fields, so a caller could construct or mutate a profile naming a
  generation the crate does not implement while the crate applied
  generation 0. They are private now with read-only accessors; the checked
  constructors are the only way to hold a profile.
- The imported function count was recounted by scanning the import list on
  every signature lookup (each thunk, each function export). The measurement
  pass already records the count; the helper returns it.

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

Copy link
Copy Markdown
Collaborator Author

Note on the red codecov/project status on a9073b5: it is a report-mapping artifact from the self-hosted runner, not lost coverage.

  • codecov/patch is green at 91.58% (target 50%) and every job in the workspace run passed.
  • The project drop (77.85%, minus 8.51 points) is concentrated in files this PR does not touch. For one of them, packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/masternode_vote/mod.rs, codecov's file report for this head lists instrumented lines up to 11,653 while the file on this branch has 11,587 lines, with 2,580 misses inside test functions the job log shows passing. The line table belongs to another branch's binaries that were still present in the runner's persistent cargo target when the coverage report was assembled.
  • The same pattern has been seen on other PRs against this base on the same runners since 2026-09-12. Rerunning the job merges a second upload with the same stale mapping and lowers the figure further, so I am not rerunning it. The status is not required by the v5.0-dev ruleset.

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

⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2

⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models: gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped at high effort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.

Part 1 foundations are sound and all six prior findings verify as fixed, but two in-scope gaps remain in the new admission pass: unbounded distinct function-reference accumulation before rejection and unbounded, unmeasured, unchecked defined globals.

🔴 1 blocking | 🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: architecture-layering); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: platform-versioning); reviewer 4: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 5: gemini-3.8-flash-high (agent: phase1-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 8: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 9: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 10: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)

  • Degraded mode: gpt-6-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe); stand-ins gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor; Phase 1 effort capped at high
  • Triage: normal by muse-spark-1.3-contributor (standing in for gpt-6-astra) (effort low) — Large additive change (+6496 lines with new validation crate and v17 protocol table) but behaviour-preserving for shipped versions with no modification to existing consensus, funds, crypto, or migration logic.
  • Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — architecture-layering (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — platform-versioning (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — rust-quality (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — security-auditor (completed, effort high); agent phase1-reviewer
  • Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 100% left, 5h 100% left
  • Fresh verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) — final-verifier; agent astra-verifier
  • Phase 2 reviewers: muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — architecture-layering (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — platform-versioning (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — rust-quality (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — security-auditor (completed, effort high); 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-dashvm-validation/src/structure.rs`:
- [BLOCKING] packages/rs-dashvm-validation/src/structure.rs:622-631: Bound distinct function references before validation rejects them
  The element-section Functions arm inserts every decoded index into referenced_functions before validator.payload can reject it, with no bound. A single ~16 MiB element segment can encode millions of distinct small-LEB indices (1-3 bytes each on the wire, heap B-tree entries each), and only afterwards does validation fail on the out-of-range index. Any valid module has all referenced indices below its total function count, which is capped at 50,000, so a distinct set larger than the cap is proof of invalidity regardless of section order. The same unbounded insert exists in const_expr (ref.func in global/element/data init exprs) and in function_body (ref.func in code). This is the same allocation-before-rejection class as the fixed global-import issue and needs the same per-entry guard.
- [SUGGESTION] packages/rs-dashvm-validation/src/structure.rs:570-580: Bound defined globals: no cap, measurement, or provenance check
  Defined globals are the one section with no structural bound: DashVmLimits caps functions, types, exports, table elements and data bytes but has no globals cap; global_section records no count and enforces no cap; StructuralMeasurements and InitializationMeasurements record no globals; and validate_prepared compares imports, types, functions, exports, memory, table and segments but never globals, so global drift is invisible to provenance. A 16 MiB submission of ~5-byte global definitions admits ~3M globals whose instantiation cost evades the memory-page and data-byte caps. Add a max_defined_globals limit, record it, enforce it on the submitted stage, and compare globals in validate_prepared.

Comment thread packages/rs-dashvm-validation/src/structure.rs
Comment thread packages/rs-dashvm-validation/src/structure.rs
…sion

Two findings from the third automated review round:

- Every function reference (a table element item or a `ref.func` in a body
  or constant expression) was inserted into the distinct-reference set before
  the pinned validator, which runs after each section is read, could refuse
  an out-of-range index. A segment under the byte cap could therefore grow
  the set into the millions. References are now bounded where they appear:
  the function section precedes every section a reference can occur in, so
  an index past the declared count is refused with the validator's own
  message and the set can never outgrow the function cap. A hand-encoded
  segment of 200,000 distinct indices against a one-function module is
  refused at the second index; bodies and constant expressions are covered.
- Defined globals had no cap, no measurement and no provenance check. The
  DashVM table gains `max_globals_per_module` (provisional, 1,024, sized
  like the export cap and far above what a Rust guest emits), the
  measurement pass counts globals and enforces the cap on every entry in the
  submitted stage, and the provenance check refuses output whose defined
  global count differs from the submission (the instrumenter's two counters
  are imports, so the count is unchanged by design).

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

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

⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2

⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models: gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped at high effort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.

Part 1 of the DashVM stack adds the versioned limits table and deterministic admission crate with all eight prior allocation and provenance issues fixed at this head. Two minor in-scope items remain: the data-segment cap doc understates enforcement and dependencies_of relies on constructor sort order for dedup.

🟡 2 suggestion(s)

Review provenance

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: architecture-layering); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 8: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 9: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 10: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); reviewer 11: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 12: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 13: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 14: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 15: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)

  • Degraded mode: gpt-6-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-ins gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor; Phase 1 effort capped at high
  • Triage: normal by muse-spark-1.3-contributor (standing in for gpt-6-astra) (effort low) — Large new crate and version table adds future DashVM validation logic but does not itself change existing consensus rules, funds movement, cryptography, network deserialization, or migrations.
  • Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — architecture-layering (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — platform-versioning (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (completed, effort high); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (zai below 15% reserve: 5h 99% left, weekly 14% left)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) — final-verifier; agent astra-verifier
  • Phase 2 reviewers: muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — architecture-layering (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — platform-versioning (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — rust-quality (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — security-auditor (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — architecture-layering (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — platform-versioning (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — rust-quality (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — security-auditor (completed, effort high); 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-version/src/version/dashvm_versions/mod.rs`:
- [SUGGESTION] packages/rs-platform-version/src/version/dashvm_versions/mod.rs:110-111: Data-segment cap doc says active-only but enforcement counts passive too
  The docstring says active data segments only, but structure.rs data_section() adds datum.data.len() into data_bytes for both Active and Passive kinds and enforces StructuralCap::DataSegmentBytes against that total. The enforcement is the safe reading and matches the field name; the doc understates it and could mislead a reader checking whether passive bytes are bounded.

In `packages/rs-dashvm-validation/src/bundle.rs`:
- [SUGGESTION] packages/rs-dashvm-validation/src/bundle.rs:343-352: dependencies_of dedups without sorting, relying on constructor order
  Vec::dedup only removes consecutive duplicates, so correctness depends on bindings arriving sorted by the derived BundleBinding Ord (importer, target, export, signature). validate_and_prepare_bundle does sort, but PreparedBundle fields are pub so any other construction path with unsorted bindings silently returns duplicate targets. Sorting the small local list before dedup removes the hidden precondition without changing behavior on well-formed bundles.

Comment thread packages/rs-platform-version/src/version/dashvm_versions/mod.rs Outdated
Comment thread packages/rs-dashvm-validation/src/bundle.rs
…sely

Two review suggestions, both accurate:

- The doc comment on `max_data_segment_bytes_per_module` said active
  segments only, while the measurement pass has always counted active and
  passive bytes together (passive segments are copied by `memory.init` at
  run time). The comment now says what is enforced.
- `PreparedBundle::dependencies_of` deduplicated without sorting, relying on
  the binding list arriving in the sorted order preparation produces; the
  fields are public, so the sort is now repeated on the small local list. A
  test fills the bindings unsorted and interleaved and checks each target is
  listed once.

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

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

⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2

⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models: gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped at high effort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.

Part 1 of R08-01 adds the DashVM protocol table (v17) and the deterministic dashvm-validation crate. All 10 prior findings verify as fixed at this head, and the new crate is unwired from consensus so no divergence is introduced. One minor robustness suggestion remains on a new public lookup with a hidden sorted-order precondition.

🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: architecture-layering); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: platform-versioning); reviewer 4: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 5: gemini-3.8-flash-high (agent: phase1-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 8: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 9: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 10: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); reviewer 11: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 12: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 13: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 14: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 15: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)

  • Degraded mode: gpt-6-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-ins gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor; Phase 1 effort capped at high
  • Triage: normal by muse-spark-1.3-contributor (standing in for gpt-6-astra) (effort low) — Large new validation crate and version table, but it adds inactive foundations with behavior-preserving Nones for shipped versions rather than changing live consensus, funds, crypto, or migration logic.
  • Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — architecture-layering (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — platform-versioning (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — rust-quality (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — security-auditor (completed, effort high); agent phase1-reviewer
  • Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 100% left, 5h 100% left
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) — final-verifier; agent astra-verifier
  • Phase 2 reviewers: muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — architecture-layering (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — platform-versioning (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — rust-quality (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — security-auditor (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — architecture-layering (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — platform-versioning (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — rust-quality (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — security-auditor (completed, effort high); 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-dashvm-validation/src/bundle.rs`:
- [SUGGESTION] packages/rs-dashvm-validation/src/bundle.rs:334-340: module() binary-searches a publicly-constructible Vec
  PreparedBundle::module uses binary_search, which returns a wrong None when modules is not in canonical name order. The field docs state that order and the canonical producer sorts, but every field of PreparedBundle is pub, so any other construction path with unsorted modules silently breaks the lookup. With max_modules_per_bundle at 16 a linear scan costs the same and removes the hidden precondition, matching the fix just applied to dependencies_of.

Comment thread packages/rs-dashvm-validation/src/bundle.rs
`PreparedBundle::module` binary-searched the public `modules` list, so a
bundle assembled out of canonical order returned `None` for a present name.
The module cap is small, so a linear scan costs the same and drops the
hidden ordering precondition, matching `dependencies_of`. The order test
now reverses the module list and checks the lookup still finds each module.

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

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

⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2

⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models: gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped at high effort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.

Final gate on PR #4712 at head 144bf1f. All eleven prior findings revalidate as fixed against the current source, and the head commit's linear-scan refactor of PreparedBundle::module is correct with test coverage. No new in-scope defects found in the validation crate, bundle preparation, or protocol table.

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

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: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 8: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 9: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 10: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); reviewer 11: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 12: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 13: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 14: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 15: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)

  • Degraded mode: gpt-6-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-ins gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor; Phase 1 effort capped at high
  • Triage: normal by muse-spark-1.3-contributor (standing in for gpt-6-astra) (effort low) — Large new isolated validation crate and version table that adds future DashVM admission logic without altering existing consensus, funds, crypto, or migration paths.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — architecture-layering (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — platform-versioning (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (completed, effort high); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (zai below 15% reserve: 5h 99% left, weekly 14% left)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) — final-verifier; agent astra-verifier
  • Phase 2 reviewers: muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — architecture-layering (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — platform-versioning (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — rust-quality (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — security-auditor (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — architecture-layering (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — platform-versioning (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — rust-quality (completed, effort high); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — security-auditor (completed, effort high); 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.
Out-of-scope follow-up suggestions (2)

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.

  • Seal bundle descriptors when the ABI crate lands — PreparedBundle and its fields remain pub and constructible outside validate_and_prepare_bundle; ordering assumptions are now removed from its methods, but post-validation mutation enforceability belongs to the forthcoming ABI/linker owner, not this foundation PR.
    • Follow-up: In the Part 2/3 PR, move manifest-facing shapes into the ABI crate with validated construction and private fields.
  • Preparation work is not yet fee-metered or block-bounded — A worst-case bundle within the caps still represents tens of MB of preparation work; which party pays and the per-block preparation budget must be settled before contract submission becomes a live state transition.
    • Follow-up: Cover charging or rate-limiting bundle preparation in the runtime/linker PRs.

@github-actions

Copy link
Copy Markdown
Contributor

PR Hygiene

State: waiting-bots · commit 144bf1f70a2c27f116564e010513977b1e2830f3

  • coderabbitai has not reported for the current head

Self-review is an author attestation that you have read the diff:
/self-reviewed — covers everything pushed so far; post it again after a new push.

This check passes when the policy is satisfied; the repository decides whether merging requires it.

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