Skip to content

feat(platform)!: token shielded pools - #4760

Open
QuantumExplorer wants to merge 14 commits into
v4.3-devfrom
claude/tokens-shielded-pools-6642a6
Open

QuantumExplorer wants to merge 14 commits into
v4.3-devfrom
claude/tokens-shielded-pools-6642a6

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Sep 15, 2026

Copy link
Copy Markdown
Member

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 15 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 now targets v4.3-dev and carries its own PV15 scaffolding: v15.rs, DRIVE_VERSION_V10, DRIVE_ABCI_METHOD_VERSIONS_V11, DRIVE_ABCI_VALIDATION_VERSIONS_V11 and CONTRACT_VERSIONS_V7; the v14 tables keep their released values. It will conflict with #4730, which adds the same scaffolding for its own feature; whichever merges second rebases.

Configuration

  • TokenConfiguration::V1 (format version 1) adds hasShieldedPool. CONTRACT_VERSIONS_V7 admits format versions 0 and 1 (token_configuration_format bounds); earlier versions admit only 0, and contract create/update reject anything else with UnsupportedVersionError. The flag is immutable on update.
  • A pool makes freezing and confiscation unenforceable (shielded notes belong to no identity account), so a token with the flag must permanently disable freezeRules, unfreezeRules and destroyFrozenFundsRules (no action takers and no admins); contract create and update reject anything else with TokenShieldedPoolIncompatibleRulesError (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.
  • Inserting or updating a contract with the flag creates the pool trees; the token pools root tree is created by transition_to_version_15 and by create_initial_state_structure v4 (a genesis-versus-upgrade equivalence test pins the two paths to the same bytes).

Storage (rs-drive)

  • Pools live at [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.
  • Composite operations for shield, unshield, shielded transfer, mint to pool and burn from pool, and the TokenOperationType lowering for them. calculate_total_tokens_balance v1 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).
  • TokenMintToPool and TokenBurnFromPool follow 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. TokenClaimToPool releases a distribution into a note (a perpetual claim must name the cycle-aligned moment it claims up to, resolved by the shared resolve_token_claim). TokenDirectPurchaseToPool pays credits at the direct purchase price for tokens minted into the pool.
  • Sighash binding: an unshield binds token_id || owner_id || recipient_id || amount, a shielded transfer token_id || owner_id, a burn token_id || burner_id || amount where 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); outputs-only bundles bind nothing extra since the identity signature covers the batch. 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.
  • CheckTx proof admission is keyed by the identity contract nonce (ShieldedProofAdmissionKey), and below protocol version 15 the batch is rejected with StateTransitionNotActiveError.

Documents paid from the pool

  • TokenPaymentInfo::V1 carries a TokenShieldedPayment (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 (TokenShieldedPaymentAmountMismatchError 40724, TokenShieldedPaymentNotRequiredError 40725), 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, TokenUnshieldWithShieldedFee and TokenPurchaseFromShieldedPool carry 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.
  • The minimum-fee validation pins credit_amount to 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 a PaidFromShieldedPool event; 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_anchors records its anchor and prunes old ones, always keeping the newest.

Queries and clients

  • The six shielded pool requests take an optional token_id; handlers route through a ShieldedPoolSelector, the proof verifier routes on the same field, and the Rust SDK adds TokenShieldedPoolQuery, TokenShieldedEncryptedNotesQuery, TokenShieldedNullifiersQuery.
  • DPP builders for every transition (build_token_*_transition, build_document_shielded_token_payment, and the identity-less builders taking a TokenPoolSpender and a ShieldedFeePayer).
  • wasm-dpp2 wrappers for every transition, TokenPaymentInfo.shieldedPayment with a TokenShieldedPayment wrapper, hasShieldedPool and formatVersion on TokenConfiguration, TokenEventVariant entries; 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 v15 module doc.

Review notes (2026-09-22)

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-v15 versus upgrade-to-v15 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.

Breaking Changes

None for released protocol versions. The new configuration format, trees, transitions and queries are gated on protocol version 15; TokenPaymentInfo is no longer Copy.

Checklist

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

🤖 Generated with Claude Code

PR Hygiene · d1123cf

  • Bots — coderabbitai not yet · thepastaclaw not yet — /skip-bots proceeds without the ones not yet reported
  • Self-review — post /self-reviewed once the bots are done
  • Within your 5 open PRs
  • Build failed
  • Approvals
    • files with no dedicated owner — you own it
    • dpp — you own it
    • rs-drive-abci — you own it
    • rs-drive — you own it
    • rust-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.rs and 4 more) — lklimek or shumkov

When every box is checked the PR Hygiene check passes and this can merge.

@github-actions github-actions Bot added this to the v4.2.0 milestone Sep 15, 2026
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: dashpay/platform/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 14a0e31e-390c-4b68-a090-abbcd9da9543

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

⚠️ DEGRADED — Queued for automated review — 8th in line, estimated start in ~4 h (commit d1123cf)
Estimated review time once started: ~1.2 h (two-phase automated review; median of recent runs).
The primary review models are currently out of quota; this review will run on stand-in models and be marked as degraded.

  • Request priority review — click to move this review to the front of the queue.

@QuantumExplorer
QuantumExplorer force-pushed the claude/tokens-shielded-pools-6642a6 branch from a58dc2c to 13f4c55 Compare September 15, 2026 04:09
@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

📖 Book Preview built successfully.

Download the preview from the workflow artifacts.
To view locally: download the artifact, unzip, and open index.html.

Updated at 2026-09-21T17:03:12.884Z

@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.86985% with 2289 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (v4.3-dev@d3de958). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...tate_transition/processor/traits/shielded_proof.rs 35.07% 274 Missing ⚠️
packages/rs-dpp/src/tokens/token_event.rs 1.78% 165 Missing ⚠️
..._transition/batched_transition/token_transition.rs 18.54% 123 Missing ⚠️
packages/rs-dpp/src/shielded/sighash.rs 73.27% 93 Missing ⚠️
...pp/src/shielded/builder/token_shielded_transfer.rs 50.00% 70 Missing ⚠️
packages/rs-dpp/src/state_transition/mod.rs 26.37% 67 Missing ⚠️
...s-dpp/src/shielded/builder/token_burn_from_pool.rs 67.77% 58 Missing ⚠️
...ate_transition/processor/traits/basic_structure.rs 35.95% 57 Missing ⚠️
...ges/rs-dpp/src/shielded/builder/token_pool_paid.rs 87.88% 55 Missing ⚠️
...ages/rs-dpp/src/shielded/builder/token_unshield.rs 64.96% 55 Missing ⚠️
... and 98 more
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           
Components Coverage Δ
dpp 69.55% <0.00%> (?)
drive 75.81% <0.00%> (?)
drive-abci 73.49% <0.00%> (?)
sdk ∅ <0.00%> (?)
dapi-client ∅ <0.00%> (?)
platform-version ∅ <0.00%> (?)
platform-value 85.68% <0.00%> (?)
platform-wallet ∅ <0.00%> (?)
drive-proof-verifier 29.03% <0.00%> (?)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@QuantumExplorer QuantumExplorer left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +290 to +296
if matches!(
execution_result,
StateTransitionExecutionResult::SuccessfulExecution { .. }
| StateTransitionExecutionResult::PaidConsensusError { .. }
) {
processing_result
.add_token_shielded_pools_touched(token_shielded_pools_touched);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +268 to +272
let verification_fee = compute_shielded_verification_fee(actions.len(), platform_version)?;
execution_context.add_operation(ValidationOperation::PrecalculatedOperation(FeeResult {
processing_fee: verification_fee,
..Default::default()
}));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: critical by gpt-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); agent phase1-reviewer, muse-spark-1.3-contributor — architecture-layering (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — ffi-engineer (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — platform-versioning (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity 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; agent astra-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.

Comment thread packages/rs-dpp/src/shielded/builder/token_shield.rs Outdated
Comment thread packages/rs-dpp/src/shielded/builder/token_unshield.rs Outdated
@thephez thephez added the dapi-endpoint DAPI endpoint addition or modification label Sep 15, 2026
@QuantumExplorer
QuantumExplorer changed the base branch from v4.2-dev to v4.3-dev September 15, 2026 19:14
@github-actions github-actions Bot modified the milestones: v4.2.0, v4.3.0 Sep 15, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 2 only (queue backlog)

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: critical by gpt-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; agent astra-verifier
  • Phase 2 reviewers: gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — platform-versioning (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `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.

@QuantumExplorer

Copy link
Copy Markdown
Member Author

Review round 2 (review 5228577296) addressed in e305b85:

  • Group burns bound the bundle to each confirmer (blocking): confirmed. The TokenBurnFromPool sighash now binds the burner rather than the batch owner: the batch owner for a direct burn, the stored group action's proposer for every other signer (token_burn_from_pool_extra_sighash_data(token_id, burner_id, amount)). The group action id cannot serve, since it hashes the digest of the signed actions (circular with the sighash). The builder refuses to prove a fresh bundle for GroupStateTransitionInfoOtherSigner, because the digest check would reject it anyway. New drive-abci test test_token_burn_from_pool_by_group_of_two runs the proposer plus confirmer flow with real proofs, checks nothing leaves the pool after the first signature, rejects a substituted bundle with ModificationOfGroupActionMainParametersNotPermittedError, and asserts the burn and conservation after the second.
  • Purchase builder panics on token_count == 1 << 63 (blocking): confirmed. build_token_purchase_from_shielded_pool_transition rejects a count above i64::MAX right after the zero check, before any proving; unit test rejects_a_purchase_count_outside_the_value_balance_range. Consensus was already safe (MAX_DISTRIBUTION_PARAM bound in the structure validation).
  • Purchase builder proves a throwaway bundle (suggestion): confirmed. The outputs-only bundle is now built and proven once over the purchase sighash; the unused build_output_only_bundle call is gone.

Book chapter and PR body updated for the burner binding.

🤖 Addressed by Claude Code

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ DEGRADED — Re-review — Preliminary review — Phase 1 blocker gate

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

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-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-ins gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor; Phase 1 effort capped at high
  • Triage: critical by muse-spark-1.3-contributor (standing in for gpt-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); agent phase1-reviewer, gemini-3.8-flash-high — architecture-layering (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — platform-versioning (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — rust-quality (completed, effort high); agent phase1-reviewer, gemini-3.8-flash-high — security-auditor (completed, effort high); agent phase1-reviewer
  • Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 100% left, 5h 100% left
  • Fresh verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) — verifier; agent astra-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).

@github-actions

github-actions Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

PR Hygiene: the checklist is in the description.

@github-actions github-actions Bot added bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. waiting-bots Waiting for the review bots to report on this head and removed bot-review-skipped A required review bot did not report; it was skipped by the window or by a person. labels Sep 20, 2026
QuantumExplorer and others added 8 commits September 21, 2026 08:04
A token can own its own Orchard shielded pool from protocol version 14.
`TokenConfiguration::V1` adds `hasShieldedPool`; contracts with the flag get
a pool at `[Tokens, TOKEN_SHIELDED_POOLS_KEY, token_id]` laid out like the
credit pool, and three batch token transitions (TokenShield, TokenUnshield,
TokenShieldedTransfer) move tokens into, out of and inside it. The identity
signs and pays the fee in credits; the token id, owner id and, for an
unshield, recipient and amount are bound into the Orchard sighash; pool
balances are a term of the token conservation check; touched pools have
their anchors recorded and pruned at block end. The six shielded queries take
an optional token_id, the proof verifier and SDK route on it, and DPP
builders plus wasm bindings expose the new transitions.

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

StateError is encoded by variant position, so the new
TokenShieldedPoolNotEnabledError must be the last variant rather than sit in
the token block, or every later variant's wire discriminant shifts; the
frozen-discriminant test now pins it at 101.

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

Shielded notes belong to no identity account, so freezing and destroying
frozen funds cannot reach them; a holder who expects a freeze simply shields
first. A token with hasShieldedPool must therefore set freezeRules,
unfreezeRules and destroyFrozenFundsRules to no action takers and no admins,
so no later configuration update can enable them. Contract create and update
reject anything else with TokenShieldedPoolIncompatibleRulesError (10277).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Four more batch token transitions move tokens straight into or out of a
token's Orchard pool while an identity still signs and pays credits:
TokenMintToPool and TokenBurnFromPool follow the manual minting and
burning rules (group actions store a digest of the notes so every signer
commits to the same bundle), TokenClaimToPool releases a distribution
into a note (a perpetual claim names the cycle-aligned moment it claims
up to so the amount is provable), and TokenDirectPurchaseToPool pays
credits for tokens delivered shielded. Outputs-only bundles bind nothing
extra; a burn binds token id, owner id and amount into its sighash. The
shielded verification fee is charged by the action transformers so
CheckTx admission and block execution price a bundle identically.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… pay document token costs from a pool

The PR now targets v4.3-dev, so the feature moves from protocol version 14
to 15: v15.rs is added with DRIVE_VERSION_V10 (genesis structure v4 and the
token pool method versions), DRIVE_ABCI_METHOD_VERSIONS_V11 (anchor
recording), DRIVE_ABCI_VALIDATION_VERSIONS_V11 and CONTRACT_VERSIONS_V7
(token configuration format 1); the v14 tables return to their released
values and the pools root is inserted by transition_to_version_15, with a
genesis-versus-upgrade equivalence test.

TokenPaymentInfo gains a format version 1 carrying a TokenShieldedPayment:
an Orchard spend bundle in the payment token's pool whose value balance is
the document action's token cost. The document base action carries it, the
transformer checks the amount against the document type's cost, state
validation v1 skips the owner's balance checks and validates the pool side
(pool exists, token not paused, anchor, unspent nullifiers, pool balance,
proof bound to token, owner, contract, document and amount), and the
lowering pays the cost out of the pool: to the contract owner's balance or
out of the supply. CheckTx admits the bundle under the identity contract
nonce like the batch token pool transitions.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…nfo in the SDK

TokenPaymentInfo lost Copy when format version 1 gained a bundle: the SDK
document builders now clone it, TokenShieldedPayment gets the JSON and
Value conversions the wasm wrapper macro expects, and the payment is boxed
inside the payment info and the document base action so the enums holding
them keep their size (clippy large_enum_variant).

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

Three top-level state transitions move tokens through a token's shielded
pool with no identity anywhere: each carries an Orchard bundle in the
token pool and a second spend bundle in the credit shielded pool that pays
the fee, both authorized only by spend keys.

TokenShieldedTransferWithShieldedFee (23) transfers inside the token pool
(value balance zero), TokenUnshieldWithShieldedFee (24) moves an amount
into an identity's token balance, and TokenPurchaseFromShieldedPool (25)
buys tokens at the direct purchase price out of the credit pool and mints
them into the token pool, crediting the contract owner. The token bundle's
sighash binds the state transition type, the token id and the transparent
fields; the fee bundle's sighash binds the type, the token id and a digest
of the token bundle's actions, so the two cannot be re-paired. The
minimum-fee validation pins the fee bundle's value balance to the two-bundle
fee (compute_token_pool_paid_shielded_fee), execution is a pool-paid event,
uniqueness is by the nullifiers of both bundles, and the transitions are
gated on the token shielded pool protocol version.

The wiring mirrors IdentityTopUpFromShieldedPool across dpp, drive, drive-abci
and the wasm bindings; dpp gains builders for the three transitions, and the
book documents the batch transitions, documents paid from the pool and the
identity-less transitions.

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

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rden the pool purchase builder

A TokenBurnFromPool bundle's sighash bound the batch owner, but a group action
pins the digest of the proposer's signed actions, so every confirmer must
submit that bundle unchanged and its proof failed under the confirmer's
identity: a group burn from the pool could never complete with two signers.
The sighash now binds the burner: the batch owner for a direct burn, the
stored group action's proposer for a confirmer. The builder refuses to prove a
fresh bundle for another signer, since consensus would reject it as a modified
group action. A drive-abci test runs a burn by a group of two with real proofs,
including a substituted bundle being rejected.

The identity-less purchase builder rejects a token count above i64::MAX before
proving instead of overflowing on negation, and proves the outputs-only bundle
once over the purchase sighash instead of proving a throwaway bundle first.

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

@QuantumExplorer QuantumExplorer left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the current PR head found two consensus/block-processing blockers and one generated-client integration issue.

&[],
)
}
BatchedTransitionRef::Token(TokenTransition::BurnFromPool(t)) => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// network until the version that introduces the format activates.

let validation_result = token_configuration.validate_format_version(platform_version);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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+.

Preserve the current protocol 14 version tables and key restrictions while enabling token pools through new protocol 15 generations. Keep transition IDs, error discriminants, signature preimages and bindings consistent with the rebased base.

Handle once-per-identity claims into pools, bind document payment fixtures to nonce-derived IDs, and include token pools in the GroveDB structure description and regression fixtures.
@QuantumExplorer
QuantumExplorer force-pushed the claude/tokens-shielded-pools-6642a6 branch from c71c0ef to 8875f29 Compare September 21, 2026 01:47
@github-actions

github-actions Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

🌳 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)

  • tokens.shielded_pools

Changed (28 nodes)

  • root
  • tokens
  • tokens.distributions
  • tokens.distributions.perpetual.token
  • tokens.distributions.timed
  • identities.identity
  • identities.identity.contract_info.bound
  • identities.identity.key_references
  • identities.identity.key_references.authentication
  • saved_block_transactions
  • prefunded_balances
  • pools.epoch
  • shielded_balances.main_pool
  • contracts.contract
  • contracts.contract.other
  • withdrawals
  • group_actions.contract.group
  • group_actions.contract.group.active.action
  • group_actions.contract.group.closed.action
  • misc
  • votes
  • votes.contested_resource
  • votes.contested_resource.active_polls.contract.document_type
  • votes.contested_resource.active_polls.contract.document_type.indexes.value.contender
  • versions
  • contract_groups
  • contract_groups.groups.group
  • contract_groups.members.contract

Compared 064c846cec with d1123cf4c9. Updated at 2026-09-21T17:02:52.283Z

Defer proposer-bound group burn proofs for confirmations to stateful validation. Restore shipped contract generations and select new pool-aware validation and storage methods only from protocol 15. Keep the format activation gate before paid processing.

Regenerate all DAPI clients for token pool query selectors and group events. Add admission, activation, storage, immutability, and cross-client serialization regression coverage.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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-astra unavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-20T20:45:34Z); stand-ins gpt-5.6-lunamuse-spark-1.3-contributor, gpt-5.6-solmuse-spark-1.3-contributor, gpt-5.6-terramuse-spark-1.3-contributor, gpt-6-astramuse-spark-1.3-contributor; Phase 1 effort capped at high
  • Triage: critical by muse-spark-1.3-contributor (standing in for gpt-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); agent phase1-reviewer, muse-spark-1.3-contributor — architecture-layering (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — platform-versioning (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort high); agent phase1-reviewer, muse-spark-1.3-contributor — security-auditor (completed, effort high); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (lane failed), glm-5.3-flash (zai below 15% reserve: 5h 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 for gpt-6-astra) — final-verifier; agent astra-verifier
  • Phase 2 reviewers: muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — architecture-layering (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — platform-versioning (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — rust-quality (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — security-auditor (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — general (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — architecture-layering (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — platform-versioning (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — rust-quality (completed, effort xhigh); agent phase2-reviewer, muse-spark-1.3-contributor (standing in for gpt-6-astra) — security-auditor (completed, effort xhigh); agent phase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `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.

@github-actions

Copy link
Copy Markdown
Contributor

@coderabbitai review

No review for d1bd1a3e yet, so PR Hygiene is asking once. If nothing arrives, the requirement is dropped for this commit and the pull request is labelled bot-review-skipped.

QuantumExplorer and others added 3 commits September 21, 2026 21:39
…s and tidy the pool validators

TokenMintToPool 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 perpetual claim into the pool must name the moment it
claims up to; the identity-less purchase treats a missing supply item as a
corrupted state like its batch counterparts.

The pool validators no longer read and bill a frozen-account check a pooled
token cannot fail. CheckTx is asserted on every sighash-binding batch
transition in the drive-abci tests, with the admission helper shared by the
test modules.

The identity-less anchor and nullifier checks reuse the batch helpers, the
three credit-pool fee lowerings share one helper, inline module paths become
imports, the document base transformer binds the token id once instead of
unwrapping it, and the v15 doc names the burner rule.

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

A contract update is estimated through the insert path, whose stateless
existence read reports no pool, so every pooled token the contract already
had was priced as a freshly created pool on each update. The insert
generation now skips a pool that exists when estimating; a real insert never
finds one. The drive test estimates both the registration and the update of
a pooled contract and pins that the update estimate is the smaller.

The three copies of the token configuration checks (format admitted, pool
rules compatible) in the contract create and update basic structure
generations and the pre-activation gate become one dpp helper. A perpetual
claim into the pool is now covered end to end: refused without the moment it
claims up to, and paying the accrued rewards into the pool with it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… pause every pool inflow

Review round 4 on the token shielded pools:

- `all_purchases_amount` now counts `DirectPurchaseToPoolAction`, so a buyer
  whose credits cover the fee but not the price is refused with
  `IdentityInsufficientBalanceError` before execution instead of failing the
  balance removal on the execution path (a Drive error that would have stalled
  block processing); pinned by a test.
- Mint, claim and purchase into the pool check the token status, so pause
  covers every pool operation as the PR describes; the book says so.
- The identity-less purchase transform splits the fee with `checked_sub` and
  refuses a price over the credits leaving the pool as a consensus error.
- The seven pool transformers share `bump_with_errors` for their paid
  failures.
- The pool total balance's read-modify-write documents its dependence on the
  one-transition batch cap; the missing minimum-notes floor for token pools is
  documented in the book.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dapi-endpoint DAPI endpoint addition or modification waiting-bots Waiting for the review bots to report on this head

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants