docs(drive-abci): clarify intentional absence of pool notes check in shielded transfer#3294
Merged
QuantumExplorer merged 2 commits intoMar 15, 2026
Merged
Conversation
ShieldedTransfer was missing the validate_minimum_pool_notes() call that both Unshield and ShieldedWithdrawal perform. Since all three transition types spend notes (reveal nullifiers) and reduce the anonymity set, shielded transfers must also enforce the minimum pool notes threshold. Without this check, shielded transfers could proceed even when the pool has too few notes, potentially allowing deanonymization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis change introduces a new validation step for minimum pool notes in shielded transfer processing. The validation is added to the transaction pipeline, checked after reading the pool balance but before other validations. Corresponding test infrastructure is updated to include pool notes in test scenarios and verify the new validation behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/tests.rs (1)
229-229: Avoid hardcoding250for encrypted notes in tests.
minimum_pool_notes_for_outgoingis platform-version dependent. Hardcoding250can make these tests brittle when constants change. Derive the inserted count fromplatform_version(e.g.,min + 1) so tests remain stable and intention-revealing.Refactor sketch
+fn notes_above_minimum(platform_version: &PlatformVersion) -> u64 { + let min_notes = platform_version + .drive_abci + .validation_and_processing + .event_constants + .minimum_pool_notes_for_outgoing as u64; + min_notes.saturating_add(1) +} ... - insert_dummy_encrypted_notes(&platform, 250); + insert_dummy_encrypted_notes(&platform, notes_above_minimum(platform_version));Also applies to: 258-258, 423-423, 732-732, 764-764, 1164-1164
🤖 Prompt for AI Agents
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/shielded_transfer/tests.rs` at line 229, The tests currently call insert_dummy_encrypted_notes(&platform, 250) with a hardcoded 250; instead compute the count from the platform version using the platform_version API and the minimum_pool_notes_for_outgoing constant (e.g., let min = platform_version.drive.methods.core.minimum_pool_notes_for_outgoing(...) or the equivalent accessor) and pass min + 1 to insert_dummy_encrypted_notes so the test uses platform-version-derived values; update every occurrence (calls at the locations using insert_dummy_encrypted_notes) to derive the count from minimum_pool_notes_for_outgoing via the same platform_version accessor so tests remain stable across version changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/tests.rs`:
- Around line 195-214: The test test_insufficient_pool_notes_returns_error
currently uses a dummy/invalid proof and therefore fails at proof verification
instead of exercising the pool-note count check; update the test to use a
transition that contains a valid shielded proof (e.g. replace
create_default_shielded_transfer_transition() with the helper that produces a
valid-proof transition such as create_valid_shielded_transfer_transition() or
construct a transition and inject a valid proof via the appropriate builder),
keep the state anchor setup (insert_anchor_into_state) and then change the final
assertion to expect
StateTransitionExecutionResult::UnpaidConsensusError(ConsensusError::StateError(StateError::InsufficientPoolNotesError(_)))
so the test validates the insufficient-pool-notes error path.
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/tests.rs`:
- Line 229: The tests currently call insert_dummy_encrypted_notes(&platform,
250) with a hardcoded 250; instead compute the count from the platform version
using the platform_version API and the minimum_pool_notes_for_outgoing constant
(e.g., let min =
platform_version.drive.methods.core.minimum_pool_notes_for_outgoing(...) or the
equivalent accessor) and pass min + 1 to insert_dummy_encrypted_notes so the
test uses platform-version-derived values; update every occurrence (calls at the
locations using insert_dummy_encrypted_notes) to derive the count from
minimum_pool_notes_for_outgoing via the same platform_version accessor so tests
remain stable across version changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7db7da1-f98d-4741-a4e0-73aac04f390f
📒 Files selected for processing (2)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/tests.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/transform_into_action/v0/mod.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3294 +/- ##
============================================
- Coverage 70.42% 70.39% -0.04%
============================================
Files 3293 3293
Lines 262598 262666 +68
============================================
- Hits 184931 184892 -39
- Misses 77667 77774 +107
🚀 New features to boost your workflow:
|
…tes check Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QuantumExplorer
changed the title
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
1 participant
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.
Summary
validate_minimum_pool_notesis intentionally not called for ShieldedTransferContext
A security audit flagged that ShieldedTransfer was missing the
validate_minimum_pool_notes()check present in Unshield and ShieldedWithdrawal. After review, this is intentionally correct:The minimum pool notes threshold protects the anonymity set when outflows have visible destinations. Since shielded transfers have no visible destination, the check does not apply.
Test plan
🤖 Generated with Claude Code