Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (34)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughProtocol version 14 activates Shield transition format V1. The validation pipeline defers Shield proof verification to a versioned transformer, which can charge eligible funded failures through a nonce-bump action. The change also updates client handling, compatibility checks, tests, and protocol documentation. ChangesShield V1 and paid proof-failure handling
Priority: ⬆️ High Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant StateTransitionProcessor
participant ShieldTransitionTransformerV2
participant ShieldState
StateTransitionProcessor->>ShieldTransitionTransformerV2: Transform Shield with validation mode
ShieldTransitionTransformerV2->>ShieldState: Read pool balance and check nullifiers
ShieldTransitionTransformerV2->>ShieldTransitionTransformerV2: Verify proof in validator mode
alt Invalid proof and base fee is covered
ShieldTransitionTransformerV2-->>StateTransitionProcessor: Nonce-bump action and paid proof error
else Base fee is not covered
ShieldTransitionTransformerV2-->>StateTransitionProcessor: Unpaid insufficient-funds error
end
Suggested reviewers: Merge Risk: 🔵 Low · up to From protocol version 14, a funded, authenticated Shield transition whose proof fails is charged a capped penalty, and its input nonces are consumed. Legacy-format Shields are refused without charge. The reported blocking issues appear addressed, and tests and CI pass. The remaining risk is whether the estimated failure fee exactly matches the fee actually deducted, so owners should confirm fee parity before activation. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Protocol 14 introduces financial side effects for authenticated proof failures and requires pending legacy transactions to be rebuilt and re-signed. The inspected authorization and fee controls support the intended behavior, but deployment and failure-recovery assurance remains incomplete. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 65.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 136 functions across 38 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
|
🔍 Review in progress — actively reviewing now (commit 15f4719) · triage: critical |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-06T00:05:54.494Z |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification of the complete PR range at head 7cb6ced found no actionable in-scope defects. The processor duplication follows the repository's explicit frozen-generation convention, and the affordability calculation matches fee deduction through guaranteed BTreeMap ordering and preserved input keys. No local builds or tests were run; the supplied exact-head CI snapshot reports Rust workspace tests passing, with the platform test suite and one browser shard still pending.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The new processor/v1/mod.rs and shield/transform_into_action/v2/mod.rs implement intricate consensus and funds-accounting changes that make authenticated bad-proof Shields charge fees and consume nonces while preserving principal under payer-strategy and affordability constraints. - 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 15% left, 5h 100% 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
|
/self-reviewed |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete 12-file diff at exact head 4c1a125 and traced the surrounding authentication, proof-verification, fee-estimation, and execution paths; no blocking defects were found. The behavior is confined to PV14, preserves historical processing and CheckTx rejection, and reuses existing nonce and fee machinery. One non-blocking regression-coverage gap remains; validation was static, with no local builds or tests run, and the supplied exact-head CI snapshot shows all listed checks passing.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 10: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 12: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 16: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 17: 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 new processor/v1/mod.rs and shield/transform_into_action/v2/mod.rs implement intricate consensus and funds-accounting changes governing bad-proof acceptance, nonce consumption, fee affordability, and signed payer strategies. - 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 15% left, 5h 100% 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-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,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 - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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/shield/tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/tests.rs:3275-3277: Cover failure fees split across multiple signed payers
The new paid-proof-failure tests use only one DeductFromInput step, including the multi-input affordability boundary and maximum-input cases. They therefore do not cover the new transform's aggregation across multiple signed payers or its agreement with sequential fee deduction when the first payer is exhausted. Add a committed failure case with two payers whose combined balance covers the estimated base fee plus the prepared penalty but neither can cover that charge alone, and order the strategy differently from the input map. Assert the prepared penalty, each address's deduction, and the consumed nonces. This pins the composition of the new affordability gate with the existing stable-index deduction machinery without implying that the current implementation is incorrect.
| AddressFundsFeeStrategy::from(vec![ | ||
| AddressFundsFeeStrategyStep::DeductFromInput(payer_index), | ||
| ]), |
There was a problem hiding this comment.
✅ Withdrawn at
5975278f; see the replies below.
🟡 Suggestion: Cover failure fees split across multiple signed payers
The new paid-proof-failure tests use only one DeductFromInput step, including the multi-input affordability boundary and maximum-input cases. They therefore do not cover the new transform's aggregation across multiple signed payers or its agreement with sequential fee deduction when the first payer is exhausted. Add a committed failure case with two payers whose combined balance covers the estimated base fee plus the prepared penalty but neither can cover that charge alone, and order the strategy differently from the input map. Assert the prepared penalty, each address's deduction, and the consumed nonces. This pins the composition of the new affordability gate with the existing stable-index deduction machinery without implying that the current implementation is incorrect.
source: gpt-6-astra (phase2-reviewer: rust-quality)
There was a problem hiding this comment.
Resolved (re-reviewed at 56b03be7): Your new should_split_failed_proof_fees_across_signed_payers_in_strategy_order test independently signs both inputs and exercises both payer orders, requiring the actual charge to exceed either payer's individual balance. It checks the full prepared penalty, exact committed deductions, both consumed nonces, and unchanged pool balance, note count, and nullifier state, resolving the requested coverage.
There was a problem hiding this comment.
Withdrawn (re-reviewed at 5975278f): Your two-payer regression verifies both payment orders, the full prepared penalty, exact deductions, consumed nonces, and unchanged pool/nullifier state. I confirmed that the tests file is byte-identical at the prior reviewed head and this head, so I withdraw the earlier coverage request rather than crediting the restack with a new fix.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete 12-file diff at head 56b03be. The multi-payer regression resolves the prior coverage finding, but the new paid-failure path exposes valid pending PV12/13 Shields to fee deductions and nonce consumption after PV14 activation without reauthorization. This was a static review; Rust workspace tests and numerous other checks were still pending in the supplied exact-head CI snapshot.
🔴 1 blocking
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: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); 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) — The new processor/v1/mod.rs and shield/transform_into_action/v2/mod.rs implement intricate consensus-changing failure handling that calculates and charges fees across signed payers, consumes nonces, and preserves shielded-pool principal under PV14. - 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 15% left, 5h 100% 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/shield/transform_into_action/v2/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/transform_into_action/v2/mod.rs:94-104: Retire legacy Shield signatures before charging PV14 proof failures
A valid Shield signed under PV12/13 reaches this branch with unchanged, valid address witnesses, but its Orchard signatures fail the PV14 signing domain. `shield_extra_sighash_data` returns an empty preimage for PV12/13 and `kind tag || funding digest` for PV14. Shield still has only transition version 0, PV14's serialization bounds admit version 0, and `StateTransition::active_version_range()` admits Shield from 12 through the latest version. Address witnesses authenticate `self.signable_bytes()` without a protocol-version binding, so they do not reject the unchanged legacy bytes.
A malicious proposer can retain an observed, uncommitted PV13 Shield and include its exact bytes after PV14 activates. If its input nonces remain current, nullifiers are unused, and the signed payers have sufficient funds, this proof failure returns the nonce-bump action at lines 168–170. The resulting address-paid event deducts the metered fee plus up to 50,000,000 penalty credits and consumes the victim's input nonces without shielding anything. No forgery or reauthorization is needed. CheckTx rejection does not prevent direct proposal inclusion. At the PR base, the same proof failure returned an unpaid refusal from processor v0, so this PR newly introduces the loss.
Introduce a distinct Shield transition version for the PV14 signing domain and reject legacy version 0 unpaid at activation, following the existing ShieldFromAssetLock version/decode protection; do not restore acceptance of unbound bundles. Add a regression that builds and signs a valid PV13 Shield, submits its unchanged serialized bytes under PV14 with current input nonces, and verifies unchanged balances and nonces.
| let extra_sighash_data = | ||
| shield_extra_sighash_data(&transition.inputs, platform_version)?; | ||
| if let Err(error) = reconstruct_and_verify_bundle( | ||
| &transition.actions, | ||
| FLAGS_OUTPUTS_ONLY, | ||
| -(transition.amount as i64), | ||
| &transition.anchor, | ||
| &transition.proof, | ||
| &transition.binding_signature, | ||
| &extra_sighash_data, | ||
| ) { |
There was a problem hiding this comment.
✅ Resolved at
32c92fe3; see the replies below.
🔴 Blocking: Retire legacy Shield signatures before charging PV14 proof failures
A valid Shield signed under PV12/13 reaches this branch with unchanged, valid address witnesses, but its Orchard signatures fail the PV14 signing domain. shield_extra_sighash_data returns an empty preimage for PV12/13 and kind tag || funding digest for PV14. Shield still has only transition version 0, PV14's serialization bounds admit version 0, and StateTransition::active_version_range() admits Shield from 12 through the latest version. Address witnesses authenticate self.signable_bytes() without a protocol-version binding, so they do not reject the unchanged legacy bytes.
A malicious proposer can retain an observed, uncommitted PV13 Shield and include its exact bytes after PV14 activates. If its input nonces remain current, nullifiers are unused, and the signed payers have sufficient funds, this proof failure returns the nonce-bump action at lines 168–170. The resulting address-paid event deducts the metered fee plus up to 50,000,000 penalty credits and consumes the victim's input nonces without shielding anything. No forgery or reauthorization is needed. CheckTx rejection does not prevent direct proposal inclusion. At the PR base, the same proof failure returned an unpaid refusal from processor v0, so this PR newly introduces the loss.
Introduce a distinct Shield transition version for the PV14 signing domain and reject legacy version 0 unpaid at activation, following the existing ShieldFromAssetLock version/decode protection; do not restore acceptance of unbound bundles. Add a regression that builds and signs a valid PV13 Shield, submits its unchanged serialized bytes under PV14 with current input nonces, and verifies unchanged balances and nonces.
source: gpt-6.1-sol (phase2-reviewer: security-auditor)
There was a problem hiding this comment.
Resolved (re-reviewed at 32c92fe3): Your signed V1 format and early V0 refusals now prevent legacy PV12/13 signatures from authorizing PV14 charges or nonce consumption. The activation regression exercises unchanged historically valid bytes through CheckTx, Recheck, committed processing, and direct processor entry, and verifies that changing only the format tag fails authentication unpaid.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
Select new processor and Shield transform generations at protocol 14. Restore principal on proof failure, reserve the estimated nonce-update fee, and cap the fixed penalty by the signed payer strategy. Unfunded failures stay unpaid; shipped protocol replay and CheckTx proof admission stay unchanged. Test would have caught this in CI: RED before the fix (funded proof failure was unpaid), GREEN after. The underfunded boundary was also RED without the affordability guard (InternalError), GREEN with it. Verified 52 Shield tests, independent PrepareProposal/ProcessProposal/FinalizeBlock, 25 platform-version tests, all-feature all-target Clippy, and formatting.
Describe how processor v1 delegates Shield proof verification to the action transformer and how transform v2 turns affordable proof failures into nonce updates with fees and a bounded penalty while preserving principal. Comment-only change with no behavior delta. Tests omitted because no executable code changed; cargo fmt --all -- --check and git diff --check passed, and the non-documentation source is byte-identical to the parent.
Commit an authenticated bad-proof Shield with two signed payers whose combined balances cover the estimated fee and full penalty while neither can cover the actual charge alone. Exercise both fee orders, including reverse BTreeMap order, and verify exact deductions, consumed nonces, and unchanged pool balance, notes, and nullifiers. Validation: all 8 paid-proof-failure tests passed, zero ignored; formatting and whitespace checks passed. A mutation counting only the first payer failed the prepared-penalty assertion (22,287,880 versus 50,000,000); restoring production code made the new test pass. Independent accounting, consensus, and scope reviews were clean. No production behavior changed.
56b03be to
5975278
Compare
|
Waiting for bot review — coderabbitai not yet · thepastaclaw 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static verification at head 5975278 confirms one blocking activation issue: an unchanged, previously valid PV12/13 Shield can become a paid failure at PV14 without new sender authorization. The multi-payer coverage claim is withdrawn because the regression already existed at the prior reviewed head and exercises both payment orders with exact accounting. The supplied exact-head CI snapshot reports successful Rust workspace tests; PR Hygiene remains pending, and no local builds or tests were run.
🔴 1 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The new processor/v1/mod.rs and shield/transform_into_action/v2/mod.rs implement intricate consensus-changing failure handling involving fee affordability, signed payer ordering, principal restoration, and nonce consumption. - 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 15% left, 5h 100% 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/state_transitions/shield/transform_into_action/v2/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/transform_into_action/v2/mod.rs:94-104: Retire legacy Shield signatures before charging PV14 proof failures
(existing thread: https://github.com/dashpay/platform/pull/5262#discussion_r4182548716)
This verification changes the Orchard authorization context without establishing a corresponding transparent-signature activation boundary. PV12/13 select `credit_pool_bundle_binding: None`, so the client builder signs the bundle with empty extra sighash data. PV14 selects `Some(0)`, supplying the kind tag and funding-address digest here. However, `StateTransition::active_version_range` still admits Shield from PV12 through the latest version, PV14's Shield serialization bounds remain version 0 only, and address witnesses verify the unchanged `signable_bytes()` without a target protocol version.
A proposer can retain a genuinely valid, uncommitted PV13 Shield and include its identical signed bytes after activation. With usable input nonces, absent nullifiers, and sufficient fee-payer funds, it passes transparent authentication and balance checks but fails Orchard authorization because the sighash changed. The new branch at lines 168–170 then returns a paid nonce-bump action, consuming the sender's nonces and deducting the metered fee plus the capped penalty without shielding any principal. The base processor returned this verification failure without an action; this PR newly makes it chargeable and retainable in a block. CheckTx rejection does not protect against direct proposal inclusion.
Introduce a distinct signed Shield format for the PV14-bound generation and retire the legacy format before proof work or charging, following the existing ShieldFromAssetLock V0/V1 activation gate, or implement an equivalent explicit unpaid legacy refusal. Update the serialization bounds and client-builder selection while preserving PV12/13 execution; do not accept unbound bundles as successful PV14 shields. Add an activation regression that processes the exact bytes of a genuinely valid PV13-built Shield at PV14 and asserts unchanged balances and nonces.
Use a signed Shield V1 format for the bound proof domain and paid-failure policy. Refuse legacy V0 before authentication, fees, nonce changes, and proof work at activation; preserve PV12/13 bytes and behavior. Update builder, WASM, wallet activity, and DAPI proof-budget consumers. Test would have caught this in CI: RED before fix, GREEN after. An exact valid PV13 transaction previously lost 50,991,540 credits and its nonce at PV14. The unchanged PV12/13 fixtures now execute historically and are refused unpaid after activation, including CheckTx/Recheck and direct processing. A one-byte format retag fails authentication unpaid. All 55 Shield tests and 10 fresh WASM wrapper tests pass; historical wire/signable hashes are fixed regression oracles. Independent consensus, accounting, and client reviews are clean.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
shumkov
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The signed Shield V1 activation boundary and independently signed multi-payer regression address both prior findings. One blocking accounting issue remains in the new paid-failure path: validation-read costs never reach the fee estimate or execution event. This was a static review only; the supplied CI snapshot shows Rust and WASM checks passing, an independently confirmed inherited Swift failure, and two test-suite jobs still running.
🔴 1 blocking
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: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); 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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: 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 intricate changes in processor/v1/mod.rs and shield/transform_into_action/v2/mod.rs alter consensus-critical fee deductions and nonce consumption, while Shield V1 changes signed authorization and protocol-gated wire decoding. - 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 15% left, 5h 100% 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— ffi-engineer (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— ffi-engineer (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/shield/transform_into_action/v2/mod.rs`:
- [BLOCKING] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/transform_into_action/v2/mod.rs:137-141: Include validation-read costs in the paid Shield failure fee
The pool-balance read and per-action nullifier lookups append metered costs to the local `drive_operations`, but this accumulator is never converted into an execution-context operation. Consequently, the new bad-proof path reserves and charges only the nonce-update batch and previously recorded authentication costs, plus the capped penalty. Unlike a successful `ShieldAction`, its `BumpAddressInputNoncesAction` receives no shielded compute fee containing the per-action lookup charge. This omits validation work from the base fee F: a payer can pass the affordability gate despite being unable to cover the complete metered base fee, and fully funded failures are undercharged. The boundary tests derive their estimate from the same incomplete execution event, so they cannot detect the omission. In the failed-proof branch, convert the accumulated reads through versioned `Drive::calculate_fee` and append a `ValidationOperation::PrecalculatedOperation` before estimating F; the execution event will then retain those costs too. Follow the handoff used by `shield_from_identity/transform_into_action/v0`. Preserve unpaid duplicate-nullifier refusals, historical generations, and successful Shield's existing flat per-action pricing rather than indiscriminately adding another read charge to successful actions.
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.
- Refresh the inherited Swift identity-fetch proof fixture — Out of scope — head job 111877989660 and base job 111857912433 both report rejection of GroveDB proof envelope version 0 because version 1 is required. The complete PR diff leaves the Swift tests, recorded fixtures, FFI identity-fetch implementation, and proof verifier unchanged. This is already documented in the PR as separate base maintenance, not an introduced Shield regression or an exceptional new follow-up.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
| ValidationOperation::add_many_to_fee_result( | ||
| execution_context.operations_slice(), | ||
| &mut estimated, | ||
| platform_version, | ||
| )?; |
There was a problem hiding this comment.
🔴 Blocking: Include validation-read costs in the paid Shield failure fee
The pool-balance read and per-action nullifier lookups append metered costs to the local drive_operations, but this accumulator is never converted into an execution-context operation. Consequently, the new bad-proof path reserves and charges only the nonce-update batch and previously recorded authentication costs, plus the capped penalty. Unlike a successful ShieldAction, its BumpAddressInputNoncesAction receives no shielded compute fee containing the per-action lookup charge. This omits validation work from the base fee F: a payer can pass the affordability gate despite being unable to cover the complete metered base fee, and fully funded failures are undercharged. The boundary tests derive their estimate from the same incomplete execution event, so they cannot detect the omission. In the failed-proof branch, convert the accumulated reads through versioned Drive::calculate_fee and append a ValidationOperation::PrecalculatedOperation before estimating F; the execution event will then retain those costs too. Follow the handoff used by shield_from_identity/transform_into_action/v0. Preserve unpaid duplicate-nullifier refusals, historical generations, and successful Shield's existing flat per-action pricing rather than indiscriminately adding another read charge to successful actions.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
Meter the pool-balance and nullifier reads in the PV14 failed-proof branch before reserving the complete base fee and capping its fixed penalty. Keep the same operation in the execution context for the committed charge. Successful Shield keeps its existing flat compute fee; legacy and nullifier refusals remain unpaid. Strengthen the affordability oracle with independently priced Drive reads, exact single inclusion and a nonce/authentication/read estimate assembled before the ordinary fee increase. Pin successful fee composition too. Test would have caught this in CI: ✖ before fix (0 read-fee operations, expected 1), ✔ after fix on the same regression. All 55 Shield tests and the independent proposal/finalization regression passed, zero ignored. Formatting and three independent code reviews are clean. Addresses the validation-read accounting review on #5262; fixes #5181.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
This branch has not been deployed
2 participants
Basic explanation
What this does: From protocol version 14, an address-authenticated Shield with a bad Orchard proof pays its failure fee and consumes its input nonces. Its principal stays in the addresses. A new signed Shield format separates this authorization from legacy transactions, which are refused unpaid after activation.
Value: Validators can retain an adequately funded bad proof as a paid failure, matching the other shielding transitions. Honest pending PV12/13 transactions cannot lose fees or nonces merely because activation changed the proof domain. CheckTx continues to keep bad proofs out of the mempool.
Risks: Consensus and wire-format changes at unreleased PV14. Pending Shield V0 transactions must be rebuilt and re-signed after activation; changing the tag alone cannot upgrade the old signatures. Historical PV12/13 bytes and execution are preserved. Current-head Shield/proposal tests pass; fresh CI and bot re-review are pending. The preceding run has an inherited Swift V0-proof fixture failure and a quorum-cache failure in the Node suite. The subsequent validation-read accounting finding is fixed in
15f4719a51.Issue being fixed or feature implemented
Closes #5181. Based on
v5.0-devafter #5014 merged. Also fixes the activation finding in the legacy-signature review: an unchanged, valid PV13 Shield must not become chargeable at PV14 without new address authorization.What was done?
min(configured_penalty, A - F). IfA < F, refuse unpaid. Apply the penalty once, without the user fee increase. Duplicate-nullifier refusals stay unpaid and precede proof work.In-place changes to shipped generations
validate_shielded_proof_v0: read the same V0 fields through accessors, retaining its empty extra preimage and all verification arguments. PV12/13 still select it. V1 is unreachable from their external decoding.is_allowedremain unchanged.How Has This Been Tested?
Actual RED before the activation fix: an exact, genuinely valid PV13-built Shield executed historically, then lost 50,991,540 credits and its nonce at PV14. GREEN submits those identical signed bytes at PV12/13 and PV14; CheckTx, Recheck, committed block processing, and direct processor calls now refuse legacy authorization unpaid. A one-byte version retag fails address authentication, with balances/nonces/pool/notes/nullifiers unchanged. Fixed historical wire/signable SHA-256 hashes were captured before production edits.
Earlier local wire-format verification on
32c92fe3d4, with zero ignored in these targeted runs:CI on the preceding head
32c92fe3d4passed the full Rust workspace suite, workspace all-target/all-feature compilation and Clippy (--locked -- --no-deps -D warnings), formatting, wallet dependency closure, unused-dependency checks, JS package tests, WASM/browser unit tests, and all three Docker image builds. This covers the planned workspace compile/lint gate; no separate standalonecargo checkwas run for this head. Functional tests, all Dashmate E2E jobs, and both browser shards passed. The Node platform suite finished with 69 passing, 10 pending, and one failure: the identity document-after-top-up case could not verify its proof because the quorum hash was missing from the context-provider cache. This is not yet confirmed against a base-only run. No manual live-network Shield test was run.Current-head accounting verification (
15f4719a51): all 55 Shield tests and the real independent proposal/finalization regression passed, zero ignored. Actual RED before the correction: the failed event had zero validation-read operations where independent Drive reads required one. The same regression is GREEN after adding the unsurcharged read fee once, before estimating F. The independent oracle prices pool/nullifier reads separately from the prepared event, verifies exact inclusion and the complete nonce/authentication/read estimate before fee increase, and pins the old-affordable/full-fee-minus-one refusal. Existing maximum-action, multiple-payer, fee-increase and committed-state assertions remain green. The genuine successful-proof test verifies the exact flat compute fee and absence of an extra read surcharge. Formatting/whitespace and three independent code reviews are clean. Fresh CI for this head is queued; its results and the bot re-review are pending.Inherited CI failure: Swift
SDKMethodTests.testSimpleIdentityFetchreplays a recorded protocol-9 GroveDB V0 proof, rejected by the unconditional V1 floor merged in #5294. The current base run fails the same test with the same error; the test, fixture, and identity-fetch paths are unchanged by this PR. Repair requires a fresh V1 proof and matching quorum-key fixture from a seeded Platform. It remains a separate base repair; proof verification and test assertions were not weakened here.Breaking Changes
PV14 admits only Shield wire format 1 and changes bad-proof block acceptance and nonce/fee effects. Pending V0 Shields must be rebuilt and re-signed after activation. PV12/13 retain format 0 and their original behavior.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
PR Hygiene ·
15f4719/self-reviewedrust-dapi(packages/rs-dapi/src/services/platform_service/shielded_proof_failure_budget.rs) — QuantumExplorer or lklimekdpp— you own itrs-drive-abci— you own itrs-drive— you own itrs-platform-wallet(packages/rs-platform-wallet/src/wallet/shielded/operations.rs,packages/rs-platform-wallet/src/wallet/shielded/sync/memo_roundtrip_tests.rs,packages/rs-platform-wallet/src/wallet/shielded/sync/ovk_builder_roundtrip_tests.rsand 1 more) — HashEngineering or ZocoLini or llbartekll or romchornyiWhen every box is checked the
PR Hygienecheck passes and this can merge.Summary by CodeRabbit
New Features
Documentation