feat: add IdentityCreateFromShieldedPool state transition (shielded-pool-funded identity creation)

[QuantumExplorer]

Issue being fixed or feature implemented

Implements #3813 — a new Dash Platform state transition IdentityCreateFromShieldedPool (StateTransitionType = 20) that spends Orchard shielded-pool notes to create a brand-new Platform identity, without first unshielding to a transparent address (which would deanonymize the funding source and require a second transition).

It is a hybrid: spend mechanics from Unshield, variable-key output mechanics from IdentityCreate, and the strict merged prove/verify from ShieldFromAssetLock. Version-gated to protocol v12 (SHIELDED_POOL_INITIAL_PROTOCOL_VERSION), like the rest of the shielded family.

The exit amount is a fixed denomination (versioned set {0.1, 0.3, 0.5, 1.0} DASH) so every identity-creation exit of a given size is indistinguishable on-chain, maximizing the anonymity set (mirroring ShieldedTransfer's exact-fee uniformity).

What was done?

Implemented end-to-end across the stack (the issue's staged checklist, delivered as one mega PR):

DPP (rs-dpp) — new identity_create_from_shielded_pool_transition tree (StateTransitionType = 20, TAIL-appended; all StateTransition enum/macro/match arms). identity_id = double_sha256(sorted nullifiers) (deterministic, unique, non-malleable). New identity_create_from_shielded_extra_sighash_data helper binding identity id + denomination + the full public-key set into the Orchard sighash (anti-redirection / anti-key-swap, the surplus_output lesson). New compute_shielded_identity_create_fee predictor + storage-byte constants. Versioned denomination set in event_constants. New basic error ShieldedInvalidDenominationError (code 10827).

Action + booking + conservation (rs-drive) — new action/transformer/converter and StateTransitionAction variant. The converter emits insert_nullifiers + AddNewIdentity{balance = denomination} + AddToSystemCredits(denomination) + change-note inserts + UpdateTotalBalance(pool − denomination). Credit conservation: pool −= denomination, system credits += denomination, then the fee is moved from the identity balance into the fee pools (net 0).

drive-abci validation + execution — new validation module (stateful pool/anchor/nullifier-replay checks + key-structure validation + per-key proof-of-possession over the signable bytes + id re-derivation); shielded_proof proof-verify arm (Orchard bundle reconstruct/verify, value_balance == denomination exactly, extra-sighash binding) + new ShieldedMinFeeKind::IdentityCreate{num_keys} min-fee gate; new ExecutionEvent::PaidFromShieldedPoolToNewIdentity (create-then-deduct, funded from the pool — meters the GroveDB write, adds the flat compute fee, moves total_fee out of the new identity); validate_fees_of_event affordability gate (denomination ≥ total_fee → IdentityInsufficientBalanceError); all processor predicate/dispatch trait arms; v12 gating via is_allowed.

Strict merged prove/verify (rs-drive) — built STRICT from day one (cf. #3812): merged nullifier-tree + full_identity_query PathQuery, verified with verify_query_with_absence_proof (limit u16::MAX), partitioned by path (nullifier tree vs identity subtrees — both keys are 32 bytes). New StateTransitionProofResult::VerifiedIdentityWithShieldedNullifiers.

Client surface — dash-sdk broadcast helper; wasm-dpp2 (type-code 20 + the five grouping matches + new VerifiedIdentityWithShieldedNullifiers wrapper); wasm-dpp legacy error/factory; rs-platform-wallet operation + ShieldedFeeKind::IdentityCreate note-selection; rs-platform-wallet-ffi extern "C" fn; swift-sdk shieldedIdentityCreateFromPool bridge (built via the swift-rust-ffi-engineer agent; all business logic stays in Rust behind the FFI).

Version gating (rs-platform-version) — serialization version, validation version, converter method version, and the denomination set wired across v1–v8 (active in v8 / v12, disabled earlier).

Docs — book/ shielded-fees fee-extraction row + error-code 10827.

How Has This Been Tested?

cargo check --workspace is green; cargo check -p drive --no-default-features --features verify is green; dpp clippy clean. New unit/integration tests, all passing:

• Credit conservation (converter): AddToSystemCredits == denomination (not net), identity balance == full denomination, pool −= denomination, op-level supply delta == 0, underflow rejection.
• Structure validation: each denomination accepted, non-member rejected (ShieldedInvalidDenominationError), empty actions/keys/proof and zero anchor rejected, serialization round-trip.
• Sighash binding: layout + that identity id / denomination / the full key set are each committed (changing any changes the preimage); identity_id excluded from the platform sighash.
• Fee: compute_shielded_identity_create_fee matches base + (BASE + num_keys×PER_KEY)×rate and grows strictly with the key count.
• Strict prove/verify: empty-proof rejection through the merged verify_query_with_absence_proof arm.
• No regressions: existing shielded_proof (11) and StateTransitionType (7) suites still pass.

Remaining test follow-up (documented, not in this PR): the full-block sum-tree conservation / prove-verify roundtrip / malleability strategy-test integration with real Orchard proving — the heaviest harness-dependent piece. The conservation invariant itself is unit-tested at the converter level (the consensus-critical part). The Swift wrapper compiles syntactically and bridges the FFI correctly; a full iOS xcframework build was out of scope here.

Breaking Changes

None to an activated protocol version. This is additive and v12-gated (SHIELDED_POOL_INITIAL_PROTOCOL_VERSION); pre-v12 the transition returns StateTransitionNotActiveError. All new enum variants / error codes / version fields are TAIL-appended so existing bincode wire discriminants are unchanged.

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

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Create Platform identities by spending shielded-pool funds (deterministic identity IDs); end-to-end support in Wallet, SDK, FFI and Swift APIs

• Behavior / Fees

  • Client-side identity-create fee prediction that scales with key count; denomination membership enforced and transitions rejected if denomination < predicted fee
  • Shielded verification fee is moved out of the new identity into fee pools

• Documentation

  • Updated fee and execution docs; clarified allowed denominations

• Chores

  • Extended shielded error-code range and added versioning/validation entries for the new transition

[github-actions]

📖 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-06-09T10:44:31.282Z

[thepastaclaw]

✅ Review complete (commit 73b58d7)

[thepastaclaw]

Code Review

Convergent blocking issue across four agents: the wire identity_id field is excluded from the platform sighash, not Orchard-bound, and never validated against derive_identity_id_from_actions(&actions). Consensus creates the identity at the derived id, but proof generation (rs-drive prove) and SDK proof verification both query the trusted wire id, so a relayer can mutate that field and make an honestly-executed transition unverifiable through the SDK proof API. A single equality check in validate_structure fixes the source of the divergence. Also flagging a wallet/SDK broadcast inconsistency where the doc-comment claims the helper waits for proven execution while it only broadcasts.

🔴 1 blocking | 🟡 2 suggestion(s) | 💬 2 nitpick(s)

3 additional finding(s) omitted (not in diff).

🤖 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-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/mod.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/mod.rs:49-54: Wire `identity_id` is unauthenticated and malleable — desyncs prove/verify from the canonical state
  `identity_id` carries `#[platform_signable(exclude_from_sig_hash)]`, the Orchard `extra_sighash_data` is built from `derive_identity_id_from_actions(&v0.actions)` (not from this field), and `validate_structure` (packages/rs-dpp/.../v0/state_transition_validation.rs) never checks `v0.identity_id == derive_identity_id_from_actions(&v0.actions)`. The transformer at packages/rs-drive/src/state_transition_action/shielded/identity_create_from_shielded_pool/v0/transformer.rs:33 silently re-derives and ignores the wire field, so a relayer or block proposer can overwrite this field with any 32 bytes without invalidating the Orchard binding or any per-key PoP, and the transition still executes — creating the identity at the derived id.

  The wire field is then trusted as canonical downstream:
   - `IdentityCreateFromShieldedPoolTransitionV0::modified_data_ids()` returns `vec![self.identity_id]` (state_transition_like.rs:31) — block events and indexers see a different id than was written.
   - `prove_state_transition_v0` builds the merged path query from `st.identity_id()` (prove_state_transition/v0/mod.rs:454).
   - `verify_state_transition_was_executed_with_proof` rebuilds the same merged query from `st.identity_id()` (verify_state_transition_was_executed_with_proof/v0/mod.rs:1520).

  Result: a mutated wire id makes the prover prove absence at the wrong path; the verifier returns `IncompleteProof("... created identity is absent or incomplete in the proof")` for a successfully-executed transition. Not a fund-theft / overwrite vector (the derived id is collision-resistant and consensus uses it), but a soundness break in the SDK proof API and a state/indexer consistency footgun.

  Fix at the source: enforce equality in `validate_structure` (or at the top of `transform_into_action_v0`), rejecting the transition with a basic consensus error when `wire_identity_id != derive_identity_id_from_actions(&actions)`. That single check makes the wire id non-malleable consensus-wide and leaves the existing prove/verify/`modified_data_ids` paths correct without further changes.

In `packages/rs-platform-wallet/src/wallet/shielded/operations.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:738-743: Wallet helper claims to wait for proven execution but only broadcasts; `finalize_pending` fires on relay-ACK
  The doc-comment at lines 739-740 says the SDK helper '…waits for proven execution', but `Sdk::identity_create_from_shielded_pool` at packages/rs-sdk/src/platform/transition/identity_create_from_shielded_pool.rs:60 only calls `state_transition.broadcast(self, settings)`. The three sibling shielded spends in the same file — `unshield` (~line 410), `transfer` (~line 515), `withdraw` (~line 620) — all call `state_transition.broadcast_and_wait::<StateTransitionProofResult>(sdk, None)` so `finalize_pending` runs only after proven inclusion. As written, this helper marks notes spent on broadcast-ACK, so a Platform-level rejection after relay leaves the local store believing the spend succeeded (and the `cancel_pending` fallback never fires).

  Either switch the SDK helper to `broadcast_and_wait::<StateTransitionProofResult>` for parity with the other shielded ops, or drop the misleading 'waits for proven execution' wording.

In `packages/rs-drive-abci/src/execution/validation/state_transition/processor/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/processor/v0/mod.rs:233-239: Halo2 verification runs before per-key PoP validation — partial DoS amplifier
  For `IdentityCreateFromShieldedPool`, `validate_shielded_proof` runs the full Halo2 verification at line 234, while per-key proof-of-possession validation only happens later inside `transform_into_action`. Per-key PoP signature bytes are excluded from the platform `signable_bytes` (Signable form) and are not bound into the Orchard `extra_sighash_data` (which binds id+denomination+keys, not PoP signatures), so an attacker who sees one valid mempool transition can flip a PoP byte and resubmit. Each variant pays full Orchard verification cost before being rejected at the later PoP loop, without consuming nullifiers or charging fees.

  The nullifier-based `unique_identifiers()` does help dedupe at the mempool level, but uniqueness rejection typically happens after stateless validation has already done the expensive work. Move the new identity's key-structure + PoP validation ahead of `validate_shielded_proof` so malformed PoP signatures are rejected before paying for Halo2 verification.

[QuantumExplorer]

Pushed 08679ff addressing all three findings from the review:

• 🔴 Blocking — wire identity_id malleability: validate_structure now rejects identity_id != derive_identity_id_from_actions(&actions) (InvalidIdentifierError, mirroring IdentityCreate's outpoint check), so the wire id is authoritative consensus-wide and prove/verify/modified_data_ids can't desync. + rejection test. (thread resolved)
• 🟡 Wallet broadcast-on-relay: the SDK helper now broadcast_and_wait::<StateTransitionProofResult> for parity with the sibling shielded spends, so finalize_pending only runs on proven inclusion. (thread resolved)
• 🟡 Halo2-before-PoP DoS (summary finding): moved the cheap key-structure + per-key proof-of-possession checks into validate_shielded_proof before the Orchard bundle verification (using the same StateTransition signable bytes), so a flipped-PoP-byte resubmit is rejected without paying for Halo2; removed the now-redundant checks from transform_into_action (which no longer threads signable_bytes/execution_context).

cargo check --workspace green; 16 dpp transition tests pass (incl. the new id-mismatch rejection).

[thepastaclaw]

Code Review

Cumulative incremental review at head 08679ff vs prior 7e1734e. Prior reconciliation: (1) wire identity_id malleability — FIXED via validate_structure equality check + regression test; (2) wallet broadcast_and_wait — FIXED, SDK helper now returns StateTransitionProofResult; (3) Halo2-before-PoP DoS amplifier — FIXED, key structure + per-key PoP now run inside validate_shielded_proof before Halo2; (4) unchecked u64 note accumulation — STILL VALID, carried forward; (5) classifier-trait test fixtures — STILL VALID, carried forward. One new latest-delta blocking issue: the PoP-before-Halo2 move dropped fee accounting for the per-key signature verifications because validate_shielded_proof carries no execution context and the transformer's add_operation calls were removed, so successful Type 20 transitions underpay by one SignatureVerification cost per key.

🔴 1 blocking | 💬 2 nitpick(s)

2 additional finding(s) omitted (not in diff).

🤖 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-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs:385-408: New latest-delta: PoP-before-Halo2 move dropped per-key signature-verification fee accounting
  Moving `IdentityCreateFromShieldedPool` key-structure and per-key PoP validation into `validate_shielded_proof` correctly fixes the prior DoS-amplifier ordering, but `validate_shielded_proof` takes only `&self` and `platform_version` — no `StateTransitionExecutionContext`. The previous transformer-side loop added one `ValidationOperation::SignatureVerification(SignatureVerificationOperation::new(key.key_type()))` per key (matching `IdentityCreate` at `identity_create/identity_and_signatures/v0/mod.rs:32-34`); those `add_operation` calls were removed alongside this move and not re-added anywhere. The downstream `PaidFromShieldedPoolToNewIdentity` event consumes `execution_context.operations_consume()` (`execution_event/mod.rs:612`) and `validate_fees_of_event` folds those operations into the metered fee (`validate_fees_of_event/v0/mod.rs:308-312`), while `additional_fixed_fee_cost = compute_shielded_identity_create_fee` only covers per-key STORAGE bytes (`compute_shielded_identity_create_fee_v0` at `rs-dpp/.../compute_minimum_shielded_fee/v0/mod.rs:214-249`), not signature-verification CPU. Net effect: successful Type 20 transitions underpay by one signature-verification cost per key (consensus-critical fee divergence vs. plain `IdentityCreate`). Keep the early PoP rejection (cheap reverify is fine), but either (a) thread an execution context / `&mut Vec<ValidationOperation>` into this validation path and record one `SignatureVerification` per key here, or (b) leave a thin accounting-only PoP pass in `transform_into_action_v0` that records the operations on the success path.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds IdentityCreateFromShieldedPool across DPP, Drive/ABCI, wallet/SDK/FFI, versioning, docs, and WASM surfaces. Introduces denomination validation, identity-create fee computation, builder, transform/action flow, proof/verify plumbing, and a VerifiedIdentityWithShieldedNullifiers result.

Changes

Shielded identity create from pool

Layer / File(s)

Summary

IdentityCreateFromShieldedPool end-to-end packages/rs-dpp/..., packages/rs-drive-abci/..., packages/rs-drive/..., packages/rs-platform-wallet/..., packages/rs-sdk/..., packages/rs-platform-wallet-ffi/..., packages/swift-sdk/..., packages/rs-platform-version/..., packages/wasm-dpp*, book/*

Introduces the IdentityCreateFromShieldedPool transition: docs and error-code update (10827), DPP transition type V20 and V0 payloads, fee computation (v0), identity sighash preimage, builder that produces signed PoP keys + Orchard bundle, structure/min-fee/proof validation, transform-into-action and state validation with unshield fallback, Drive high-level ops and conservation tests, merged prove/strict verify producing VerifiedIdentityWithShieldedNullifiers, wallet note-selection/reservation and operations, SDK/FFI/Swift APIs, and platform version gating and constants.

Estimated code review effort
🎯 5 (Critical) | ⏱️ ~90+ minutes

Possibly related issues

• fix(shielded): fee and transfer calculation correctness issues dash-evo-tool#795 — Related to note-selection and fee-estimation correctness referenced by the new wallet note-selection changes.
• feat: add IdentityCreateFromShieldedPool state transition (shielded-pool-funded identity creation) #3813 — Matches implementing IdentityCreateFromShieldedPool transition and associated changes.
• Shielded: Type 15/18 (shield, shield-from-asset-lock) report success on relay-ACK, not proven execution #3704 — Related to wallet broadcast-and-wait semantics and shielded wallet flow.

Possibly related PRs

• dashpay/platform#3220 — Related shielded-pool Drive/ABCI integration and validation extensions.
• dashpay/platform#3800 — Related shielded minimum-fee consensus validation changes.
• dashpay/platform#3814 — Related strict merged Grovedb query verification changes.

Suggested labels
ready for final review

Suggested reviewers

• shumkov

Poem

🐰 I dug a tunnel through the code so deep,
Type twenty woke and hopped from shadowed sleep.
Nullifiers hum, new keys find their name,
A pool-born id hops out of flame.
Hooray — a hop, a patch, and then a leap!

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/jovial-khorana-5f2bba

[thepastaclaw]

Code Review

Latest delta 08679ff..febc057 is docs-only (one row added to book/src/fees/overview.md). No new findings in this delta. Reconciling prior findings against current head: all 3 prior findings remain STILL VALID and are carried forward. The blocking per-key signature-verification fee-accounting regression (introduced by the PoP-before-Halo2 hoist) still drives REQUEST_CHANGES; the two nitpicks remain. The earlier wire-id malleability, broadcast-and-wait, and Halo2-before-PoP findings were FIXED at 08679ff and remain fixed at febc057.

🔴 1 blocking | 💬 2 nitpick(s)

1 additional finding(s) omitted (not in diff).

🤖 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-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs:385-409: [Carried-forward STILL VALID] Hoisted per-key PoP loop does not record SignatureVerification operations (fee-accounting regression vs IdentityCreate)
  The PoP-before-Halo2 hoist at lines 385-409 correctly rejects malformed key structure and bad per-key proof-of-possession before paying for Orchard verification — good DoS hardening — but `validate_shielded_proof` takes only `&self` and `platform_version`, so the per-key `verify_signature` calls cannot record `ValidationOperation::SignatureVerification(SignatureVerificationOperation::new(key.key_type()))` into a `StateTransitionExecutionContext`. The analogous plain-identity path adds exactly this operation per key (`state_transitions/identity_create/identity_and_signatures/v0/mod.rs:32` and `state_transitions/identity_create/state/v0/mod.rs:233`); the prior transformer-side loop did the same for Type 20 and was removed alongside the move. `PaidFromShieldedPoolToNewIdentity` later folds `execution_context.operations_consume()` into the metered fee, while the fixed `compute_shielded_identity_create_fee` only prices Halo2/per-action shielded work plus per-key STORAGE bytes — not per-key signature-verification CPU. Net effect: every successful Type 20 transition underpays by one signature-verification cost per key (consensus-critical economic divergence vs IdentityCreate; also lets up to `num_keys` unmetered asymmetric verifications run on rejection paths before nullifiers burn). Fix options: (a) thread an execution context or `&mut Vec<ValidationOperation>` into `validate_shielded_proof` for this arm and record one SignatureVerification per key here, or (b) keep this loop as a cheap DoS short-circuit and re-add a thin accounting-only PoP pass to `transform_into_action_v0` on the success path so the operations land in the cost ledger.

[QuantumExplorer]

Heads-up: the latest thepastaclaw review ran against febc057f (the docs-only commit) and re-flagged the per-key SignatureVerification fee-accounting regression as still-blocking — but that was already fixed in 52bb6d7 (the very next commit), using exactly the suggested option (b): the early PoP loop in validate_shielded_proof stays a cheap DoS short-circuit, and transform_into_action_v0 now has a thin accounting-only pass that records one SignatureVerification op per key on the success path, so the per-key signature-verification CPU lands in the cost ledger (parity with IdentityCreate). See the resolved thread at #discussion_r3375549375.

Since then, 408568394d also added the identity-absence + unique-public-key-hash state checks (a P1 that would otherwise fail inside AddNewIdentity at execution). A re-review at the current head (408568394d) should clear the blocking. cargo check --workspace is green.

[github-actions]

✅ DashSDKFFI.xcframework built for this PR.

• Workflow run: https://github.com/dashpay/platform/actions/runs/27195324037
• Artifacts: DashSDKFFI.xcframework (folder), DashSDKFFI.xcframework.zip, xc_checksum.txt

SwiftPM (host the zip at a stable URL, then use):

.binaryTarget(
  name: "DashSDKFFI",
  url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
  checksum: "250d8387d5083f283ca265412eb97e60be04afda4e166020bb0ad26e4a7eb566"
)

Xcode manual integration:

• Download 'DashSDKFFI.xcframework' artifact from the run link above.
• Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
• If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (3)

packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/is_allowed.rs (1)

237-270: ⚡ Quick win

Add the new transition to the has_is_allowed_validation test matrix.

StateTransition::IdentityCreateFromShieldedPool(_) is now classified as requiring allowed-validation, but it isn’t included in transitions_requiring_allowed_validation(), leaving this behavior unguarded by regression tests.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/is_allowed.rs`
around lines 237 - 270, Add the missing
StateTransition::IdentityCreateFromShieldedPool variant to the
transitions_requiring_allowed_validation() test matrix so that
IdentityCreateFromShieldedPool is covered by the has_is_allowed_validation
tests; update the Vec returned by transitions_requiring_allowed_validation() to
include a StateTransition::IdentityCreateFromShieldedPool entry (construct the
V0 variant with its default value or call the existing factory helper, e.g.
StateTransition::IdentityCreateFromShieldedPool(IdentityCreateFromShieldedPoolTransition::V0(IdentityCreateFromShieldedPoolTransitionV0::default()))
or make_identity_create_from_shielded_pool_transition() if that helper exists).

packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs (1)

202-230: ⚡ Quick win

Add IdentityCreateFromShieldedPool to the “no pre-check” classifier test.

Line 85 updates runtime classification, but the negative test set doesn’t include this new variant yet. Add it to lock the behavior and prevent future regressions in has_identity_minimum_balance_pre_check_validation().

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs`
around lines 202 - 230, The test
should_return_false_for_transitions_without_balance_pre_check is missing the new
StateTransition variant IdentityCreateFromShieldedPool; update the transitions
Vec in that test to include a tuple like ("IdentityCreateFromShieldedPool",
StateTransition::IdentityCreateFromShieldedPool(IdentityCreateFromShieldedPoolTransition::V0(IdentityCreateFromShieldedPoolTransitionV0::default())))
so the negative classifier for
has_identity_minimum_balance_pre_check_validation() covers the new variant and
prevents regressions.

packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs (1)

726-877: ⚡ Quick win

Add direct fee-gate tests for IdentityCreateFromShieldedPool.

The new ShieldedMinFeeKind::IdentityCreate { num_keys } path is implemented, but validate_minimum_shielded_fee tests here still only exercise withdrawal cases. Please add at least: denomination < min_fee reject, boundary accept at minimum, and a key-count-scaling case.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs`
around lines 726 - 877, Add unit tests exercising validate_minimum_shielded_fee
for the new ShieldedMinFeeKind::IdentityCreate { num_keys } path: create a
StateTransition representing an IdentityCreateFromShieldedPool (the
IdentityCreate variant) and call validate_minimum_shielded_fee(platform_version)
in three tests — (1) denomination < computed_identity_min_fee should be
rejected, (2) denomination == computed_identity_min_fee should be accepted, and
(3) a multi-key case where num_keys > 1 increases the required fee so that a
denomination that would pass for one key is rejected for the scaled fee (and
accepted when increased to the scaled minimum). In each test compute the
expected minimum fee the same way validate_minimum_shielded_fee does for
ShieldedMinFeeKind::IdentityCreate { num_keys }, use PlatformVersion::latest(),
and assert result.is_valid() or matches the appropriate ConsensusError as in the
existing withdrawal tests.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs`:
- Around line 119-130: The accumulation and subtraction use unchecked u64
arithmetic and must be made safe: replace the unchecked sum of spends (currently
using spends.iter().map(...).sum() assigned to total_spent) with a checked fold
(e.g., try_fold or checked_add) that returns a ProtocolError::ShieldedBuildError
on overflow, and compute change_amount using checked_sub(denomination)
(returning the same ShieldedBuildError on underflow) instead of direct
subtraction; update the error messages to indicate overflow/underflow and
reference the same symbols (spends, note.value().inner(), total_spent,
denomination, change_amount, ProtocolError::ShieldedBuildError) so callers can
locate the fix.

In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/transform_into_action/v0/mod.rs`:
- Around line 58-103: The code builds a local drive_operations vec (created in
this block and populated by read_pool_total_balance,
validate_minimum_pool_notes, validate_anchor_exists, validate_nullifiers) but
never flushes those operations into the execution_context before any early
return or on success, so metering is lost; before each return (every place that
returns Ok(...) including the ConsensusValidationResult error returns and the
final success return) call the execution_context method that records drive
operations (e.g., add_drive_operations or extend
execution_context.drive_operations) to push the local drive_operations into
execution_context, then continue returning the same value—ensure this flush
occurs right before all early returns from this function and the final return so
reads are correctly metered.

In
`@packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/identity_create_from_shielded_pool_transition.rs`:
- Around line 33-51: Before emitting the IdentityOperation and SystemOperation,
validate that v0.identity.balance() equals v0.denomination and abort/return an
error if not; specifically, in the conversion path that creates the new identity
(where you push IdentityOperation(IdentityOperationType::AddNewIdentity {
identity: v0.identity, ... }) and
SystemOperation(SystemOperationType::AddToSystemCredits { amount:
v0.denomination }), add a guard that compares v0.identity.balance() to
v0.denomination and fails the conversion (or returns a clear error/result) when
they differ so you cannot emit inconsistent accounting ops.

In
`@packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- Around line 1520-1651: Recompute the identity id from the transition actions
(using the same derivation logic the state transition uses) and verify it equals
st.identity_id(); then, before returning VerifiedIdentityWithShieldedNullifiers,
compare the reconstructed identity's balance to st.denomination() and its public
key set to st.public_keys() (order/ID-insensitive comparison), and return an
appropriate ProofError (e.g. IncorrectProof or IncompleteProof) if any mismatch
occurs; locate this logic near the post-proof reconstruction (the loop over
proved_key_values, the identity build into dpp::prelude::Identity, and the
return of VerifiedIdentityWithShieldedNullifiers) and insert these checks there.

In `@packages/rs-platform-wallet/src/wallet/shielded/note_selection.rs`:
- Around line 216-240: select_notes_for_denomination currently only checks
denomination > predicted_fee, but must first reject denominations not in the
protocol-versioned allowed set to avoid wasting expensive prove work; before
calling select_notes (and before reserving notes) validate denomination against
the platform-version rules (e.g., call the protocol helper that returns allowed
shielded denominations or an is_valid_shielded_denomination(platform_version,
denomination) function) and return a PlatformWalletError::ShieldedBuildError if
not allowed; update select_notes_for_denomination to perform this check early
(use the function name select_notes_for_denomination to locate the code) and add
a regression test that calls the public entry (the path that leads into
build_identity_create_from_shielded_pool_transition) with a non-member
denomination to assert it fails fast without performing proof construction.

In `@packages/wasm-dpp/src/state_transition/state_transition_factory.rs`:
- Around line 86-89: In the match arm handling
StateTransition::ShieldedWithdrawal and
StateTransition::IdentityCreateFromShieldedPool inside state_transition_factory
(state_transition_factory.rs), replace the panic-inducing todo! with returning a
recoverable error: construct and return Err(JsValue) (e.g., JsValue::from_str)
containing a clear message like "shielded transitions not yet implemented" so
the wasm caller receives a JS error instead of panicking; ensure the enclosing
function's Result error type is compatible with JsValue (or convert
appropriately) so the match arm returns Err(...) rather than panicking.

---

Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs`:
- Around line 202-230: The test
should_return_false_for_transitions_without_balance_pre_check is missing the new
StateTransition variant IdentityCreateFromShieldedPool; update the transitions
Vec in that test to include a tuple like ("IdentityCreateFromShieldedPool",
StateTransition::IdentityCreateFromShieldedPool(IdentityCreateFromShieldedPoolTransition::V0(IdentityCreateFromShieldedPoolTransitionV0::default())))
so the negative classifier for
has_identity_minimum_balance_pre_check_validation() covers the new variant and
prevents regressions.

In
`@packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/is_allowed.rs`:
- Around line 237-270: Add the missing
StateTransition::IdentityCreateFromShieldedPool variant to the
transitions_requiring_allowed_validation() test matrix so that
IdentityCreateFromShieldedPool is covered by the has_is_allowed_validation
tests; update the Vec returned by transitions_requiring_allowed_validation() to
include a StateTransition::IdentityCreateFromShieldedPool entry (construct the
V0 variant with its default value or call the existing factory helper, e.g.
StateTransition::IdentityCreateFromShieldedPool(IdentityCreateFromShieldedPoolTransition::V0(IdentityCreateFromShieldedPoolTransitionV0::default()))
or make_identity_create_from_shielded_pool_transition() if that helper exists).

In
`@packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs`:
- Around line 726-877: Add unit tests exercising validate_minimum_shielded_fee
for the new ShieldedMinFeeKind::IdentityCreate { num_keys } path: create a
StateTransition representing an IdentityCreateFromShieldedPool (the
IdentityCreate variant) and call validate_minimum_shielded_fee(platform_version)
in three tests — (1) denomination < computed_identity_min_fee should be
rejected, (2) denomination == computed_identity_min_fee should be accepted, and
(3) a multi-key case where num_keys > 1 increases the required fee so that a
denomination that would pass for one key is rejected for the scaled fee (and
accepted when increased to the scaled minimum). In each test compute the
expected minimum fee the same way validate_minimum_shielded_fee does for
ShieldedMinFeeKind::IdentityCreate { num_keys }, use PlatformVersion::latest(),
and assert result.is_valid() or matches the appropriate ConsensusError as in the
existing withdrawal tests.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2e22d148-90ed-4a65-b38d-0e868b5d98bc

📥 Commits

Reviewing files that changed from the base of the PR and between 1235dd3 and 4085683.

📒 Files selected for processing (89)

• book/src/error-handling/error-codes.md
• book/src/fees/overview.md
• book/src/fees/shielded-fees.md
• packages/rs-dpp/src/errors/consensus/basic/basic_error.rs
• packages/rs-dpp/src/errors/consensus/basic/state_transition/mod.rs
• packages/rs-dpp/src/errors/consensus/basic/state_transition/shielded_invalid_denomination_error.rs
• packages/rs-dpp/src/errors/consensus/codes.rs
• packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs
• packages/rs-dpp/src/shielded/builder/mod.rs
• packages/rs-dpp/src/shielded/compute_minimum_shielded_fee/mod.rs
• packages/rs-dpp/src/shielded/compute_minimum_shielded_fee/v0/mod.rs
• packages/rs-dpp/src/shielded/mod.rs
• packages/rs-dpp/src/state_transition/mod.rs
• packages/rs-dpp/src/state_transition/proof_result.rs
• packages/rs-dpp/src/state_transition/state_transition_types.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/accessors/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/accessors/v0/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/methods/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/methods/v0/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/state_transition_estimated_fee_validation.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/state_transition_like.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/state_transition_validation.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/state_transition_like.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/state_transition_validation.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/types.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/v0_methods.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/version.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/version.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/mod.rs
• packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs
• packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs
• packages/rs-drive-abci/src/execution/types/execution_event/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/address_balances_and_nonces.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/address_witnesses.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/addresses_minimum_balance.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/basic_structure.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_based_signature.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_nonces.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/is_allowed.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/state.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/tests.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/transform_into_action/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/transform_into_action/v0/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/transformer/mod.rs
• packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs
• packages/rs-drive/src/state_transition_action/action_convert_to_operations/mod.rs
• packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/identity_create_from_shielded_pool_transition.rs
• packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/mod.rs
• packages/rs-drive/src/state_transition_action/mod.rs
• packages/rs-drive/src/state_transition_action/shielded/identity_create_from_shielded_pool/mod.rs
• packages/rs-drive/src/state_transition_action/shielded/identity_create_from_shielded_pool/transformer.rs
• packages/rs-drive/src/state_transition_action/shielded/identity_create_from_shielded_pool/v0/mod.rs
• packages/rs-drive/src/state_transition_action/shielded/identity_create_from_shielded_pool/v0/transformer.rs
• packages/rs-drive/src/state_transition_action/shielded/mod.rs
• packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
• packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/mod.rs
• packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/v1.rs
• packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/v2.rs
• packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/mod.rs
• packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v1.rs
• packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v2.rs
• packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v3.rs
• packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v4.rs
• packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v5.rs
• packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v6.rs
• packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v7.rs
• packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rs
• packages/rs-platform-version/src/version/drive_versions/drive_state_transition_method_versions/mod.rs
• packages/rs-platform-version/src/version/drive_versions/drive_state_transition_method_versions/v1.rs
• packages/rs-platform-version/src/version/drive_versions/drive_state_transition_method_versions/v2.rs
• packages/rs-platform-wallet-ffi/src/shielded_send.rs
• packages/rs-platform-wallet/src/wallet/platform_wallet.rs
• packages/rs-platform-wallet/src/wallet/shielded/note_selection.rs
• packages/rs-platform-wallet/src/wallet/shielded/operations.rs
• packages/rs-sdk/src/platform/transition.rs
• packages/rs-sdk/src/platform/transition/identity_create_from_shielded_pool.rs
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift
• packages/wasm-dpp/src/errors/consensus/consensus_error.rs
• packages/wasm-dpp/src/state_transition/state_transition_factory.rs
• packages/wasm-dpp2/src/state_transitions/base/state_transition.rs
• packages/wasm-dpp2/src/state_transitions/proof_result/convert.rs
• packages/wasm-dpp2/src/state_transitions/proof_result/shielded.rs

[thepastaclaw]

Code Review

Latest delta 52bb6d7..4085683 adds identity-absence and unique-key-hash state checks in transform_into_action_v0 (mirroring IdentityCreate::validate_state_v0), with no new defects in the change itself. All three previously-fixed prior findings remain fixed at head, and the three carried-forward STILL VALID findings have been resolved by 52bb6d7 for PoP fee accounting and checked u64 sum, and partially resolved for classifier fixtures (shielded_proof.rs now includes Type 20; is_allowed.rs and identity_based_signature.rs fixtures still omit it). Validate_minimum_shielded_fee still does not exercise the IdentityCreate { num_keys } arm, and the new state checks added in this delta ship without targeted unit tests.

🟡 4 suggestion(s)

3 additional finding(s) omitted (not in diff).

🤖 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-drive-abci/src/execution/validation/state_transition/processor/traits/is_allowed.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/is_allowed.rs:238-271: Carried-forward (narrowed): transitions_requiring_allowed_validation fixture omits IdentityCreateFromShieldedPool
  Production at lines 40 and 83 includes `IdentityCreateFromShieldedPool(_)` in both `has_is_allowed_validation` and `validate_is_allowed` (gated by `SHIELDED_POOL_INITIAL_PROTOCOL_VERSION`), but the test fixture `transitions_requiring_allowed_validation()` lists only Shield/ShieldedTransfer/Unshield/ShieldFromAssetLock/ShieldedWithdrawal among shielded variants. The shielded_proof.rs portion of the prior classifier-fixture finding was fixed at 52bb6d7e, but this fixture was not updated. A future refactor that accidentally drops Type 20 from the v12 activation gate or the `has_is_allowed_validation` arm would not be caught by `should_return_true_for_transitions_requiring_allowed_check`. Add Type 20 to the fixture alongside the other shielded variants.

In `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_based_signature.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_based_signature.rs:354-396: Carried-forward (narrowed): transitions_not_using_identity_in_state fixture omits IdentityCreateFromShieldedPool
  Production at line 182 (`uses_identity_in_state`) and line 211 (`validates_signature_based_on_identity_info`) both classify `IdentityCreateFromShieldedPool(_)` as `false`, but the test fixture `transitions_not_using_identity_in_state()` only enumerates the original five shielded variants plus the address-based and IdentityCreate transitions. A future refactor that flips Type 20 into the identity-based-signature group — which would cause the processor to expect a fetched identity for an id that does not yet exist — would not be caught. Add Type 20 to this fixture so the two `#[test]`s downstream of it cover the new variant.

In `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs:726-878: Carried-forward (narrowed): validate_minimum_shielded_fee tests do not exercise the IdentityCreate { num_keys } arm
  The `validate_minimum_shielded_fee` test module has thorough boundary tests for ShieldedWithdrawal (below-min, at-min, at-max, above-max, i64::MAX guard) but never exercises the new `ShieldedMinFeeKind::IdentityCreate { num_keys }` dispatch arm wired at lines 218 and 275. The min-fee gate is consensus-critical (the affordability gate uses `compute_shielded_identity_create_fee` and rejects when `denomination ≥ total_fee` does not hold) and scales with `num_keys`. Add at least three boundary tests parallel to the withdrawal ones: (a) denomination == compute_shielded_identity_create_fee(actions, num_keys) → rejected; (b) denomination > compute_shielded_identity_create_fee(...) → accepted; (c) min-fee strictly monotonic in `num_keys`. Without these, a refactor that swapped the wrong fee formula into the IdentityCreate arm or routed the variant to the wrong `ShieldedMinFeeKind` would slip through.

In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/transform_into_action/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/transform_into_action/v0/mod.rs:112-137: New latest-delta finding: IdentityAlreadyExists + unique-key-hash state checks ship without targeted tests
  The latest delta adds two consensus-facing early-out branches in `transform_into_action_v0`: (1) `derive_identity_id_from_actions` → `fetch_identity_balance` → `IdentityAlreadyExistsError` (lines 112-120), and (2) `validate_unique_identity_public_key_hashes_not_in_state` → returns the key-already-in-state errors (lines 122-137). These are the exact branches that convert a would-be internal Drive error during execution into a clean consensus rejection — the stated goal of the delta — but the local `tests.rs` module has no test exercising either rejection path (no hits for `IdentityAlreadyExistsError` or `validate_unique_identity_public_key_hashes_not_in_state` in the variant's test file). This does not require the heavy real-Orchard full-block harness: a targeted transformer test can seed pool balance/anchor/nullifier state, pre-create an identity whose id equals `derive_identity_id_from_actions(&actions)`, and pre-register a public-key hash, then assert the transformer returns the expected consensus errors with no action. Without coverage, the exact regression this delta fixes can come back as an internal Drive error.

[thepastaclaw]

Code Review

Latest delta ed377d5 lands the targeted transformer tests for the IdentityAlreadyExists / DuplicatedIdentityPublicKey state checks and the missing IdentityCreateFromShieldedPool entry in the transitions_not_using_identity_in_state classifier fixture. All four carried-forward findings from prior review at 4085683 are resolved. Three new low-severity, in-scope issues remain on the cumulative diff: a narrowed classifier-fixture gap in a different test in the same file, a *const vs *mut SignerHandle declaration inconsistency in the new FFI entry point, and a divergence in the new WASM proof-result wrapper that returns an IdentityWasm instance instead of a plain object and omits fromObject/fromJSON (the sibling VerifiedIdentityWithAddressInfos does both). No blockers.

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

1 additional finding(s) omitted (not in diff).

🤖 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/wasm-dpp2/src/state_transitions/proof_result/shielded.rs`:
- [SUGGESTION] packages/wasm-dpp2/src/state_transitions/proof_result/shielded.rs:441-454: `VerifiedIdentityWithShieldedNullifiersWasm.toObject` returns an `IdentityWasm` wrapper, and `fromObject`/`fromJSON` are missing
  `to_object()` places `self.identity.clone().into()` into the returned object, which gives JS consumers the exported `IdentityWasm` class instance under `.identity` — not the plain identity object. The sibling wrapper for the address-funded variant in `proof_result/address_funds.rs:77` calls `self.identity.to_object()?` so callers get a plain JS object that round-trips through `JSON.stringify`/`JSON.parse`. The same `address_funds.rs` wrapper also exposes `fromObject`/`fromJSON` constructors that this new wrapper omits, so JS callers cannot round-trip the serialized form back into the typed wrapper. Mirror the address-funded pattern for consistency.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.91%. Comparing base (81ae447) to head (9a855a1).
⚠️ Report is 3 commits behind head on v3.1-dev.

Additional details and impacted files

@@              Coverage Diff              @@
##           v3.1-dev    #3816       +/-   ##
=============================================
- Coverage     87.16%   69.91%   -17.26%     
=============================================
  Files          2627       19     -2608     
  Lines        322573     2712   -319861     
=============================================
- Hits         281185     1896   -279289     
+ Misses        41388      816    -40572     

Components	Coverage Δ
dpp	∅ <ø> (∅)
drive	∅ <ø> (∅)
drive-abci	∅ <ø> (∅)
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	∅ <ø> (∅)
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	∅ <ø> (∅)

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

[QuantumExplorer]

⚠️ Blocking consensus bug found & fixed in d879f4c — please scrutinize

A deep multi-agent review of this branch surfaced a chain-halt that every prior automated pass (incl. the green CI on earlier commits) missed:

The converter emitted AddToSystemCredits{denomination} for a pool→new-identity move. Since v12 the shielded pool balance is itself a right-hand-side term of the credit-conservation equation (calculate_total_credits_balance_v2 reads RootTree::ShieldedBalances), so this move is RHS-internal — AddNewIdentity{+denom} is already offset by the pool decrement {-denom}, exactly like Unshield (pool→address, which emits no AddToSystemCredits). AddToSystemCredits writes the LHS scalar, so it over-minted by denomination → CorruptedCreditsNotBalanced at the end-of-block sum-tree check → deterministic chain halt on the first type-20 block.

Verified three ways: the equation (TotalCreditsBalance::ok), the storage layer (AddToSystemCredits→LHS, AddNewIdentity→RHS only), and the Shield/Unshield precedent (both emit 0 AddToSystemCredits; only genuine asset-lock inflow does).

Fix: remove the op (the transition now mirrors Unshield). The two converter unit tests that asserted the mint as correct are rewritten to model the real equation (no LHS change; identity balance offset by the pool decrement) — test_does_not_touch_system_credits is the new regression guard.

Also in d879f4c: the strategy_tests build break (the failing macOS CI job), the Orchard sighash now binds read_only/contract_bounds (medium malleability fix), and the FFI/WASM review nits.

The CI failure on the prior commit was the strategy_tests E0004 (now fixed); the over-mint had no failing test because no suite drove a successful type-20 block through the end-of-block sum-tree check.

[thepastaclaw]

Code Review

Carried-forward prior findings are both FIXED at current head: wasm-dpp2 now rebuilds nullifiers through read_map_property, and the Type 20 transition is now included in the non-identity-signed negative fixture. New latest-delta findings: none. Cumulative review still leaves one in-scope test-coverage suggestion for the new consensus-critical Type 20 path because the strategy/full-block happy path remains skipped/deferred.

🟡 1 suggestion(s)

🤖 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-drive-abci/tests/strategy_tests/verify_state_transitions.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs:1457-1463: Add a full happy-path execution/prove/verify test for Type 20
  This PR adds the consensus-critical `IdentityCreateFromShieldedPool` transition, but the strategy proof-verification harness explicitly skips `IdentityCreateFromShieldedPoolAction`. The surrounding coverage is mostly isolated unit coverage plus an empty-proof verifier rejection; it does not execute a valid Orchard-backed Type 20 transition through the production block path and then prove/verify the resulting merged nullifier-plus-identity query. That leaves the riskiest composition points untested: create-then-deduct fee execution, pool balance decrement, post-fee identity balance, spent nullifiers, and byte-identical strict proof reconstruction. Add one full-block or strategy-style happy-path test that builds a valid transition, finalizes it, proves the result, and verifies `VerifiedIdentityWithShieldedNullifiers`.

[thepastaclaw]

Code Review

Incremental + cumulative review for 4e13526d17ae32729c1154b82124c04e1e6710b1 complete.

Carried-forward prior finding classification: STILL VALID / intentionally deferred. The latest delta adds a useful Drive-backed sum-tree credit-conservation regression for IdentityCreateFromShieldedPool and introduces no new findings, but it still does not add the full Orchard-backed production execution plus strict merged nullifier+identity prove/verify happy path. That prior Type 20 coverage finding was already raised on the PR, so I am not re-posting a duplicate inline thread on this commit.

CodeRabbit reported 0 inline findings for this push; no CodeRabbit reactions were needed.

🟡 1 carried-forward suggestion(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs (2)

184-191: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make the bundle/nullifier consistency check a real runtime error.

debug_assert_eq! disappears in release builds, so the one check meant to catch a drift between the precomputed identity_id and the bundle’s published nullifiers will not run in production. If this assumption ever breaks, callers burn Orchard proving time and only see a downstream rejection.

Suggested change

-    debug_assert_eq!(
-        identity_id,
-        derive_identity_id_from_actions(&sb.actions),
-        "bound identity id must match the id re-derived from the bundle's published nullifiers"
-    );
+    let derived_identity_id = derive_identity_id_from_actions(&sb.actions);
+    if identity_id != derived_identity_id {
+        return Err(ProtocolError::ShieldedBuildError(
+            "bound identity id does not match the id re-derived from the bundle's published nullifiers"
+                .to_string(),
+        ));
+    }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs`
around lines 184 - 191, The debug-only check using debug_assert_eq!(identity_id,
derive_identity_id_from_actions(&sb.actions)) must be made a runtime guard:
replace the debug_assert_eq! with an actual conditional that compares
identity_id to derive_identity_id_from_actions(&sb.actions) and returns an
appropriate Err (or early error result) when they differ, including a clear
error message; ensure the function signature propagates this error type (or map
to the existing InvalidShieldedProofError / builder error path) so callers
receive a deterministic runtime failure instead of relying on a debug-only
assertion.

---

108-147: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail fast on consensus-invalid denominations before proving.

This builder still accepts any denomination that fits in u64/i64 and is covered by the selected notes, even though Type-20 exits are protocol-versioned fixed denominations and the new identity must also remain affordable after fees. That means callers can pay the Orchard proving/signing cost here only to be rejected later by transition validation/ABCI. Please reuse the same denomination-set check used by the transition validator, and reject predicted_fee >= denomination in this builder as well so impossible transitions never reach build_spend_bundle.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs`
around lines 108 - 147, After computing fee via
compute_shielded_identity_create_fee, validate that the requested denomination
is one of the protocol's allowed Type-20 denominations (reuse the same
denomination-set logic the transition validator uses) and also reject cases
where the predicted fee is greater-or-equal to the denomination (i.e., if fee >=
denomination return ProtocolError::ShieldedBuildError with a clear message);
perform these checks (using the existing denomination variable and fee) before
proceeding to change_amount or calling build_spend_bundle so impossible
transitions are rejected early.

packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/tests.rs (1)

188-198: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert that the derived-id collision stays actionless.

This test documents a free rejection, but it only checks the error. Please also assert !result.has_data() (or the equivalent) so a regression that accidentally starts returning a fallback action cannot slip through.

Suggested assertion

     assert!(!result.is_valid(), "expected a consensus rejection");
+    assert!(
+        !result.has_data(),
+        "identity-id collisions should remain a free rejection without a carried action"
+    );
     assert_matches!(
         result.errors.as_slice(),
         [ConsensusError::StateError(

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/tests.rs`
around lines 188 - 198, Add an assertion to ensure the derived-id collision
produces no fallback action: after the existing checks on result.is_valid() and
result.errors, assert that the transition produced no state-changing data by
calling assert!(!result.has_data(), "expected no fallback action on identity
collision"); locate this near the existing uses of result in the test (the block
that checks result.is_valid() and result.errors) so regressions that add a
fallback are caught.

🧹 Nitpick comments (2)

packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/mod.rs (1)

150-185: ⚡ Quick win

Add the fallback address to the signable-bytes regression test.

send_to_address_on_creation_failure is now part of the authorization story, but this test only proves that keys and denomination affect signable_bytes(). Adding one mutation/assertion for the fallback address would lock in the non-redirectability guarantee.

Suggested test extension

         let mut other_denom = base.clone();
         other_denom.denomination = 30_000_000_000;
         assert_ne!(
             base_bytes,
             other_denom.signable_bytes().expect("signable bytes"),
             "changing the denomination must change the signable bytes"
         );
+
+        let mut other_failure_address = base.clone();
+        other_failure_address.send_to_address_on_creation_failure =
+            PlatformAddress::P2pkh([1u8; 20]);
+        assert_ne!(
+            base_bytes,
+            other_failure_address.signable_bytes().expect("signable bytes"),
+            "changing the failure redirect address must change the signable bytes"
+        );
 
         // identity_id is excluded from the sighash (it is derived from the nullifiers), so changing
         // it alone must NOT change the signable bytes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/mod.rs`
around lines 150 - 185, Extend the
test_signable_bytes_commit_to_keys_and_denomination test to also mutate the
send_to_address_on_creation_failure (fallback) address on the transition created
by make_v0() and assert that base.signable_bytes() != mutated.signable_bytes(),
ensuring that changing the fallback address changes signable_bytes(); locate the
test function and add a block similar to the other mutations (use base.clone(),
set send_to_address_on_creation_failure to a different value, then call
signable_bytes() and assert_ne) so the non-redirectability guarantee is locked
in.

packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/state.rs (1)

249-253: ⚡ Quick win

Cover the new has_state_validation() arm in the regression fixture.

IdentityCreateFromShieldedPool(_) was added to the true set here, but the should_return_true_for_transitions_with_state_validation test below never instantiates that variant. Adding it will catch future regressions in this dispatch table.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/state.rs`
around lines 249 - 253, The test
should_return_true_for_transitions_with_state_validation does not instantiate
the new StateTransition::IdentityCreateFromShieldedPool variant added to
has_state_validation; update the regression fixture used by that test to include
a case constructing StateTransition::IdentityCreateFromShieldedPool so the test
covers this new true arm. Locate the test
(should_return_true_for_transitions_with_state_validation) and the fixture that
builds transition instances, add an instance of
StateTransition::IdentityCreateFromShieldedPool to the list of transitions
asserted true, and ensure any required mock data or helper constructors used by
the fixture are provided so the new variant can be instantiated.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- Around line 368-379: The error path handling parse_optional_surplus_output for
send_to_address_on_creation_failure_bytes is returning a generic surplus_output
decode error to callers; update the failure branch so that when
parse_optional_surplus_output returns Err(...) you convert or rewrap that error
into a PlatformWalletFFIResult::err that names the parameter
send_to_address_on_creation_failure_bytes (not surplus_output). Locate the call
site using parse_optional_surplus_output and the variable
send_to_address_on_creation_failure_bytes in shielded_send.rs and replace the
Err(result) return with code that returns
PlatformWalletFFIResult::err(PlatformWalletFFIResultCode::ErrorInvalidParameter,
"<parameter_name> must be 21 PlatformAddress bytes") or otherwise include the
parameter name, or modify parse_optional_surplus_output to accept a
parameter-name argument and propagate it through to the returned error so
callers (like this one) report the correct parameter.

---

Outside diff comments:
In `@packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs`:
- Around line 184-191: The debug-only check using debug_assert_eq!(identity_id,
derive_identity_id_from_actions(&sb.actions)) must be made a runtime guard:
replace the debug_assert_eq! with an actual conditional that compares
identity_id to derive_identity_id_from_actions(&sb.actions) and returns an
appropriate Err (or early error result) when they differ, including a clear
error message; ensure the function signature propagates this error type (or map
to the existing InvalidShieldedProofError / builder error path) so callers
receive a deterministic runtime failure instead of relying on a debug-only
assertion.
- Around line 108-147: After computing fee via
compute_shielded_identity_create_fee, validate that the requested denomination
is one of the protocol's allowed Type-20 denominations (reuse the same
denomination-set logic the transition validator uses) and also reject cases
where the predicted fee is greater-or-equal to the denomination (i.e., if fee >=
denomination return ProtocolError::ShieldedBuildError with a clear message);
perform these checks (using the existing denomination variable and fee) before
proceeding to change_amount or calling build_spend_bundle so impossible
transitions are rejected early.

In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/tests.rs`:
- Around line 188-198: Add an assertion to ensure the derived-id collision
produces no fallback action: after the existing checks on result.is_valid() and
result.errors, assert that the transition produced no state-changing data by
calling assert!(!result.has_data(), "expected no fallback action on identity
collision"); locate this near the existing uses of result in the test (the block
that checks result.is_valid() and result.errors) so regressions that add a
fallback are caught.

---

Nitpick comments:
In
`@packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/mod.rs`:
- Around line 150-185: Extend the
test_signable_bytes_commit_to_keys_and_denomination test to also mutate the
send_to_address_on_creation_failure (fallback) address on the transition created
by make_v0() and assert that base.signable_bytes() != mutated.signable_bytes(),
ensuring that changing the fallback address changes signable_bytes(); locate the
test function and add a block similar to the other mutations (use base.clone(),
set send_to_address_on_creation_failure to a different value, then call
signable_bytes() and assert_ne) so the non-redirectability guarantee is locked
in.

In
`@packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/state.rs`:
- Around line 249-253: The test
should_return_true_for_transitions_with_state_validation does not instantiate
the new StateTransition::IdentityCreateFromShieldedPool variant added to
has_state_validation; update the regression fixture used by that test to include
a case constructing StateTransition::IdentityCreateFromShieldedPool so the test
covers this new true arm. Locate the test
(should_return_true_for_transitions_with_state_validation) and the fixture that
builds transition instances, add an instance of
StateTransition::IdentityCreateFromShieldedPool to the list of transitions
asserted true, and ensure any required mock data or helper constructors used by
the fixture are provided so the new variant can be instantiated.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 477a2079-184c-4581-880b-8a2ab37c0af4

📥 Commits

Reviewing files that changed from the base of the PR and between 4e13526 and 56881e8.

📒 Files selected for processing (26)

• packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs
• packages/rs-dpp/src/shielded/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/accessors/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/accessors/v0/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/methods/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/methods/v0/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/mod.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/state_transition_validation.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/v0_methods.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_based_signature.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/is_allowed.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/state.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/state/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/state/v0/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/tests.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/transform_into_action/v0/mod.rs
• packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
• packages/rs-platform-wallet-ffi/src/shielded_send.rs
• packages/rs-platform-wallet/src/wallet/platform_wallet.rs
• packages/rs-platform-wallet/src/wallet/shielded/operations.rs
• packages/rs-sdk/src/platform/transition/identity_create_from_shielded_pool.rs
• packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift

✅ Files skipped from review due to trivial changes (1)

• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/state/mod.rs

🚧 Files skipped from review as they are similar to previous changes (11)

• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/accessors/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/is_allowed.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/methods/v0/mod.rs
• packages/rs-platform-wallet/src/wallet/platform_wallet.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/v0_methods.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs
• packages/rs-dpp/src/state_transition/state_transitions/shielded/identity_create_from_shielded_pool_transition/v0/state_transition_validation.rs
• packages/rs-dpp/src/shielded/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_based_signature.rs
• packages/rs-platform-wallet/src/wallet/shielded/operations.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs

[thepastaclaw]

Code Review

BLOCKING execution bug in the new fallback path: validate_state_v0 returns the duplicate-key Unshield fallback with attached consensus errors, but the routed PaidFromShieldedPool execution event only applies its operations when consensus_errors.is_empty(); otherwise it returns UnpaidConsensusExecutionError and process_validation_result_v0 maps that to InternalError. The intended chargeable behavior (nullifiers consumed, pool debited, fallback address credited, penalty booked) never happens — the unit test for failure-path conservation bypasses execute_event_v0 by directly applying the converter ops, so this is uncaught. Carried-forward prior finding (strategy-harness Type 20 happy-path) re-validated as STILL VALID / INTENTIONALLY DEFERRED.

🔴 1 blocking | 🟡 2 suggestion(s) | 💬 2 nitpick(s)

1 additional finding(s) omitted (not in diff).

🤖 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-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/state/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/state/v0/mod.rs:112-115: Duplicate-key fallback Unshield is never executed — nullifiers not consumed, pool not debited, penalty never paid
  The fallback path returns `new_with_data_and_errors(StateTransitionAction::UnshieldAction(failure_action), unique_public_key_validation_result.errors)` so the design intent is to finalize the spend and credit the fallback address minus the penalty. But the downstream routing breaks that:

  - `UnshieldAction` is converted to `ExecutionEvent::PaidFromShieldedPool` (`execution_event/mod.rs:546`).
  - `execute_event_v0` for `PaidFromShieldedPool` (`execute_event/v0/mod.rs:548-583`) only calls `apply_drive_operations` when `consensus_errors.is_empty()`; otherwise it returns `UnpaidConsensusExecutionError(consensus_errors)` with no state changes.
  - `process_validation_result_v0` (`process_raw_state_transitions/v0/mod.rs:334`) then maps that to `StateTransitionExecutionResult::InternalError`.

  Net effect on a duplicate public-key-hash collision: nullifiers are NOT inserted, the shielded pool is NOT decremented, the fallback address is NOT credited, the penalty is NOT booked into the fee pools, and the transition is reported as an internal error instead of a chargeable failure. An attacker that can build a valid Halo 2 proof against a victim's already-registered key hash can force validators to do the (expensive) proof verification repeatedly because the nullifiers never get consumed.

  The new `failure_path_unshield_converter_ops_preserve_sum_tree_credit_conservation` unit test does not catch this because it manually calls `apply_drive_operations` on the converter ops; it never exercises the `PaidFromShieldedPool` execution arm.

  Fix: route the fallback through an execution path that applies the ops despite attached consensus errors (mirroring `Paid`/`PaidFromAddressInputs`, which support `UnsuccessfulPaidExecution` with errors), and add an end-to-end test that builds a duplicate-key Type 20 transition, runs it through `process_state_transitions`, and asserts the nullifiers are inserted, the pool is decremented by `denomination`, the fallback address is credited by `denomination − penalty`, and the fee pools gain `penalty`.
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_shielded_pool/state/v0/mod.rs:90-117: Failure-path penalty does not include the flat shielded compute fee the success path charges
  The SUCCESS path's `PaidFromShieldedPoolToNewIdentity` event adds `additional_fixed_fee_cost = compute_shielded_verification_fee` (the flat fee covering Halo 2 verification CPU GroveDB metering cannot measure). The FAILURE path's penalty is `unique_key_already_present + processing_fee`, capped at `denomination`, with no flat compute-fee component. Validators still ran the same Halo 2 verification on the failure path, so the proposer is undercompensated relative to the success path (and relative to the other shielded transitions' fee model). Not exploitable — conservation holds and the penalty cap prevents underflow — but it is a quiet asymmetry. Either document it or fold `compute_shielded_verification_fee` (or `ShieldedMinFeeKind::IdentityCreate{num_keys}`) into the penalty floor.

In `packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs`:
- [SUGGESTION] packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs:1457-1463: Carried-forward (STILL VALID, INTENTIONALLY DEFERRED): no full Orchard-backed Type 20 execute/prove/verify happy path
  Re-validated against head 56881e88. The strategy proof-verification harness still groups `IdentityCreateFromShieldedPoolAction` with the other shielded variants and skips the full build→prove→execute→prove/verify roundtrip. The latest delta added a real-Drive failure-path conservation regression but no Orchard-backed end-to-end Type 20 success-path or fallback-path coverage. The repo-wide shielded `OperationType` build handlers are still commented out in `strategy.rs`, so this is intentionally deferred to a shielded-strategy enablement effort rather than this PR. Carrying forward as a suggestion only.

[thepastaclaw]

Code Review

The only delta vs the prior review (56881e8→e636a06) is a one-line clippy fix replacing a redundant .clone() on Copy PlatformAddress. All four substantive prior findings remain valid at HEAD e636a06. The duplicate-key chargeable fallback is still routed through ExecutionEvent::PaidFromShieldedPool, whose execute_event arm explicitly returns UnpaidConsensusExecutionError when consensus_errors is non-empty — so the fallback's spend finalization (nullifier insert, pool debit, fallback-address credit, penalty booking) never runs. Note: claude-security-auditor's claim that finding #1 was FIXED is incorrect; the code at execute_event/v0/mod.rs:553-582 still gates ops on consensus_errors.is_empty().

🔴 1 blocking | 💬 1 nitpick(s)

1 additional finding(s) omitted (not in diff).

2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 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-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs:548-583: Type 20 duplicate-key fallback UnshieldAction is silently discarded — nullifiers not consumed, pool not debited, no penalty paid
  Verified at HEAD e636a069. The chargeable fallback in `identity_create_from_shielded_pool/state/v0/mod.rs:103-115` constructs a real `UnshieldTransitionAction` (full denomination spend, penalty fee, original notes/anchor) and wraps it with `new_with_data_and_errors(UnshieldAction, errors)`. `ExecutionEvent::create_from_state_transition_action` (execution_event/mod.rs:542-550) maps that into `ExecutionEvent::PaidFromShieldedPool { operations, fees_to_add_to_pool }` while preserving the errors. But the `PaidFromShieldedPool` arm at execute_event/v0/mod.rs:553-582 is:

  ```rust
  if consensus_errors.is_empty() {
      apply_drive_operations(...) ; book storage/processing fees
  } else {
      Ok(UnpaidConsensusExecutionError(consensus_errors))
  }

The fallback by construction always carries a DuplicatedIdentityPublicKeyIdStateError, so the else branch fires every time. Consequences:

1. Orchard spend nullifiers are never inserted into the recent-blocks nullifier tree — replay protection is broken on this path; the same valid Type 20 proof can be re-submitted across blocks, forcing every validator to re-run Halo 2 verification for free.
2. The shielded pool total_balance is never debited.
3. send_to_address_on_creation_failure never receives denomination - penalty.
4. The penalty/fee never reaches the fee pools.
5. process_validation_result_v0 maps UnpaidConsensusExecutionError to StateTransitionExecutionResult::InternalError(...) — a corrupted-code-style outcome, not the documented PaidConsensusError.

The docstring at state/v0/mod.rs:65-69 explicitly claims this fallback reuses PaidFromShieldedPool execution event and conservation — but the executor does not honor that for the error-bearing variant. Compare to PaidFromAssetLockToPool (execute_event/v0/mod.rs:584-627) which folds fee-validation errors into all_errors but is structurally the same trap. The model the docs reference is closer to the paid_from_identity_function machinery used by PaidFromShieldedPoolToNewIdentity / asset-lock-to-identity paths, which book the penalty even on failure.

The unit test at identity_create_from_shielded_pool/tests.rs:421-572 hand-applies the converter ops directly through Drive, bypassing execute_event entirely, which is why this regressed in CI. Fix options: (a) introduce a distinct ExecutionEvent variant (e.g. ChargeableUnshieldOnFailure) whose arm applies ops + carves the penalty regardless of consensus_errors, then route the Type 20 fallback through it from create_from_state_transition_action; or (b) split the existing PaidFromShieldedPool arm so a fallback-marked action still calls apply_drive_operations even with errors (preferably after an affordability check that fee_amount <= amount). Either way, add an end-to-end test that drives process_state_transition (not just validate_state) and asserts: PaidConsensusError (not UnpaidConsensusError); nullifiers in the nullifier tree post-block; fallback-address balance += denomination - penalty; pool balance -= denomination; sum-tree conservation holds.

</details>

_Note: the PR branch advanced to `c85f0d8a` before posting, so this assigned-SHA review for `e636a069` is posted as a top-level review body after the exact-SHA duplicate guard._

[QuantumExplorer]

Heads-up: this thepastaclaw review was generated against e636a06 (its own footnote notes the branch advanced before posting), which is the commit before the blocking-fix commit. The blocking duplicate-key-fallback execution bug is already fixed in c85f0d8 (current HEAD).

At HEAD, execute_event/v0/mod.rs PaidFromShieldedPool no longer gates the apply on consensus_errors.is_empty() — it calls apply_drive_operations unconditionally, then uses consensus_errors.is_empty() only to pick SuccessfulPaidExecution vs UnsuccessfulPaidExecution (→ PaidConsensusError, not InternalError). So the fallback now finalizes the spend (nullifiers consumed, pool debited, fallback credited, penalty booked).

The implemented fix is option (b) from the prompt (apply ops despite errors in the existing arm; penalty already capped at the amount so fee_amount <= amount). And failure_path_charge_executes_through_execute_event drives the real execute_event path and asserts UnsuccessfulPaidExecution + pool debited by the denomination — I verified it fails (UnpaidConsensusExecutionError) without the fix. The inline thread for this exact finding (discussion_r3377404843) was already replied-to and resolved against c85f0d8. A re-review at HEAD should clear the blocker.

[thepastaclaw]

Code Review

Latest push resolves both prior consensus-critical findings: the Type 20 fallback now executes through execute_event (ops applied regardless of attached errors → UnsuccessfulPaidExecution, nullifiers consumed, pool debited), and the failure-path penalty now includes compute_shielded_verification_fee for fee parity with the success path. A new regression test (failure_path_charge_executes_through_execute_event) exercises the real path. Remaining items are non-blocking: prior nitpicks #3 (UnshieldTransitionActionV0::fee_amount doc invariant is now violated by the Type 20 producer) and #4 (unreachable Some(action) branch retained behind a debug_assert), plus one new architectural concern — the unconditional apply branch in PaidFromShieldedPool relies on a comment-enforced (not type-enforced) invariant that only Type 20 ever attaches errors to that event.

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

1 additional finding(s) omitted (not in diff).

🤖 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-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs:548-598: New: PaidFromShieldedPool applies ops unconditionally — invariant is comment-enforced, not type-enforced
  The new arm removes the prior `consensus_errors.is_empty()` gate and applies drive operations regardless of attached errors. The comment correctly states that today only the IdentityCreateFromShieldedPool duplicate-key fallback constructs a `PaidFromShieldedPool` event carrying both data and errors (other shielded variants are data-only on success, error-only on rejection). However, nothing in the type system enforces that: `StateTransitionAction::UnshieldAction` is shared between ordinary Unshield and the Type 20 fallback, so a future shielded validator that mistakenly calls `new_with_data_and_errors` on an ordinary Unshield/ShieldedTransfer/ShieldedWithdrawal would silently commit a side-effectful spend on the failure path and pay the proposer for a rejected transition — a hard-to-spot consensus regression. Harden by either (a) carrying an explicit `chargeable_failure: bool` (or new `PaidFromShieldedPoolWithFallback` variant) on the event so the contract is mechanical, or (b) at this site, asserting that when `consensus_errors` is non-empty the originating action came from the Type 20 fallback (panic otherwise) so divergence surfaces in tests instead of in production state.

[thepastaclaw]

Code Review

Cumulative incremental review of PR #3816 at head 37de254. Prior findings: #1 (PaidFromShieldedPool apply-despite-errors invariant) FIXED via type-enforced chargeable_failure: bool on ExecutionEvent::PaidFromShieldedPool and UnshieldTransitionActionV0, with executor fail-safe gate; #2 (Unshield fee_amount doc) FIXED with both-producer documentation; #3 (Type 20 unreachable Some(action) branch) FIXED — now returns CorruptedCodeExecution in release; #4 (FFI hardcoded len=21) FIXED via new parse_required_platform_address helper. Latest delta introduces one new blocking regression: the new required chargeable_failure field was not propagated to five test-only struct literals in rs-drive's unshield_transition.rs, breaking cargo check -p drive --tests with five E0063 errors.

🔴 1 blocking

1 additional finding(s) omitted (not in diff).

🤖 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-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.rs:108-117: New `chargeable_failure` field breaks `cargo test -p drive` — five test constructors not updated
  The latest delta added a required `chargeable_failure: bool` field to `UnshieldTransitionActionV0` (packages/rs-drive/src/state_transition_action/shielded/unshield/v0/mod.rs:34). The struct does not derive `Default`, so every construction site must list the field explicitly. The production transformer and the two test sites in `shielded/unshield/mod.rs` were updated, but the five `UnshieldTransitionActionV0 { ... }` literals in this test module were missed: `make_action` at line 109, `test_fee_exceeds_amount_returns_error` at line 228, `test_balance_underflow_returns_error` at line 245, `test_minimum_shielded_fee_covers_actual_grovedb_write_cost` at line 299, and `test_net_zero_unshield_skips_address_credit` at line 352. `cargo check -p drive --tests` fails with five `E0063: missing field 'chargeable_failure'` errors at exactly those lines; the entire drive unit-test target — including the new consensus-critical fee/conservation regression tests this PR depends on — is unbuildable as shipped. All five are ordinary `Unshield` actions, not the Type 20 fallback, so each needs `chargeable_failure: false`. The suggestion patches `make_action`; apply the same one-line addition at 228, 245, 299, and 352.

[QuantumExplorer]

Fixed in 94f42ae: added chargeable_failure: false to the 5 ordinary-Unshield test literals in unshield_transition.rs (make_action + the 4 fee/conservation regression tests) that the new required field missed. cargo check -p drive --tests is green and the 18 drive unshield/converter tests pass.

(My earlier verification ran cargo check -p drive lib-only + clippy without --all-targets, which didn't compile that test module — my miss. The blocking finding's other four substantive items are confirmed FIXED at this head.)

[thepastaclaw]

Code Review

Cumulative incremental review. Prior blocking finding (chargeable_failure missing from 5 Unshield converter test literals) is FIXED at current head 94f42ae — the latest delta consists exclusively of those 5 single-line additions at lines 116, 236, 254, 309, and 363 of packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.rs, each correctly setting chargeable_failure: false for ordinary (non-Type-20) Unshield actions. No new findings in the latest delta; no carried-forward prior findings remain.

[thepastaclaw]

Code Review

Cumulative incremental review at head 9a855a1. Prior review at 94f42ae had no findings to reconcile (carried-forward prior findings: none). The latest delta is a focused refactor swapping bespoke storage-byte constants for the existing consensus identity_create_base_cost + identity_key_in_creation_cost constants, aligning the shielded identity-create predictor with the non-shielded predictors; the smaller floor is dwarfed by the smallest legal denomination and the authoritative metered check is unchanged. One in-scope documentation suggestion: the updated docstring overstates the fallback-on-failure coverage (the fallback applies only to unique-key-hash collisions, not to metered insufficiency).

🟡 1 suggestion(s)

🤖 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-dpp/src/shielded/compute_minimum_shielded_fee/v0/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/compute_minimum_shielded_fee/v0/mod.rs:211-214: Docstring overstates the fallback-on-failure coverage of shielded identity-create
  The new comment claims residual under-funding from a metered cost above this floor is 'absorbed by the transition's fallback-on-failure path, which credits the fallback address minus a penalty.' That fallback is only emitted on the unique-public-key-hash collision branch in `identity_create_from_shielded_pool/state/v0/mod.rs`. If `validate_fees_of_event` later finds `denomination < total_fee`, it returns `IdentityInsufficientBalanceError` and execution rejects the transition through the standard unpaid path — no `UnshieldTransitionAction` is built, no fallback address is credited, and the nullifiers are not consumed. The text should scope the fallback wording to key-hash collisions only and call out that metered insufficiency is a plain free rejection. Practically harmless because the smallest legitimate denomination (10^10 credits) far exceeds the max-key floor (~41M credits), but the predictor's docstring is read by clients and validators and shouldn't claim coverage the executor doesn't provide.

[thepastaclaw]

Code Review

Cumulative review of PR #3816 at head 04fa1b6. Prior finding (docstring overstating fallback coverage in compute_shielded_identity_create_fee_v0) is FIXED — the new docstring correctly identifies validate_fees_of_event as the affordability gate, scopes fallback to the unique-public-key-hash collision branch, and disclaims authoritative-consensus-fee status. New findings in the latest delta: one nitpick — a stale accounting comment in execute_event_v0 still describes the obsolete AddToSystemCredits/RemoveFromSystemCredits booking that the IdentityCreateFromShieldedPool converter explicitly forbids (the converter enforces no LHS scalar movement because the credits move within the RHS balance trees). No blocking or suggestion-level defects observed.

💬 1 nitpick(s)