Repository navigation
fix(dapi): meter token pool transitions in the shielded proof failure budget - #5801
Conversation
… budget orchard_action_count predates token shielded pools and returned 0 for kinds Drive's CheckTx verifies, so their broadcasts reserved nothing against the per-source budget. It now counts what drive-abci's shielded_proof_action_count admits: - TokenShieldedTransferWithShieldedFee, TokenUnshieldWithShieldedFee and TokenPurchaseFromShieldedPool: token_actions + fee_actions; - ShieldFromAssetLock V1: its actions (only V0 was matched); - Batch: each token pool bundle and each shielded token payment. The match names every StateTransition kind and version instead of falling through `_ => 0`, so a new one has to declare its count before rs-dapi compiles. Part of #5676. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
✅ Final review complete — no blockers (commit 704e7fe) |
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Updated after discussion: 1 finding(s) of this review were settled in the inline threads.
- ⏭️ Deferred (maintainer decision): 🟡 Share structural proof-action counting through DPP (tracked in #5676)
Verified the complete diff at 704e7fe: the added token-pool, batch, and ShieldFromAssetLock V1 counts match Drive-ABCI's current admission counter, with no blocking correctness defects found. One non-blocking suggestion remains to consolidate the duplicated structural count through DPP. This was a static review; current CI reports successful Rust workspace tests, with PastaClaw review and PR Hygiene pending.
🟡 1 0 suggestion(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: glm-5.3-flash (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: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff makes a small, contained extension to orchard_action_count with substantial regression tests, changing only DAPI-local admission accounting rather than consensus, cryptography, or funds movement. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer,glm-5.3-flash— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 100% left, weekly 30% left; passed overgemini-3.8-flash-high(antigravity quota lookup failed) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort medium); 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-dapi/src/services/platform_service/shielded_proof_failure_budget.rs`:
- [SUGGESTION → DEFERRED by a maintainer; not blocking, do not fix in this PR] packages/rs-dapi/src/services/platform_service/shielded_proof_failure_budget.rs:157-181: Share structural proof-action counting through DPP
These additions extend a second implementation of the structural traversal in Drive-ABCI's `shielded_proof_action_count` (`processor/traits/shielded_proof.rs:163–234`). The implementations agree today, and the exhaustive DAPI match catches new StateTransition variants, but it cannot catch changed proof-bearing fields or counting behavior within an existing variant. That leaves the same maintenance mechanism that produced this bypass. The book's lowest-capable-layer guidance points to DPP for this pure-data traversal, consistent with the existing shared `shielded_pool_actions()` accessor. Use the already-linked https://github.com/dashpay/platform/issues/5676 to track a shared StateTransition accessor consumed by both counters; keep decoding, source reservations, clamping, and settlement in DAPI. This is a non-blocking consolidation follow-up, not a defect in the counts introduced here.
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.
- Complete the tracked multi-bundle budget-clamp follow-up — The unchanged reservation code clamps the total broadcast count to one bundle's maximum, although token transitions and batches can carry multiple independently verified bundles. A failure in a later bundle can therefore incur more verification work than the charged debt represents. This patch improves those broadcasts from zero accounting to bounded accounting and does not introduce or worsen the limitation.
| StateTransition::TokenShieldedTransferWithShieldedFee( | ||
| TokenShieldedTransferWithShieldedFeeTransition::V0(v0), | ||
| ) => v0.token_actions.len() + v0.fee_actions.len(), | ||
| StateTransition::TokenUnshieldWithShieldedFee( | ||
| TokenUnshieldWithShieldedFeeTransition::V0(v0), | ||
| ) => v0.token_actions.len() + v0.fee_actions.len(), | ||
| StateTransition::TokenPurchaseFromShieldedPool( | ||
| TokenPurchaseFromShieldedPoolTransition::V0(v0), | ||
| ) => v0.token_actions.len() + v0.fee_actions.len(), | ||
| // A batch carries a bundle for each token transition against a token's | ||
| // shielded pool and each document whose token price is paid from one. | ||
| StateTransition::Batch(batch) => batch | ||
| .transitions_iter() | ||
| .map(|transition| match transition { | ||
| BatchedTransitionRef::Token(token_transition) => token_transition | ||
| .shielded_pool_actions() | ||
| .map_or(0, |actions| actions.len()), | ||
| BatchedTransitionRef::Document(document_transition) => document_transition | ||
| .base() | ||
| .token_payment_info_ref() | ||
| .as_ref() | ||
| .and_then(|info| info.shielded_payment()) | ||
| .map_or(0, |payment| payment.actions.len()), | ||
| }) | ||
| .sum(), |
There was a problem hiding this comment.
⏭️ Deferred (maintainer decision; not blocking; tracked in #5676) at
704e7fe3; see the replies below.
🟡 Suggestion: Share structural proof-action counting through DPP
These additions extend a second implementation of the structural traversal in Drive-ABCI's shielded_proof_action_count (processor/traits/shielded_proof.rs:163–234). The implementations agree today, and the exhaustive DAPI match catches new StateTransition variants, but it cannot catch changed proof-bearing fields or counting behavior within an existing variant. That leaves the same maintenance mechanism that produced this bypass. The book's lowest-capable-layer guidance points to DPP for this pure-data traversal, consistent with the existing shared shielded_pool_actions() accessor. Use the already-linked #5676 to track a shared StateTransition accessor consumed by both counters; keep decoding, source reservations, clamping, and settlement in DAPI. This is a non-blocking consolidation follow-up, not a defect in the counts introduced here.
source: glm-5.3-flash (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality, security-auditor)
There was a problem hiding this comment.
Agreed. It is consensus-neutral too: Drive only calls shielded_proof_action_count from CheckTx admission. Leaving it out of this PR because it touches dpp and drive-abci on a patch branch, and #5799 (v5.1-dev) adds type 29 to both counters. Planned as a follow-up on v5.1-dev after #5799: one exhaustive DPP accessor that both DAPI and Drive call (Drive's copy still ends in _ => 0). Tracking under #5676.
There was a problem hiding this comment.
I confirmed the counter’s callers are confined to CheckTx admission; the shared exhaustive DPP accessor is a reasonable follow-up on v5.1-dev after #5799, tracked under #5676, rather than expanding this patch.
Tracked in #5676. Deferred on a maintainer's call: this finding no longer blocks this pull request.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review after discussion — commit 704e7fe
Standing review was COMMENTED; this re-review is APPROVE: no blocking findings remain.
Approved: the discussion withdrew or deferred every finding the full review of 704e7fe3 (run 9228) raised, and the approval rests on that review.
Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.
Review provenance
Source: conversation lane gpt-6.1-sol (agent: conversation); no reviewer or verifier lanes ran for this follow-up
- Verdict moved because the inline discussion withdrew or deferred findings of the standing review of this commit; the code was not re-reviewed
Basic explanation
What this does: DAPI keeps a small per-client allowance for shielded transactions whose zero-knowledge proof turns out to be invalid (#5001). To do that it has to know how many proof "actions" (the units Drive verifies) a transaction carries. Its counter only knew the shielded transaction types that existed when the allowance was added, so the token shielded-pool transactions added later in 5.0 were counted as having none and never went through the allowance. This PR counts them, using the same count Drive uses.
Value: Every transaction whose proof Drive checks before admitting it now goes through the same per-client allowance on each node, not only the eight shielded types that existed in #5001.
Risks: Low. This is node-local admission policy in DAPI; no Drive, protocol or consensus change. A client whose proofs verify is never charged, as before. The one difference a client could see: one client address can have at most one largest bundle's worth (16 actions) of these transactions waiting on Drive at once, the limit the other shielded types already had. A service sending many shielded token batches concurrently from one address may get
RESOURCE_EXHAUSTEDwithratelimit-reset: 1, and the SDK then retries on another node.Issue being fixed or feature implemented
Covers the first part of #5676. DAPI's
orchard_action_countpredates token shielded pools (#4760) and returned 0 for kinds Drive's CheckTx verifies (shielded_proof_action_countin drive-abci):TokenShieldedTransferWithShieldedFee,TokenUnshieldWithShieldedFee,TokenPurchaseFromShieldedPool(two bundles each: the token bundle and the fee bundle);ShieldFromAssetLockV1 (only V0 was matched; V1 is the only version protocol version 14 admits);Batchwith token transitions against a token's shielded pool, or documents paying their token price from one.A broadcast counted at 0 reserves nothing, so these went to Drive outside the budget.
What was done?
packages/rs-dapi/src/services/platform_service/shielded_proof_failure_budget.rs:orchard_action_countcounts the same actions as drive-abci'sshielded_proof_action_count:token_actions.len() + fee_actions.len();ShieldFromAssetLockV1:actions.len();Batch: the sum over its transitions ofTokenTransition::shielded_pool_actions()and each document's shielded token payment._ => 0arm is gone. The match names everyStateTransitionkind (and every version of the shielded ones), so a new kind or version has to say whether it carries actions before rs-dapi compiles. The batch arm usesshielded_pool_actions(), which itself names every token transition kind.Before / after, for a client whose proofs keep failing:
Not in this PR: the second part of #5676. The budget still clamps one broadcast to
max_shielded_transition_actions, so a two-bundle transition whose proof fails is charged at most 16 actions even when it carries more. #5659 changes that clamp's bound, so the per-bundle clamp is left for after it.How Has This Been Tested?
cargo test -p rs-dapi --lib -- shielded_proof_failure_budget: 22 passed (5 new)ShieldFromAssetLockV1 counts its actions;cargo clippy -p rs-dapi --all-targetsandcargo fmt --all --check: clean.Breaking Changes
None. Node-local DAPI policy; nothing nodes have to agree on changes.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
704e7fe/self-reviewedrust-dapi(packages/rs-dapi/src/services/platform_service/shielded_proof_failure_budget.rs) — lklimek or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.