Repository navigation
feat(platform)!: token shielded pools - #4760
Conversation
|
Important Review skippedToo many files! This PR contains 550 files, which is 450 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (550)
You can disable this status message by setting the
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. Comment |
|
⛔ Final review complete — 1 blocking finding(s) (commit cb7386b) · triage: critical |
a58dc2c to
13f4c55
Compare
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-02T15:37:45.550Z |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.3-dev #4760 +/- ##
===========================================
Coverage ? 73.36%
===========================================
Files ? 2981
Lines ? 442963
Branches ? 0
===========================================
Hits ? 324978
Misses ? 117985
Partials ? 0
🚀 New features to boost your workflow:
|
QuantumExplorer
left a comment
There was a problem hiding this comment.
Reviewed af95a95d5b8fbbae020a8c854907351c9935f6ec against 62c299cc043e36ec6fc8f746634cc67a68f0cdd1. Found two issues: a P1 proposal-availability failure in paid-error anchor bookkeeping and a P2 omission from token-pool proof fee admission.
All 230 changed files were accounted for. Findings are validated by source tracing; no local runtime reproductions were run. Rust CI passed, including the shielded tests, but the new transition tests do not exercise CheckTx or full block-end processing.
| if matches!( | ||
| execution_result, | ||
| StateTransitionExecutionResult::SuccessfulExecution { .. } | ||
| | StateTransitionExecutionResult::PaidConsensusError { .. } | ||
| ) { | ||
| processing_result | ||
| .add_token_shielded_pools_touched(token_shielded_pools_touched); |
There was a problem hiding this comment.
[P1] Exclude nonexistent pools from anchor bookkeeping
A correctly signed TokenShield with a valid output-only Orchard proof can pass CheckTx for an existing token whose hasShieldedPool flag is false: CheckTx skips batch state validation, and output-only proof verification does not require a registered pool. Block execution returns the expected paid TokenShieldedPoolNotEnabledError, but this branch still adds the raw token ID to the touched set.
At block-end, record_token_shielded_pool_anchors tries to read that nonexistent pool's commitment tree. The missing-path error propagates through run_block_proposal and prepare_proposal, aborting proposal creation before transaction results and fee/nonce changes can commit. The admitted transaction can therefore disrupt subsequent proposal rounds.
Please collect pools that were actually written, or exclude nonexistent pools before anchor recording. Extend test_token_shield_rejected_when_pool_not_enabled through CheckTx and full proposal/block-end processing; its current helper stops after processing transitions, so it misses this failure.
There was a problem hiding this comment.
Fixed, and now pinned by a test.
process_raw_state_transitions v0 registers the touched pools only for StateTransitionExecutionResult::SuccessfulExecution, so a paid rejection — a token without the flag, or any paid failure before the pool is written — never reaches the block-end anchor recorder.
On the test: you were right that the helper stopped too early to catch this, but it does return the StateTransitionsProcessingResult, so the touched-pool set is assertable directly without driving a full proposal. test_token_shield_rejected_when_pool_not_enabled now asserts that set is empty, mirroring the contains(&token) assertion the success path already had. Verified against the unfixed code: dropping the SuccessfulExecution guard makes it report the pool id it must not have registered, ✖ before / ✔ after.
| let verification_fee = compute_shielded_verification_fee(actions.len(), platform_version)?; | ||
| execution_context.add_operation(ValidationOperation::PrecalculatedOperation(FeeResult { | ||
| processing_fee: verification_fee, | ||
| ..Default::default() | ||
| })); |
There was a problem hiding this comment.
[P2] Include proof compute fees in CheckTx admission
This is the only insertion of the token-pool compute fee, but CheckTx skips batch state validation entirely (validates_full_state_on_check_tx() defaults to false). It therefore never reaches this helper, including the non-Validator return below. Batch execution events also set additional_fixed_fee_cost: None, and the preliminary batch minimum has no Orchard action component.
Consequently, the authoritative affordability check omits the 40,000,000-credit bundle fee plus 22,000,000 credits per action before CheckTx performs Orchard verification. An identity funded for the incomplete estimate can trigger proof work and, with a valid proof, enter the mempool despite lacking enough credits for execution. Block validation then adds this fee and rejects the event as unpaid after verification runs again. Existing nonce windows, weighted permits and proof caching constrain the resource impact but do not correct admission.
Please include exactly one compute fee per bundle in CheckTx's execution-event estimate, keeping block accounting consistent. Add underfunded-credit CheckTx cases for Shield, Unshield and ShieldedTransfer that reject before proof verification; the current low-balance test checks token balance instead.
There was a problem hiding this comment.
Fixed, though not at this line — the fee moved rather than being added here.
The compute fee is no longer charged in token_shielded_pool_common. All seven token pool action transformers in rs-drive now add compute_shielded_verification_fee(actions.len()) to the action's fee_result (token_shield, token_unshield, token_shielded_transfer, token_mint_to_pool, token_burn_from_pool, token_claim_to_pool, token_direct_purchase_to_pool). Action construction runs inside transform_into_action, which CheckTx does execute, so admission and block execution now price the bundle once and identically. verify_token_pool_bundle returns early for non-Validator modes because the fee is already in the estimate by then.
On the test: added test_token_shield_rejected_by_check_tx_when_credits_cannot_cover_the_compute_fee. Your note that "the preliminary batch minimum has no Orchard action component" turned out to matter for writing it — an identity funded at 1 credit is rejected by that minimum (required_balance 100000) and never reaches the real estimate, which made the first version of the test tautological. It now funds the identity at exactly one compute fee, past that minimum, and asserts the IdentityInsufficientBalanceError's required_balance covers the compute fee. Verified against the unfixed code: removing the fee from the shield transformer makes CheckTx admit the transition outright — the exact failure you described — ✖ before / ✔ after.
I did not add the Unshield and ShieldedTransfer variants; they share the identical code path through their action transformers, so the Shield case pins the property. Happy to add them if you want each spelled out.
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Phase 1 blocker gate
The token shielded pool implementation has three blocking correctness issues. Block-end anchor bookkeeping can turn a paid validation failure for a nonexistent or disabled pool into a proposal-aborting storage error, CheckTx does not account for the token-pool Orchard verification fee, and protocol-v13 nodes can accept newly encoded token transitions instead of rejecting them unpaid, causing version-divergent processing.
Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.
🔴 3 blocking | 🟡 3 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-gate-verifier, role: verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This is a large, intricate diff that changes consensus validation and execution, cryptographic shielded-proof and signature handling, token funds movement, and persistent Drive storage/migration paths, including functions such as token_shield_operations, token_unshield_operations, token_shielded_transfer_operations, proof verification, and pool initialization. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— ffi-engineer (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 11% left, 5h 100% left),glm-5.3-flash(zai below 15% reserve: 5h 0% left, weekly 53% left) - Fresh verifier:
gpt-6-astra— verifier; agentastra-gate-verifier - Phase 2 reviewers: not run (deferred by blocker gate)
🤖 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/process_raw_state_transitions/v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/process_raw_state_transitions/v0/mod.rs:286-297: Paid-invalid token transitions can abort block processing while recording a nonexistent pool
The touched-pool set is populated from the transition's token ID before execution and is retained for both successful execution and paid consensus errors. A `TokenShield`, `Unshield`, or `ShieldedTransfer` for a token whose pool does not exist, including a token with `hasShieldedPool` disabled, can therefore add a nonexistent pool to the set after returning a paid validation error. At block end, `record_token_shielded_pool_anchors` attempts to read the commitment tree under `[Tokens, TOKEN_SHIELDED_POOLS_KEY, token_id]`; because the pool path is absent, the missing-path error propagates instead of being treated as an ordinary rejected transaction. This can make `run_block_proposal`/`prepare_proposal` fail before results, fees, and nonce changes are committed. The PR's CheckTx behavior allows this path to reach block processing. Record anchors only for pools actually created or mutated, filter the set against pool existence, or make the anchor recorder safely ignore missing pools.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_shielded_pool_common/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_shielded_pool_common/mod.rs:256-293: CheckTx admission omits token-pool Orchard verification fees
`verify_token_pool_bundle` adds the token-pool compute fee (the fixed bundle cost plus the per-action cost) only during block-path state validation. Batch transitions skip full state validation in CheckTx, while their execution-event estimate leaves `additional_fixed_fee_cost` unset and the preliminary batch estimate contains no Orchard verification component. CheckTx can therefore admit a valid token shielded batch whose identity can pay the incomplete estimate but cannot pay the fee charged during block execution. The transaction still causes proof verification work during admission and is later rejected as underfunded during block validation. Include exactly one token-pool compute fee per bundle in the shared CheckTx/execution-event estimate, and add underfunded admission coverage for all three token transition types.
In `packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v10.rs`:
- [BLOCKING] packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v10.rs:230-235: Token shield transitions are not gated off below protocol version 14
The new `TokenTransition::{Shield, Unshield, ShieldedTransfer}` variants are appended correctly for serialization, but the nested validation and conversion version slots remain `0` in both the v13 and v14 platform-version tables. The v0 implementations do not check `TOKEN_SHIELDED_POOL_INITIAL_PROTOCOL_VERSION`, and the batch `is_allowed` path does not reject these token transition kinds before nonce/fee processing. Consequently, a v14-encoded batch containing one of these variants can be decoded and paid-processed by a new binary at protocol v13, while an old v13 binary cannot decode the variant and strips or rejects the batch unpaid. That creates divergent transaction acceptance and can halt the chain. Add an unpaid pre-nonce activation gate below v14, following the existing inactive-feature/base-structure pattern, and select the new versioned implementation only in v14.
In `packages/rs-dpp/src/shielded/builder/token_shield.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/builder/token_shield.rs:42-56: Token shield builder does not enforce the Orchard amount range or bundle amount consistency
The token shield builder rejects zero but does not reject amounts greater than `i64::MAX`, even though the Orchard bundle's `value_balance` is `i64`. It also derives the transition amount from `value_balance.unsigned_abs()` without checking that it equals the caller-provided amount. Large inputs therefore fail later with a generic build/proof error, and any future bundle-shape mismatch could produce a transition for a different amount than requested. Apply the same explicit range and equality checks used by the sibling unshield builder.
In `packages/rs-dpp/src/shielded/builder/token_unshield.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/builder/token_unshield.rs:59-66: Token unshield builder uses unchecked spend summation
The builder sums caller-provided note values with `Iterator::sum::<u64>()`. A sufficiently large spend list can overflow: in debug builds this can panic, while in release builds it wraps and may produce an invalid total for subsequent amount and balance checks. Use checked accumulation and return `ShieldedBuildError` on overflow. The same issue is present in the token shielded transfer builder and should be fixed there as well.
In `packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/associated_token/token_configuration/accessors/mod.rs:247-262: The shielded-pool setter creates a second representation of an unpooled token
`set_has_shielded_pool(true)` upgrades a V0 configuration to V1, but setting the flag back to false on that V1 leaves the V1 variant in place. This permits both V0 and V1(false) to represent the same semantic unpooled configuration while having different serialized format versions; pre-v14 validation rejects the V1 representation even though it has no pool. A variant-changing setter also makes the format upgrade implicit. Use an explicit upgrade operation or preserve a canonical representation for the false case so callers cannot create V1(false) configurations that are rejected solely because of their hidden format version.
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The protocol-version, fee-accounting, pool-bookkeeping, and builder-overflow issues from the prior review have been fixed. Two in-scope correctness issues remain: group-authorized pool burns bind the Orchard proof to each individual confirmer instead of a common group action identity, and the purchase builder can panic on an out-of-range public input. The purchase builder also performs a redundant full Orchard proof generation.
🔴 2 blocking | 🟡 1 suggestion(s)
3 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Group token burns bind the shielded bundle to each confirmer's identity
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_burn_from_pool_transition_action/state_v0/mod.rs:234-250
The burn proof sighash includes the current batch owner_id, while the group-action validation above requires every confirmer to preserve the same amount and serialized_actions_digest. The proposer and a later confirmer have different batch owner identities, so the proposer’s bundle cannot be reused by the confirmer: keeping the original bundle causes the proof verification to use a different owner-bound sighash, while rebuilding the bundle for the confirmer changes the spend-auth signatures and therefore changes serialized_actions_digest, which is rejected as a modification of the group action. As a result, the newly supported group-authorized TokenBurnFromPool flow cannot complete for a group with multiple signers. The sighash needs to bind to a stable group-action identity or otherwise use the same authorization context for every confirmer.
source: gpt-6-astra (phase2-reviewer: general)
🔴 Blocking: Token purchase builder can panic when negating an unchecked out-of-range amount
packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:348-353
token_count is a public u64 input, but the builder only rejects zero before constructing the bundle. For token_count == 1u64 << 63, the later -(token_count as i64) at line 390 panics with integer overflow in debug and test builds; the value is also outside Orchard's signed value-balance range. Malformed caller input therefore causes a process panic instead of returning the documented ProtocolError, and the invalid-range check should occur before the expensive proof generation.
if token_count == 0 {
return Err(ProtocolError::ShieldedBuildError(
"token purchase count must be greater than zero".to_string(),
));
}
if token_count > i64::MAX as u64 {
return Err(ProtocolError::ShieldedBuildError(format!(
"token purchase count {} exceeds maximum allowed value {}",
token_count,
i64::MAX as u64
)));
}
source: gpt-6-astra (phase2-reviewer: rust-quality)
🟡 Suggestion: Token purchase builder generates and discards a complete Orchard proof
packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:353-388
The first build_output_only_bundle call proves and signs an outputs-only bundle, but its result is never used. The function then reconstructs the same output in the following builder and calls prove_and_sign_bundle again with the purchase-specific sighash. Orchard proof generation is expensive, so every token purchase performs the full proving work twice. Construct and prove the output bundle only once, using the purchase-specific extra sighash data from the start.
source: gpt-6-astra (phase2-reviewer: rust-quality)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
criticalbygpt-6-astra(effort low) — This large, intricate diff changes consensus validation, cryptographic shielded-proof and signature handling, funds movement, token accounting, peer-facing transition serialization, and storage migrations across protocol version 15. - Phase 1 reviewers: not run (skipped for throughput: 13 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_burn_from_pool_transition_action/state_v0/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/token/token_burn_from_pool_transition_action/state_v0/mod.rs:234-250: Group token burns bind the shielded bundle to each confirmer's identity
The burn proof sighash includes the current batch `owner_id`, while the group-action validation above requires every confirmer to preserve the same amount and `serialized_actions_digest`. The proposer and a later confirmer have different batch owner identities, so the proposer’s bundle cannot be reused by the confirmer: keeping the original bundle causes the proof verification to use a different owner-bound sighash, while rebuilding the bundle for the confirmer changes the spend-auth signatures and therefore changes `serialized_actions_digest`, which is rejected as a modification of the group action. As a result, the newly supported group-authorized `TokenBurnFromPool` flow cannot complete for a group with multiple signers. The sighash needs to bind to a stable group-action identity or otherwise use the same authorization context for every confirmer.
In `packages/rs-dpp/src/shielded/builder/token_pool_paid.rs`:
- [BLOCKING] packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:348-353: Token purchase builder can panic when negating an unchecked out-of-range amount
`token_count` is a public `u64` input, but the builder only rejects zero before constructing the bundle. For `token_count == 1u64 << 63`, the later `-(token_count as i64)` at line 390 panics with integer overflow in debug and test builds; the value is also outside Orchard's signed value-balance range. Malformed caller input therefore causes a process panic instead of returning the documented `ProtocolError`, and the invalid-range check should occur before the expensive proof generation.
- [SUGGESTION] packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:353-388: Token purchase builder generates and discards a complete Orchard proof
The first `build_output_only_bundle` call proves and signs an outputs-only bundle, but its result is never used. The function then reconstructs the same output in the following builder and calls `prove_and_sign_bundle` again with the purchase-specific sighash. Orchard proof generation is expensive, so every token purchase performs the full proving work twice. Construct and prove the output bundle only once, using the purchase-specific extra sighash data from the start.
|
Review round 2 (review 5228577296) addressed in e305b85:
Book chapter and PR body updated for the burner binding. 🤖 Addressed by Claude Code |
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Re-review — Preliminary review — Phase 1 blocker gate
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
Protocol version 15 token shielded pools are well-implemented and all 9 prior findings verify as fixed, but the head commit's group-burn proposer binding was only applied to block execution: CheckTx's stateless proof validator still binds the submitter's identity, so every valid multi-sig group burn confirmation is rejected at mempool admission.
Validated blockers were found by the Phase-1 review and confirmed by a fresh verifier. Phase 2 is deferred until a fresh same-head revalidation clears the blocker gate.
🔴 1 blocking
1 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: CheckTx rejects group token burn confirmations due to mismatched owner_id sighash binding
packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs:1067-1083
Block execution (token_burn_from_pool_transition_action/state_v0/mod.rs:237-246) binds a group burn's sighash to the stored group action's proposer_id, since every signer must submit the proposer's bundle unchanged (pinned by the serialized actions digest). But the stateless CheckTx pass in validate_batch_token_shielded_proofs unconditionally derives extra_sighash_data from batch.owner_id(). When a non-proposing group member submits their confirmation batch, batch.owner_id() is the confirmer, not the proposer, so reconstruct_and_verify_bundle recomputes the sighash with the wrong identity and binding-signature verification fails with InvalidShieldedProofError. Every valid group token burn confirmation is therefore rejected during CheckTx admission and can never enter the mempool. The proposer identity lives in state under the group action tree and is unavailable statelessly, so like ClaimToPool (which already skips this pass because its amount is only known from state), non-proposer group burn confirmations must skip stateless verification and defer to validate_state_v0, where the proposer ID is resolved from state. Only BurnFromPool needs this: it is the sole pool transition with an owner-bound sighash that implements AllowedAsMultiPartyAction (MintToPool and outputs-only bundles bind no extra data; Shield/Unshield/ShieldedTransfer do not support group actions).
BatchedTransitionRef::Token(TokenTransition::BurnFromPool(t)) => {
if t
.base()
.using_group_info()
.is_some_and(|info| !info.action_is_proposer)
{
// A group burn confirmation reuses the proposer's bundle, whose burner
// identity is only known from state. Proof verification is deferred
// to block validation, where the proposer ID is resolved from the
// stored group action.
continue;
}
let extra_sighash_data = dpp::shielded::token_burn_from_pool_extra_sighash_data(
&t.base().token_id().to_buffer(),
&owner_id,
t.amount(),
platform_version,
)?;
source: gemini-3.8-flash-high (phase1-reviewer: rust-quality)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: architecture-layering); reviewer 3: gemini-3.8-flash-high (agent: phase1-reviewer, role: platform-versioning); reviewer 4: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 5: gemini-3.8-flash-high (agent: phase1-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-gate-verifier, role: verifier)
- Degraded mode:
gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
criticalbymuse-spark-1.3-contributor(standing in forgpt-6-astra) (effort low) — Massive 32k-line change adds per-token Orchard shielded pools with new consensus rules, shielded proof verification, sighash/signature binding, and funds-movement/storage logic (e.g. shielded_proof.rs and token_shielded_pool_common execution). - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— architecture-layering (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— platform-versioning (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 100% left, 5h 100% left - Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — verifier; agentastra-gate-verifier - Phase 2 reviewers: not run (deferred by blocker gate)
🤖 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:1067-1083: CheckTx rejects group token burn confirmations due to mismatched owner_id sighash binding
Block execution (token_burn_from_pool_transition_action/state_v0/mod.rs:237-246) binds a group burn's sighash to the stored group action's proposer_id, since every signer must submit the proposer's bundle unchanged (pinned by the serialized actions digest). But the stateless CheckTx pass in validate_batch_token_shielded_proofs unconditionally derives extra_sighash_data from batch.owner_id(). When a non-proposing group member submits their confirmation batch, batch.owner_id() is the confirmer, not the proposer, so reconstruct_and_verify_bundle recomputes the sighash with the wrong identity and binding-signature verification fails with InvalidShieldedProofError. Every valid group token burn confirmation is therefore rejected during CheckTx admission and can never enter the mempool. The proposer identity lives in state under the group action tree and is unavailable statelessly, so like ClaimToPool (which already skips this pass because its amount is only known from state), non-proposer group burn confirmations must skip stateless verification and defer to validate_state_v0, where the proposer ID is resolved from state. Only BurnFromPool needs this: it is the sole pool transition with an owner-bound sighash that implements AllowedAsMultiPartyAction (MintToPool and outputs-only bundles bind no extra data; Shield/Unshield/ShieldedTransfer do not support group actions).
QuantumExplorer
left a comment
There was a problem hiding this comment.
Review of the current PR head found two consensus/block-processing blockers and one generated-client integration issue.
| &[], | ||
| ) | ||
| } | ||
| BatchedTransitionRef::Token(TokenTransition::BurnFromPool(t)) => { |
There was a problem hiding this comment.
[P1] Verify group burns against the proposer
For a non-proposer group confirmation, this uses the current batch owner (the confirmer) in the sighash. The unchanged bundle was signed against the original proposer’s identity, and the stateful validator correctly uses original_group_action.proposer_id(), so valid confirmations fail CheckTx. Defer non-proposer verification to stateful validation or resolve the original proposer here.
There was a problem hiding this comment.
Fixed, taking the first of your two options.
BurnFromPool now checks t.base().using_group_info() and continues when !action_is_proposer, so a confirmer's bundle is not verified statelessly against the wrong identity. The burner identity comes from the stored group action, so the stateful validator — which already uses original_group_action.proposer_id() — is the one that verifies it. Valid confirmations pass CheckTx again.
|
|
||
| // network until the version that introduces the format activates. | ||
|
|
||
| let validation_result = token_configuration.validate_format_version(platform_version); |
There was a problem hiding this comment.
[P1] Keep shipped generations frozen
These PV15 format and shielded-pool checks were added to shipped v0 behavior; create v2 and update v1 call it transitively. The PR similarly changes DPP validate_token_config_update v0 and Drive contract insert/update v1 while PV15 retains the PV14 table slots. Move the new behavior into new generations and select them only from PV15 so historical replay remains structurally isolated.
There was a problem hiding this comment.
Fixed — and worth flagging that the PR has since been retargeted, which changes the version numbers in your comment but not the property you asked for.
First the fix: the new behaviour is out of the shipped generations. data_contract_create/basic_structure v0 and v1, data_contract_update/basic_structure v0 and v1, DPP validate_token_config_update v0, and Drive insert_contract/update_contract v1 now contain no shielded-pool or token-configuration-format code at all. (The only shielded_pool hits left in shipped code are in initialization/v3, which is the credit pool from protocol 12 and untouched by this PR.)
Then the retarget: the PR was rebased from v4.3-dev onto v4.2-dev, where 14 is the unreleased protocol version, so the feature lands in PV14 and there is no v15 — v15.rs and the seven table constants it pulled in are gone, folded into the constants PV14 already selects.
That does not weaken the isolation. New behaviour still lives only in generations no released protocol version selects:
basic_structure— v13 selectsSome(1), PV14 selectsSome(2), so v2 is itself unreleasedinsert_contract/update_contract— v13 selects 1, PV14 selects 2create_initial_state_structure— v13 selects 3, PV14 selects 4validate_shielded_proof— v13 selects 0, PV14 selects 1
Those generations are unreleased, so per the book ("in the existing vN if that generation is still unreleased") and the precedent in this very file — eight PV14 features have amended data_contract_create/basic_structure/v2 in place — the behaviour was folded into them rather than stacking a v3 that nothing would select. The three cases that would have touched genuinely shipped code kept their own new generation instead: validate_token_config_update v1, calculate_total_tokens_balance v1 and document_base_transition_state_validation v1, since v0 of each is selected by v13 and earlier.
| uint64 start_index = 1; | ||
| uint32 count = 2; | ||
| bool prove = 3; | ||
| optional bytes token_id = 4; // target a token's shielded pool instead of the credit pool |
There was a problem hiding this comment.
[P2] Regenerate shielded-query clients
After adding token_id to these requests, regenerate and commit every checked-in DAPI client. For example, platform_pb.d.ts still exposes only startIndex/count/prove here, so consumers cannot set tokenId and can only query the credit pool. The nearby protocol-version comment should also say version 15+, not 14+.
There was a problem hiding this comment.
Clients regenerated; the version note now reads the other way round.
Every checked-in client carries token_id on the shielded requests. platform_pb.d.ts exposes hasTokenId/getTokenId/setTokenId and tokenId in AsObject on GetShieldedEncryptedNotesRequestV0, GetShieldedAnchorsRequestV0, GetShieldedPoolStateRequestV0, GetShieldedNotesCountRequestV0 and GetShieldedNullifiersRequestV0, and the nodejs pbjs/protoc, web, Objective-C and Python clients carry it too. platform_pb2.py was regenerated with the pinned rvolosatovs/protoc:4.0.0 image from the build script rather than hand-merged; platform_pb2_grpc.py came back byte-identical, which confirms the image matches whatever produced the committed files.
On the comment: it should say 14+, not 15+. The PR has been rebased onto v4.2-dev, where 14 is the unreleased protocol version, so token shielded pools activate at 14 and there is no v15 in this branch any more — TOKEN_SHIELDED_POOL_INITIAL_PROTOCOL_VERSION is 14. Your point was right; the retarget just inverted which number is correct.
c71c0ef to
8875f29
Compare
|
🌳 GroveDB structure This pull request changes the described GroveDB structure. Open it in the structure viewer: new nodes glow, removed ones stay as ghosts, and the tour walks through each change. Added (10 nodes)
Changed (1 node)
Compared |
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Re-review — Final validation — Phase 1 + Phase 2
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
Token shielded pools verify clean at this head: all ten prior findings are fixed in code (PV15 gating, proposer-bound group burns, single-prove purchase builder, checked arithmetic, transformer-charged fees, guarded anchors). Remaining issues are new-code quality and stale docs: SDK panics, an unguarded fee cast, and version-number comments.
🟡 3 suggestion(s) | 💬 4 nitpick(s)
7 finding(s) not shown inline (GitHub refused the PR diff as too large)
🟡 Suggestion: New token-pool Query impls panic with unimplemented! when prove=false
packages/rs-sdk/src/platform/query.rs:1443-1462
Five new Query impls (TokenShieldedPoolQuery pool state, anchors, most-recent anchor; TokenShieldedEncryptedNotesQuery; TokenShieldedNullifiersQuery) call unimplemented!("queries without proofs are not supported yet") when settings.prove is false. QuerySettings defaults to prove: false, so a default caller aborts the process instead of getting a recoverable Err. The same file already shows the correct pattern: the new Query<GetShieldedNotesCountRequest> for TokenShieldedPoolQuery returns Err(Error::Generic(...)) for the identical condition. Downgraded from blocking because this is client-side robustness, not a consensus break, but the new code should follow the Err pattern rather than extending the file's legacy panic pattern.
let prove = settings.prove;
if !prove {
return Err(Error::Generic(
"GetShieldedPoolStateRequest requires proofs; unproved queries are not supported"
.to_string(),
));
}
source: muse-spark-1.3-contributor (phase2-reviewer: rust-quality)
🟡 Suggestion: Fee-bundle credits_leaving as i64 has no i64-range guard
packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:90-97
Every sibling token builder rejects amount > i64::MAX as u64 before writing amount as i64, but build_fee_bundle checks only credits_leaving > total_spent and then compares sb.value_balance != credits_leaving as i64 (line 116). credits_leaving is caller-influenced (fee plus agreed price) and total_spent is a u64 sum, so credits_leaving can legally exceed i64::MAX while passing the spendable check; the as cast then wraps and the equality check compares against a wrapped value. The Orchard value_balance is i64, so values above range need an explicit up-front ShieldedBuildError like the other builders.
let total_spent = total_value(&payer.spends)?;
if credits_leaving > i64::MAX as u64 {
return Err(ProtocolError::ShieldedBuildError(format!(
"credits leaving the pool {} exceeds maximum allowed value {}",
credits_leaving,
i64::MAX as u64
)));
}
if credits_leaving > total_spent {
return Err(ProtocolError::ShieldedBuildError(format!(
"credits leaving the pool {} exceed the total spendable value {}",
credits_leaving, total_spent
)));
}
let change_amount = total_spent - credits_leaving;
source: muse-spark-1.3-contributor (phase2-reviewer: rust-quality)
💬 Nitpick: Proto comments say version 14+ for pool-only events
packages/dapi-grpc/protos/platform/v0/platform.proto:3522-3534
MintToPoolEvent and BurnFromPoolEvent comments say "protocol version 14+" but the feature gates on TOKEN_SHIELDED_POOL_INITIAL_PROTOCOL_VERSION = 15 and is_allowed rejects below 15. A client reading the comments would gate on 14 and expect events that cannot occur before 15. No wire issue (fields are correctly appended to the oneof); the documented activation boundary is wrong.
// Mint straight into the token's shielded pool (protocol version 15+)
source: muse-spark-1.3-contributor (phase2-reviewer: general)
💬 Nitpick: Book says batch pool gate is below 14
book/src/data-model/token-shielded-pools.md:195-196
The chapter says validate_is_allowed rejects a batch carrying pool transitions "below 14", but the code gates on TOKEN_SHIELDED_POOL_INITIAL_PROTOCOL_VERSION = 15. Should read below 15.
source: muse-spark-1.3-contributor (phase2-reviewer: general)
💬 Nitpick: Book names V10 for token anchor recorder
book/src/data-model/token-shielded-pools.md:218-223
Block-end section says record_token_shielded_pool_anchors is enabled by DRIVE_ABCI_METHOD_VERSIONS_V10, but the method is introduced in DRIVE_ABCI_METHOD_VERSIONS_V11 and selected only from PLATFORM_V15 (v15.rs). V10 does not contain the method.
source: muse-spark-1.3-contributor (phase2-reviewer: general)
🟡 Suggestion: Token-pool type bytes duplicate StateTransitionType discriminants as magic numbers
packages/rs-dpp/src/shielded/sighash.rs:22-28
TOKEN_SHIELDED_TRANSFER_WITH_SHIELDED_FEE_TYPE = 26, TOKEN_UNSHIELD_WITH_SHIELDED_FEE_TYPE = 27, and TOKEN_PURCHASE_FROM_SHIELDED_POOL_TYPE = 28 restate the discriminants already owned by StateTransitionType (26/27/28). Two sources of truth for one consensus-critical byte mean a future enum edit silently diverges the sighash preimage from the transition type, and the fee-bundle helper takes the type as raw u8 so any byte compiles. Derive the constants from the enum (and ideally take the enum as the parameter, casting once inside).
source: muse-spark-1.3-contributor (phase2-reviewer: rust-quality)
💬 Nitpick: Purchase fee uses saturating_add where every sibling uses checked arithmetic
packages/rs-dpp/src/shielded/compute_minimum_shielded_fee/mod.rs:288-300
compute_token_purchase_from_shielded_pool_fee sums the two storage-byte constants with saturating_add while the module documents and implements checked arithmetic surfacing ProtocolError::Overflow. The sum cannot overflow today (both constants are small), so this is not a live bug, but it breaks the module's own invariant and silently caps instead of erroring if the constants ever grow.
source: muse-spark-1.3-contributor (phase2-reviewer: rust-quality)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 8: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 9: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 10: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); reviewer 11: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 12: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 13: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: platform-versioning); reviewer 14: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: rust-quality); reviewer 15: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)
- Degraded mode:
gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-20T20:45:34Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
criticalbymuse-spark-1.3-contributor(standing in forgpt-6-astra) (effort low) — Massive 38k-line change adds consensus-critical shielded pool validation and proof verification in shielded_proof.rs with storage migration via transition_to_version_15. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(zai below 15% reserve: 5h 100% left, weekly 13% left) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — final-verifier; agentastra-verifier - Phase 2 reviewers:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — architecture-layering (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — platform-versioning (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — rust-quality (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — architecture-layering (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — platform-versioning (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — rust-quality (completed, effort xhigh); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk/src/platform/query.rs`:
- [SUGGESTION] packages/rs-sdk/src/platform/query.rs:1443-1462: New token-pool Query impls panic with `unimplemented!` when prove=false
Five new Query impls (TokenShieldedPoolQuery pool state, anchors, most-recent anchor; TokenShieldedEncryptedNotesQuery; TokenShieldedNullifiersQuery) call `unimplemented!("queries without proofs are not supported yet")` when `settings.prove` is false. QuerySettings defaults to `prove: false`, so a default caller aborts the process instead of getting a recoverable Err. The same file already shows the correct pattern: the new `Query<GetShieldedNotesCountRequest> for TokenShieldedPoolQuery` returns `Err(Error::Generic(...))` for the identical condition. Downgraded from blocking because this is client-side robustness, not a consensus break, but the new code should follow the Err pattern rather than extending the file's legacy panic pattern.
In `packages/rs-dpp/src/shielded/builder/token_pool_paid.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:90-97: Fee-bundle `credits_leaving as i64` has no i64-range guard
Every sibling token builder rejects `amount > i64::MAX as u64` before writing `amount as i64`, but `build_fee_bundle` checks only `credits_leaving > total_spent` and then compares `sb.value_balance != credits_leaving as i64` (line 116). `credits_leaving` is caller-influenced (fee plus agreed price) and `total_spent` is a u64 sum, so `credits_leaving` can legally exceed `i64::MAX` while passing the spendable check; the `as` cast then wraps and the equality check compares against a wrapped value. The Orchard `value_balance` is `i64`, so values above range need an explicit up-front `ShieldedBuildError` like the other builders.
In `packages/dapi-grpc/protos/platform/v0/platform.proto`:
- [NITPICK] packages/dapi-grpc/protos/platform/v0/platform.proto:3522-3534: Proto comments say version 14+ for pool-only events
MintToPoolEvent and BurnFromPoolEvent comments say "protocol version 14+" but the feature gates on TOKEN_SHIELDED_POOL_INITIAL_PROTOCOL_VERSION = 15 and is_allowed rejects below 15. A client reading the comments would gate on 14 and expect events that cannot occur before 15. No wire issue (fields are correctly appended to the oneof); the documented activation boundary is wrong.
In `book/src/data-model/token-shielded-pools.md`:
- [NITPICK] book/src/data-model/token-shielded-pools.md:195-196: Book says batch pool gate is below 14
The chapter says validate_is_allowed rejects a batch carrying pool transitions "below 14", but the code gates on TOKEN_SHIELDED_POOL_INITIAL_PROTOCOL_VERSION = 15. Should read below 15.
- [NITPICK] book/src/data-model/token-shielded-pools.md:218-223: Book names V10 for token anchor recorder
Block-end section says record_token_shielded_pool_anchors is enabled by DRIVE_ABCI_METHOD_VERSIONS_V10, but the method is introduced in DRIVE_ABCI_METHOD_VERSIONS_V11 and selected only from PLATFORM_V15 (v15.rs). V10 does not contain the method.
In `packages/rs-dpp/src/shielded/sighash.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/sighash.rs:22-28: Token-pool type bytes duplicate StateTransitionType discriminants as magic numbers
TOKEN_SHIELDED_TRANSFER_WITH_SHIELDED_FEE_TYPE = 26, TOKEN_UNSHIELD_WITH_SHIELDED_FEE_TYPE = 27, and TOKEN_PURCHASE_FROM_SHIELDED_POOL_TYPE = 28 restate the discriminants already owned by StateTransitionType (26/27/28). Two sources of truth for one consensus-critical byte mean a future enum edit silently diverges the sighash preimage from the transition type, and the fee-bundle helper takes the type as raw u8 so any byte compiles. Derive the constants from the enum (and ideally take the enum as the parameter, casting once inside).
In `packages/rs-dpp/src/shielded/compute_minimum_shielded_fee/mod.rs`:
- [NITPICK] packages/rs-dpp/src/shielded/compute_minimum_shielded_fee/mod.rs:288-300: Purchase fee uses `saturating_add` where every sibling uses checked arithmetic
compute_token_purchase_from_shielded_pool_fee sums the two storage-byte constants with `saturating_add` while the module documents and implements checked arithmetic surfacing ProtocolError::Overflow. The sum cannot overflow today (both constants are small), so this is not a live bug, but it breaks the module's own invariant and silently caps instead of erroring if the constants ever grow.
|
@coderabbitai review No review for |
d1123cf to
a4e805f
Compare
… transfer A valid proof is classified as binding the execution of the requested transition, or as authenticating only the state that transition affects. The token shielded transfer families claimed the former on evidence that cannot carry it: their verification shows that every nullifier the request names is spent, and nothing else. An Orchard spend derives its nullifier from the note it consumes and the viewing key, never from the outputs that replace it. A note owner can therefore authorize two transfers of the same notes to different recipients, and with no dummy padding their nullifier sets are identical. Whichever executes leaves evidence that fits the other equally well, and this verification authenticates no output commitment, ciphertext or bundle commitment that would separate them. So both transfer families now report the state they affect. The same evidence backs an ungrouped burn from a pool, where the nullifiers name the notes spent and not the amount burned: two burns of the same notes leaving different change spend the same nullifiers. Grouped, the signer's recorded entry under the action id binds it, and that id is derived from the digest of the burn's actions, so the burn follows the mint into the pool in being classified by whether it is grouped. The credit pool's own transfer, unshield and withdrawal arms rest on the same reading and are shipped from protocol version 12. They are left alone here and recorded separately: their evidence differs — a withdrawal proves a withdrawal document, which records its destination and amount — so each needs judging on its own rather than changing as a set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ool query Five of the new token shielded pool queries called `unimplemented!` when a caller set `prove` to false: pool state, anchors, most recent anchor, encrypted notes and nullifiers. The flag belongs to the caller, so the library unwound the application that passed it. Each returns `Result`, and the token notes count beside them already answered the same setting with an error. Older query implementations share the pattern. They are untouched: that they do it is not a reason for a newly added API to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Configuration named `minimumPoolNotesForOutgoing` without saying what it does. It is optional, absent reads as zero and no threshold, an issuer may set at most 250 so none can name a floor its pool never reaches, and unlike the flag beside it the threshold is not immutable: it changes through `TokenConfigUpdate` under rules that authorize no one when absent. Validation now also names the identity-less `TokenUnshieldWithShieldedFee` among the outflows the threshold governs, which is one of its four enforcement sites. Block end bookkeeping said every successful or paid pool transition records its pool, but only a successful execution does. A paid refusal bumps a nonce and writes to no pool, and the pool it named may not exist at all, so recording one would hand the anchor recorder a pool the chain does not hold. Saying otherwise invites restoring exactly what that refusal is there to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static review of the exact head found two blocking fee and storage-accounting defects in the newly added token shielded-pool paths. The review also retains several nonblocking improvements covering proof-builder validation, proof-guarantee documentation and regression coverage; the supplied CI snapshot shows Rust workspace tests passing while the main Test Suite job failed.
🔴 2 blocking | 🟡 5 suggestion(s)
7 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Fund the failure payer before verifying a sponsored shielded payment
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v2/mod.rs:155-187
The strict-sponsor check only establishes that the sponsor can pay on the successful path. A sponsored batch can pass the principal balance pre-check with an underfunded signer, then have an invalid shielded document payment replaced by BumpIdentityDataContractNonce during state validation. ExecutionEvent::create_from_state_transition_action removes the sponsor whenever such a bump is present, so fee validation falls back to the signer after Orchard verification has already run. If the signer cannot cover the verification fee, the event is rejected without charging the fee or committing the nonce bump, defeating the paid-invalid-bundle accounting introduced by this PR. Resolve the payer before verification or preserve an authorized sponsor liability for verification failures, and add a regression with an underfunded signer and a sponsor that covers the successful-path fee.
source: gpt-6.1-sol (phase2-reviewer: general)
🔴 Blocking: Enforce pool balance write exclusivity in Drive's batch guard
packages/rs-drive/src/util/batch/drive_op_batch/mod.rs:489-527
refuse_repeated_token_balance_writes only classifies the older TokenOperationType variants. The new pool operations are handled by TokenOperationType but fall through to _ => vec![], while their lowering reads the pre-batch pool total and emits an absolute replacement. Therefore a composed apply_drive_operations call can combine, for example, a pool mint with a transparent mint for the same token, or multiple pool operations, without detecting repeated supply or pool-total writes; the later absolute write can discard the earlier change. The ABCI transition-count limit currently masks this defect for the normal node path, but it does not protect Drive's public batch-application API or future callers that compose operations. Extend the classifier with holder, supply and pool-total keys for every writing pool operation and add Drive-level composed-operation coverage.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
🟡 Suggestion: Reject oversized spend selections before generating proofs
packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:153-213
The identity-less builders generate the token and credit Orchard proofs before the resulting action lists are checked against system_limits.max_shielded_transition_actions by transition structure validation. build_fee_bundle checks only that the generated action count equals the count used for fee calculation, not that the count is within the protocol limit. An oversized token or fee spend can therefore perform expensive proof generation and only then return a transition that structure validation rejects. Check both padded spend counts before invoking the prover, and apply the same guard to unshield and purchase builders.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
🟡 Suggestion: Avoid deep-cloning the token configuration for every token pool spend
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs:65-69
resolve_pooled_token clones the complete TokenConfiguration from the contract map while also returning the Arc<DataContractFetchInfo> that owns the original contract. The identity-less transfer discards the configuration, and the other callers only read a few policy fields. This creates configuration-sized allocations on every validation, including CheckTx and paths that will soon reject. Return the retained contract with a token position, or return only the required scalar policy values, so callers can borrow the configuration without changing validation order or behavior.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
🟡 Suggestion: Token-pool type bytes duplicate StateTransitionType discriminants as magic numbers
packages/rs-dpp/src/shielded/sighash.rs:28-34
The three identity-less token sighash constants independently repeat the StateTransitionType discriminants 26, 27 and 28. Their current values are correct and the frozen-layout tests protect today's bytes, but the type system does not connect the signing-domain constants to the enum that owns the transition discriminants. Derive the constants from the corresponding enum variants, or add compile-time equality assertions while retaining the frozen numeric-layout tests, so the sighash binding cannot drift from the transition type.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning)
🟡 Suggestion: Describe token pool proofs as affected-state evidence
book/src/data-model/token-shielded-pools.md:218-219
The chapter says that a token-pool transfer proves its execution, while the classifier deliberately returns only AffectedState for the transfer forms and for ungrouped burns. Spent nullifiers prove that particular notes were spent, but they do not bind the requested recipients, outputs or competing transfer. The Queries section repeats the same overclaim for token-pool transitions. Update both descriptions to state that they authenticate affected state, and distinguish the stronger grouped-action evidence where applicable.
source: gpt-6.1-sol (phase2-reviewer: general)
🟡 Suggestion: Pin the downgraded token proof guarantees with regression assertions
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:6272-6352
The classifier correctly returns affected-state guarantees for batched and identity-less shielded transfers and for ungrouped pool burns, but the snapshot table does not include these new transition families. Existing pool tests cover proof results and grouped execution guarantees, not the negative classification that prevents spent-nullifier evidence from being advertised as execution proof. Add affected-state assertions for both transfer forms and an ungrouped burn, while retaining an execution-proved assertion for grouped burns.
source: gpt-6.1-sol (phase2-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate diff changes consensus-critical state-transition validation, cryptographic shielded proofs and signatures, token funds movement, and storage tree migrations across rs-dpp, rs-drive, and rs-drive-abci. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 85% left, 5h 7% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v2/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v2/mod.rs:155-187: Fund the failure payer before verifying a sponsored shielded payment
The strict-sponsor check only establishes that the sponsor can pay on the successful path. A sponsored batch can pass the principal balance pre-check with an underfunded signer, then have an invalid shielded document payment replaced by `BumpIdentityDataContractNonce` during state validation. `ExecutionEvent::create_from_state_transition_action` removes the sponsor whenever such a bump is present, so fee validation falls back to the signer after Orchard verification has already run. If the signer cannot cover the verification fee, the event is rejected without charging the fee or committing the nonce bump, defeating the paid-invalid-bundle accounting introduced by this PR. Resolve the payer before verification or preserve an authorized sponsor liability for verification failures, and add a regression with an underfunded signer and a sponsor that covers the successful-path fee.
In `packages/rs-drive/src/util/batch/drive_op_batch/mod.rs`:
- [BLOCKING] packages/rs-drive/src/util/batch/drive_op_batch/mod.rs:489-527: Enforce pool balance write exclusivity in Drive's batch guard
`refuse_repeated_token_balance_writes` only classifies the older `TokenOperationType` variants. The new pool operations are handled by `TokenOperationType` but fall through to `_ => vec![]`, while their lowering reads the pre-batch pool total and emits an absolute replacement. Therefore a composed `apply_drive_operations` call can combine, for example, a pool mint with a transparent mint for the same token, or multiple pool operations, without detecting repeated supply or pool-total writes; the later absolute write can discard the earlier change. The ABCI transition-count limit currently masks this defect for the normal node path, but it does not protect Drive's public batch-application API or future callers that compose operations. Extend the classifier with holder, supply and pool-total keys for every writing pool operation and add Drive-level composed-operation coverage.
In `packages/rs-dpp/src/shielded/builder/token_pool_paid.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:153-213: Reject oversized spend selections before generating proofs
The identity-less builders generate the token and credit Orchard proofs before the resulting action lists are checked against `system_limits.max_shielded_transition_actions` by transition structure validation. `build_fee_bundle` checks only that the generated action count equals the count used for fee calculation, not that the count is within the protocol limit. An oversized token or fee spend can therefore perform expensive proof generation and only then return a transition that structure validation rejects. Check both padded spend counts before invoking the prover, and apply the same guard to unshield and purchase builders.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs:65-69: Avoid deep-cloning the token configuration for every token pool spend
`resolve_pooled_token` clones the complete `TokenConfiguration` from the contract map while also returning the `Arc<DataContractFetchInfo>` that owns the original contract. The identity-less transfer discards the configuration, and the other callers only read a few policy fields. This creates configuration-sized allocations on every validation, including CheckTx and paths that will soon reject. Return the retained contract with a token position, or return only the required scalar policy values, so callers can borrow the configuration without changing validation order or behavior.
In `packages/rs-dpp/src/shielded/sighash.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/sighash.rs:28-34: Token-pool type bytes duplicate StateTransitionType discriminants as magic numbers
The three identity-less token sighash constants independently repeat the `StateTransitionType` discriminants 26, 27 and 28. Their current values are correct and the frozen-layout tests protect today's bytes, but the type system does not connect the signing-domain constants to the enum that owns the transition discriminants. Derive the constants from the corresponding enum variants, or add compile-time equality assertions while retaining the frozen numeric-layout tests, so the sighash binding cannot drift from the transition type.
In `book/src/data-model/token-shielded-pools.md`:
- [SUGGESTION] book/src/data-model/token-shielded-pools.md:218-219: Describe token pool proofs as affected-state evidence
The chapter says that a token-pool transfer proves its execution, while the classifier deliberately returns only `AffectedState` for the transfer forms and for ungrouped burns. Spent nullifiers prove that particular notes were spent, but they do not bind the requested recipients, outputs or competing transfer. The Queries section repeats the same overclaim for token-pool transitions. Update both descriptions to state that they authenticate affected state, and distinguish the stronger grouped-action evidence where applicable.
In `packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:6272-6352: Pin the downgraded token proof guarantees with regression assertions
The classifier correctly returns affected-state guarantees for batched and identity-less shielded transfers and for ungrouped pool burns, but the snapshot table does not include these new transition families. Existing pool tests cover proof results and grouped execution guarantees, not the negative classification that prevents spent-nullifier evidence from being advertised as execution proof. Add affected-state assertions for both transfer forms and an ungrouped burn, while retaining an execution-proved assertion for grouped burns.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Audit the pre-existing credit-pool transfer execution guarantee — The credit-pool
ShieldedTransferclassifier still reports execution-proved evidence even though its nullifier-only proof does not bind replacement outputs. This behavior exists at the base of the PR and is unchanged here; it should be tracked separately rather than expanded into this token-pool feature review.- Follow-up: Open a separate maintainer-requested issue or PR with competing-spend regression coverage for the credit-pool classifier.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
`refuse_repeated_token_balance_writes` refuses a batch that writes one token balance or supply twice, because each such write computes its new value from the one committed before the batch and the later write would discard the earlier. The classifier it reads ended in a catch-all, so all five pool operations fell through it as writing nothing: a composed batch could pair a mint into a pool with a transparent mint of the same token, or two pool operations, and the absolute replacement written last would silently drop the first. Each pool operation now reports its keys, and the pool total has a key of its own. What each writes was read off its lowering rather than its name: a shield writes the shielding identity's balance and the pool total, an unshield the recipient's and the total, a mint into or burn from the pool the supply and the total, and a transfer inside the pool neither — its balance change is `Unchanged`, which produces no total write at all. The catch-all is gone. The match names every remaining variant explicitly, so a new token operation cannot compile until someone says what it writes, which is the point: the old default was that an unclassified operation wrote nothing. The node path was shielded from this by the one-transition cap on a documents batch, but that cap is a tripwire and Drive's batch application is a public API. No released generation changes: the guard is reached only from `apply_drive_operations` generation 1, which protocol version 14 selects, and it only ever refuses — it never alters a value in a batch it admits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…o verify A batch asking the contract owner to pay its gas was held only to its principal, so a sponsor who could cover the verification was enough to let it through. That covers the batch that executes. It does not cover the one that does not: a sub-transition state validation replaces with a nonce bump takes the sponsor off the whole batch, and the signer then owes the verification that already ran. An underfunded signer there is refused without being charged and without the bump committing, so the proof work is free and the bytes stay valid. The signer now holds the compute fee of the bundles the batch carries alongside its principal, whoever is expected to pay. On the batch that executes the sponsor still pays, as sponsorship intends; the signer's reserve is what makes somebody chargeable on the batch that does not. A batch carrying no bundle adds nothing and is asked for its principal alone. The sponsor is not made liable instead, because the rule that drops it on a nonce bump is not this branch's: it is in the base, and a bump records only the identity, contract, nonce and fee multiplier, so the one raised by a shielded payment cannot be told from any other to gate the change on protocol version 14. The cost of this is real and worth naming: a sponsored signer must hold credits it will not usually spend. Test would have caught this in CI: the test that pinned the old rule asserted a sponsored signer with no balance passes, and is inverted — it now fails against the old arm with "a sponsor is not certain to pay" and passes against the new one. A second test pins the amount at the compute fee exactly, one credit less being short, with the figure written out rather than recomputed from the code under test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/self-reviewed |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The token shielded-pool implementation has addressed the earlier pool-write, activation, proof-classification, admission-fee, and bookkeeping blockers. Two in-scope blocking issues remain: sponsored signers are reserved only for proof-compute fees, not the complete chargeable failure event, and document proofs can still report execution-bound evidence without binding the newly added shielded payment. Several non-blocking regression, documentation, ownership, and builder-preflight improvements also remain.
🔴 2 blocking | 🟡 5 suggestion(s)
7 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Fund the failure payer before verifying a sponsored shielded payment
packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs:86-88
The sponsored-batch pre-check now reserves the signer's principal plus the raw Orchard compute fee, but it still does not establish that the signer can pay the complete failure event. If state validation replaces a document action with BumpIdentityDataContractNonce, ExecutionEvent::create_from_state_transition_action removes the sponsor. validate_fees_of_event then charges the signer for the metered validation and nonce-bump operations and applies user_fee_increase before checking its balance. A signer holding exactly the newly required compute fee can therefore pass admission, undergo proof verification, and then receive an unpaid execution error without the nonce bump or fee committing. The pre-verification admission path must reserve the same bounded failure-event cost that execution will charge, or failure settlement must be changed to a bounded penalty covered by the reserve. Add an execution-level boundary test with a failing sponsored batch and a nonzero fee increase.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
🔴 Blocking: Downgrade document proofs that do not bind the shielded payment
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:3084-3097
The document classifier returns ExecutionProved for every non-indexOnly document transition after verifying the document and optional owner balance. The new shielded-payment side effect is not included in that proof or examined by state_transition_proof_binds_execution. Different shielded payment bundles can therefore produce the same authenticated document state, allowing a valid document proof to be accepted as confirmation of a particular payment that the proof never identifies. Classify document transitions carrying shielded payments as AffectedState unless the proof also authenticates a transaction-bound payment receipt.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
🟡 Suggestion: Pin the downgraded token proof guarantees with regression assertions
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:6272-6352
The classifier correctly downgrades token-pool snapshot evidence, but classifier_tags_snapshot_only_families_as_affected_state still constructs only identity and address transitions. It does not assert AffectedState for token shield, unshield, token shielded transfer, token purchase or ungrouped burn/mint pool families, nor does it preserve explicit stronger-guarantee assertions for grouped cases. A future classifier refactor could silently promote nullifier or balance snapshots back to ExecutionProved without this test failing. Add token-pool cases at this shared Drive verifier boundary, including the grouped exceptions.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, rust-quality)
🟡 Suggestion: Describe token pool proofs as affected-state evidence
book/src/data-model/token-shielded-pools.md:218-219
The chapter still says that a token-pool transfer proves execution from spent nullifiers, that an unshield proves execution from the recipient balance, and that a purchase proves execution from the pool total. The verifier deliberately returns AffectedState for these identity-less families because nullifiers identify spent notes rather than recipients or outputs, while balances and totals are state snapshots. The Queries section repeats the same stronger claim. Reword both passages to describe authenticated affected-state evidence and distinguish it from grouped action-participation evidence that can remain execution-bound.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general)
🟡 Suggestion: Token-pool type bytes duplicate StateTransitionType discriminants as magic numbers
packages/rs-dpp/src/shielded/sighash.rs:28-34
The three token-pool sighash constants independently repeat the StateTransitionType discriminants 26, 27, and 28. The frozen layout tests preserve today's bytes but do not express the relationship between the sighash tags and the canonical enum. If the enum is changed later, the code can compile while signing and verification silently use a divergent type byte. Derive these constants from the corresponding enum variants, or add explicit equality assertions alongside the layout tests; keep the separate high-range outputs-only tags as domain separators.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, platform-versioning, rust-quality)
🟡 Suggestion: Avoid deep-cloning the token configuration for every token pool spend
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs:65-69
resolve_pooled_token clones the complete TokenConfiguration from the contract even though it also returns the owning Arc<DataContractFetchInfo>. The configuration contains nested distribution, marketplace, and change-control data, while callers only inspect a few properties and one caller discards the configuration. This creates an avoidable allocation on every identity-less token-pool validation. Return the retained contract handle and borrow the configuration in callers, or project only the scalar values required by the shared resolver without creating a self-referential tuple.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, rust-quality)
🟡 Suggestion: Reject oversized spend selections before generating proofs
packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:153-213
The identity-less builders begin token proof construction before checking either the token or fee spend selection against platform_version.system_limits.max_shielded_transition_actions. The transfer path proves the token bundle before even calculating the fee bundle's action count, and the final bundle constructors only wrap fields rather than performing structural validation. A wallet selection that consensus must reject can therefore incur expensive proving work first. Preflight both selections, including Orchard padding and required outputs, before invoking either prover.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, rust-quality)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate diff changes consensus validation and storage for shielded token funds, adds cryptographic proof and signature handling across numerous state transitions, and introduces peer-facing protocol serialization in functions such as token shielded-pool validators and execution paths. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 70% left, 5h 12% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs:86-88: Fund the failure payer before verifying a sponsored shielded payment
The sponsored-batch pre-check now reserves the signer's principal plus the raw Orchard compute fee, but it still does not establish that the signer can pay the complete failure event. If state validation replaces a document action with `BumpIdentityDataContractNonce`, `ExecutionEvent::create_from_state_transition_action` removes the sponsor. `validate_fees_of_event` then charges the signer for the metered validation and nonce-bump operations and applies `user_fee_increase` before checking its balance. A signer holding exactly the newly required compute fee can therefore pass admission, undergo proof verification, and then receive an unpaid execution error without the nonce bump or fee committing. The pre-verification admission path must reserve the same bounded failure-event cost that execution will charge, or failure settlement must be changed to a bounded penalty covered by the reserve. Add an execution-level boundary test with a failing sponsored batch and a nonzero fee increase.
In `packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- [BLOCKING] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:3084-3097: Downgrade document proofs that do not bind the shielded payment
The document classifier returns `ExecutionProved` for every non-`indexOnly` document transition after verifying the document and optional owner balance. The new shielded-payment side effect is not included in that proof or examined by `state_transition_proof_binds_execution`. Different shielded payment bundles can therefore produce the same authenticated document state, allowing a valid document proof to be accepted as confirmation of a particular payment that the proof never identifies. Classify document transitions carrying shielded payments as `AffectedState` unless the proof also authenticates a transaction-bound payment receipt.
- [SUGGESTION] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:6272-6352: Pin the downgraded token proof guarantees with regression assertions
The classifier correctly downgrades token-pool snapshot evidence, but `classifier_tags_snapshot_only_families_as_affected_state` still constructs only identity and address transitions. It does not assert `AffectedState` for token shield, unshield, token shielded transfer, token purchase or ungrouped burn/mint pool families, nor does it preserve explicit stronger-guarantee assertions for grouped cases. A future classifier refactor could silently promote nullifier or balance snapshots back to `ExecutionProved` without this test failing. Add token-pool cases at this shared Drive verifier boundary, including the grouped exceptions.
In `book/src/data-model/token-shielded-pools.md`:
- [SUGGESTION] book/src/data-model/token-shielded-pools.md:218-219: Describe token pool proofs as affected-state evidence
The chapter still says that a token-pool transfer proves execution from spent nullifiers, that an unshield proves execution from the recipient balance, and that a purchase proves execution from the pool total. The verifier deliberately returns `AffectedState` for these identity-less families because nullifiers identify spent notes rather than recipients or outputs, while balances and totals are state snapshots. The Queries section repeats the same stronger claim. Reword both passages to describe authenticated affected-state evidence and distinguish it from grouped action-participation evidence that can remain execution-bound.
In `packages/rs-dpp/src/shielded/sighash.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/sighash.rs:28-34: Token-pool type bytes duplicate StateTransitionType discriminants as magic numbers
The three token-pool sighash constants independently repeat the `StateTransitionType` discriminants 26, 27, and 28. The frozen layout tests preserve today's bytes but do not express the relationship between the sighash tags and the canonical enum. If the enum is changed later, the code can compile while signing and verification silently use a divergent type byte. Derive these constants from the corresponding enum variants, or add explicit equality assertions alongside the layout tests; keep the separate high-range outputs-only tags as domain separators.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs:65-69: Avoid deep-cloning the token configuration for every token pool spend
`resolve_pooled_token` clones the complete `TokenConfiguration` from the contract even though it also returns the owning `Arc<DataContractFetchInfo>`. The configuration contains nested distribution, marketplace, and change-control data, while callers only inspect a few properties and one caller discards the configuration. This creates an avoidable allocation on every identity-less token-pool validation. Return the retained contract handle and borrow the configuration in callers, or project only the scalar values required by the shared resolver without creating a self-referential tuple.
In `packages/rs-dpp/src/shielded/builder/token_pool_paid.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:153-213: Reject oversized spend selections before generating proofs
The identity-less builders begin token proof construction before checking either the token or fee spend selection against `platform_version.system_limits.max_shielded_transition_actions`. The transfer path proves the token bundle before even calculating the fee bundle's action count, and the final bundle constructors only wrap fields rather than performing structural validation. A wallet selection that consensus must reject can therefore incur expensive proving work first. Preflight both selections, including Orchard padding and required outputs, before invoking either prover.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Audit the existing credit-pool shielded-transfer execution-proof classification — The pre-existing credit-pool
ShieldedTransferverifier authenticates spent-nullifier membership while its classifier still reports execution-bound evidence. Nullifiers identify consumed notes rather than the requested outputs, so this deserves a separate audit of the credit-pool proof guarantee.- Follow-up: Track as a separate security issue or maintainer-requested change; it predates the token-pool changes in this PR.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
…r chose Settlement raises the processing fee by the percentage the signer asked for and only then judges the balance against it. No floor did: every one of them summed purchases, collateral and the base fee and stopped. A signer holding exactly the floor therefore passed admission and could not be charged what settlement asked for. A review found this on the sponsored path, where it is worst: a sub-transition replaced by a nonce bump takes the sponsor off the batch, so the signer owes a verification that already ran, and an unpaid refusal leaves the bytes valid. The same arithmetic was missing unsponsored, where the refusal is at least paid, so the fix is one place rather than the sponsored arm alone. The increase is applied the way `FeeResult::apply_user_fee_increase` applies it — the fee plus the fee times the percentage over a hundred, saturating — and only over the fee. A document purchase and a contested collateral are amounts a signer owes, not fees, and settlement does not raise them either. This asks more of a signer who chose an increase, which is deliberate: raising what the network charges while holding only the unraised amount is what let the work happen with nobody able to pay for it. Test would have caught this in CI: asserted relationally, the same batch and the same balance differing only in the increase, so it cannot pass by agreeing with whatever the floor computes. It fails against the old floor, which treated both alike. The first version of this test was itself worthless — it chose a balance the floor rejected for other reasons and so never exercised the increase. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…aid for it The document classifier judged by the document type: anything but an indexOnly type bound its execution. A document whose token cost is paid from a shielded pool is proven by the document and, where one is read, the owner's balance — neither of which names the bundle that paid. Two different payments leave the same document behind, so the proof confirms a payment it never identifies. Such a transition now reports the state it affects. An audit of every arm that can claim execution followed, because this was the third round in which one arm of the same defect was found at a time. All eight token pool arms came back correct, each resting on a named mechanism: a history document whose id carries the owner's nonce, with its content compared against a rebuilt document; or a group signer entry that errors on absence, on the wrong element type and on a cardinality other than one, under an action id that for the pool operations folds in the digest of their actions. Two comments went with it. One claimed the token pool nullifiers bind the exact bundle of a transfer, directly above the arm that says they do not. The other said nullifier-spend proofs bind the exact Orchard actions of three credit pool families, which is true of the withdrawal alone — its document id derives from its first nullifier and its output script, so that one binds its destination. The snapshot test now carries the three identity-less token pool families, which nothing pinned in either direction. It cannot carry the arms that claim execution: its contract lookup returns none and both batch branches refuse on that, so a batch entry makes the test's own expect panic. Pinning those needs a contract fixture, which is filed rather than done here. Filed, not fixed here, all reached from released protocol versions: the credit pool transfer and unshield arms and the withdrawal's uncompared amount (#5243); the document arm's delete, transfer and purchase kinds, which bind an owner or an absence rather than the request (#5255). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the combined reviewer claims and all 31 prior findings against head 856b790. Two blockers remain: the fee-floor uplift changes released validation behavior, and the sponsored signer reserve cannot cover the complete failed event; five non-blocking suggestions also remain. This was a static review only: the supplied CI snapshot still had Rust workspace tests, the main test suite, one browser suite, and PR Hygiene pending.
🔴 2 blocking | 🟡 5 suggestion(s)
7 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Keep the fee-floor uplift out of released batch prechecks
packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/state_transition_estimated_fee_validation.rs:217-225
The shared helper now applies user_fee_increase to every caller, including StateTransitionIdentityEstimatedFeeValidation::validate_estimated_fee. ABCI's generation-0 identity_minimum_balance_pre_check calls that implementation, and the released validation tables retain generation 0 through protocol version 13. Compared with the base, an ordinary batch with a nonzero fee increase now has a different early rejection threshold and IdentityInsufficientBalanceError.required_balance, despite containing no token-pool feature. Matching eventual settlement does not make this historical validation change output-identical, and the token-pool activation gate does not isolate it. Preserve the unraised calculation for generation 0 and apply the uplift only through the generation-1 entry points selected by protocol version 14. Add historical-version assertions for both the threshold and error payload, alongside the latest-version uplift test.
source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
🔴 Blocking: Fund the failure payer before verifying a sponsored shielded payment
packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs:79-88
The reserve now includes the bundle compute fee and the requested fee increase, but it still excludes the other charges retained on a failed event. The new should_ask_a_sponsored_signer_for_the_compute_fee_and_no_more test explicitly accepts a signer holding exactly the compute component. If document payment validation fails, batch state validation replaces the action with a nonce bump, ExecutionEvent construction removes the sponsor, and the accumulated signature, contract, pool and other validation charges remain alongside the compute fee. Fee validation then requires the full total_base_fee from the signer; execute_event returns an unpaid error without applying the nonce bump when that total does not fit. A sufficient compute reserve therefore does not establish that completed verification is chargeable. Estimate or reserve the complete signer-paid failure charge before stateful verification, or define a charging policy that can settle that failure. Cover the exact-floor boundary through event settlement; the current sponsored integration fixture funds the buyer substantially above it.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
🟡 Suggestion: Describe token pool proofs as affected-state evidence
book/src/data-model/token-shielded-pools.md:218-219
This paragraph says the three identity-less transitions prove execution using nullifiers or balances, and the Queries section repeats the execution claim for batch pool operations. The verifier deliberately returns affected-state evidence for all three identity-less families and the ungrouped pool operations: those proofs authenticate spent-note status or a balance snapshot, not the complete submitted request. Update both passages to state that distinction. Retain execution-binding wording only where separately recorded evidence, such as grouped mint or burn participation under the action ID, actually binds the request.
source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general)
🟡 Suggestion: Reject oversized spend selections before generating proofs
packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:153-162
Neither the token spend selection nor fee_payer.spends is checked against the selected protocol's action limit before proving. The pinned Orchard builder pads to max(spends, outputs, minimum actions) without enforcing Platform's limit of 16, the fee calculation does not enforce that limit, and try_from_bundles only constructs the transition. These builders can therefore perform expensive proving and return a transition that structure validation rejects. Preflight both resulting action counts before generating either proof, accounting for output count and Orchard padding, and share the check with the unshield and purchase builders.
source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering)
🟡 Suggestion: Avoid deep-cloning the token configuration for every token pool spend
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs:65-69
resolve_pooled_token deep-clones TokenConfiguration even though the returned Arc already owns it. That configuration contains owned localization maps, distribution data and change-control structures, so each identity-less transfer, unshield and purchase performs avoidable allocations; the transfer caller immediately discards the cloned configuration. Return the retained contract and resolve a borrowed configuration in the caller, or return only the small resolved fields needed by each operation. Do not return a reference into an Arc owned by the same tuple.
source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, rust-quality)
🟡 Suggestion: Token-pool type bytes duplicate StateTransitionType discriminants as magic numbers
packages/rs-dpp/src/shielded/sighash.rs:28-34
These constants independently repeat StateTransitionType's 26, 27 and 28 discriminants. Their values currently agree, but builders and validators agreeing through the same constants does not check agreement with the wire enum. The literal preimage tests and outputs-only domain-tag collision test protect different invariants. Derive the constants from the corresponding enum variants, or add explicit equality assertions if feature separation requires standalone constants, while preserving the frozen sighash bytes.
source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality)
🟡 Suggestion: Pin the downgraded token proof guarantees with regression assertions
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:6292-6299
The added classifier rows now cover the three identity-less transitions, but the table still has no Batch cases. The negative classifications for batched shielded transfers, ungrouped burns and documents carrying a shielded payment remain unasserted. The pool integration suite asserts the grouped mint's positive execution guarantee, while its other proof checks inspect payloads without asserting the weaker outcome. Add negative batch assertions with a known-contract provider, including the newly downgraded document payment, and retain the existing grouped and ordinary-document positive controls. Otherwise a future classifier change can silently strengthen these guarantees while all payload assertions remain green.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large, intricate diff directly changes funds movement and consensus validation through new token pool transitions such as token_unshield_with_shielded_fee_transition/v0/state_transition_validation.rs, cryptographic signature binding in packages/rs-dpp/src/shielded/sighash.rs, and pool-inclusive supply conservation in calculate_total_tokens_balance v1. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 46% left, 5h 54% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/state_transition_estimated_fee_validation.rs`:
- [BLOCKING] packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/state_transition_estimated_fee_validation.rs:217-225: Keep the fee-floor uplift out of released batch prechecks
The shared helper now applies user_fee_increase to every caller, including StateTransitionIdentityEstimatedFeeValidation::validate_estimated_fee. ABCI's generation-0 identity_minimum_balance_pre_check calls that implementation, and the released validation tables retain generation 0 through protocol version 13. Compared with the base, an ordinary batch with a nonzero fee increase now has a different early rejection threshold and IdentityInsufficientBalanceError.required_balance, despite containing no token-pool feature. Matching eventual settlement does not make this historical validation change output-identical, and the token-pool activation gate does not isolate it. Preserve the unraised calculation for generation 0 and apply the uplift only through the generation-1 entry points selected by protocol version 14. Add historical-version assertions for both the threshold and error payload, alongside the latest-version uplift test.
In `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs:79-88: Fund the failure payer before verifying a sponsored shielded payment
The reserve now includes the bundle compute fee and the requested fee increase, but it still excludes the other charges retained on a failed event. The new should_ask_a_sponsored_signer_for_the_compute_fee_and_no_more test explicitly accepts a signer holding exactly the compute component. If document payment validation fails, batch state validation replaces the action with a nonce bump, ExecutionEvent construction removes the sponsor, and the accumulated signature, contract, pool and other validation charges remain alongside the compute fee. Fee validation then requires the full total_base_fee from the signer; execute_event returns an unpaid error without applying the nonce bump when that total does not fit. A sufficient compute reserve therefore does not establish that completed verification is chargeable. Estimate or reserve the complete signer-paid failure charge before stateful verification, or define a charging policy that can settle that failure. Cover the exact-floor boundary through event settlement; the current sponsored integration fixture funds the buyer substantially above it.
In `book/src/data-model/token-shielded-pools.md`:
- [SUGGESTION] book/src/data-model/token-shielded-pools.md:218-219: Describe token pool proofs as affected-state evidence
This paragraph says the three identity-less transitions prove execution using nullifiers or balances, and the Queries section repeats the execution claim for batch pool operations. The verifier deliberately returns affected-state evidence for all three identity-less families and the ungrouped pool operations: those proofs authenticate spent-note status or a balance snapshot, not the complete submitted request. Update both passages to state that distinction. Retain execution-binding wording only where separately recorded evidence, such as grouped mint or burn participation under the action ID, actually binds the request.
In `packages/rs-dpp/src/shielded/builder/token_pool_paid.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:153-162: Reject oversized spend selections before generating proofs
Neither the token spend selection nor fee_payer.spends is checked against the selected protocol's action limit before proving. The pinned Orchard builder pads to max(spends, outputs, minimum actions) without enforcing Platform's limit of 16, the fee calculation does not enforce that limit, and try_from_bundles only constructs the transition. These builders can therefore perform expensive proving and return a transition that structure validation rejects. Preflight both resulting action counts before generating either proof, accounting for output count and Orchard padding, and share the check with the unshield and purchase builders.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs:65-69: Avoid deep-cloning the token configuration for every token pool spend
resolve_pooled_token deep-clones TokenConfiguration even though the returned Arc<DataContractFetchInfo> already owns it. That configuration contains owned localization maps, distribution data and change-control structures, so each identity-less transfer, unshield and purchase performs avoidable allocations; the transfer caller immediately discards the cloned configuration. Return the retained contract and resolve a borrowed configuration in the caller, or return only the small resolved fields needed by each operation. Do not return a reference into an Arc owned by the same tuple.
In `packages/rs-dpp/src/shielded/sighash.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/sighash.rs:28-34: Token-pool type bytes duplicate StateTransitionType discriminants as magic numbers
These constants independently repeat StateTransitionType's 26, 27 and 28 discriminants. Their values currently agree, but builders and validators agreeing through the same constants does not check agreement with the wire enum. The literal preimage tests and outputs-only domain-tag collision test protect different invariants. Derive the constants from the corresponding enum variants, or add explicit equality assertions if feature separation requires standalone constants, while preserving the frozen sighash bytes.
In `packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:6292-6299: Pin the downgraded token proof guarantees with regression assertions
The added classifier rows now cover the three identity-less transitions, but the table still has no Batch cases. The negative classifications for batched shielded transfers, ungrouped burns and documents carrying a shielded payment remain unasserted. The pool integration suite asserts the grouped mint's positive execution guarantee, while its other proof checks inspect payloads without asserting the weaker outcome. Add negative batch assertions with a known-contract provider, including the newly downgraded document payment, and retain the existing grouped and ordinary-document positive controls. Otherwise a future classifier change can silently strengthen these guarantees while all payload assertions remain green.
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Audit execution-binding classifications for existing credit-pool spends — The existing credit-pool Unshield and ShieldedTransfer classifiers still return execution-binding outcomes. Their verification authenticates spent nullifiers and, for Unshield, an output-address balance snapshot, without binding the complete submitted Orchard request. These branches and classifications are already present in the base; this PR's token equivalents correctly use affected-state outcomes.
- Follow-up: Handle the credit-pool classifications separately with a compatibility assessment and request-binding regression coverage.
- Audit existing document delete, transfer and purchase proof guarantees — For ordinary non-index-only documents without shielded payments, the classifier grants an execution-binding outcome, while delete verification checks absence and transfer/purchase verification checks resulting ownership. Those checks do not identify the complete submitted transition. The same verification branches exist in the base, so this is a separate existing proof-contract concern rather than a reason to expand the token-pool PR.
- Follow-up: Audit these document proof classifications in the separate document-proof follow-up identified by the reviewer.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
…ions report The increase was raised in `validate_estimated_principal_and_fees`, the helper both generations share. Generation 0 of the batch fee check is what every protocol version up to 13 selects, and those are released: raising the fee there moved the balance they admit and the `required_balance` their `IdentityInsufficientBalanceError` reports. Protocol version 14 has no business changing either. The raise moves into a private helper the unreleased generation calls, so the released one reaches the shared helper with the figures it has always used. Test would have caught this: `should_leave_the_released_floor_alone_when_the_signer_chose_an_increase` fails on the parent with "generation 0 is released: an increase of 50 must not move its floor", passes here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A sponsored batch carrying a bundle was asked only for the verification fee. That is not what a failure costs. A sub-transition replaced by a nonce bump takes the sponsor off the batch, and the signer is then left owing the signature check, the contract reads, the pool reads and the bump itself, beside the verification that already ran. Such a batch is now held to the floor an unsponsored one meets. That inherits the floor's own guarantee, that a batch meeting it can be charged, rather than guessing at a bound for the failed event. A batch carrying no bundle has no such work to pay for and keeps the exemption. `should_ask_a_sponsored_signer_for_the_compute_fee_and_no_more` is replaced: it pinned the insufficient rule, asserting the compute fee was the whole of what a sponsored signer is asked for. Its successor `should_hold_a_sponsored_signer_to_the_whole_floor_when_its_batch_carries_a_bundle` fails on the parent with "the compute fee alone leaves the rest of a failed event unfunded", passes here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The sponsored-signer reservation still does not cover the complete failed-event fee, so one blocking funding issue remains at cb7386b. All 32 prior findings were revalidated; the released-generation fee-floor change and the previously identified proof, activation, and bookkeeping defects are addressed. This was a static review: the supplied CI snapshot reports passing Rust workspace tests, while the main Test Suite, browser suite (1), and PR Hygiene remain pending.
🔴 1 blocking | 🟡 6 suggestion(s)
7 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Fund the failure payer before verifying a sponsored shielded payment
packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs:86-88
The sponsored path now reserves the bundle compute fee plus the ordinary batch minimum, but that minimum is an admission threshold, not a bound on failed-event settlement. For one sub-transition it adds 100,000 credits. PV14's selected fee schedule charges 300,000 credits for a supported BLS identity signature alone; independently, the failed nonce-bump estimate inserts an eight-byte nonce whose payload costs 216,000 credits at the selected storage rate, before key and tree overhead. These costs are additional to the bundle compute fee retained in the execution context. When state validation replaces the document action with BumpIdentityDataContractNonce, event construction removes sponsorship, and settlement compares the signer against the complete metered estimate. A signer passing this floor can therefore still receive an unpaid failure without applying the nonce bump. Establish complete fallback affordability before bundle verification using the accumulated validation costs and the failure-operation estimate, or a demonstrably sufficient versioned bound. The new boundary test only checks acceptance by the floor; add processing-path assertions for fee collection and nonce advancement at that admitted boundary.
source: gpt-6.1-sol (phase2-reviewer: general, rust-quality)
🟡 Suggestion: Pin the downgraded token proof guarantees with regression assertions
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:3153-3177
The snapshot-only classifier table covers the three identity-less token transitions, but not the new batch pool classifications or documents carrying shielded payments. The shield integration test checks outcome.result() and the owner balance without checking the guarantee, while the grouped mint test explicitly checks the stronger classification. Reverting an ungrouped transfer, burn, or shielded document payment to ExecutionProved would leave those payload assertions passing. Add a classifier matrix or explicit is_execution_proved() assertions covering the batch pool families, grouped versus ungrouped mint and burn, and transparent versus shielded document payments. These checks can pin the trust contract without generating additional Orchard proofs.
source: gpt-6.1-sol (phase2-reviewer: general, rust-quality)
🟡 Suggestion: Token-pool type bytes duplicate StateTransitionType discriminants as magic numbers
packages/rs-dpp/src/shielded/sighash.rs:28-34
These constants independently repeat the explicit 26, 27, and 28 discriminants in StateTransitionType. Builders and validators share the constants, so their agreement and the frozen helper-preimage tests do not establish equality with the corresponding transition types. Derive the constants from their StateTransitionType variants, or add explicit equality assertions. This preserves the current authentication-domain bytes while eliminating an independently maintained mapping; the separate high-range outputs-only tags are intentionally not transition discriminants.
source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality)
🟡 Suggestion: Avoid deep-cloning the token configuration for every token pool spend
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs:65-69
resolve_pooled_token retains the contract through Arc but also deep-clones its TokenConfiguration, including nested configuration data. The transfer caller discards this copy, the unshield caller needs the outgoing-notes threshold, and the purchase caller reads the supply ceiling. Return the contract handle and resolve the configuration by token position at the caller, or return the small resolved fields actually needed. Either approach avoids per-transition copying without requiring a self-referential borrowed return value.
source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, rust-quality)
🟡 Suggestion: Reject oversized spend selections before generating proofs
packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:153-162
The identity-less builders start constructing and proving bundles without checking both caller-selected spend lists against SystemLimits::max_shielded_transition_actions. The shared proving helper does not enforce Platform's maximum, and try_from_bundles wraps the fields without running structural validation. Consequently, an oversized token or fee selection can consume proving work and return a transition that node-side structure validation necessarily rejects; an oversized fee selection is not caught before proving the token bundle. Preflight both predicted action counts, including output count and Orchard padding, before either proof, and check the built action count before create_proof. Apply the guard to transfer, unshield, and purchase, with boundary tests that reject before consulting the proving key.
source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, rust-quality)
🟡 Suggestion: Describe token pool proofs as affected-state evidence
book/src/data-model/token-shielded-pools.md:218-219
This paragraph says the three identity-less families prove their execution using nullifiers or balance snapshots, and the Queries section repeats the execution claim for batch pool operations. Drive deliberately classifies these observations as AffectedState: they authenticate spent status or resulting balances without identifying the complete submitted request. Describe the authenticated affected state and explicitly distinguish it from ExecutionProved. Document the stronger grouped mint/burn exception separately, where the recorded signer entry under the action-derived ID binds participation.
source: gemini-3.8-flash-high (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general)
🟡 Suggestion: Check the pool flag before accepting a threshold update
packages/wasm-dpp2/src/tokens/configuration/token_configuration.rs:130-134
The helper promises to reject a threshold on a token without a pool, but its V1 arm checks only the format variant. TokenConfigurationV1::from_v0(base, false) and deserialization can produce V1 configurations with has_shielded_pool=false, and DataContractWasm.tokens exposes those configurations to this setter. Such an object accepts minimumPoolNotesForOutgoing even though the equivalent V0 object is rejected. Check has_shielded_pool before accepting a non-empty threshold rather than treating V1 as proof that a pool exists. A shared DPP mutation helper could own this check, but the essential fix is to make the binding enforce the rule it advertises and test both V0 and V1(false).
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, intricate diff changes funds movement and consensus validation through new token pool transitions such as token_unshield_with_shielded_fee_transition/v0/state_transition_validation.rs, cryptographic signature binding in packages/rs-dpp/src/shielded/sighash.rs, and pool-inclusive supply conservation in packages/rs-dpp/src/balances/total_tokens_balance/mod.rs. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 31% left, 5h 52% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs:86-88: Fund the failure payer before verifying a sponsored shielded payment
The sponsored path now reserves the bundle compute fee plus the ordinary batch minimum, but that minimum is an admission threshold, not a bound on failed-event settlement. For one sub-transition it adds 100,000 credits. PV14's selected fee schedule charges 300,000 credits for a supported BLS identity signature alone; independently, the failed nonce-bump estimate inserts an eight-byte nonce whose payload costs 216,000 credits at the selected storage rate, before key and tree overhead. These costs are additional to the bundle compute fee retained in the execution context. When state validation replaces the document action with BumpIdentityDataContractNonce, event construction removes sponsorship, and settlement compares the signer against the complete metered estimate. A signer passing this floor can therefore still receive an unpaid failure without applying the nonce bump. Establish complete fallback affordability before bundle verification using the accumulated validation costs and the failure-operation estimate, or a demonstrably sufficient versioned bound. The new boundary test only checks acceptance by the floor; add processing-path assertions for fee collection and nonce advancement at that admitted boundary.
In `packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:3153-3177: Pin the downgraded token proof guarantees with regression assertions
The snapshot-only classifier table covers the three identity-less token transitions, but not the new batch pool classifications or documents carrying shielded payments. The shield integration test checks outcome.result() and the owner balance without checking the guarantee, while the grouped mint test explicitly checks the stronger classification. Reverting an ungrouped transfer, burn, or shielded document payment to ExecutionProved would leave those payload assertions passing. Add a classifier matrix or explicit is_execution_proved() assertions covering the batch pool families, grouped versus ungrouped mint and burn, and transparent versus shielded document payments. These checks can pin the trust contract without generating additional Orchard proofs.
In `packages/rs-dpp/src/shielded/sighash.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/sighash.rs:28-34: Token-pool type bytes duplicate StateTransitionType discriminants as magic numbers
These constants independently repeat the explicit 26, 27, and 28 discriminants in StateTransitionType. Builders and validators share the constants, so their agreement and the frozen helper-preimage tests do not establish equality with the corresponding transition types. Derive the constants from their StateTransitionType variants, or add explicit equality assertions. This preserves the current authentication-domain bytes while eliminating an independently maintained mapping; the separate high-range outputs-only tags are intentionally not transition discriminants.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/token_pool_paid_common/mod.rs:65-69: Avoid deep-cloning the token configuration for every token pool spend
resolve_pooled_token retains the contract through Arc<DataContractFetchInfo> but also deep-clones its TokenConfiguration, including nested configuration data. The transfer caller discards this copy, the unshield caller needs the outgoing-notes threshold, and the purchase caller reads the supply ceiling. Return the contract handle and resolve the configuration by token position at the caller, or return the small resolved fields actually needed. Either approach avoids per-transition copying without requiring a self-referential borrowed return value.
In `packages/rs-dpp/src/shielded/builder/token_pool_paid.rs`:
- [SUGGESTION] packages/rs-dpp/src/shielded/builder/token_pool_paid.rs:153-162: Reject oversized spend selections before generating proofs
The identity-less builders start constructing and proving bundles without checking both caller-selected spend lists against SystemLimits::max_shielded_transition_actions. The shared proving helper does not enforce Platform's maximum, and try_from_bundles wraps the fields without running structural validation. Consequently, an oversized token or fee selection can consume proving work and return a transition that node-side structure validation necessarily rejects; an oversized fee selection is not caught before proving the token bundle. Preflight both predicted action counts, including output count and Orchard padding, before either proof, and check the built action count before create_proof. Apply the guard to transfer, unshield, and purchase, with boundary tests that reject before consulting the proving key.
In `book/src/data-model/token-shielded-pools.md`:
- [SUGGESTION] book/src/data-model/token-shielded-pools.md:218-219: Describe token pool proofs as affected-state evidence
This paragraph says the three identity-less families prove their execution using nullifiers or balance snapshots, and the Queries section repeats the execution claim for batch pool operations. Drive deliberately classifies these observations as AffectedState: they authenticate spent status or resulting balances without identifying the complete submitted request. Describe the authenticated affected state and explicitly distinguish it from ExecutionProved. Document the stronger grouped mint/burn exception separately, where the recorded signer entry under the action-derived ID binds participation.
In `packages/wasm-dpp2/src/tokens/configuration/token_configuration.rs`:
- [SUGGESTION] packages/wasm-dpp2/src/tokens/configuration/token_configuration.rs:130-134: Check the pool flag before accepting a threshold update
The helper promises to reject a threshold on a token without a pool, but its V1 arm checks only the format variant. TokenConfigurationV1::from_v0(base, false) and deserialization can produce V1 configurations with has_shielded_pool=false, and DataContractWasm.tokens exposes those configurations to this setter. Such an object accepts minimumPoolNotesForOutgoing even though the equivalent V0 object is rejected. Check has_shielded_pool before accepting a non-empty threshold rather than treating V1 as proof that a pool exists. A shared DPP mutation helper could own this check, but the essential fix is to make the binding enforce the rule it advertises and test both V0 and V1(false).
Out-of-scope follow-up suggestions (2)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Audit legacy credit-pool execution-proof classifications — Outside this PR's introduced behavior. The legacy credit-pool classifications are deliberately preserved for released versions; a broader historical proof-contract audit is not required to validate the new token-pool classifications.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
- Proof binding verification improvements for legacy credit pool families — Outside this PR's introduced behavior and already described as separately tracked work. Do not expand this token-pool review into a redesign of released credit-pool and document proof guarantees.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
|
/self-reviewed |
Issue being fixed or feature implemented
Tokens have no privacy: every balance and transfer is public. The credit shielded pool cannot hold tokens because the Orchard construction Platform uses has no asset base (a note carries a value, not an asset id), so one pool cannot keep token A and token B apart.
What was done?
Protocol version 14 lets a token own its own shielded pool, laid out exactly like the credit pool and rooted under the token. Seven batch token transitions move tokens in, out and inside it, document costs can be paid out of it, and three identity-less state transitions move tokens with the fee paid from the credit shielded pool, so no identity appears anywhere. The identity-paid transitions stay; the identity-less ones are additions.
This PR targets v4.2-dev and lands inside the unreleased protocol version 14: it adds no version scaffolding of its own but amends
v14.rsand the tables that version selects (DRIVE_VERSION_V9,DRIVE_ABCI_METHOD_VERSIONS_V10,DRIVE_ABCI_VALIDATION_VERSIONS_V10,DPP_VALIDATION_VERSIONS_V5,CONTRACT_VERSIONS_V6).TOKEN_SHIELDED_POOL_INITIAL_PROTOCOL_VERSIONis 14.Configuration
TokenConfiguration::V1(format version 1) addshasShieldedPool.CONTRACT_VERSIONS_V6admits format versions 0 and 1 (token_configuration_formatbounds); earlier versions admit only 0, and contract create/update reject anything else withUnsupportedVersionError. The flag is immutable on update.freezeRules,unfreezeRulesanddestroyFrozenFundsRules(no action takers and no admins); contract create and update reject anything else withTokenShieldedPoolIncompatibleRulesError(10278). Pause still applies to every pool operation; a pooled token can never freeze an account, so the pool validators read and bill no frozen-account check.transition_to_version_14and bycreate_initial_state_structurev4 (a genesis-versus-upgrade equivalence test pins the two paths to the same bytes).Storage (rs-drive)
[Tokens, TOKEN_SHIELDED_POOLS_KEY(224), token_id](SumTree under a BigSumTree) with the credit pool's five children. The credit pool primitives were made pool-agnostic and given token twins, so notes, nullifiers, balances and anchors share one implementation.TokenOperationTypelowering for them.calculate_total_tokens_balancev1 adds the pool balances to the conservation check: identity balances + pool balances == total supply.verify_*twins for the six shielded proofs and execution-proof queries for every new transition.Batch transitions (rs-dpp, rs-drive-abci), identity-signed and paid in credits
TokenShield,TokenUnshield,TokenShieldedTransfer(value balance-amount,+amount,0).TokenMintToPoolandTokenBurnFromPoolfollow the manual minting and burning rules (a mint into the pool is refused where the configuration forbids the minter to choose the destination of minted tokens, since the notes' recipients are the minter's choice); a group action stores a digest of the notes (serialized_actions_digest) so every signer commits to the same bundle.TokenClaimToPoolreleases a distribution into a note (a perpetual claim must name the cycle-aligned moment it claims up to, resolved by the sharedresolve_token_claim).TokenDirectPurchaseToPoolpays credits at the direct purchase price for tokens minted into the pool.token_id || owner_id || recipient_id || amount, a shielded transfertoken_id || owner_id, a burntoken_id || burner_id || amountwhere the burner is the batch owner or, for a group action, its proposer (the group action pins the digest of the signed actions, so every signer submits the proposer's bundle and the sighash cannot depend on whose batch carries it); an outputs-only bundle binds a one-byte kind tag and the token id. It carries no anchor of its own — every token pool starts from the same empty-tree anchor — and the identity signature only binds it inside its own batch, so without that data the authorized bytes verify against any pool and any of those kinds. The tag alone cannot stop the same authorized bundle landing twice where the preimage still matches — the sender resubmitting, an SDK retrying with a fresh nonce, a copier who satisfies the binding — so the two writers that add to a token pool now also record each note's dummy nullifier, and a bundle whose nullifier is already recorded is refused. All five pool-entry paths lower into those two writers, including the identity-lessTokenPurchaseFromShieldedPool, which reaches the pool through the mint writer rather than a batch transition of its own. The intra-bundle duplicate check runs first on every path: once these nullifiers are written it is also the only thing keeping two identical inserts out of one GroveDB batch, which a node running the shipped batching default would fold silently while a verifying node refused. The proof verification fee is charged by the action transformers, so CheckTx admission and block execution price a bundle identically and an invalid proof is a paid failure with a nonce bump.ShieldedProofAdmissionKey), and below protocol version 14 the batch is rejected withStateTransitionNotActiveError.The credit pool's outputs-only bundles get a layout too
Shield,ShieldFromIdentityandShieldFromAssetLockcreate notes and spend nothing, so like the token-pool kinds above they carried no layout of their own and nothing in the bundle named the transition carrying it. Each now binds a kind tag and what funds it: a hash overShield's input addresses in canonical order with nonces and amounts excluded, the identity id forShieldFromIdentity, and forShieldFromAssetLockthe asset lock identifier derived from the full outpoint — the txid alone would not do, since multi-output asset locks exist.Shieldis verified only invalidate_shielded_proof, whose generation 1 no shipped table selects.ShieldFromIdentityis verified twice, and its transformer edit is in place: inert below protocol version 14 both because the transition is refused there and because the preimage method isNonein every table those versions use, rebuilding a byte-identical empty preimage.ShieldFromAssetLockis admitted from protocol version 12, so it gets a new generation and a new transition version; at 14 only version 1 is accepted and a version 0 is refused on its version byte during decode, before any proof work, with no penalty and the asset lock left unspent.Minimum pool notes, per token
TokenConfigurationV1rather than one constant for every token, with the cap inSystemLimits. The default is 0: a non-zero floor traps the first depositors, and no count of commitments guarantees distinct holders anyway, since one bundle carries up to sixteen actions. The cap of 250 is the real protection against stranding a pool.TokenConfigUpdateunder the token's ownChangeControlRules, so no new DataContractvalidate_updategeneration is needed — a contract update that tries to change it is refused by the rule that already refuses every other token config edit. Change control rules default to permitting no one, so an issuer who never opens them has a threshold nothing can move.TokenConfigurationV1outside its V0 base needed real V1 arms in the apply, permission and action-taker dispatchers, which previously delegated everything to the base. Those arms are exhaustive — a macro names all thirty-two V0 items — so a future item fails to compile there too rather than becoming a silent no-op that charges a fee and changes nothing.TokenUnshieldWithShieldedFeeis closed as well: it is the only identity-less outflow and it checked the threshold zero times.Documents paid from the pool
TokenPaymentInfo::V1carries aTokenShieldedPayment(a spend bundle in the payment token's pool whose value balance is the document action's token cost; boxed so the payment info stays small). The base action carries it, the transformer checks the amount against the document type's cost (TokenShieldedPaymentAmountMismatchError40724,TokenShieldedPaymentNotRequiredError40725), document base state validation v1 skips the owner's balance checks, the pool side and the proof (bound to token, owner, contract, document and amount) are validated after the document action, and the lowering pays the cost from the pool into the contract owner's balance or out of the supply.Identity-less transitions (types 26, 27, 28)
TokenShieldedTransferWithShieldedFee,TokenUnshieldWithShieldedFeeandTokenPurchaseFromShieldedPoolcarry a bundle in the token's pool and a fee bundle in the credit pool, both authorized only by spend keys. The token bundle binds the type byte, the token id and the transparent fields; the fee bundle binds the type byte, the token id and a digest of the token bundle's actions, so the two cannot be re-paired.credit_amountto the two-bundle fee (compute_token_pool_paid_shielded_fee: the base fee of each bundle plus the flat storage written outside the pools; a purchase adds the agreed price). Execution is aPaidFromShieldedPoolevent; uniqueness is by the spent nullifiers of both bundles; a purchase credits the contract owner and respects the pricing schedule and the max supply.Block end: every touched pool (batch transitions, documents paid from a pool, identity-less transitions) is collected in the processing result and
record_token_shielded_pool_anchorsrecords its anchor and prunes old ones, always keeping the newest.Queries and clients
token_id; handlers route through aShieldedPoolSelector, the proof verifier routes on the same field, and the Rust SDK addsTokenShieldedPoolQuery,TokenShieldedEncryptedNotesQuery,TokenShieldedNullifiersQuery.build_token_*_transition,build_document_shielded_token_payment, and the identity-less builders taking aTokenPoolSpenderand aShieldedFeePayer).TokenPaymentInfo.shieldedPaymentwith aTokenShieldedPaymentwrapper,hasShieldedPoolandformatVersiononTokenConfiguration,TokenEventVariantentries; wasm-dpp arms and error mapping.Not in this PR: the generated dapi-grpc JS, Python and Objective-C clients were not regenerated (the Docker-based protoc image was unavailable on the build machine); the proto change is a single optional field on six requests plus two group-action event messages, so a regeneration run can follow separately.
Docs: book chapter "Token Shielded Pools" (batch transitions, documents paid from the pool, identity-less transitions), the token sections of the shielded fees chapter, and the v14 module doc.
Review notes (2026-09-22)
StateErrorvariants ahead of the ones the base added, silently moving those other errors down the wire:StateErroris bincode-encoded positionally. The enum itself merges without a conflict every time; only the frozen-discriminant test conflicts, and that test is the sole reason it was caught. The rule on every merge is that the base keeps its positions and this branch's variants move to the tail. Current positions: 153 to 155 inStateError(the base took 151 and 152 for its own version 14 errors), 200 inBasicError, codes 40723 to 40725 and 10278. The identity-less transitions are types 26 to 28.minimum_token_pool_notes_for_outgoingis 0 (the credit pool requires 250) so a small token's pool is usable from its first note, at the price that a spend from a nearly empty pool is linkable to the shield that filled it. Documented in the book; the constant can be raised by a later protocol version if the floor is wanted.TokenDirectPurchaseToPoolprice now counts toward the batch's required balance (all_purchases_amount), so an under-funded buyer is a consensus rejection instead of an execution error.Fixes from a review of this branch's own diff
A multi-agent review of the incremental diff produced 33 findings. Fixed here:
two failed with an internal error and logged at
error!on every unauthenticated request, and theother four answered with an empty pool —
nullifiersamong them reporting a note as unspent, whichis the answer a wallet uses to decide a note is spendable. All six now refuse with
NotFound. Theproved path is deliberately asymmetric: it answers with a verifiable absence proof, which is worth
more to a proving client than a
NotFoundit must trust, and a test pins that asymmetry.allow_saturation: falsebut checked only an explicitly configured max supply. With none set, anoverflowing amount reached Drive and returned
CorruptedCodeExecution: reported as an internalerror, stripped from the block, and the submitter paid nothing, so it could be resubmitted forever.
The supply lives in a sum item, so
i64::MAXbounds it either way; all four pool mints now derivethat ceiling and return
TokenMintPastMaxSupplyError.configuration asking for a shielded pool could decode as a token that can never have one.
TokenConfigurationV0now refuses unknown keys, asContractModerationConfigalready does for thesame reason: what a contract is created with cannot be changed afterwards. The version envelope
stays permissive so a future generation can still be dispatched on.
The
_ => Nonethat hid it is gone; every variant is listed, so a new one is a compile error.inserting one for a first-time holder costs up to 223. The shortfall came out of the proposer's
reward. The insert is now priced unconditionally: the fee check is structurally stateless, and the
transition publishes its exact minimum, so a state-dependent price would reveal whether the
recipient holds the token for the first time.
a pool's total, charged to nobody. Now priced in both modes through the stateless query, which
models the read without touching state, so the operation whose visible amount is its fee still
cannot leak the total.
action, and the group entry is written on every signature while the pool write waits for the action
to close. The verifier's pool branches ignored that, so an open burn-from-pool proposal's proof was
rejected for not showing nullifiers nothing had spent yet. Both branches now follow the six older
token branches' pattern. The classifier also understated a grouped mint to pool as unproved.
JSON input did not parse; the parser was the repository's only raw-byte reader on the base58 side.
TokenEventfield names the TypeScript doc promised are not what the serializer emits, soa JS consumer reading them got
undefined. The new test reads the promised names out of the docblock and compares them against a serialized instance of all eighteen variants, so it fails on a
doc-only drift as well as on a rename.
Examined and found not to be defects: the end-of-block anchor pruning is already bounded by the
retention window, and
BurnFromPool's unconditional execution classification is correct because thesigner's entry sits under the action id derived from the transition.
How Has This Been Tested?
drive-abci integration tests with real Orchard proofs, every sighash-binding batch transition also passed through CheckTx (including both signers of the group burn): shield, unshield, shielded transfer and a replay (nullifier already spent), pool not enabled, balance too low, paused token, invalid proof as a paid failure with nonce bump, unknown anchor, protocol version 14 rejection of every new transition and of a version 1 configuration, contract create creating the pool and rejecting a pooled token with freeze rules; mint to pool by the owner, past max supply, by an unauthorized identity, with a fixed destination; burn from pool after a shield and its replay, a burn from the pool by a group of two with a substituted bundle rejected; a pre-programmed claim into the pool; a direct purchase into the pool with underpayment rejected; a document created with a shielded payment burning the cost and paying it to the contract owner, an amount mismatch, a replay; the three identity-less transitions with their fee accounting, a replay and an underpaid purchase; token conservation asserted after every successful step.
The genesis-v14 versus upgrade-to-v14 equivalence of the token pools root.
drive unit tests for pool creation, the composite operations, estimation, and anchor record/prune.
dpp tests for structure validation, JSON and Value round trips of every new transition and of
TokenPaymentInfo::V1, the frozen sighash layouts, the state error discriminants, the builders, the fee formulas; query selector tests, proof verifier tests.Every fix above landed with a test that fails on the unfixed code and passes after, with the
failing text quoted in its commit. The frozen-discriminant test for
StateErrorwas additionallychecked to bind rather than merely pass: flipping its last expected value reports
left: 155, right: 154.Breaking Changes
Protocol version 14 is unreleased, so nothing released changes behaviour.
TokenConfigurationV1's bincode layout changes in place to carry the minimum-notes threshold. Anynetwork holding contracts with pooled tokens must be reset rather than upgraded — stored
configurations written before this change will not decode.
A token configuration carrying a key its
$formatVersiondoes not define is now refused instead ofsilently decoding without it, at format version 1 as well as 0. Version 1 needed the refusal of its
own: it takes version 0's fields through
serde(flatten), and an unrecognized key arriving at theouter struct is never offered to the flattened one, so version 0's refusal could not reach the only
version that carries the pool fields. Two in-repo contract fixtures carried
directPurchaseRules, akey that names no field anywhere in this repository; it had been dropped on every parse since it was
written.
A token unshield's minimum fee rises by 6,082,800 credits, 2.66% of a minimal unshield. The
over-collection is not burned: it is routed to the block proposer.
TokenPaymentInfono longer implementsCopy, which is a source break for SDK and FFI consumers.In-place changes to shipped generations
Protocol versions 1–13 are released; 14 is not. Each generation below is selected by at least one
released version, so each edit carries a comment at the edited lines naming why consensus cannot
move there. All 68 modified versioned modules were audited; none can change consensus at a released
version.
The gate most arguments rest on:
validate_is_allowed(processor/traits/is_allowed.rs) refuses —unpaid, as the first check in
process_state_transition_v0, before signature, nonce or fee — everytoken pool transition kind, the pool-threshold
ConfigUpdateitems, a document whose token paymentinfo carries a shielded payment, and the three new top-level state transitions, below
TOKEN_SHIELDED_POOL_INITIAL_PROTOCOL_VERSION.Unreachable at released versions via that gate
rs-drive-abci/.../batch/state/v0/mod.rs(PV1–13) — new token pool arms and the documentshielded-payment validation; both refused before this stage runs.
rs-drive-abci/.../batch/transformer/v0/mod.rs(PV1–13) — new token pool arms and the shieldedverification fee charge. This file is the shared body of
transform_into_action0, 1 and 2, soPV14 reaches the arms as its live path.
rs-drive-abci/.../process_raw_state_transitions/v0/mod.rs(PV1–13) — the touched-pool set isempty because the kinds it reports are the kinds the gate refuses; the set is in-memory only,
never serialized or hashed.
rs-dpp/.../token_configuration/methods/{apply,can_apply,authorized_action_takers_for}_*_configuration_item/v0(PV1–13) — the three pool-threshold items are appended at the tail of
TokenConfigurationChangeItem, and neither match has a catch-all.rs-dpp/.../batch_transition/validation/validate_basic_structure/v0(PV1–13) and.../token_config_update_transition/validate_structure/v0(PV1–13) — both strictly downstream ofthe gate.
rs-drive/.../document_base_transition_action/v0/{mod,transformer}.rs— the action is in-memoryonly (never encoded, hashed or stored) and the field is
Nonefor every document a releasedversion accepts.
rs-drive/src/state_transition_action/batch/v0/mod.rs— a newfilter_maparm the catch-all usedto swallow; the action cannot be in a batch there.
rs-drive-abci/src/query/group_queries/group_actions/v0(PV1–13) — pool token events are appendedat the tail of
TokenEvent, so no stored event's discriminant moves, and the transitions thatrecord them are refused.
Inert because the method's feature version is
Nonetherers-drive-abci/.../run_block_proposal/v0/mod.rs(PV1–13) —record_token_shielded_pool_anchorsis
Nonein every shippedDRIVE_ABCI_METHOD_VERSIONS_V*; its dispatcher returnsOk(())untouched.
Guarded by the upgrade condition
rs-drive-abci/.../perform_events_on_first_block_of_protocol_change/v0/mod.rs(PV4–13) — theadded insert is inside
transition_to_version_14, reached only underplatform_version.protocol_version >= 14.Output-identical
rs-drive/src/drive/shielded/{has_anchor,has_nullifier,insert_note,insert_nullifiers,notes_count,read_total_balance,record_anchor_if_changed,update_total_balance}/v0and
rs-drive/src/verify/shielded/*/v0(all released versions) — body split behind a pathparameter; the credit entry point passes the path the body used to build inline, so the same
operations in the same order and count, the same query types, the same errors, and the same proofs.
rs-drive-abci/src/query/shielded/*/v0(PV1–13) —token_idis absent in any request a releasedversion can make,
from_requestmaps that to the credit pool without reading the version, andvalidate_pool_existsis a no-op for it.rs-drive/.../token_claim_transition_action/v0/{mod,transformer}.rsand.../token_direct_purchase_transition_action/v0/{mod,transformer}.rs— bodies extracted forreuse; reads, fee accumulations and early returns keep their order and each rejection carries the
same single error.
claim_up_to = Nonerebinds the current interval to itself.rs-drive/src/prove/prove_state_transition/v0/mod.rs(PV1–13) — an accessor replaces adestructure that stopped compiling when the enum gained a variant; same field, same query. This
file is the shared body of both prove generations.
rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs(PV1–13) — likewise the shared body of both verify generations. The token-history document type
moved inside the closure, which is inert because every kind a released version admits resolves a
document type in both schema versions of the untouched history contract.
rs-drive/.../shielded/shield_from_asset_lock/v0/transformer.rs(PV12–13) — same accessor change;those versions admit version 0 alone.
rs-dpp/.../shield_from_asset_lock_transition/v0/state_transition_validation.rs(PV12–13) — pureextraction; six checks keep their order and their messages.
rs-dpp/.../validate_token_configuration_groups_exist/v1(PV13) — reading rules through the enumdelegates straight to the V0 list for a V0 configuration, so same rules, same order, same first
violation.
rs-dpp/src/document/extended_document/v0/mod.rs— borrow-and-clone forced by the payment infotype losing
Copy; same value, same object built.rs-drive/src/drive/tokens/calculate_total_tokens_balance/v0(PV1–13) — reports a pool total of 0,making the new sum arithmetically identical.
rs-dpp/.../document_base_transition/v1/v1_methods.rs(PV9–13) — the token payment info is readthrough a borrow; same value.
Corrections to comments that named a gate which does not hold
The audit found two justifications this branch had written that were themselves wrong. Neither
changed a verdict, but a wrong proof is worse than no proof, and both are corrected in place.
batch/transformer/v0said protocol versions 1–11 select it and that version 14 does not select itat all, so the new arms were only there to keep the match exhaustive. All three claims were wrong:
try_into_action_v0is the shared body oftransform_into_action0, 1 and 2, so the released spanis 1–13 and version 14 runs those arms as its live path. As written the comment told the next
reader that deleting them was safe.
batch/state/v0justified the document shielded payment by anactive_version_rangeonTokenPaymentInfo::V1. No such range exists.StateTransition::active_version_range'sBatcharmnarrows to
14..=on the document base's own feature version, so a V1 payment info on a version 1base is not refused at decode.
validate_is_allowedis the whole argument.Checklist
🤖 Generated with Claude Code
PR Hygiene ·
cb7386bdpp— you own itrs-drive-abci— you own itrs-drive— you own itrs-platform-wallet(packages/rs-platform-wallet/src/wallet/shielded/fund_from_asset_lock.rs,packages/rs-platform-wallet/src/wallet/shielded/operations.rs,packages/rs-platform-wallet/src/wallet/shielded/store.rs) — HashEngineering or ZocoLini or llbartekll or romchornyirust-sdk(packages/rs-sdk/src/platform/documents/transitions/create.rs,packages/rs-sdk/src/platform/documents/transitions/delete.rs,packages/rs-sdk/src/platform/documents/transitions/purchase.rsand 5 more) — lklimekWhen every box is checked the
PR Hygienecheck passes and this can merge.