fix(drive): charge fees for unshield and shielded withdrawal

[QuantumExplorer]

Issue being fixed or feature implemented

Unshield and shielded-withdrawal were free (fee bypass / griefing).

Both transitions hardcoded fee_amount: 0 (// TODO), so validators were paid nothing for verifying the expensive Halo2 proof + storing nullifiers, while the user received the full value back. This makes the operations free and lets an attacker force unbounded proof-verification + storage work on the network at zero cost. Value is conserved (not money-printing), but the sibling ShieldedTransfer charges correctly — these two paths just never implemented it.

UnshieldTransitionV0.unshielding_amount is documented as the total leaving the shielded pool (recipient/net amount + fee), and the builder already sets it to user_amount + compute_minimum_shielded_fee(...), intending the recipient to get user_amount and the fee to be the computed minimum. Consensus simply never charged it.

What was done?

Consensus now charges the same computed minimum fee the builder and the existing validate_minimum_shielded_fee already use:

• The transform layer (unshield/shielded_withdrawal transform_into_action_v0) computes fee_amount = dpp::shielded::compute_minimum_shielded_fee(num_actions, platform_version) — the exact value that validate_minimum_shielded_fee already enforces unshielding_amount >=, so the net (unshielding_amount - fee) is guaranteed non-negative — and passes it into the action.
• Unshield conversion: credits the output address unshielding_amount - fee (not the full amount); decrements the pool by unshielding_amount (not amount + fee). Net: pool -= amount, recipient += (amount - fee), fee pools += fee (via the existing PaidFromShieldedPool { fees_to_add_to_pool: fee_amount }) → conserved, and the pool decrement equals the proof's value_balance.
• Shielded withdrawal: the prepared withdrawal document records the net unshielding_amount - fee sent to Core; RemoveFromSystemCredits uses the net; the pool decrements by unshielding_amount. The fee stays in-platform and flows to the fee pools, so the sum trees (pool -amount, fee pools +fee) and the total_credits_in_platform counter (-(amount - fee)) both drop by exactly the net.

The misleading validate_minimum_shielded_fee comment ("fee = value_balance - amount") is corrected; the check unshielding_amount >= min_fee is unchanged (it now correctly guarantees the net amount is non-negative). All checked/guarded against underflow.

No serialization / ABI / sighash change. The fee is enforced by consensus validation rather than a proof-bound field. A user-controllable (priority) fee bound into the sighash would be a separate follow-up; today any builder fee override above the minimum returns to the user's own recipient/net rather than tipping validators (their own funds — not a security issue).

How Has This Been Tested?

• cargo test -p drive --features fixtures-and-mocks (conversions): unshield 7 / withdrawal 8 pass, including conservation tests (address +=(amount-fee), pool -=amount, net sum-tree = -fee; withdrawal RemoveFromSystemCredits = amount-fee), non-zero-fee regression guards, and fee > amount underflow guards.
• cargo test -p drive-abci --lib 'state_transitions::unshield::' → 16 pass; 'state_transitions::shielded_withdrawal::' → 18 pass (real-proof transition execution).
• cargo fmt --all --check clean.

Breaking Changes

None. Pre-release shielded-pool feature (protocol v12, behind feature gates); no deployed network depends on the prior (zero-fee) behavior, and the builder already produces unshielding_amount = user_amount + fee.

Checklist:

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

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

Summary by CodeRabbit

• Bug Fixes

  • Enforced minimum shielded fees for Shielded Transfer / Unshield / Withdrawal; recipients are credited the net (amount − fee). Withdrawals whose net is below the minimum are rejected; fee > amount now errors. Shielded-pool deltas subtract only the gross amount; system-credit deltas reflect net transfers. Net-zero unshield skips creating a zero-balance credit.

• Documentation

  • Clarified fee semantics and which transition fields are checked for minimum-fee computations.

• Tests

  • Added regression and conservation tests for fee computation, net-zero behavior, exact-floor acceptance, and withdrawal minimum enforcement.

[coderabbitai]

Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes.

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

Compute minimum shielded fees from each transition's action count, thread computed fee_amount into transformers, validate ShieldedWithdrawal net against MIN_WITHDRAWAL_AMOUNT, and apply NET accounting when converting transitions into drive operations with expanded tests.

Changes

Shielded Fee Flow Refactor

Layer / File(s)	Summary
Minimum-fee validation and tests packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs	Validation now selects the public checked quantity per variant, enforces minimum shielded fee, and for ShieldedWithdrawal enforces a minimum net withdrawal; tests and a helper constructor were added.
Validator extraction and fee precompute packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/transform_into_action/v0/mod.rs, packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/transform_into_action/v0/mod.rs	Unshield and ShieldedWithdrawal validators extract amount and action count, compute fee_amount = compute_minimum_shielded_fee(num_actions, platform_version), and pass it to transformer entry points.
Transformer signatures and forwarding packages/rs-drive/src/state_transition_action/shielded/unshield/transformer.rs, packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/transformer.rs	Transformer entry points now accept an explicit fee_amount (and platform_version for withdrawal) and forward it into nested V0 try_from_transition functions.
ShieldedWithdrawal transformer net and document update packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs, packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/mod.rs	Computes net_withdrawal_amount = unshielding_amount - fee_amount (checked), validates against MIN_WITHDRAWAL_AMOUNT, stores fee_amount in action, and populates withdrawal document AMOUNT with the net.
ShieldedWithdrawal operation accounting and tests packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_withdrawal_transition.rs	Operation converter subtracts full amount from shielded pool and removes NET (amount - fee_amount) from system credits, defensively errors on fee_amount > amount, and adds regression and conservation tests.
Unshield operation accounting and tests packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.rs	Operation converter credits recipients with NET via checked subtraction, decrements pool by amount, skips zero-net credits, and adds tests for NET behavior, conservation, and error cases.
Integration: shielded withdrawal credit conservation packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs	End-to-end test constructs a valid shielded withdrawal, seeds platform state, runs processing, and asserts total credit conservation, pool delta of -unshielding_amount, and system-credit delta of -(unshielding_amount - fee).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

ready for final review

Suggested reviewers

• thepastaclaw
• shumkov

Poem

🐰 I threaded fees from action counts today,

Net hops past the dust-floor's fray,
Pool falls whole, the system takes its share,
Tests keep watch with careful care,
A tiny hop, a balanced sway.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(drive): charge fees for unshield and shielded withdrawal' accurately and concisely describes the main objective of the changeset: implementing proper fee charging for unshield and shielded withdrawal transitions that were previously bypassed.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/fix-shielded-unshield-withdrawal-fees

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[thepastaclaw]

✅ Review complete (commit 35d9cce)

[coderabbitai]

Actionable comments posted: 3

🤖 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-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_withdrawal_transition.rs`:
- Around line 315-320: The test test_fee_amount_is_non_zero currently checks a
hardcoded fixture via make_action(), so change it to build the action through
the real transform/validation pipeline used in production (invoke the actual
transformer/validator function that produces a
ShieldedWithdrawalTransitionAction and handle/unwrap its Result), then match on
the returned ShieldedWithdrawalTransitionAction::V0 and assert that
v0.fee_amount > 0; replace the direct make_action() call with the
transform/validate call, propagate any errors with expect/unwrap so the test
fails on transform/validation errors, and keep the assertion on the computed
fee_amount.

In
`@packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.rs`:
- Around line 173-178: The test test_fee_amount_is_non_zero currently inspects a
hardcoded fixture via make_action() and so only validates the fixture; change it
to construct and obtain the UnshieldTransitionAction through the real unshield
transform/validation path (invoke the transformer/validator function(s) used in
production instead of make_action()), then match on UnshieldTransitionAction::V0
and assert that the resulting v0.fee_amount > 0 on the validated/transformed
result; reference the test function name (test_fee_amount_is_non_zero), the
fixture helper (make_action), and the real transform/validator functions you use
in the codebase to locate where to replace the fixture with the actual
transform/validation call.

In
`@packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs`:
- Around line 22-28: Replace the saturating subtraction that produces
net_withdrawal_amount with a checked subtraction and surface a validation error
if it returns None: use value.unshielding_amount.checked_sub(fee_amount) instead
of saturating_sub, and if None, return or propagate a clear validation error
(the same invariant enforced by validate_minimum_shielded_fee) rather than
writing 0 into prepared_withdrawal_document; ensure any callers of the
transformer (the code that builds prepared_withdrawal_document) handle or
propagate this validation failure so the operation-conversion path remains
consistent.

🪄 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: ed17d0c3-fd80-4e28-88e4-b1b832d57805

📥 Commits

Reviewing files that changed from the base of the PR and between 25e6c1b and ea869d8.

📒 Files selected for processing (11)

• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/transform_into_action/v0/mod.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/transform_into_action/v0/mod.rs
• packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_withdrawal_transition.rs
• packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.rs
• packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/transformer.rs
• packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/mod.rs
• packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs
• packages/rs-drive/src/state_transition_action/shielded/unshield/transformer.rs
• packages/rs-drive/src/state_transition_action/shielded/unshield/v0/mod.rs
• packages/rs-drive/src/state_transition_action/shielded/unshield/v0/transformer.rs

[codecov]

Codecov Report

❌ Patch coverage is 93.02650% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.16%. Comparing base (9eca622) to head (35d9cce).
⚠️ Report is 9 commits behind head on v3.1-dev.

Files with missing lines	Patch %	Lines
...rs-dpp/src/shielded/builder/shielded_withdrawal.rs	33.33%	10 Missing ⚠️
...tate_transition/processor/traits/shielded_proof.rs	95.03%	8 Missing ⚠️
...ransition/state_transitions/shielded_common/mod.rs	89.83%	6 Missing ⚠️
...ion/shielded/shielded_withdrawal/v0/transformer.rs	73.91%	6 Missing ⚠️
...p/src/shielded/compute_minimum_shielded_fee/mod.rs	58.33%	5 Missing ⚠️
...s/rs-dpp/src/shielded/builder/shielded_transfer.rs	70.00%	3 Missing ⚠️
packages/rs-dpp/src/shielded/builder/unshield.rs	75.00%	3 Missing ⚠️
...vert_to_operations/shielded/unshield_transition.rs	96.00%	3 Missing ⚠️
...perations/shielded/shielded_transfer_transition.rs	94.44%	2 Missing ⚠️
...rations/shielded/shielded_withdrawal_transition.rs	98.19%	2 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##           v3.1-dev    #3800      +/-   ##
============================================
- Coverage     87.18%   87.16%   -0.03%     
============================================
  Files          2624     2627       +3     
  Lines        321014   321557     +543     
============================================
+ Hits         279892   280299     +407     
- Misses        41122    41258     +136     

Components	Coverage Δ
dpp	87.68% <88.99%> (-0.05%)	⬇️
drive	86.07% <94.92%> (+0.01%)	⬆️
drive-abci	89.48% <94.44%> (-0.06%)	⬇️
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	92.17% <ø> (ø)
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	47.85% <ø> (ø)

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

[github-actions]

✅ DashSDKFFI.xcframework built for this PR.

• Workflow run: https://github.com/dashpay/platform/actions/runs/27076084773
• 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: "ee5a3eabdcfcd2e44fe0f457ab21ca848b5ee11b7d7271f1ab0b2e6864f2e4aa"
)

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.

[thepastaclaw]

Code Review

PR correctly closes the fee-bypass vulnerability by carving the minimum shielded fee out of unshielding_amount, with pool/sum-tree accounting conserved. However, it introduces a new state where the resulting withdrawal document's amount field can be below the withdrawals contract minimum (1000) — or even zero — because validate_minimum_shielded_fee only requires unshielding_amount >= min_fee and no MIN_WITHDRAWAL_AMOUNT check exists on the shielded path. Recommend rejecting shielded withdrawals whose net amount falls below the withdrawal-document minimum before the document is built.

🔴 1 blocking | 🟡 2 suggestion(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/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs`:
- [BLOCKING] packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs:22-48: Net withdrawal amount can be below MIN_WITHDRAWAL_AMOUNT (or zero) — produces an invalid withdrawal document
  Before this PR, `prepared_withdrawal_document.amount` was `unshielding_amount` (the gross value), which `validate_minimum_shielded_fee` guaranteed to be >= the minimum shielded fee. After this PR, `prepared_withdrawal_document.amount` is `unshielding_amount - fee_amount`, and the upstream validation pipeline does not enforce any floor on that net value: `validate_structure` only requires `unshielding_amount > 0` and `<= i64::MAX`; `validate_minimum_shielded_fee` only requires `unshielding_amount >= compute_minimum_shielded_fee(num_actions)`. No counterpart of address_credit_withdrawal's `MIN_WITHDRAWAL_AMOUNT` (190_000 credits) check exists for the shielded path.

  Concretely, a transition with `unshielding_amount == compute_minimum_shielded_fee(num_actions)` produces a withdrawal document with `amount = 0`; any value in `[min_fee, min_fee + MIN_WITHDRAWAL_AMOUNT)` produces a sub-minimum amount. The withdrawals contract schema requires `amount >= 1000` and downstream withdrawal processing converts this amount directly into a Core `TxOut` (which itself rejects dust). Either the contract insert fails inside consensus (CorruptedDriveState — a hard liveness/consensus issue when triggered by a validly-signed transition) or a malformed/dust withdrawal document enters the queue. Both outcomes are unacceptable.

  Reject the transition (in `transform_into_action_v0` or earlier) whenever `unshielding_amount - compute_minimum_shielded_fee(num_actions) < MIN_WITHDRAWAL_AMOUNT`, ideally with a dedicated `InsufficientShieldedWithdrawalNetAmountError`. The check should mirror `address_credit_withdrawal`'s `MIN_WITHDRAWAL_AMOUNT` enforcement so the two withdrawal paths have the same dust floor.
- [SUGGESTION] packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs:22-28: saturating_sub silently builds an amount=0 document if the fee/amount invariant is ever violated
  `net_withdrawal_amount = value.unshielding_amount.saturating_sub(fee_amount)` quietly produces `amount: 0` in the prepared withdrawal document whenever `fee_amount > unshielding_amount`. The companion check in the operations converter (`shielded_withdrawal_transition.rs`) and in the unshield converter both use `checked_sub` and return `CorruptedDriveState`, so the converter is the only authoritative guard. Two different policies for the same invariant on the same consensus-critical action is a smell: in test code, dry-run estimation, or any future caller that inspects the action before calling `into_high_level_drive_operations`, the broken invariant goes undetected and a silently-malformed document is observed instead of a loud failure.

  For consensus code the failure mode should be uniform: use `checked_sub` and return `ConsensusValidationResult::new_with_error(...)` (or surface an `Error`) at the same layer that already computes the subtraction. This becomes especially relevant once a positive net-amount floor is added (see blocking finding above) — the natural place to enforce that floor is at this same site.

In `packages/rs-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:52-65: Two divergent implementations of the shielded fee formula in different crates
  `compute_minimum_shielded_fee` (rs-dpp, used by the SDK builder and now by `transform_into_action_v0` for both unshield and shielded_withdrawal) uses unchecked `+`/`*`, while the consensus check in `validate_minimum_shielded_fee` (rs-drive-abci shielded_proof.rs:136-160) computes the same formula using `checked_mul`/`checked_add` and returns `Execution::Overflow`. Today the divergence is invisible because the validator runs first in the pipeline and rejects overflow paths — but the PR newly creates a hard dependency from consensus on the unchecked helper.

  Failure modes if the two ever drift: (a) `compute_minimum_shielded_fee` panics in debug builds for inputs the validator accepts; (b) in release, `compute_minimum_shielded_fee` wraps to a smaller value than `validate_minimum_shielded_fee` enforced, causing the carve-out to remove less credit than validated — a soft consensus / accounting bug.

  Fold both call sites onto a single helper that returns `Result<Credits, Error>` with checked arithmetic, or at minimum add a unit test that asserts the two implementations produce identical values for a range of `num_actions` on each `PlatformVersion`.

[thepastaclaw]

Code Review

Latest delta (ea869d8..b7ff23c) is a small follow-up: clarified comment in shielded_proof.rs, a defensive net-zero skip in the unshield op converter, and a new block-level credit-conservation test for shielded withdrawal. No new defects were introduced in this delta, but all three prior findings remain STILL VALID at b7ff23c — none were touched. The transformer still uses saturating_sub with no MIN_WITHDRAWAL_AMOUNT floor on the net withdrawal document, and the shielded fee formula remains duplicated across rs-dpp (unchecked) and rs-drive-abci (checked).

🟡 1 suggestion(s) | 💬 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-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:52-65: Two divergent implementations of the minimum shielded fee formula (unchecked vs checked)
  STILL VALID at b7ff23c6. `compute_minimum_shielded_fee` in rs-dpp uses unchecked `+`/`*`, while `validate_minimum_shielded_fee` in `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs` recomputes the same formula independently via `checked_add`/`checked_mul`. After this PR, the dpp helper is the one called by both `unshield::transform_into_action_v0` and `shielded_withdrawal::transform_into_action_v0` to derive the `fee_amount` carved out of `unshielding_amount`, while a separate implementation gates the validation. The entire conservation argument relies on the invariant that the two formulas always agree. Today, with bounded `num_actions` and the supported version, neither implementation overflows — but a future fee-constants change in only one place, or a wrap that produces a smaller `fee_amount` than the one validated against, will break that invariant silently (release mode wraps, debug mode panics; the validation crate errors loudly). Consolidate into a single fallible helper called from both crates, or at minimum add a unit test asserting both computations agree at the upper end of `num_actions` for `PlatformVersion::latest()`. The new conservation test in `tests.rs` now imports `compute_minimum_shielded_fee` directly, making the cross-crate consistency dependency more concrete.

[coderabbitai]

🧹 Nitpick comments (2)

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

145-177: ⚡ Quick win

Reuse the shared shielded-fee helper here.

This validator now carries its own copy of the minimum-fee formula while the transformer path and the new tests use dpp::shielded::compute_minimum_shielded_fee(...). That duplication makes the validation contract easy to drift from the fee actually charged, which is exactly the kind of mismatch that turns into consensus bugs later.

🤖 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 145 - 177, The validator duplicates the minimum shielded fee
computation (calculating storage_fee_per_action, per_action_fee, and
minimum_shielded_fee) instead of reusing the shared helper; replace this local
arithmetic with a call to dpp::shielded::compute_minimum_shielded_fee(...) using
the same inputs (platform_version / fee_version details,
SHIELDED_STORAGE_BYTES_PER_ACTION, and num_actions) and return/convert its
Result into the same Error::Execution/ExecutionError path expected here,
removing the duplicated calculations (storage_fee_per_action, per_action_fee,
minimum_shielded_fee) so the validation uses the single source of truth.

---

520-533: ⚡ Quick win

Assert the specific error variant in the below-floor test.

Right now this only checks that validation fails. It would still pass if the transition started failing for a different reason, so it does not fully lock in the new WithdrawalBelowMinAmountError behavior you just added.

🤖 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 520 - 533, Update the test
should_reject_shielded_withdrawal_with_net_below_min_withdrawal_amount to assert
the specific WithdrawalBelowMinAmountError is returned instead of only checking
is_valid(); call shielded_withdrawal_with_amount(...) and run
validate_minimum_shielded_fee(platform_version) as you do now, then assert the
ValidationResult is invalid and contains the WithdrawalBelowMinAmountError
(e.g., by matching the error variant or checking
result.errors()/result.get_errors() contains WithdrawalBelowMinAmountError).
Reference the test function name, shielded_withdrawal_with_amount,
validate_minimum_shielded_fee, and the WithdrawalBelowMinAmountError variant
when making the assertion so the test fails if a different error is produced.

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

Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs`:
- Around line 145-177: The validator duplicates the minimum shielded fee
computation (calculating storage_fee_per_action, per_action_fee, and
minimum_shielded_fee) instead of reusing the shared helper; replace this local
arithmetic with a call to dpp::shielded::compute_minimum_shielded_fee(...) using
the same inputs (platform_version / fee_version details,
SHIELDED_STORAGE_BYTES_PER_ACTION, and num_actions) and return/convert its
Result into the same Error::Execution/ExecutionError path expected here,
removing the duplicated calculations (storage_fee_per_action, per_action_fee,
minimum_shielded_fee) so the validation uses the single source of truth.
- Around line 520-533: Update the test
should_reject_shielded_withdrawal_with_net_below_min_withdrawal_amount to assert
the specific WithdrawalBelowMinAmountError is returned instead of only checking
is_valid(); call shielded_withdrawal_with_amount(...) and run
validate_minimum_shielded_fee(platform_version) as you do now, then assert the
ValidationResult is invalid and contains the WithdrawalBelowMinAmountError
(e.g., by matching the error variant or checking
result.errors()/result.get_errors() contains WithdrawalBelowMinAmountError).
Reference the test function name, shielded_withdrawal_with_amount,
validate_minimum_shielded_fee, and the WithdrawalBelowMinAmountError variant
when making the assertion so the test fails if a different error is produced.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cba1605b-5aaa-4d5c-9f7b-a65536a5ab3b

📥 Commits

Reviewing files that changed from the base of the PR and between ea869d8 and a732b10.

📒 Files selected for processing (7)

• packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/transform_into_action/v0/mod.rs
• packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_withdrawal_transition.rs
• packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.rs
• packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/transformer.rs
• packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs

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

• packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/transformer.rs
• packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/transform_into_action/v0/mod.rs
• packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs
• packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.rs
• packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_withdrawal_transition.rs

[thepastaclaw]

Code Review

Latest push at a732b10 successfully resolves the prior blocking dust-floor issue (validator + transformer both enforce net >= MIN_WITHDRAWAL_AMOUNT via checked_sub), the saturating_sub mis-use, and the OsRng→StdRng convention for the new conservation test. One prior suggestion (divergent unchecked vs checked fee-formula implementations between rs-dpp and rs-drive-abci) remains valid. One minor new doc nit: the comment introducing the fee_amount in transform_into_action understates the now-stronger invariant. No blockers; no new defects.

🟡 1 suggestion(s) | 💬 1 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-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:52-65: Duplicate, divergent implementations of the minimum shielded fee formula
  `dpp::shielded::compute_minimum_shielded_fee` computes `proof_verification_fee + num_actions × (per_action_processing_fee + SHIELDED_STORAGE_BYTES_PER_ACTION × (disk + processing))` with unchecked `+` and `*`, while `StateTransitionShieldedMinimumFeeValidationV0::validate_minimum_shielded_fee` (packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs:152-177) recomputes the same formula independently with `checked_add`/`checked_mul` and surfaces overflow as `ExecutionError::Overflow`. This PR now relies on the unchecked helper to derive the consensus `fee_amount` carved from `unshielding_amount` for both Unshield and ShieldedWithdrawal (transform_into_action/v0/mod.rs:134), and the SDK builder also uses the helper. With today's constants and the 16-action cap the two cannot diverge in practice, but two parallel arithmetic implementations of a consensus-critical fee formula is a fragile pattern: a future constants bump or action-limit change could cause the helper to silently wrap in release while the validator returns a typed overflow error, causing validators to accept a transition whose carved fee differs from the validated minimum and breaking the conservation invariant this PR establishes. Collapse to a single source of truth — either have the validator call the helper after switching the helper to checked arithmetic returning `Result<Credits, _>`, or extract a shared `checked_*` helper consumed by both call sites.

[thepastaclaw]

Code Review

Latest delta (c05b470) only raises two test-fixture unshielding_amount constants so net amounts clear the new MIN_WITHDRAWAL_AMOUNT floor. No production code changed. Both prior findings remain STILL VALID at this head and are carried forward as a suggestion (duplicated unchecked vs. checked min-fee formula) and a nitpick (under-stated invariant comment).

🟡 1 suggestion(s)

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

1 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-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:52-65: Duplicate, divergent implementations of the minimum shielded fee formula
  `dpp::shielded::compute_minimum_shielded_fee` computes `proof_verification_fee + num_actions × (per_action_processing_fee + SHIELDED_STORAGE_BYTES_PER_ACTION × (disk + processing))` with unchecked `+` and `*`. `StateTransitionShieldedMinimumFeeValidationV0::validate_minimum_shielded_fee` in `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs` recomputes the identical formula independently with `checked_add`/`checked_mul`, surfacing overflow as `ExecutionError::Overflow`. This PR now relies on the unchecked helper to derive the consensus `fee_amount` carved out of `unshielding_amount` in both Unshield and ShieldedWithdrawal transformers, *after* the separate checked formula validates the gross amount. With today's constants and the action cap the unchecked arithmetic cannot overflow, but two independent transcriptions of consensus-critical fee math is fragile: a future constant bump (e.g. `SHIELDED_STORAGE_BYTES_PER_ACTION`, fee constants) or relaxed action cap could silently make them diverge — the validator would reject with `Overflow` while the helper wraps and produces a small `fee_amount`, leading to a divergent state (recipient credited more than intended, pool/fee accounting inconsistent). Collapse to a single source of truth: either migrate the helper to `checked_*` returning `Option<Credits>`/`Result<Credits, _>` and have validation and transform code both consume it, or extract a shared checked helper. Not addressed by the latest delta.

[thepastaclaw]

Code Review

Incremental review of e7c1eb6 over c05b470. The latest delta is a comment-only change in shielded_withdrawal/transform_into_action/v0/mod.rs that strengthens the documented invariant to MIN_WITHDRAWAL_AMOUNT — directly resolving prior nitpick #2. The duplicate fee-formula concern from prior suggestion #1 remains valid at HEAD: dpp::shielded::compute_minimum_shielded_fee still uses unchecked +/* while validate_minimum_shielded_fee independently re-encodes the same consensus formula with checked arithmetic. No new defects in this delta.

🟡 1 suggestion(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-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:52-65: Duplicate, divergent implementations of the minimum shielded fee formula
  `dpp::shielded::compute_minimum_shielded_fee` computes `proof_verification_fee + num_actions × (per_action_processing_fee + SHIELDED_STORAGE_BYTES_PER_ACTION × (disk + processing))` with unchecked `+` and `*` (lines 61-64). `StateTransitionShieldedMinimumFeeValidationV0::validate_minimum_shielded_fee` in `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs` independently re-encodes the identical formula with `checked_add`/`checked_mul`, surfacing overflow as `ExecutionError::Overflow`.

  This PR now relies on the unchecked DPP helper to derive the consensus `fee_amount` carved from `unshielding_amount` in both `Unshield` (`transform_into_action/v0/mod.rs:128`) and `ShieldedWithdrawal` (`transform_into_action/v0/mod.rs:136`) transformers — after the separate checked formula has validated the gross amount. Today the action cap (`max_shielded_transition_actions = 16`) plus current u64 fee constants keep both paths far from `u64::MAX` and `validate_minimum_shielded_fee` always runs first in the pipeline, so this is not exploitable today.

  It is fragile going forward: two independent transcriptions of a consensus-critical fee formula can drift if constants, the per-action storage size, or the action cap change in a future protocol version. A drift could cause validation to error/accept on one threshold while the transformer wraps to a smaller fee, breaking the conservation invariant this PR is establishing. Collapse to one source of truth — for example, migrate the helper to checked arithmetic returning `Result<Credits, _>` and have `validate_minimum_shielded_fee` and both transformers consume it.

[thepastaclaw]

Code Review

Latest delta (e7c1eb6..7db77b4) is documentation-only — it expands the SHIELDED_STORAGE_BYTES_PER_ACTION doc-comment in packages/rs-dpp/src/shielded/mod.rs to describe Orchard's 216-byte TransmittedNoteCiphertext layout. No executable code or fee math changed in this push. Carried-forward: the prior architecture finding on duplicated unchecked vs. checked minimum shielded fee formulas remains STILL VALID at HEAD (all six agents converged on it). Not exploitable today given the action cap (16) and current u64 fee constants, and the checked validator always runs before the transformer in the pipeline, so this is a suggestion, not a blocker.

🟡 1 suggestion(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-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:74-87: Carried-forward: duplicate, divergent implementations of the minimum shielded fee formula
  STILL VALID at HEAD 7db77b4 — the latest push only expanded the `SHIELDED_STORAGE_BYTES_PER_ACTION` doc-comment; the helper body is unchanged. `dpp::shielded::compute_minimum_shielded_fee` (packages/rs-dpp/src/shielded/mod.rs:74-87) computes `proof_verification_fee + num_actions × (per_action_processing_fee + SHIELDED_STORAGE_BYTES_PER_ACTION × (disk + processing))` with unchecked `+` and `*`. `StateTransitionShieldedMinimumFeeValidationV0::validate_minimum_shielded_fee` in `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs:152-177` independently re-encodes the identical consensus formula with `checked_add`/`checked_mul`, surfacing overflow as `ExecutionError::Overflow`.

  This PR's `Unshield` and `ShieldedWithdrawal` transformers now derive the consensus `fee_amount` carved from `unshielding_amount` via the unchecked DPP helper *after* the checked validator has accepted the gross amount. Today this is safe: the system action cap (`max_shielded_transition_actions = 16`) plus current u64 fee constants keep both paths far from `u64::MAX`, and the checked validator always runs first in the pipeline.

  The risk is forward-looking. Two independent transcriptions of a consensus-critical fee formula in different crates are fragile: a future protocol version that changes fee constants, the per-action storage size, or the action cap could cause the two implementations to diverge — letting validation accept (or reject) at a different threshold than what the transformer wraps (release mode) or panics on (debug), breaking the fee/conservation invariant this PR establishes. Collapse to one source of truth: migrate `compute_minimum_shielded_fee` to checked arithmetic returning `Result<Credits, _>` (or extract the validator's checked sequence into a shared helper in `dpp::shielded`) and have `validate_minimum_shielded_fee` and both transformers consume it.

[thepastaclaw]

Code Review

Latest delta (7db77b4..f8ec408) correctly pins ShieldedTransfer value_balance to exact minimum fee at consensus, properly gated by requires_exact_fee and placed after the underflow-safe lower-bound check. New latest-delta issue: this consensus tightening creates a mismatch with the DPP ShieldedTransfer builder, which still accepts fee overrides up to 1000x min_fee — callers using the override produce transitions the network now deterministically rejects. Carried-forward: the duplicate unchecked-vs-checked transcriptions of the minimum shielded fee formula remain STILL VALID; the new exact-equality rule makes drift between the two implementations consensus-critical for transfers.

🔴 1 blocking | 🟡 1 suggestion(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-dpp/src/shielded/builder/shielded_transfer.rs`:
- [BLOCKING] packages/rs-dpp/src/shielded/builder/shielded_transfer.rs:33-70: ShieldedTransfer builder accepts fees consensus now rejects
  The latest delta makes ShieldedTransfer validation require `value_balance == minimum_shielded_fee` exactly (shielded_proof.rs:209-225, rejected with `ShieldedInvalidValueBalanceError`/10822). The DPP builder, however, still documents the `fee` override as `>= the minimum fee` (line 33-34) and accepts any value in `[min_fee, min_fee * 1000]` (lines 56-71), then proves/signs the bundle and stores that `effective_fee` as the transition's `value_balance`. Any caller passing `Some(f)` with `min_fee < f <= min_fee * 1000` therefore gets `Ok(StateTransition)` from `try_from_bundle`, but `check_tx` deterministically rejects the result. This is a public API/consensus mismatch introduced by this PR.

  Fix: reject `Some(f) > min_fee` in the builder for ShieldedTransfer (or remove the override entirely for transfers and always use `min_fee`), update the doc comment on `fee` from `>=` to `must equal`, and adjust `test_shielded_transfer_fee_above_upper_bound` accordingly. Unshield/ShieldedWithdrawal builders should keep their current `>=` semantics since their excess is the recipient/net amount, matching consensus.

In `packages/rs-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:74-87: STILL VALID — duplicate, divergent transcriptions of the minimum shielded fee formula
  Carried forward at HEAD f8ec408e. `dpp::shielded::compute_minimum_shielded_fee` (lines 83-86) still computes the consensus formula with unchecked `+`/`*`, while `StateTransitionShieldedMinimumFeeValidationV0::validate_minimum_shielded_fee` (shielded_proof.rs:161-185) independently re-encodes the identical formula with `checked_add`/`checked_mul`. The Unshield and ShieldedWithdrawal transformers derive the consensus `fee_amount` carved from `unshielding_amount` via the unchecked DPP helper after the checked validator accepts the gross amount.

  Today this is safe: action cap (`max_shielded_transition_actions = 16`) and current u64 fee constants keep both formulas far from overflow, and the checked validator always runs first. The risk is forward-looking and is reinforced by this delta: the new exact-equality rule rejects any ShieldedTransfer whose `value_balance != minimum_shielded_fee`. SDK builders use the unchecked DPP helper to set `value_balance`; the validator enforces the checked formula. A future protocol version that changes fee constants, per-action storage size, or the action cap could cause the unchecked helper to wrap silently (release) or panic (debug) at a value the checked formula correctly errors on — and with the new equality rule, the smallest drift makes every shielded transfer unspendable.

  Collapse to one source of truth: migrate `compute_minimum_shielded_fee` to checked arithmetic returning `Result<Credits, _>` (or extract the validator's checked sequence into a shared helper in `dpp::shielded`), and have `validate_minimum_shielded_fee`, the unshield/withdrawal transformers, and the SDK builders all consume it.

[thepastaclaw]

Code Review

Latest delta (e04dc7d) resolves the prior ShieldedTransfer builder/consensus mismatch by pinning the transfer builder to require fee == min_fee exactly. Re-validation surfaces one new blocking issue (carried by codex-rust-quality cumulative pass): unshield and shielded-withdrawal builders/wallet compute fees for spends.len().max(1) actions, but Orchard's BundleType::DEFAULT pads to MIN_ACTIONS=2, so a single-spend unshield/withdrawal emits a 2-action bundle. Combined with this PR's switch to charging compute_minimum_shielded_fee(actions.len()) in the transformer, this silently shortchanges the recipient by one per-action fee for the common single-note case. The prior duplicate-formula suggestion is also still valid and carried forward.

🔴 1 blocking | 🟡 1 suggestion(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-dpp/src/shielded/builder/unshield.rs`:
- [BLOCKING] packages/rs-dpp/src/shielded/builder/unshield.rs:57-59: Unshield/withdrawal builders undercount Orchard actions for the single-spend case
  The builder computes the fee against `spends.len().max(1)` actions, but the bundle is built with `BundleType::DEFAULT` (Orchard's `Transactional { bundle_required: false }`). That bundle type pads to `max(num_spends, num_outputs, MIN_ACTIONS)` and `MIN_ACTIONS = 2` (orchard `src/builder.rs:37`). With exactly 1 spend and 1 change output, `BundleType::DEFAULT.num_actions(1, 1) = 2`, so the serialized bundle carries 2 actions while the builder only carved `compute_minimum_shielded_fee(1)` out of `total_spent`.

  This PR makes the consequence consensus-visible. The unshield transformer at `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/transform_into_action/v0/mod.rs:128` now charges `compute_minimum_shielded_fee(num_actions, platform_version)` where `num_actions` comes from `v0.actions.len()` (the padded value). The validator at `shielded_proof.rs:153-185` already accepted `unshielding_amount >= min_fee(2)`, so the transformer charges `min_fee(2)` and the recipient receives `unshielding_amount - min_fee(2) = amount + min_fee(1) - min_fee(2) = amount - per_action_fee` instead of the requested `amount`.

  The sibling `shielded_withdrawal` builder at `packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs:64-65` has the same off-by-one, and the wallet reserves the same undersized fee for both paths at `packages/rs-platform-wallet/src/wallet/shielded/operations.rs:287` and `:475` via `reserve_unspent_notes(..., 1)`. The transfer builder already handles this correctly with `spends.len().max(2)` because it has two outputs (recipient + change).

  Fix: have the unshield and shielded-withdrawal builders (and the wallet's `select_notes_with_fee` callers that pass `min_actions = 1`) use a minimum of 2 actions when emitting `BundleType::DEFAULT`, or derive the count directly from `BundleType::DEFAULT.num_actions(num_spends, num_outputs)` so the SDK fee always matches the on-chain `actions.len()`. Add a regression test that builds a single-spend unshield/withdrawal end-to-end and verifies `value_balance == compute_minimum_shielded_fee(emitted_actions.len(), version)`.

In `packages/rs-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:74-87: Carried forward: minimum shielded fee formula is duplicated with divergent arithmetic discipline
  STILL VALID at e04dc7d8. `dpp::shielded::compute_minimum_shielded_fee` (mod.rs:83-86) computes the consensus fee with unchecked `+`/`*`, while `StateTransitionShieldedMinimumFeeValidationV0::validate_minimum_shielded_fee` in `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs:160-185` independently re-encodes the identical formula using `checked_add`/`checked_mul`. The unshield and shielded-withdrawal transformers (`unshield/transform_into_action/v0/mod.rs:128`, `shielded_withdrawal/transform_into_action/v0/mod.rs:136`) now carve the consensus `fee_amount` out of `unshielding_amount` via the unchecked DPP helper after the checked validator has already accepted the gross amount, and the new ShieldedTransfer exact-equality rule (`shielded_proof.rs:209-225`) means any future drift between the two transcriptions is immediately consensus-affecting: validators would compute X, the builder would compute X', and every transfer in flight would fail with `ShieldedInvalidValueBalanceError`.

  Today's constants make the two implementations agree numerically for all reachable inputs, so this is a maintenance/hardening hazard rather than an active exploit. Collapse to one source of truth: migrate `compute_minimum_shielded_fee` to checked arithmetic returning `Result<Credits, ProtocolError>` (or expose the validator's checked sequence as a shared helper in `dpp::shielded`), and have validation, both transformers, and the SDK builders consume the same implementation.

[thepastaclaw]

Code Review

Latest delta (c3519de) only adds a Drive-side test asserting compute_minimum_shielded_fee covers the actual GroveDB write cost — no new code paths or new defects introduced. Both prior findings remain STILL VALID at the new head: the unshield and shielded_withdrawal builders still undercount Orchard actions (spends.len().max(1)) for the single-spend case while build_spend_bundle uses BundleType::DEFAULT (MIN_ACTIONS=2), and the consensus minimum-fee formula remains duplicated between an unchecked dpp helper and a checked validator. The sibling shielded_transfer builder was already fixed to .max(2), confirming the intended fix was simply not propagated.

🔴 2 blocking | 🟡 1 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-dpp/src/shielded/builder/unshield.rs`:
- [BLOCKING] packages/rs-dpp/src/shielded/builder/unshield.rs:57-59: Unshield builder undercounts Orchard actions for single-spend bundles (DEFAULT MIN_ACTIONS=2)
  `build_unshield_transition` computes `num_actions = spends.len().max(1)` and feeds that to `compute_minimum_shielded_fee`, but the shared `build_spend_bundle` (packages/rs-dpp/src/shielded/builder/mod.rs:185) constructs the bundle with `BundleType::DEFAULT`, which Orchard pads to `max(num_spends, num_outputs, MIN_ACTIONS=2)`. A one-spend / one-change-output unshield bundle is therefore signed, proven and serialized with 2 actions while the transition's `unshielding_amount` only includes the 1-action fee. Consensus validation in `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs` recomputes `min_fee` from the on-wire `v0.actions.len() = 2` and rejects the transition with `InsufficientShieldedFeeError`. The sibling `shielded_transfer.rs:55` was already corrected to `spends.len().max(2)` (with an explanatory comment), confirming the intended fix; it was simply not propagated to unshield. Net effect: builder-produced single-spend unshield transitions are guaranteed to be rejected by consensus.

In `packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs`:
- [BLOCKING] packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs:63-65: ShieldedWithdrawal builder undercounts Orchard actions for single-spend bundles
  Same root cause as unshield: `num_actions = spends.len().max(1)` while `build_spend_bundle` constructs `BundleType::DEFAULT` (MIN_ACTIONS=2). For a single-spend shielded withdrawal the bundle is built with 2 actions but the transition's `unshielding_amount` only covers the 1-action fee. The validator in `shielded_proof.rs:144-148` not only requires `unshielding_amount >= min_fee(actions.len())` but also requires the net (`unshielding_amount − min_fee`) to clear `MIN_WITHDRAWAL_AMOUNT`. Both checks use the validator-recomputed `min_fee(2)`, so single-spend builder withdrawals fail with `InsufficientShieldedFeeError` and — if `withdrawal_amount` is bumped just enough — would still risk `WithdrawalBelowMinAmountError` because the dust floor is checked against the higher fee. Propagate the same fix already applied to `shielded_transfer.rs:55`.

In `packages/rs-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:74-87: Minimum shielded fee formula duplicated with divergent arithmetic discipline (unchecked vs checked)
  `dpp::shielded::compute_minimum_shielded_fee` (mod.rs:83-86) computes `storage_fee`, `per_action`, and the total minimum fee using unchecked `+` and `*`. The validator `StateTransitionShieldedMinimumFeeValidationV0::validate_minimum_shielded_fee` in `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs:161-185` independently transcribes the same formula with `checked_add`/`checked_mul`, returning `Error::Execution(Overflow)` on overflow. Both copies are consensus-critical: transformers/builders consume the unchecked helper for the `fee_amount` actually carved out at execution, while validation uses the checked copy. At current v12 constants there is no overflow, but a future fee-constant or action-limit bump could push one path to wrap silently and the other to reject — a consensus-divergence trap. Collapse to one checked source of truth: change `compute_minimum_shielded_fee` to return `Result<Credits, ProtocolError>` (or extract a shared checked helper in `dpp::shielded`) and have validation, transformers, and builders all consume it.

[thepastaclaw]

Code Review

Latest delta c3519de..055dd7b only adds an #[ignore]d benchmark for Halo 2 proof verification scaling — no consensus or production logic changed. All three prior findings remain STILL VALID at head 055dd7b: both unshield and shielded-withdrawal builders still compute num_actions = spends.len().max(1) while the shared build_spend_bundle uses BundleType::DEFAULT (Orchard MIN_ACTIONS=2), so single-spend builder transitions are signed/proven with 2 actions but only carry a 1-action fee and are deterministically rejected by consensus with InsufficientShieldedFeeError; and compute_minimum_shielded_fee is still duplicated as unchecked arithmetic in DPP vs. checked arithmetic in the validator. Carrying all three forward; no new latest-delta findings.

🔴 2 blocking | 🟡 1 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-dpp/src/shielded/builder/unshield.rs`:
- [BLOCKING] packages/rs-dpp/src/shielded/builder/unshield.rs:57-59: Unshield builder undercounts Orchard actions for single-spend bundles
  Verified unchanged at head 055dd7b0: `build_unshield_transition` computes `num_actions = spends.len().max(1)` and feeds that to `compute_minimum_shielded_fee`, while the shared `build_spend_bundle` (packages/rs-dpp/src/shielded/builder/mod.rs:185) constructs the Orchard bundle with `BundleType::DEFAULT`, which pads to `max(num_spends, num_outputs, MIN_ACTIONS=2)`. A one-spend / one-change-output unshield bundle is therefore signed, proven, and serialized with 2 actions while the transition's `unshielding_amount` only carves out the 1-action fee. Consensus validation in `shielded_proof.rs::validate_minimum_shielded_fee` recomputes `min_fee` from on-wire `v0.actions.len() = 2` and rejects the transition with `InsufficientShieldedFeeError`. The sibling `shielded_transfer.rs:55` was already corrected to `spends.len().max(2)`, confirming the intended fix has not been propagated here. Impact: every single-spend unshield constructed via the canonical builder is deterministically rejected — users cannot exit the shielded pool through this path. A regression test driving a single-spend builder transition end-to-end through validation is needed.

In `packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs`:
- [BLOCKING] packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs:63-65: ShieldedWithdrawal builder undercounts Orchard actions for single-spend bundles
  Verified unchanged at head 055dd7b0: identical root cause as the unshield builder — `num_actions = spends.len().max(1)` while `build_spend_bundle` constructs `BundleType::DEFAULT` (MIN_ACTIONS=2). For a single-spend shielded withdrawal the on-wire bundle has 2 actions but `unshielding_amount` only covers the 1-action fee. The validator in `shielded_proof.rs` enforces both `unshielding_amount >= min_fee(actions.len())` and the net (`unshielding_amount - min_fee`) clearing `MIN_WITHDRAWAL_AMOUNT`; both checks use the validator-recomputed `min_fee(2)`, so single-spend builder withdrawals fail with `InsufficientShieldedFeeError` and may additionally fail the dust-floor net check. Propagate the same fix as the unshield builder and add a regression test for a single-spend shielded-withdrawal builder transition passing validation.

In `packages/rs-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:74-87: Minimum shielded fee formula duplicated with divergent arithmetic discipline (unchecked vs checked)
  Verified unchanged at head 055dd7b0: `dpp::shielded::compute_minimum_shielded_fee` computes `storage_fee`, `per_action`, and the total minimum fee using unchecked `+` and `*` on `u64`. The validator `StateTransitionShieldedMinimumFeeValidationV0::validate_minimum_shielded_fee` in `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs` independently transcribes the same formula using `checked_add`/`checked_mul` and surfaces overflow errors. Both copies are consensus-critical: transformers/builders consume the unchecked helper for the `fee_amount` actually carved out at execution, while validation uses the checked copy. At current constants there is no overflow, but a future fee-constant or action-cap change could make one path wrap silently while the other rejects — a classic divergence that breaks consensus. Collapse to a single checked source of truth: either have `compute_minimum_shielded_fee` return `Result<Credits, ProtocolError>` and have validation/transformers/builders consume it, or extract a shared `checked` helper in `dpp::shielded`.

[thepastaclaw]

Code Review

Three prior blocking/suggestion findings from review at 055dd7b are all STILL VALID at head 4652b5b: the unshield and shielded_withdrawal builders still compute num_actions = spends.len().max(1) while the shared build_spend_bundle uses BundleType::DEFAULT (which pads to MIN_ACTIONS=2), so single-spend builder transitions underfund the consensus minimum fee. The latest delta (per-action processing fee 3M → 22M) widens the under-funded gap from ~11.5M to ~30.5M credits, magnifying the rejection/loss surface. New in this delta: the published shielded-fee documentation (book/src/fees/shielded-fees.md) was not updated alongside the constant bump and now misstates per-action and total fee values.

🔴 2 blocking | 🟡 2 suggestion(s)

4 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/shielded/builder/unshield.rs`:
- [BLOCKING] packages/rs-dpp/src/shielded/builder/unshield.rs:57-59: Unshield builder undercounts Orchard actions for single-spend bundles (carried forward — STILL VALID)
  `build_unshield_transition` computes `num_actions = spends.len().max(1)` and feeds that to `compute_minimum_shielded_fee`, but the shared `build_spend_bundle` (packages/rs-dpp/src/shielded/builder/mod.rs:185) constructs the Orchard bundle with `BundleType::DEFAULT`, which pads to MIN_ACTIONS=2. For a single-spend unshield, the on-wire `actions.len() == 2` while the builder funds only the 1-action minimum. Consensus validation (`shielded_proof.rs`) recomputes `min_fee` from `v0.actions.len()` and rejects with `InsufficientShieldedFeeError`, or — if the user-amount cushion absorbs the gap — the action-aware transformer silently carves out the larger min_fee from the recipient's unshielding amount. With the new per-action fee at 22M (this push), the silent under-credit grew from ~3M to ~22M credits per missing action.

  The asymmetry is local: the sibling `shielded_transfer` builder already uses `.max(2)` for exactly this reason. Mirror that here and add a regression test that drives a single-spend unshield builder transition all the way through `validate_minimum_shielded_fee`.

In `packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs`:
- [BLOCKING] packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs:63-65: ShieldedWithdrawal builder undercounts Orchard actions for single-spend bundles (carried forward — STILL VALID)
  Identical root cause as the unshield builder: `num_actions = spends.len().max(1)` while `build_spend_bundle` uses `BundleType::DEFAULT` (MIN_ACTIONS=2). For a single-spend shielded withdrawal the on-wire bundle has 2 actions but `unshielding_amount` is sized for the 1-action fee. The validator recomputes `min_fee` from `v0.actions.len()` and additionally enforces `(unshielding_amount - min_fee) >= MIN_WITHDRAWAL_AMOUNT` (`WithdrawalBelowMinAmountError`, newly added in this PR), so single-spend builder withdrawals can fail on either the insufficient-fee branch or the dust-floor branch. With the new 22M per-action fee, the shortfall is larger and pushes more single-spend cases over the dust cliff. Mirror the `.max(2)` fix and add a regression test for a single-spend withdrawal through full validation.

In `packages/rs-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:74-87: Minimum shielded fee formula duplicated with divergent arithmetic discipline (unchecked vs checked) — STILL VALID
  `dpp::shielded::compute_minimum_shielded_fee` computes `storage_fee`, `per_action`, and the total minimum fee with unchecked `+` and `*` on `u64`. The validator in `rs-drive-abci/.../traits/shielded_proof.rs` independently transcribes the same formula with `checked_add`/`checked_mul`, returning `Error::Execution(Overflow)` on overflow. Both paths are consensus-critical: builders and the new action-aware transformers (`unshield/transform_into_action/v0/mod.rs`, `shielded_withdrawal/transform_into_action/v0/mod.rs`) consume the unchecked helper for `fee_amount`, while validation uses the checked copy to gate acceptance.

  Current constants do not overflow, but this PR demonstrates exactly the kind of change that can drift the two formulas — `shielded_per_action_processing_fee` jumped 7× (3M → 22M). A future bump to per-action/storage/proof constants or to the action cap could wrap silently in `compute_minimum_shielded_fee` while the validator rejects with `Overflow`, producing divergent behavior across consensus-critical call sites. Collapse to one checked source of truth: change `compute_minimum_shielded_fee` to return `Result<Credits, ProtocolError>` (or extract a shared checked helper) and have validation, transformers, and builders all consume it.

In `book/src/fees/shielded-fees.md`:
- [SUGGESTION] book/src/fees/shielded-fees.md:79-163: Shielded fee documentation is stale after the per-action fee bump
  This push raises `shielded_per_action_processing_fee` from 3,000,000 to 22,000,000 in DRIVE_ABCI_VALIDATION_VERSIONS_V8 and updates every hardcoded test fixture to match — but the published fee documentation in `book/src/fees/shielded-fees.md` still cites the old values:

  - L79: `**Current value:** \`3,000,000\` credits (3M)`
  - L111-L113 fee table: 123,097,600 / 134,646,400 / 146,195,200 for 2/3/4 actions
  - L162 struct comment: `// 3_000_000`
  - L76-L77 calibration comment: `33:1 ratio against the proof verification cost` (no longer accurate with 22M per-action vs the new comment in v8.rs)

  With the new value (22M per action; per-action total 30,548,800) the actual minima are 161,097,600 / 191,646,400 / 222,195,200. Integrators reading this page to pre-fund transitions will undershoot by tens of millions of credits per action and hit `InsufficientShieldedFeeError` — exactly the consensus regression this PR is calibrating. The PR description's documentation checkbox is left unchecked; updating the page (and the calibration justification) keeps the public spec aligned with consensus.

[thepastaclaw]

Code Review

Latest delta (4652b5b..667f4ce) cleanly routes the carved shielded fee into storage_fee/processing_fee via applied_fees.storage_fee.min(fees_to_add_to_pool) with conservation preserved; no new defect in that delta. All four prior findings (two single-spend builder action-count blockers, the duplicated unchecked vs checked minimum-fee math, and the stale shielded-fees book) are STILL VALID at 667f4ce. One new cumulative blocker: ShieldedWithdrawal structure validation does not validate output_script, pooling, or core_fee_per_byte, while the transparent withdrawal counterpart does and the transformer writes them straight into a withdrawals document.

🔴 3 blocking | 🟡 2 suggestion(s)

5 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/shielded_withdrawal_transition/v0/state_transition_validation.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/state_transitions/shielded/shielded_withdrawal_transition/v0/state_transition_validation.rs:12-71: ShieldedWithdrawal accepts unvalidated output_script, pooling, and core_fee_per_byte
  `ShieldedWithdrawalTransitionV0::validate_structure` validates action count, encrypted-note sizes, unshielding amount, proof, and anchor, but never validates `output_script`, `pooling`, or `core_fee_per_byte`. The transformer at `packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs:71-73` writes these fields directly into a withdrawals system-contract document (`OUTPUT_SCRIPT`, `POOLING`, `CORE_FEE_PER_BYTE`).

  The transparent counterpart `IdentityCreditWithdrawalTransitionV1` (`packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_credit_withdrawal/structure/v1/mod.rs:45-69`) requires `pooling == Never`, `core_fee_per_byte` to be a non-zero Fibonacci number, and `output_script` to be P2PKH or P2SH. Shielded withdrawals bypass all three checks.

  Consequences: (1) Attacker can submit a shielded withdrawal whose extra_sighash_data binds an oversized or arbitrarily-shaped `output_script` (e.g., raw `OP_RETURN`, multi-KB script). Validators persist that script as part of the queued withdrawal document and Core ultimately receives an invalid/unspendable output. (2) `pooling != Never` and unsupported `core_fee_per_byte` values silently pass into the withdrawals queue. (3) Because the minimum shielded fee is computed only from `actions.len()`, an attacker pays the fixed fee while forcing variable, attacker-controlled storage cost — undermining the storage-fee accounting this PR is fixing.

  Mirror the transparent-withdrawal structure checks here before the transition is accepted.

In `packages/rs-dpp/src/shielded/builder/unshield.rs`:
- [BLOCKING] packages/rs-dpp/src/shielded/builder/unshield.rs:57-59: Unshield builder undercounts Orchard actions for single-spend bundles
  Carried forward — STILL VALID at 667f4cec. Line 58 still computes `num_actions = spends.len().max(1)`, but `build_spend_bundle` (rs-dpp/src/shielded/builder/mod.rs:185) constructs the bundle with `BundleType::DEFAULT`, whose `MIN_ACTIONS = 2` floor pads a single-spend / one-output bundle to two actions. The serialized `actions.len()` is therefore 2 while `unshielding_amount = unshield_amount + compute_minimum_shielded_fee(1, …)`. The consensus validator (`StateTransitionShieldedMinimumFeeValidationV0::validate_minimum_shielded_fee`) recomputes `min_fee` from `actions.len() = 2` and rejects with `InsufficientShieldedFeeError`. The sibling `shielded_transfer.rs:55` already uses `spends.len().max(2)`, confirming the intended fix was simply not propagated. With the per-action fee now 22M, single-spend default-fee unshield transitions built via this path are deterministically rejected.

In `packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs`:
- [BLOCKING] packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs:63-65: ShieldedWithdrawal builder undercounts Orchard actions for single-spend bundles
  Carried forward — STILL VALID at 667f4cec. Line 64 still uses `spends.len().max(1)`. Identical root cause to the unshield builder: `build_spend_bundle` constructs `BundleType::DEFAULT` (MIN_ACTIONS=2 floor), so a single-spend withdrawal is serialized with 2 actions while `unshielding_amount` only covers the 1-action fee. The validator at `shielded_proof.rs` enforces both `unshielding_amount >= min_fee(actions.len())` and `(unshielding_amount - min_fee) >= MIN_WITHDRAWAL_AMOUNT`, so single-spend builder withdrawals fail with `InsufficientShieldedFeeError` and, depending on `withdrawal_amount`, also `WithdrawalBelowMinAmountError`. Mirror the `shielded_transfer.rs:55` fix.

In `packages/rs-dpp/src/shielded/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/mod.rs:74-87: Minimum shielded fee formula duplicated with divergent arithmetic discipline
  Carried forward — STILL VALID at 667f4cec. `dpp::shielded::compute_minimum_shielded_fee` (lines 83-86) computes `proof_verification_fee + num_actions × (per_action_processing_fee + SHIELDED_STORAGE_BYTES_PER_ACTION × (disk + processing))` with unchecked `+` and `*`. The validator (`StateTransitionShieldedMinimumFeeValidationV0::validate_minimum_shielded_fee` in `rs-drive-abci/.../shielded_proof.rs`) independently transcribes the identical consensus formula with `checked_add`/`checked_mul`, returning `ExecutionError::Overflow`. Both Unshield and ShieldedWithdrawal `transform_into_action_v0` consume the unchecked helper to derive the `fee_amount` carved from `unshielding_amount` after validation. The latest delta makes the helper's output even more load-bearing: it now drives the storage/processing split in `execute_event/v0/mod.rs`. At current constants there is no overflow risk, but the per-action fee just moved 3M → 22M in this PR — the kind of change two transcriptions can absorb unequally. Collapse to one source of truth (migrate the helper to checked arithmetic returning `Result<Credits, ProtocolError>` and have validation, transformers, and builders consume it).

In `book/src/fees/shielded-fees.md`:
- [SUGGESTION] book/src/fees/shielded-fees.md:79-163: Shielded fee documentation is stale after per-action fee bump (3M → 22M)
  Carried forward — STILL VALID at 667f4cec. `DRIVE_ABCI_VALIDATION_VERSIONS_V8` sets `shielded_per_action_processing_fee = 22_000_000` (verified at `packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rs:327`), but the book still cites the pre-bump constants:
  - L79: `**Current value:** \`3,000,000\` credits (3M)`
  - L111-113 fee table: 123,097,600 / 134,646,400 / 146,195,200 for 2/3/4 actions (post-bump totals are 161,097,600 / 191,646,400 / 222,195,200)
  - L162: struct comment `// 3_000_000`

  Integrators sizing pre-funding from this page will undershoot by tens of millions of credits per action and hit `InsufficientShieldedFeeError` — the exact failure mode this PR's consensus changes close. Update the value at L79, the fee table at L109-113, the struct comment near L162, and the 33:1 calibration ratio comment at L76-77.

[thepastaclaw]

Code Review

Incremental re-review at 1d06025. All four prior blocking findings from 3695838 are FIXED: ShieldedWithdrawal sighash now binds output_script || unshielding_amount || core_fee_per_byte || pooling via a shared helper used by both builder and verifier (with a dedicated mutation regression test), and the wallet unshield/withdraw paths now reserve against the 2-action floor and use Pooling::Never. No new in-scope blocking or suggestion-level findings identified in the latest delta by any agent.

[thepastaclaw]

Code Review

All six agent reviews report no findings on the latest delta (1d06025..a6582cc), which is documentation-only — book/src/fees/shielded-fees.md updates to correct unshield/withdrawal fee and pool-decrement semantics. Prior blocking issues from earlier SHAs were already fixed and remain fixed at HEAD. No CodeRabbit inline findings to validate.

[QuantumExplorer]

There is no point in having a fee here now.

[QuantumExplorer]

Fixed in e558bc1. Removed the fee parameter from all three value_balance builders (transfer/unshield/withdrawal) — the fee was already pinned to exactly compute_minimum_shielded_fee (consensus charges the minimum and ignores any surplus), so there was nothing for the caller to choose. The builder computes it internally now; the fee-validation tests that existed only to police the parameter were removed.

[QuantumExplorer]

This and all shield transitions using this pattern should return the fee used.

[QuantumExplorer]

Fixed in e558bc1. All three builders now return (StateTransition, Credits), so the caller gets the fee that was actually applied (the platform-wallet callers now destructure it). The shield / shield_from_asset_lock builders intentionally keep no fee param — they are funded from the transparent / asset-lock side, not from value_balance, so "this pattern" does not apply to them.

[thepastaclaw]

Code Review

No prior findings were carried forward from cf926df. The latest delta at 07de324 is test-only, and the cumulative shielded fee changes are version-dispatched, consistently use compute_minimum_shielded_fee, and preserve the intended fee/accounting invariants; no in-scope blocking issues or suggestions remain.

[thepastaclaw]

Code Review

All six agent reviews converged on no in-scope findings at b2beded. The delta is test-only (tightening error-variant assertions for withdrawal range checks and adding coverage for the fee<0 overflow guard) plus a doc comment clarification. CodeRabbit reported 0 inline findings.

[thepastaclaw]

Code Review

All six agent reports converge on no in-scope findings for the delta b2beded..e558bc1. The change is a clean builder-API simplification: drops the inert fee: Option<Credits> parameter from the three shielded builders, returns (StateTransition, Credits), and updates the sole wallet caller. No consensus, sighash, or accounting paths are touched. Approving.

[thepastaclaw]

Code Review

All six agent reports converge on zero findings for the delta e558bc1..c11eef5, which is a pure mechanical refactor moving compute_minimum_shielded_fee into the standard mod.rs + v0 versioned-dispatch pattern. The v0 body is byte-identical to the prior inlined implementation, the public path dpp::shielded::compute_minimum_shielded_fee is preserved via pub use, and both DPP_METHOD_VERSIONS_V1 and _V2 are updated to set the new FeatureVersion field to 0 (covering all protocol versions v1–v12). No consensus, sighash, accounting, or fee-formula behavior changes; the cumulative shielded fee fix remains intact.

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[thepastaclaw]

Code Review

All six agent reviews converge on no in-scope findings for the delta c11eef5..cbd0cd4. The commit is a mechanical signature cleanup that drops already-unused block_info/execution_context parameters from the shielded transform_into_action chain; block_info is correctly retained for ShieldedWithdrawal where it feeds creation_time_ms. No consensus, fee, accounting, or version-dispatch behavior changes.

[thepastaclaw]

Code Review

Latest delta cbd0cd4..a909cf1 is a pure rename/comment refactor in validate_minimum_shielded_fee with no behavior change. No carried-forward prior findings from cbd0cd4. Cumulatively the PR correctly closes the zero-fee bypass on Unshield/ShieldedWithdrawal, routes fees via PaidFromShieldedPool, and preserves pool conservation; the only verified concrete finding is a minor builder/consensus i64::MAX boundary mismatch in the shielded builders.

💬 1 nitpick(s)

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

[QuantumExplorer]

Reviewed

[thepastaclaw]

Code Review

All six reviewer passes (3 Claude, 3 Codex) converged on a single carried-forward nitpick about the shielded builders capping the net amount rather than the on-wire gross. The PR's latest delta (35d9cce) cleanly versions min_withdrawal_amount via SystemLimits (v1 preserves the 190k floor; v2 raises to 1M for PLATFORM_V12) and routes all four withdrawal validation paths through the versioned field. No new in-scope defects.

💬 1 nitpick(s)

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