test(sdk): run operation-proof tests in CI and select the verifier generation from the verifier by shumkov · Pull Request #5559 · dashpay/platform

shumkov · 2026-10-23T14:06:36Z

Issue being fixed or feature implemented

Follow-up to #5541, from its two post-merge reviews:

Its main regression suite never ran in CI. CI does not run drive-proof-verifier's tests. The non-unique identity suite added later by fix(sdk)!: version the non-unique identity lookup so 4.1 clients keep working #5515 has the same gap.
The client crate built drive with server. The suites build real proofs, so fix(sdk): verify operation proofs signed by older and newer protocol versions #5541 gave drive-proof-verifier's dev-dependencies drive's fixtures-and-mocks, which pulls in full and server. With resolver 2, that compiles the client library against server-featured drive in cargo test. The architecture overview forbids this. CI's transport-free check uses cargo tree -e normal, so it could not see it.
The dispatcher picked the verifier generation from the producer's table. verify_state_transition_was_executed_with_proof_for_query_version matched on the producer's generation and only range-checked the caller's. A later verifier generation with a stricter check would then be skipped whenever an SDK verified an older producer's outcome.
Smaller issues:
- Some test assertions were weak.
- The book promised more about newer protocols than the code delivers.
- A comment in proof.rs was inaccurate.
- The newer-version vector was tied to the protocol 14 layout.

What was done?

Suites moved to drive-abci (test(sdk)). git mv moves document_outcome.rs, identity_discovery.rs, identity_discovery/mainnet.rs and the signed_response signing harness from rs-drive-proof-verifier/tests to rs-drive-abci/tests. drive-abci already has those dev-dependencies, and CI's nextest run covers it. drive-proof-verifier's dev-dependency features are back to their pre-fix(sdk): verify operation proofs signed by older and newer protocol versions #5541 state: no drive fixtures-and-mocks, no dpp random-*. The binary vectors stay in rs-drive-proof-verifier/tests/vectors, so the rs-sdk tests that replay them keep their paths. The suites and exporters reach them through ../rs-drive-proof-verifier/tests/vectors.
Dispatcher on the verifier's generation (refactor(drive)). The dispatcher matches on the caller's generation. Generations 0 and 1 share one implementation, because they apply the same checks. The only producer-dependent input is whether the proof carries the owner balance. It now comes from the producer's prove_state_transition generation, the generation that decides the layout. An unknown prover generation is refused as UnknownVersionMismatch naming prove_state_transition. The duplicated 0 | 1 guard is gone. The one-line v1 module is removed. Version-table docs: the verify v3 table no longer says the two generations must move in lockstep, and the prove_state_transition field doc now says verifiers read the producer's layout from it.
Test nits (test(sdk)):
- The security-floor test asserts the exact error: the Drive UnknownVersionMismatch for an unknown verifier generation, and GroveDBError for every newer signed version.
- The SDK layout test gains a stored-below-producer case, pinned and unpinned.
- The newer-version vector is built over PlatformVersion::latest() with its own newer-contract.bin. Rerunning the exporter after a layout change therefore gives a vector the newest rules still read.
Doc nits:
- put-operations.md says the newer-protocol fallback verifies only while the newer protocol keeps the layout, and gives the absence caveat.
- The proof.rs comment says what the producer's version actually selects.
- The corpus README notes that document-outcome/ and identity-discovery/ are signed SDK fixtures, not manifest corpus cases.
- The vector READMEs name the new generator commands.
Port of 62db704 from v5.1-dev, cherry-picked with -x: the non-unique lookup tests cover every served protocol version. With latest = 14 the version lists are unchanged on this branch.

In-place changes to shipped generations

verify_state_transition_was_executed_with_proof generations 0 (PV1–PV14) and 1 (PV14, active on testnet) are restructured in place. This is a pure refactor with identical output for every protocol version:

Before, generation N ran the shared body with carries_owner_balance = (N == 1), with N read from the producer's table.
Now the body is selected by the caller's generation 0 or 1, and carries_owner_balance = (producer's prove_state_transition == 1).
Every table pairs prover generation N with verifier generation N: PV1–PV13 and both mock tables are 0/0, and PV14 is 1/1. A temporary test over PLATFORM_VERSIONS plus TEST_PLATFORM_V2/V3 confirmed this; it is not committed.
So every known version runs the same body with the same flag.
The code is client-side verification only. drive-abci calls it from #[cfg(test)] code only, so consensus cannot change.

How Has This Been Tested?

Dispatcher, red before and green after. With the old dispatcher, both new or updated dispatcher tests fail:
- should_reject_unknown_verifier_or_producer_generation got GroveDB(CorruptedData(..)) instead of UnknownVersionMismatch for prove_state_transition.
- should_not_read_the_producers_verifier_generation got UnknownVersionMismatch(verify_state_transition_was_executed_with_proof, 255) where the known-producer result is GroveDB(CorruptedData(..)).
With the new code, cargo test -p drive --lib verify_state_transition_was_executed_with_proof passes 83 tests. The 81 existing ones are unchanged.
Mutation checks:
- Map unknown newer versions to PlatformVersion::first() in proof_query_platform_version. The strengthened security-floor loop fails (version 15: invalid signature); the old loop passed.
- Remove the pin check from maybe_update_protocol_version. The new stored-below-producer case fails (stored 13, producer 14, pinned true: 14 != 13); the old pinned loop passed.
Moved suites: DASH_SDK_EXPORT_NONUNIQUE_VECTORS=1 cargo test -p drive-abci --test document_outcome --test identity_discovery -- --include-ignored passes 7 + 14, exporters included. The exporters rewrote every committed vector byte-for-byte.
drive-proof-verifier without the removed features: cargo test -p drive-proof-verifier --all-features passes 356 lib tests plus the vectors_* suites. cargo tree -p drive-proof-verifier -e features -i drive (normal + dev) showed fixtures-and-mocks, full and server before and none after.
Other suites:
- cargo test -p dash-sdk --features mocks --lib signed_version_tests: 16 passed.
- cargo test -p drive-abci --test nonunique_merged_proof: 12 passed.
- cargo test -p drive-abci --lib identity_by_non_unique_public_key_hash: 14 passed.
Checks: cargo check --workspace --all-targets --locked passes. CI's clippy command (--workspace --all-targets --all-features -D warnings) passes, and was rerun on the touched crates after the review fixes. cargo fmt --all --check is clean.
Mock version in the security-floor loop. The test now also signs as TEST_PROTOCOL_VERSION_2. Mutation check: let proof_query_platform_version look up real mock versions. The new case fails (version 268435458: invalid signature). The 0x10000000 case passes that mutant.
CI: the moved suites run in the drive-abci nextest step of the Rust workspace job. In run 39966843902, drive-abci::document_outcome had 6 passed and drive-abci::identity_discovery had 13 passed (the two exporters are #[ignore]d). The two dispatcher tests in drive also passed. Summary: 19681 tests run, 19681 passed, 1417 skipped.

Notes for reviewers

PR 1 of this batch. PR 1 adds --package drive-proof-verifier to the CI nextest list and adds one more signed test under rs-drive-proof-verifier/tests. Once PR 1 lands, this PR merges v5.0-dev, moves that test into drive-abci too, and keeps PR 1's CI-list line, which still covers the crate's lib and vectors_* tests.
Book conflict to clean up. book/src/versioning/versioned-dispatch.md says, without exception, "Never modify a shipped generation, not even by threading a parameter" and "Old code is never deleted". coding-conventions.md allows a pure in-place refactor that is documented in the PR, as above. This PR follows the conventions chapter. It is client-side verification code that block replay never runs. The absolute bullet should get the same exception.
Kept on purpose: carries_owner_balance remains a bool on the shared body, as it already was. A third layout would need a layout enum.
Not changed: the identity-discovery newer-* vector is still built over protocol 14. Its README already says it verifies while the newest rules build the protocol 14 query.
Not added: a test of a PV13 verifier reading a PV14 owner-balance proof through the direct API. That direction is the direct-caller hazard from the fix(sdk): verify operation proofs signed by older and newer protocol versions #5541 review, and PR 1 adds guards around it.

Breaking Changes

None. The pub signatures are unchanged. Every changed path is client-side verification or test code.

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
If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request

For repository code-owners and collaborators only

I have assigned this pull request to a milestone

🤖 Generated with Claude Code

PR Hygiene · `fe8a7dd`

Bots — coderabbitai ✓ · thepastaclaw ✓
Self-review — post /self-reviewed
Build green
Approvals — you own every area touched; none needed

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

Summary by CodeRabbit

Improvements
- Updated state-transition proof verification to support both existing proof layouts, using the producer’s version to interpret the proof and the caller’s version to select verification rules.
- Expanded compatibility coverage across registered protocol versions for identity and document proofs.
- Clarified how SDKs handle signed results from newer protocol versions, including when unpinned SDKs update their stored version.
Documentation
- Added guidance on proof layouts, protocol-version handling, and signed compatibility fixtures.

The version lists of #5515's tests named 12, 13 and the latest version, which was 14 on v5.0-dev. With protocol 15 as the latest version they no longer exercised protocol 14, which testnet runs, so a protocol 14 regression would go unnoticed once protocol 15 gets tables of its own. The lists are now ranges that run up to the latest version: - drive-proof-verifier identity_discovery signed_versions: V0 from 12, V1 from 13 (a sound proof no honest node sends), through latest + 1. - drive-abci identity_by_non_unique_public_key_hash: V0 at 12..=latest, V1 at 14..=latest; the V1 test also checks the answer's protocol version. - drive-abci nonunique_merged_proof: versions() is 12..=latest, and V1 is served from 14. With latest = 14 these produce the same lists as before, so the change is equally correct on v5.0-dev. Mutation check: with PLATFORM_V14 given DRIVE_ABCI_QUERY_VERSIONS_V1 (protocol 14 refuses V1), should_answer_v1_with_one_proof_from_protocol_14 passes before this change and fails after it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 62db704)

… the verifier verify_state_transition_was_executed_with_proof_for_query_version matched on the producer's table to pick v0 or v1, and only range-checked the caller's generation. A later verifier generation with a stricter check would then be skipped whenever an SDK verified an older producer's outcome. The dispatcher now matches on the caller's generation. Generations 0 and 1 share one implementation, the former v0 internal body, because they apply the same checks. The only producer-dependent input, whether the proof carries the owner balance, comes from the producer's prove_state_transition generation: 0 means no balance, 1 means balance, and any other value is refused as UnknownVersionMismatch naming prove_state_transition. The duplicated 0|1 guard is gone. The v1 module, a one-line call with carries_owner_balance = true, is removed. No behaviour change for any real table. PV1..PV13 and both mock tables have prove 0 / verify 0, and PV14 has prove 1 / verify 1. A temporary check over PLATFORM_VERSIONS plus TEST_PLATFORM_V2/V3 confirmed this (not committed). So for every known version the same body runs with the same flag. The existing suites pass unchanged: - drive verify_state_transition_was_executed_with_proof: 83 tests - drive-proof-verifier document_outcome - dash-sdk signed_version_tests Only synthetic tables behave differently, which is the point of the change. Two dispatcher tests pin it: - should_reject_unknown_verifier_or_producer_generation: an unknown prover generation in the producer's table is refused, naming the method. - should_not_read_the_producers_verifier_generation: the producer's verifier generation is never read. Red before, green after: with the old dispatcher both fail. The first returned GroveDB CorruptedData instead of UnknownVersionMismatch for prove_state_transition. The second returned UnknownVersionMismatch(verify.., 255) where the known-producer result is GroveDB CorruptedData. The version-table docs now say the verifier reads the layout from the producer's prove_state_transition, instead of saying the two must move in lockstep. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ocol limits Tests: - The document-outcome security-floor test now asserts why each case fails. A verifier table with an unknown generation must give the Drive UnknownVersionMismatch for that method. A newer signed version (latest+1, and the start of the mock range) must give GroveDBError, because the newest rules need the owner balance the protocol 13 proof lacks. Mutation check: with unknown newer versions mapped to PlatformVersion::first(), the loop now fails ("version 15: invalid signature"). Before, it passed. - The SDK document-outcome layout test adds a stored-below-producer case (stored 13, protocol 14 answer, pinned and unpinned). It must verify in the balance layout, keep the pin, and learn 14 only when unpinned. Mutation check: with the pin check removed from maybe_update_protocol_version, the new case fails ("stored 13, producer 14, pinned true: 14 != 13"). The old loop, seeded only at latest, passed. Vectors: - The exporter builds the newer-version vector over the latest protocol, not a fixed 14, and writes its contract as newer-contract.bin. Rerunning it after a protocol changes the outcome layout yields a vector the newest rules still read. Today latest is 14, so the rerun rewrote every existing vector byte-for-byte. newer-contract.bin equals pv14-contract.bin. Docs: - The vector READMEs explain this. The corpus README notes that document-outcome/ and identity-discovery/ are signed SDK fixtures, not manifest-based corpus cases. - put-operations.md now says the newer-protocol fallback verifies only while the newer protocol keeps the layout. A layout change needs an SDK update, and an absence-proved outcome could verify at a location the newer protocol no longer uses. - The proof.rs comment now says what the producer's version selects: query construction for the non-unique lookup, but only the owner-balance layout for state-transition outcomes. No behaviour change: tests, fixtures and docs only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Before this commit, two suites that build real proofs and sign them with a test quorum lived in drive-proof-verifier: - document_outcome, the cross-version document-outcome tests - identity_discovery with its mainnet replay, the non-unique lookup tests CI does not run that crate's tests. To build the proofs, the crate's dev-dependencies enabled drive's fixtures-and-mocks feature, which pulls in full and server, plus dpp's random-identities and random-documents. With resolver 2, `cargo test -p drive-proof-verifier` then compiled the client library against server-featured drive. The architecture overview forbids that: a client-side crate never imports drive with server. The transport-free check uses `cargo tree -e normal`, so it could not see the problem. The suites and their signing harness move to rs-drive-abci/tests with `git mv`. drive-abci already has those dev-dependencies, and CI's nextest run includes it with --all-features. The dev-dependency features return to what they were before the suites were added. Now `cargo tree -p drive-proof-verifier -e features -i drive` (normal + dev) shows only the verify cut: no server, full or fixtures-and-mocks. Changes to make the suites build in drive-abci: - ContextProvider and ContextProviderError are imported through the drive_proof_verifier re-export, as nonunique_merged_proof.rs already does. - The vectors stay in rs-drive-proof-verifier/tests/vectors, next to the crate they verify, so the rs-sdk tests that replay them keep their paths. The suites and exporters read and write them via ../rs-drive-proof-verifier/tests/vectors. - The vector READMEs name the new generator commands. Verified locally: - `cargo test -p drive-abci --test document_outcome --test identity_discovery -- --include-ignored` with DASH_SDK_EXPORT_NONUNIQUE_VECTORS=1: 7 + 14 passed, exporters included. The exporters rewrote every committed vector byte-for-byte. - `cargo test -p drive-proof-verifier --all-features`: 356 lib tests and the vectors_* suites pass without the removed features. No behaviour change: tests and dev-dependency features only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

From the self-review of this branch. Test: - The document-outcome security-floor loop also signs as TEST_PROTOCOL_VERSION_2. That is a version mock-versions builds hold a table for; drive-abci's tests enable the feature. It must still be read with the newest rules. Mutation check: let proof_query_platform_version look up 0x10000002..=0x10000003 instead of mapping them to latest. The new case fails ("version 268435458: invalid signature"). The 0x10000000 case passes that mutant. Docs and comments: - The dispatcher's 0|1 arm states its inertness for the shipped versions only (PV1-13 select 0/0, PV14 selects 1/1). It also says an unknown prover generation is refused, instead of claiming any newer producer verifies. - The single-version wrapper says what layout it expects. - put-operations.md: - only an unpinned SDK raises its stored version, and never to an unknown one (verified-version recording does keep unknown versions); - outcomes that return data are expected to fail after a layout change. - proof.rs: for the non-unique lookup, the producer's version also selects the tables that decode a two-proof answer. - The verify v3 table doc and the prove_state_transition field doc name the full owned, fee-paying set, and say where a new prover generation must be mapped. - The READMEs and the identity_discovery_tests header point to drive-abci and to the mainnet replay. - The signed_response doc explains why a dev-dependency matters (resolver 2). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai · 2026-10-23T14:06:48Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Source mode: Auto
Repository: dashpay/platform
Branch: shumkov/pr5-5541-followup
File: .coderabbit.yaml
Commit: fe8a7dd
Layers: Repository YAML
Review profile: CHILL
Plan: Advanced
Run ID: a26d0bca-e72b-43fc-b793-2d310f447c43

📥 Commits

Reviewing files that changed from the base of the PR and between 78d494f and fe8a7dd.

⛔ Files ignored due to path filters: 1

packages/rs-drive-proof-verifier/tests/vectors/document-outcome/newer-contract.bin is excluded by !**/*.bin

📒 Files selected for processing: 20

book/src/sdk/put-operations.md
book/src/versioning/versioned-dispatch.md
packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/mod.rs
packages/rs-drive-abci/tests/document_outcome.rs
packages/rs-drive-abci/tests/identity_discovery.rs
packages/rs-drive-abci/tests/identity_discovery/mainnet.rs
packages/rs-drive-abci/tests/nonunique_merged_proof.rs
packages/rs-drive-abci/tests/signed_response/mod.rs
packages/rs-drive-proof-verifier/Cargo.toml
packages/rs-drive-proof-verifier/src/proof.rs
packages/rs-drive-proof-verifier/tests/vectors/README.md
packages/rs-drive-proof-verifier/tests/vectors/document-outcome/README.md
packages/rs-drive-proof-verifier/tests/vectors/identity-discovery/README.md
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/mod.rs
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v1/mod.rs
packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v3.rs
packages/rs-platform-version/src/version/drive_versions/mod.rs
packages/rs-sdk/src/sdk/identity_discovery_tests.rs
packages/rs-sdk/src/sdk/signed_version_tests.rs

💤 Files with no reviewable changes: 2

packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v1/mod.rs
packages/rs-drive-proof-verifier/Cargo.toml

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

State-transition proof verification now selects verifier rules from the caller’s version and proof layout from the producer’s version. The change also expands protocol-version tests and updates signed fixture generation, storage, and replay references.

Changes: Proof Version Compatibility

Layer / File(s)	Summary
Verifier rules and proof layout `packages/rs-drive/src/verify/state_transition/...`, `packages/rs-platform-version/src/version/drive_versions/...`, `packages/rs-drive-proof-verifier/src/proof.rs`	Verification uses the caller’s verifier generation, while the producer’s prover generation selects whether the proof includes an owner balance. Tests cover unknown generations and confirm the producer’s verifier generation does not select the layout.
Protocol-version compatibility coverage `packages/rs-drive-abci/src/query/identity_based_queries/...`, `packages/rs-drive-abci/tests/document_outcome.rs`, `packages/rs-drive-abci/tests/identity_discovery.rs`, `packages/rs-drive-abci/tests/nonunique_merged_proof.rs`, `packages/rs-sdk/src/sdk/signed_version_tests.rs`, `book/src/sdk/put-operations.md`	Tests cover a wider range of protocol versions and stored/producer version combinations. They also check pinned and unpinned SDK version behavior and newer-than-known outcomes.
Signed fixture generation and replay `packages/rs-drive-abci/tests/...`, `packages/rs-drive-proof-verifier/Cargo.toml`, `packages/rs-drive-proof-verifier/tests/vectors/...`, `packages/rs-sdk/src/sdk/identity_discovery_tests.rs`	Fixture generation and replay references move to the proof-verifier vector directories, with drive-abci tests identified as the fixture source. The proof-verifier development dependencies and related test imports are updated.

Versioned Dispatch Documentation

Layer / File(s)	Summary
Documented in-place edits `book/src/versioning/versioned-dispatch.md`	The documentation describes when a shipped generation can be edited in place without changing behavior for protocol versions that select it.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Possibly related PRs

dashpay/platform#5515: Adds the non-unique identity lookup formats and tests that this change updates and relocates.

Suggested reviewers: quantumexplorer, lklimek

Merge Risk: ⚪ Minimal · up to fe8a7

No actionable merge-blocking issue is established; the change is mergeable after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 74.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 13 files. (5 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarizes the two main changes: moving operation-proof tests into CI and selecting the verifier generation from the verifier configuration.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 74.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 13 files. (5 skipped: 5 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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 · 2026-10-23T14:07:02Z

📖 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-10-23T20:01:52.784Z

github-actions · 2026-10-23T14:10:15Z

Waiting for bot review — coderabbitai skipped after its own rate limit · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

thepastaclaw · 2026-10-23T14:10:15Z

✅ Final review complete — no blockers (commit fe8a7dd) · triage: critical

shumkov · 2026-10-23T15:20:47Z

@coderabbitai review

coderabbitai · 2026-10-23T15:20:56Z

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

thepastaclaw

Final validation — Phase 1 + Phase 2

The verifier-dispatch refactor preserves the known protocol behavior and correctly separates caller-selected security checks from producer-selected proof layout. CI reports the relevant Rust, SDK, and book checks successful; the remaining issues are documentation and maintainability suggestions, with PR Hygiene still pending.

🟡 6 suggestion(s) | 💬 1 nitpick(s)

3 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Align the versioned-dispatch guide with the in-place refactor exception
book/src/versioning/versioned-dispatch.md:745-747

This PR edits the shipped verifier generation in place, changes its internal signature, and removes the former v1 wrapper. The contributing conventions explicitly permit output-identical in-place refactors when the proof is documented, but this guide still says shipped generations must never be modified and that old code is never deleted. These rules now contradict each other and will mislead future contributors reviewing similar refactors. Qualify this guide to reference the pure-refactor exception.

_{source: gpt-6.1-sol (phase2-reviewer: general)}

🟡 Suggestion: Use an explicit proof-layout type instead of a positional boolean
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:90-104

The shared implementation now serves both verifier generations but receives the producer layout as the positional carries_owner_balance: bool argument. The call site at the dispatcher does not reveal what true means, and a future proof-layout generation would require extending or reinterpreting the boolean mapping. A small OwnerBalanceLayout enum would make the wire-layout choice explicit and force future layout cases to be handled in the mapping.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)}

🟡 Suggestion: Derive owner-balance expectations from the named layout boundary
packages/rs-drive-abci/tests/document_outcome.rs:219

This test defines FIRST_OWNER_BALANCE_VERSION but asserts version >= 14 directly. The corresponding SDK test also uses producer >= 14. If a later prover generation introduces a different layout, these assertions will silently encode the old protocol threshold rather than the actual producer layout. Use the named boundary here and derive the SDK expectation from the producer's prove_state_transition generation.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)}

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

Triage: critical by gpt-6.1-sol (effort low) — This is a large cross-cutting change to the proof-verification dispatcher in verify_state_transition_was_executed_with_proof, directly affecting proof compatibility and cryptographic verification behavior across protocol generations.
Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — 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 `book/src/versioning/versioned-dispatch.md`:
- [SUGGESTION] book/src/versioning/versioned-dispatch.md:745-747: Align the versioned-dispatch guide with the in-place refactor exception
  This PR edits the shipped verifier generation in place, changes its internal signature, and removes the former v1 wrapper. The contributing conventions explicitly permit output-identical in-place refactors when the proof is documented, but this guide still says shipped generations must never be modified and that old code is never deleted. These rules now contradict each other and will mislead future contributors reviewing similar refactors. Qualify this guide to reference the pure-refactor exception.

In `packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/mod.rs:117-123: Remove hardcoded protocol numbers from the verifier-dispatch comment
  The comment documents the current table with the literal ranges "Protocol versions 1 to 13" and "protocol version 14". When another protocol version or prover layout is added, the implementation can remain correct while this explanation becomes stale. Describe the invariant in terms of paired verifier/prover tables and the producer's layout instead of enumerating today's protocol numbers.
- [SUGGESTION] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/mod.rs:124-130: Give the shared implementation a generation-neutral name
  The dispatcher routes both generation 0 and generation 1 to `verify_state_transition_was_executed_with_proof_v0`, while the former `v1` implementation has been deleted. Generation 1 remains selectable in the version tables, so the remaining function name and directory make it appear that generation 1 has no implementation. Rename the shared body to a generation-neutral name such as `verify_state_transition_was_executed_with_proof_shared`, or place the shared implementation in the dispatcher module while retaining the versioned dispatch arms.
- [SUGGESTION] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/mod.rs:277-286: Avoid comparing Debug strings in the producer-generation dispatch test
  The test verifies dispatch independence by comparing formatted `Debug` representations of two errors. This is brittle because unrelated changes to error formatting can break the test, while matching Debug output does not document the intended error shape. Assert the relevant error variant directly, especially that the producer's unknown verifier generation does not produce `UnknownVersionMismatch` for the verifier method.

In `packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:90-104: Use an explicit proof-layout type instead of a positional boolean
  The shared implementation now serves both verifier generations but receives the producer layout as the positional `carries_owner_balance: bool` argument. The call site at the dispatcher does not reveal what `true` means, and a future proof-layout generation would require extending or reinterpreting the boolean mapping. A small `OwnerBalanceLayout` enum would make the wire-layout choice explicit and force future layout cases to be handled in the mapping.

In `packages/rs-drive-abci/tests/document_outcome.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/document_outcome.rs:219: Derive owner-balance expectations from the named layout boundary
  This test defines `FIRST_OWNER_BALANCE_VERSION` but asserts `version >= 14` directly. The corresponding SDK test also uses `producer >= 14`. If a later prover generation introduces a different layout, these assertions will silently encode the old protocol threshold rather than the actual producer layout. Use the named boundary here and derive the SDK expectation from the producer's `prove_state_transition` generation.

In `packages/rs-drive-proof-verifier/src/proof.rs`:
- [NITPICK] packages/rs-drive-proof-verifier/src/proof.rs:404-410: Wrap the reflowed state-transition comment
  The sentence ending with "Metadata is untrusted until the signature binds it to the root" is substantially longer than the surrounding wrapped comment lines. Reflowing it would keep the new explanation readable and consistent with the rest of the block.

github-actions · 2026-10-23T19:53:59Z

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

Changes from review: - versioned-dispatch.md: the "never modify a shipped generation" bullet now points to the exception already stated under "One directory per generation" and in the coding conventions. That exception covers an edit that is unreachable at, or gives identical output for, every protocol version selecting the generation, and is documented as such. Without it, the two chapters contradicted each other. - should_not_read_the_producers_verifier_generation also asserts that the producer's unknown verifier generation does not surface as UnknownVersionMismatch, so the intent is stated directly, not only through the result comparison. - document_outcome uses FIRST_OWNER_BALANCE_VERSION in place of a literal 14. - The proof.rs comment block is rewrapped. No behaviour change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

shumkov · 2026-10-23T20:02:03Z

Re @thepastaclaw's findings that had no inline thread:

versioned-dispatch.md vs. the in-place exception. Fixed in fe8a7dd. The "never modify a shipped generation" bullet now points to the exception the same chapter already states under "One directory per generation" (output-identical or unreachable, and documented).
OwnerBalanceLayout enum instead of carries_owner_balance: bool. Not taken. The bool was already a parameter of the shared body, and the prover's shared body takes the same flag. There are only two layouts today. The mapping in proof_carries_owner_balance refuses any other prover generation with UnknownVersionMismatch, so a third layout cannot be misread silently. The enum belongs with the prover generation that brings that third layout.
Derive the owner-balance threshold from the named boundary. In document_outcome.rs, fixed in fe8a7dd: it now uses FIRST_OWNER_BALANCE_VERSION. The SDK test's producer >= 14 stays. Its vectors are fixed protocol 13 and protocol 14 answers whose layouts are frozen with those protocols, so 14 is a fact about the vectors, not a table lookup.

thepastaclaw · 2026-10-23T20:04:07Z

@shumkov on it — I've queued this as job cj-20261023-200406-d831 and will reply here or link a PR when there's something to look at.

github-actions · 2026-10-23T20:05:39Z

Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

thepastaclaw · 2026-10-23T20:05:55Z

Thanks for the clarifications. reviewsys handles validation and re-review on this PR, so it will check the fixes and the two deferred suggestions on the updated head. No code changes from me.

thepastaclaw

Re-review — Final validation — Phase 1 + Phase 2

The reviewed changes correctly separate caller-selected verifier security rules from producer-selected proof layout, preserve the verify-only client dependency boundary, and move server-dependent proof fixtures into drive-abci. I found no in-scope correctness, security, consensus, or architecture defects at this head. Rust workspace tests and Kotlin tests are queued, while the JS build is in progress; no failed code check is present.

🔴 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: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

Triage: critical by gpt-6.1-sol (effort low) — The diff substantially changes proof-verification dispatch and version-dependent compatibility on the cryptographic proof and peer-facing verification surface, including verify_state_transition_was_executed_with_proof and its version tables.
Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — 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.

github-actions · 2026-10-23T21:35:03Z

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

shumkov and others added 5 commits October 23, 2026 19:47

shumkov added this to the v5.0.0 milestone Oct 23, 2026

github-actions Bot added the waiting-bots Waiting for the review bots to report on this head label Oct 23, 2026

github-actions Bot added the bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. label Oct 23, 2026

thepastaclaw reviewed Oct 23, 2026

View reviewed changes

github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 23, 2026

github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. labels Oct 23, 2026

thepastaclaw approved these changes Oct 23, 2026

View reviewed changes

github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 23, 2026

Conversation

shumkov commented Oct 23, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

In-place changes to shipped generations

How Has This Been Tested?

Notes for reviewers

Breaking Changes

Checklist:

PR Hygiene · fe8a7dd

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shumkov commented Oct 23, 2026

Uh oh!

coderabbitai Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Final validation — Phase 1 + Phase 2

3 finding(s) not shown inline (the lines are not part of this PR's diff)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Oct 23, 2026

Uh oh!

shumkov commented Oct 23, 2026

Uh oh!

thepastaclaw commented Oct 23, 2026

Uh oh!

github-actions Bot commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thepastaclaw commented Oct 23, 2026

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Re-review — Final validation — Phase 1 + Phase 2

Uh oh!

github-actions Bot commented Oct 23, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shumkov commented Oct 23, 2026 •

edited by github-actions Bot

Loading

PR Hygiene · `fe8a7dd`

coderabbitai Bot commented Oct 23, 2026 •

edited

Loading

github-actions Bot commented Oct 23, 2026 •

edited

Loading

github-actions Bot commented Oct 23, 2026 •

edited

Loading

thepastaclaw commented Oct 23, 2026 •

edited

Loading

coderabbitai Bot commented Oct 23, 2026 •

edited

Loading

github-actions Bot commented Oct 23, 2026 •

edited

Loading