fix(dpp): validate encrypted_note length in structure validation#3368
Open
QuantumExplorer wants to merge 6 commits intov3.1-devfrom
Open
fix(dpp): validate encrypted_note length in structure validation#3368QuantumExplorer wants to merge 6 commits intov3.1-devfrom
QuantumExplorer wants to merge 6 commits intov3.1-devfrom
Conversation
Defense-in-depth: add early validation at the DPP layer that each shielded action's encrypted_note field is exactly 216 bytes (epk 32 + enc_ciphertext 104 + out_ciphertext 80). This rejects malformed data before it reaches the ABCI bundle reconstruction, saving network bandwidth and processing time. Adds a new ShieldedEncryptedNoteSizeMismatchError consensus error and calls validate_encrypted_note_sizes from all 5 shielded transition types' validate_structure implementations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
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 (4)
📝 WalkthroughWalkthroughAdds ENCRYPTED_NOTE_SIZE constant and a new ShieldedEncryptedNoteSizeMismatchError type; introduces validate_encrypted_note_sizes() and integrates it into multiple shielded state-transition validators with tests; makes common_validation public; updates error codes, WASM error mapping, and Drive-ABCI to use the canonical size. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Validator as ShieldedStateTransitionValidator
participant CV as common_validation::validate_encrypted_note_sizes
participant Error as ConsensusError
participant Drive as DriveABCI
Client->>Validator: submit shielded state transition
Validator->>CV: validate_encrypted_note_sizes(actions)
alt sizes match
CV-->>Validator: Ok
Validator->>Validator: continue other validations
Validator-->>Drive: pass validated transition
else size mismatch
CV-->>Validator: ShieldedEncryptedNoteSizeMismatchError(expected, actual)
Validator->>Error: wrap as BasicError::ShieldedEncryptedNoteSizeMismatchError
Error-->>Client: return consensus error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3368 +/- ##
============================================
+ Coverage 75.87% 75.89% +0.02%
============================================
Files 2912 2914 +2
Lines 283860 284254 +394
============================================
+ Hits 215375 215735 +360
- Misses 68485 68519 +34
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/rs-dpp/src/errors/consensus/codes.rs (1)
235-241: Keep the shielded code block numerically sorted.
10823is currently listed after10825, which makes this registry a bit harder to audit for gaps or accidental reuse later.♻️ Suggested reorder
Self::ShieldedZeroAnchorError(_) => 10821, Self::ShieldedInvalidValueBalanceError(_) => 10822, - Self::ShieldedTooManyActionsError(_) => 10825, Self::ShieldedEncryptedNoteSizeMismatchError(_) => 10823, + Self::ShieldedTooManyActionsError(_) => 10825,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/errors/consensus/codes.rs` around lines 235 - 241, The shielded error code mapping is out of numerical order: move the match arm for ShieldedEncryptedNoteSizeMismatchError(_) (10823) so the sequence is sorted ascending; update the block containing Self::ShieldedNoActionsError, Self::ShieldedEmptyProofError, Self::ShieldedZeroAnchorError, Self::ShieldedInvalidValueBalanceError, Self::ShieldedEncryptedNoteSizeMismatchError, Self::ShieldedTooManyActionsError to emit codes 10819, 10820, 10821, 10822, 10823, 10825 respectively.packages/rs-dpp/src/state_transition/state_transitions/shielded/unshield_transition/v0/state_transition_validation.rs (1)
114-127: Assert the reported sizes too.This only checks the variant, so a regression in the
expected_size/actual_sizepayload would still pass. Matching those fields here would strengthen the transition-level wiring test as well as the reject path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/shielded/unshield_transition/v0/state_transition_validation.rs` around lines 114 - 127, Update the test should_reject_invalid_encrypted_note_size to assert the reported expected and actual sizes in the ShieldedEncryptedNoteSizeMismatchError payload instead of using a wildcard; after setting transition.actions[0].encrypted_note = vec![4u8; 100] and calling transition.validate_structure(platform_version), pattern-match the result.errors to ConsensusError::BasicError(BasicError::ShieldedEncryptedNoteSizeMismatchError { expected_size, actual_size }) and assert expected_size equals the known correct size and actual_size equals 100 so the test fails if either value is incorrect; locate this assertion near the existing assert_matches and adjust the pattern to capture and compare those fields.packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs (1)
33-37: Version this size check like the other consensus limits.Everything else in this validator pulls consensus bounds from
platform_version, but this new helper call makes the encrypted-note length an implicit global constant. Threadingplatform_versionthrough here now would make a future note-layout change much easier to roll out compatibly.♻️ Suggested direction
- let result = validate_encrypted_note_sizes(&self.actions); + let result = validate_encrypted_note_sizes(&self.actions, platform_version);Then resolve the expected encrypted-note size inside the helper from versioned data rather than a global constant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs` around lines 33 - 37, The encrypted-note size check in validate_encrypted_note_sizes currently relies on a global ENCRYPTED_NOTE_SIZE constant instead of using versioned consensus limits; update the call site in state_transition_validation.rs so you pass the platform_version (or versioned config) into validate_encrypted_note_sizes(&self.actions, platform_version) and change the helper validate_encrypted_note_sizes to accept that version parameter and resolve the expected size from the platform_version (instead of ENCRYPTED_NOTE_SIZE) before validating each action.encrypted_note length; keep the function name validate_encrypted_note_sizes and the actions field to locate and modify the code.packages/rs-dpp/src/state_transition/state_transitions/shielded/common_validation.rs (1)
9-12: LGTM with minor observation.The constant value 216 correctly matches the breakdown (epk 32 + enc_ciphertext 104 + out_ciphertext 80) and aligns with the
ENCRYPTED_NOTE_SIZEindrive-abci's shielded_common module as documented in the comment.Consider whether this constant could be defined in a shared location (e.g., a common crate) to prevent potential divergence if one is updated without the other. The comment mitigates this by noting the relationship, but a single source of truth would be more robust.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/shielded/common_validation.rs` around lines 9 - 12, The ENCRYPTED_NOTE_SIZE constant is duplicated; move it to a single shared location and have this module reference that constant instead of defining its own. Create or use a common crate/module that exports ENCRYPTED_NOTE_SIZE (the same 216 value) and replace the local const in this module (the ENCRYPTED_NOTE_SIZE definition and its comment) with an import/use of the shared symbol; remove the local definition to avoid divergence and update the doc comment to reference the shared constant (and keep the breakdown epk/enc_ciphertext/out_ciphertext in the comment for clarity).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-dpp/src/errors/consensus/codes.rs`:
- Around line 235-241: The shielded error code mapping is out of numerical
order: move the match arm for ShieldedEncryptedNoteSizeMismatchError(_) (10823)
so the sequence is sorted ascending; update the block containing
Self::ShieldedNoActionsError, Self::ShieldedEmptyProofError,
Self::ShieldedZeroAnchorError, Self::ShieldedInvalidValueBalanceError,
Self::ShieldedEncryptedNoteSizeMismatchError, Self::ShieldedTooManyActionsError
to emit codes 10819, 10820, 10821, 10822, 10823, 10825 respectively.
In
`@packages/rs-dpp/src/state_transition/state_transitions/shielded/common_validation.rs`:
- Around line 9-12: The ENCRYPTED_NOTE_SIZE constant is duplicated; move it to a
single shared location and have this module reference that constant instead of
defining its own. Create or use a common crate/module that exports
ENCRYPTED_NOTE_SIZE (the same 216 value) and replace the local const in this
module (the ENCRYPTED_NOTE_SIZE definition and its comment) with an import/use
of the shared symbol; remove the local definition to avoid divergence and update
the doc comment to reference the shared constant (and keep the breakdown
epk/enc_ciphertext/out_ciphertext in the comment for clarity).
In
`@packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rs`:
- Around line 33-37: The encrypted-note size check in
validate_encrypted_note_sizes currently relies on a global ENCRYPTED_NOTE_SIZE
constant instead of using versioned consensus limits; update the call site in
state_transition_validation.rs so you pass the platform_version (or versioned
config) into validate_encrypted_note_sizes(&self.actions, platform_version) and
change the helper validate_encrypted_note_sizes to accept that version parameter
and resolve the expected size from the platform_version (instead of
ENCRYPTED_NOTE_SIZE) before validating each action.encrypted_note length; keep
the function name validate_encrypted_note_sizes and the actions field to locate
and modify the code.
In
`@packages/rs-dpp/src/state_transition/state_transitions/shielded/unshield_transition/v0/state_transition_validation.rs`:
- Around line 114-127: Update the test should_reject_invalid_encrypted_note_size
to assert the reported expected and actual sizes in the
ShieldedEncryptedNoteSizeMismatchError payload instead of using a wildcard;
after setting transition.actions[0].encrypted_note = vec![4u8; 100] and calling
transition.validate_structure(platform_version), pattern-match the result.errors
to ConsensusError::BasicError(BasicError::ShieldedEncryptedNoteSizeMismatchError
{ expected_size, actual_size }) and assert expected_size equals the known
correct size and actual_size equals 100 so the test fails if either value is
incorrect; locate this assertion near the existing assert_matches and adjust the
pattern to capture and compare those fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 371c2fe6-64b7-4960-bf65-f835671f6135
📒 Files selected for processing (10)
packages/rs-dpp/src/errors/consensus/basic/basic_error.rspackages/rs-dpp/src/errors/consensus/basic/state_transition/mod.rspackages/rs-dpp/src/errors/consensus/basic/state_transition/shielded_encrypted_note_size_mismatch_error.rspackages/rs-dpp/src/errors/consensus/codes.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/common_validation.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/v0/state_transition_validation.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shielded_transfer_transition/v0/state_transition_validation.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shielded_withdrawal_transition/v0/state_transition_validation.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/unshield_transition/v0/state_transition_validation.rs
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
suggested changes
Mar 17, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Well-structured defense-in-depth PR that adds early encrypted_note length validation at the DPP layer for all 5 shielded transition types. One blocking issue: the new BasicError variant is missing from the wasm-dpp exhaustive match, which will break WASM compilation. The duplicate ENCRYPTED_NOTE_SIZE constant across DPP and drive-abci is a minor drift risk worth addressing.
Reviewed commit: ec86d05
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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/wasm-dpp/src/errors/consensus/consensus_error.rs`:
- [BLOCKING] lines 956-959: Missing WASM binding for ShieldedEncryptedNoteSizeMismatchError breaks compilation
The `from_basic_error` match (lines 469–959) is exhaustive with no wildcard arm. The new `BasicError::ShieldedEncryptedNoteSizeMismatchError` variant added in this PR has no corresponding match arm, so `wasm-dpp` will fail to compile with a non-exhaustive patterns error. Every other shielded error (`ShieldedNoActionsError`, `ShieldedTooManyActionsError`, `ShieldedEmptyProofError`, `ShieldedZeroAnchorError`, `ShieldedInvalidValueBalanceError`) has both an import at line 96 and a `generic_consensus_error!` match arm. The new variant needs both: (1) add `ShieldedEncryptedNoteSizeMismatchError` to the import at line 96, and (2) add a match arm after the `ShieldedInvalidValueBalanceError` arm at line 957.
In `packages/rs-dpp/src/state_transition/state_transitions/shielded/common_validation.rs`:
- [SUGGESTION] lines 9-12: Duplicate ENCRYPTED_NOTE_SIZE constant risks silent divergence with drive-abci
This PR introduces `pub const ENCRYPTED_NOTE_SIZE: usize = 216` as a hardcoded literal. The same value already exists in `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_common/mod.rs:50` computed from component sizes (`EPK_SIZE + ENC_CIPHERTEXT_SIZE + OUT_CIPHERTEXT_SIZE`). Both evaluate to 216 today, but if the note format changes, only one may be updated. Since drive-abci already depends on DPP and the new constant is `pub`, drive-abci should import `dpp::state_transition::state_transitions::shielded::common_validation::ENCRYPTED_NOTE_SIZE` instead of maintaining its own copy. This creates a single source of truth and makes divergence — which would cause DPP to accept notes that ABCI rejects, or vice versa — impossible.
packages/rs-dpp/src/state_transition/state_transitions/shielded/common_validation.rs
Show resolved
Hide resolved
packages/rs-dpp/src/state_transition/state_transitions/shielded/common_validation.rs
Show resolved
Hide resolved
drive-abci now imports ENCRYPTED_NOTE_SIZE from dpp::common_validation instead of defining its own copy. A compile-time assertion ensures the component sizes (EPK + enc_ciphertext + out_ciphertext) stay in sync. Made common_validation module pub so drive-abci can access it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean incremental push addressing our prior suggestion about the duplicate ENCRYPTED_NOTE_SIZE constant. DPP's constant is now the canonical source of truth (module made pub), drive-abci imports it, and a compile-time assertion guards against future divergence. Exactly the right fix.
Reviewed commit: 03352ff
The new BasicError variant needs handling in the wasm-dpp consensus error conversion to avoid a non-exhaustive match compile error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests now verify expected_size() and actual_size() instead of just matching the error variant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Trivial incremental push: strengthens 3 existing test assertions to verify error payload values (expected_size/actual_size) and adds the missing WASM binding for ShieldedEncryptedNoteSizeMismatchError. Both changes follow established patterns exactly. No issues found.
Reviewed commit: 6f940d7
Now that DPP structure validation catches wrong encrypted_note sizes before proof verification, the 4 drive-abci tests expect BasicError::ShieldedEncryptedNoteSizeMismatchError instead of StateError::InvalidShieldedProofError. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Trivial incremental push updating 4 drive-abci integration tests to expect BasicError::ShieldedEncryptedNoteSizeMismatchError instead of StateError::InvalidShieldedProofError, reflecting that the encrypted_note size check now happens in DPP structure validation rather than during proof verification. All test assertions are correctly updated. No issues found.
Reviewed commit: 444de8d
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Defense-in-depth: the
encrypted_notefield in eachSerializedActionmust be exactly 216 bytes (epk 32 + enc_ciphertext 104 + out_ciphertext 80). Currently this is only validated deep in the ABCI layer duringreconstruct_and_verify_bundle. Adding early validation in the DPPvalidate_structurerejects oversized/malformed data before it wastes network bandwidth.What was done?
ShieldedEncryptedNoteSizeMismatchErrorconsensus error (code 10823) withexpected_sizeandactual_sizefieldsENCRYPTED_NOTE_SIZEconstant (216) andvalidate_encrypted_note_sizes()function incommon_validation.rsvalidate_encrypted_note_sizesfrom all 5 shielded transition types'validate_structureimplementations:ShieldTransitionV0ShieldFromAssetLockTransitionV0ShieldedTransferTransitionV0UnshieldTransitionV0ShieldedWithdrawalTransitionV0common_validation.rscovering correct size, too short, too long, empty, multiple actions, and second-action-invalid scenariosHow Has This Been Tested?
cargo fmt -p dpp-- passescargo check -p dpp-- compiles cleanlycargo test -p dpp -- shielded-- all 74 tests pass (12 new tests added)Breaking Changes
None. This is a purely additive validation that catches invalid data earlier.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests