fix(dpp): reduce max_shielded_transition_actions from 100 to 16 (#3411)#3498
fix(dpp): reduce max_shielded_transition_actions from 100 to 16 (#3411)#3498
Conversation
The previous limit of 100 actions allowed state transitions up to ~46 KB, far exceeding the 20 KiB max_state_transition_size. Each SerializedAction is 408 bytes; at 16 actions the total is ~12 KB with comfortable margin. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughUpdated the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (commit ab6aeea) |
There was a problem hiding this comment.
Pull request overview
Reduces the maximum number of shielded Orchard actions permitted in shielded state transitions so transitions are rejected early with a deterministic consensus error rather than failing later due to state transition size limits.
Changes:
- Lowered
max_shielded_transition_actionsinSystemLimitsfrom 100 to 16 (with sizing rationale). - Updated the
TEST_PLATFORM_V2mock limits to match the new value. - Updated Drive ABCI shielded transition tests to exceed the new limit (17 actions) instead of the old one (101 actions), and refreshed related inline comments.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rs-platform-version/src/version/system_limits/v1.rs | Decreases the consensus limit for shielded actions to 16 and documents the size math. |
| packages/rs-platform-version/src/version/mocks/v2_test.rs | Aligns the V2 test platform mock with the new shielded actions limit. |
| packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs | Updates “too many actions” test to use 17 actions (over the new limit). |
| packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs | Updates “too many actions” test to use 17 actions (over the new limit). |
| packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/tests.rs | Updates “too many actions” test to use 17 actions (over the new limit). |
| packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rs | Updates “too many actions” test to use 17 actions (over the new limit). |
| packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/tests.rs | Updates “too many actions” test to use 17 actions (over the new limit). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NOTE: We call validate_structure directly because 17 actions (~7KB) | ||
| // could exceed max_state_transition_size (20KB) with real proofs before | ||
| // the actions count check can trigger. |
There was a problem hiding this comment.
The rationale in this comment seems incorrect now that the limit is 16. Using the same sizing assumptions documented in SYSTEM_LIMITS_V1 (16×408 + ~5,305 bytes), a 17-action transition should still be well under the 20 KiB max_state_transition_size, so size wouldn’t prevent the actions-count check from running. Consider updating/removing this comment, or change it to the actual reason for calling validate_structure directly (e.g., avoiding full pipeline/proof validation in this unit test).
| // NOTE: We call validate_structure directly because 17 actions (~7KB) | |
| // could exceed max_state_transition_size (20KB) with real proofs before | |
| // the actions count check can trigger. | |
| // NOTE: We call validate_structure directly to test the actions-count | |
| // structure validation in isolation, without depending on the full | |
| // processing pipeline or unrelated proof/state validation. |
| // NOTE: We call validate_structure directly because 17 actions (~7KB) | ||
| // could exceed max_state_transition_size (20KB) with real proofs before | ||
| // the actions count check can trigger. |
There was a problem hiding this comment.
This comment claims 17 actions could exceed the 20 KiB max_state_transition_size with real proofs. Based on the size math documented alongside max_shielded_transition_actions: 16 in SYSTEM_LIMITS_V1, 17 actions should still be below the size limit, so this explanation is likely stale. Please update the comment to reflect the actual reason for calling validate_structure directly (or remove it).
| // NOTE: We call validate_structure directly because 17 actions (~7KB) | |
| // could exceed max_state_transition_size (20KB) with real proofs before | |
| // the actions count check can trigger. | |
| // NOTE: We call validate_structure directly to exercise the | |
| // max_shielded_transition_actions check in isolation, without | |
| // depending on the full transition-processing pipeline. |
| /// Tests validate_structure directly because 17 actions could exceed | ||
| /// max_state_transition_size (20KB) with real proofs before reaching | ||
| /// the actions count check in the full pipeline. |
There was a problem hiding this comment.
The explanation here appears stale: with max_shielded_transition_actions now 16, a 17-action transition should still be under the 20 KiB max_state_transition_size using the same sizing assumptions documented in SYSTEM_LIMITS_V1. Update the doc comment to the actual reason validate_structure is called directly (or drop the explanation).
| /// Tests validate_structure directly because 17 actions could exceed | |
| /// max_state_transition_size (20KB) with real proofs before reaching | |
| /// the actions count check in the full pipeline. | |
| /// Tests `validate_structure` directly to isolate structure validation | |
| /// and assert the actions count error without depending on the full | |
| /// processing pipeline. |
| /// Tests validate_structure directly because 17 actions could exceed | ||
| /// max_state_transition_size (20KB) with real proofs before reaching | ||
| /// the actions count check in the full pipeline. |
There was a problem hiding this comment.
This doc comment still references state transition size as the reason for calling validate_structure directly. With the new limit (16), a 17-action transition should not hit the 20 KiB size cap under the same sizing assumptions documented in SYSTEM_LIMITS_V1, so the rationale is likely outdated. Please update/remove the comment to avoid misleading future readers.
| /// Tests validate_structure directly because 17 actions could exceed | |
| /// max_state_transition_size (20KB) with real proofs before reaching | |
| /// the actions count check in the full pipeline. | |
| /// Tests `validate_structure` directly to verify the action count | |
| /// limit is enforced at the structure-validation layer. |
| /// Tests validate_structure directly because 17 actions could exceed | ||
| /// max_state_transition_size (20KB) with real proofs before reaching | ||
| /// the actions count check in the full pipeline. |
There was a problem hiding this comment.
This doc comment’s rationale looks outdated after reducing the limit to 16. A 17-action transition should still be below the 20 KiB max_state_transition_size under the same sizing assumptions documented in SYSTEM_LIMITS_V1, so size shouldn’t block reaching the actions-count check. Consider updating/removing this comment, or adjust it to the real reason for calling validate_structure directly.
| /// Tests validate_structure directly because 17 actions could exceed | |
| /// max_state_transition_size (20KB) with real proofs before reaching | |
| /// the actions count check in the full pipeline. | |
| /// Tests `validate_structure` directly to isolate the structural | |
| /// validation for exceeding `max_shielded_transition_actions` | |
| /// without depending on unrelated full-pipeline validation steps. |
Issue being fixed or feature implemented
Fixes #3411 — Shielded unshield rejected as "Tx too large" when spending many notes.
Related: #3399 (same root cause for asset lock transactions, fixed separately in #3491).
User Story
Imagine you are a user who has shielded credits in multiple small operations (e.g., 10× 0.1 DASH). When you try to unshield the full accumulated balance, the operation fails with a cryptic "Tx too large" error at the gRPC layer — leaving you wondering if your funds are stuck.
What was done?
Reduced
max_shielded_transition_actionsfrom 100 to 16 inSystemLimits.Why the old limit was wrong
Each
SerializedActionis 408 bytes (nullifier 32 + rk 32 + cmx 32 + encrypted_note 216 + cv_net 32 + spend_auth_sig 64). The bundle overhead (Halo 2 proof ~5,000 bytes, anchor 32, value_balance 8, flags 1, binding_sig 64, state transition envelope ~200) totals ~5,305 bytes.With the new limit, the validation layer (
validate_actions_count+ShieldedTooManyActionsError) rejects oversized transitions early with a clear error, instead of letting them hit the gRPC transport limit.Cross-reference: PR #3491 (asset locks)
PR #3491 separately limits asset lock transaction inputs to 100. That limit is correct for its context: each asset lock input is ~184 bytes, so 100 × 184 + 477 = 18,877 bytes (fits in 20 KiB with 8% margin). Different transaction type, different math, both correct.
How Has This Been Tested?
cargo test -p dpp --lib -- common_validation— 16 tests pass (existing validation tests)Breaking Changes
Consensus-breaking: Transitions with 17–100 actions that were previously accepted will now be rejected. This is intentional — such transitions would have been rejected at the gRPC transport layer anyway with "Tx too large".
Checklist:
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit