feat(drive-abci): add shielded pool drive-abci integration (medusa part 3)#3220
Merged
QuantumExplorer merged 24 commits intov3.1-devfrom Mar 12, 2026
Merged
feat(drive-abci): add shielded pool drive-abci integration (medusa part 3)#3220QuantumExplorer merged 24 commits intov3.1-devfrom
QuantumExplorer merged 24 commits intov3.1-devfrom
Conversation
QuantumExplorer
changed the title
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds full shielded-pool support: new shielded state transitions, end-to-end ZK proof and minimum-fee validation, anchor and nullifier management (recording and cleanup), GroveDB-backed shielded queries, protocol v12 initialization, extensive tests, dependency additions, and startup warmup for the shielded verifying key. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Processor as StateTransition\nProcessor
participant Proof as ZK\nProofVerifier
participant Drive as Drive
participant Grove as GroveDB
Client->>Processor: submit shielded StateTransition
activate Processor
Processor->>Processor: basic & structure checks
alt invalid
Processor-->>Client: reject (structure/format)
else
Processor->>Proof: reconstruct & verify bundle (anchor, actions, proof)
activate Proof
Proof->>Grove: read anchor / nullifier state
Grove-->>Proof: anchor/nullifier data
Proof-->>Processor: proof result
deactivate Proof
alt proof invalid
Processor-->>Client: reject (InvalidShieldedProof)
else
Processor->>Drive: read pool balance & compute fees
Drive->>Grove: query/update commitment, anchors, nullifiers
Grove-->>Drive: data / ack
Drive-->>Processor: balance & operations
Processor->>Processor: fee validation & consensus checks
alt fees insufficient
Processor-->>Client: reject (InsufficientShieldedFee)
else
Processor->>Drive: apply state changes (commit)
Drive->>Grove: commit trees
Grove-->>Drive: commit ack
Drive-->>Processor: success
Processor-->>Client: accept
end
end
end
deactivate Processor
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "159e31db3275990a52d09940a5b334150b9af0ec0a79c35aee00b1f0d26f01d1"
)Xcode manual integration:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
feat/zk-drive-abci
branch
from
d09e66c to
ed3a886
Compare
![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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/addresses_minimum_balance.rs (1)
61-79:⚠️ Potential issue | 🟠 MajorKeep
Shieldon the address-balance pre-check path.Line 74 and Line 106 lump
Shieldin with non-address-funded shielded transitions, butShieldis still funded from platform addresses. That removes the cheap insufficient-balance gate before the heavier shielded validation/proof path and looks inconsistent with the purpose of this trait.Suggested change
StateTransition::AddressCreditWithdrawal(transition) => { transition.validate_estimated_fee(remaining_address_balances, platform_version) } + StateTransition::Shield(transition) => { + transition.validate_estimated_fee(remaining_address_balances, platform_version) + } // AddressFundingFromAssetLock doesn't need balance check - funds come from asset lock // Shielded transitions don't use address minimum balance validation // All other state transitions don't use address minimum balance validation StateTransition::AddressFundingFromAssetLock(_) | StateTransition::DataContractCreate(_) @@ | StateTransition::IdentityCreditWithdrawal(_) | StateTransition::IdentityCreditTransferToAddresses(_) | StateTransition::Batch(_) | StateTransition::MasternodeVote(_) - | StateTransition::Shield(_) | StateTransition::ShieldedTransfer(_) | StateTransition::Unshield(_) | StateTransition::ShieldFromAssetLock(_) | StateTransition::ShieldedWithdrawal(_) => { return Ok(SimpleConsensusValidationResult::new()); @@ StateTransition::AddressFundsTransfer(_) | StateTransition::AddressCreditWithdrawal(_) | StateTransition::IdentityCreateFromAddresses(_) - | StateTransition::IdentityTopUpFromAddresses(_) => true, + | StateTransition::IdentityTopUpFromAddresses(_) + | StateTransition::Shield(_) => true, @@ | StateTransition::Batch(_) | StateTransition::MasternodeVote(_) | StateTransition::AddressFundingFromAssetLock(_) - | StateTransition::Shield(_) | StateTransition::ShieldedTransfer(_) | StateTransition::Unshield(_) | StateTransition::ShieldFromAssetLock(_) | StateTransition::ShieldedWithdrawal(_) => false,Also applies to: 105-110
🤖 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/processor/traits/addresses_minimum_balance.rs` around lines 61 - 79, The match arm incorrectly treats StateTransition::Shield as a non-address-funded shielded transition and returns early; remove StateTransition::Shield from the early-return arm(s) (the arm listing Shield and ShieldedTransfer/Unshield/ShieldFromAssetLock/ShieldedWithdrawal) so Shield remains on the address-minimum-balance pre-check path and will be validated with address-funded transitions instead; update both occurrences where Shield was lumped into the non-funded branch so Shield is handled by the branch that enforces address minimum balance.packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs (1)
337-348:⚠️ Potential issue | 🟠 MajorValidate
PaidFromShieldedPoolfees before crediting the pool.Line 347 puts this variant on the
Nonepath, and Lines 530-532 then creditfees_to_add_to_poolstraight from the event payload. That means this executor never recomputes or validates the fee amount before committing the state changes, unlike the other paid flows here. A bad event builder / validator mismatch would silently under- or over-credit the pool.Also applies to: 513-537
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs` around lines 337 - 348, The PaidFromShieldedPool variant is currently on the None path and its fee amount is never recomputed/validated before being credited; update the match arm so PaidFromShieldedPool is handled like other paid flows by calling validate_fees_of_event (i.e., include ExecutionEvent::PaidFromShieldedPool { .. } in the Some(self.validate_fees_of_event(&event, block_info, Some(transaction), platform_version, previous_fee_versions)?) arm) and then use the validated fee result when computing fees_to_add_to_pool/crediting logic so the pool is credited only with the validated amount (also mirror the same fix in the other similar block referenced around the 513-537 region).packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/is_allowed.rs (1)
48-54:⚠️ Potential issue | 🔴 CriticalRemove unreachable duplicate match arms in
has_is_allowed_validation.Lines 35–39 match all five shielded transition variants (
Shield,ShieldedTransfer,Unshield,ShieldFromAssetLock,ShieldedWithdrawal) and returnOk(true). The match arms at lines 48–54 are unreachable duplicates with identical patterns that still containtodo!(). This dead code must be removed.Fix: remove unreachable arms
| StateTransition::MasternodeVote(_) => Ok(false), - StateTransition::Shield(_) - | StateTransition::ShieldedTransfer(_) - | StateTransition::Unshield(_) - | StateTransition::ShieldFromAssetLock(_) - | StateTransition::ShieldedWithdrawal(_) => { - todo!("shielded transitions not yet implemented") - } } }🤖 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/processor/traits/is_allowed.rs` around lines 48 - 54, The match in has_is_allowed_validation contains duplicate arms for the shielded StateTransition variants: Shield, ShieldedTransfer, Unshield, ShieldFromAssetLock, ShieldedWithdrawal; remove the unreachable duplicate match arm block that contains todo!() (the second occurrence) so only the first arm that returns Ok(true) remains, leaving no todo!() paths; ensure you only delete the redundant match arm(s) and do not change the surrounding match structure or other variants handling.packages/rs-drive-abci/tests/strategy_tests/execution.rs (1)
978-1027:⚠️ Potential issue | 🔴 Critical
ShieldedStatelacks block-level commit/rollback, risking state drift across failed blocks.Unlike
current_addresses_with_balance.commit()at line ~1114,shielded_statehas no visible per-block commit or rollback path. It persists mutated across the entire block loop viarecord_shielded_note()andcheckpoint()calls withinstate_transitions_for_block(). If block execution fails ordont_finalize_block()returns true (line 1100+), shielded state mutations are never rolled back. Subsequent blocks then generate transitions based on notes that don't actually exist on-chain, desynchronizing the test harness from platform state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/tests/strategy_tests/execution.rs` around lines 978 - 1027, ShieldedState is being mutated across blocks by state_transitions_for_block (via record_shielded_note and checkpoint) but never committed or rolled back per-block; modify the loop around state_transitions_for_block to snapshot/checkpoint shielded_state before calling it and then either commit/apply the checkpoint when the block is finalized (matching the current_addresses_with_balance.commit() behavior) or restore/rollback the checkpoint when block execution fails or dont_finalize_block() returns true; refer to the shielded_state variable, the state_transitions_for_block call, record_shielded_note/checkpoint usage, and the dont_finalize_block()/finalize_block_operations control flow to implement symmetric commit/rollback handling so shielded_state cannot leak mutations into subsequent blocks.
🟡 Minor comments (4)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs-8024-8029 (1)
8024-8029:⚠️ Potential issue | 🟡 MinorUse the latest-protocol base fee here, not the v11 one.
Line 8027 and Line 8124 now compare against
457440, but this module still pins the latest 1-in/1-out P2PKH processing fee at508740. That makes these comments misleading and weakens the sanity checks for current-protocol fixtures.Suggested fix
- // Base processing fee is 457440, with user_fee_increase=100 it should be higher + // Base processing fee for the latest protocol is 508740, so a 100% increase + // must push this above that baseline. assert!( - processing_fee > 457440, + processing_fee > 508740, "Processing fee with user_fee_increase should be higher than base" ); @@ - // 16 inputs should have higher processing fee than 1 input (base is ~457K) + // 16 inputs should have higher processing fee than the latest 1-input baseline (508740) assert!( - processing_fee > 457440, + processing_fee > 508740, "16 inputs should have processing fee > single input" );Also applies to: 8122-8126
🤖 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/address_funds_transfer/tests.rs` around lines 8024 - 8029, The test uses the outdated magic number 457440 when asserting processing_fee > 457440; update the assertion to use the current-protocol base fee instead (508740) or, better, reference the module's canonical constant for the latest 1-in/1-out P2PKH base processing fee (e.g., replace the literal 457440 with the constant/variable used elsewhere in this module such as BASE_PROCESSING_FEE or EXPECTED_P2PKH_PROCESSING_FEE or compute expected_base via the same function that calculates the base fee) so the sanity check compares against the current-protocol value using the identifier associated with processing_fee.packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs-257-263 (1)
257-263:⚠️ Potential issue | 🟡 MinorUse a fee-specific consensus error here.
This branch reports a too-low
fees_to_add_to_poolasStateError::InvalidShieldedProofError, which makes “proof invalid” and “fee below minimum” indistinguishable for callers. Please surface a dedicated insufficient-shielded-fee error instead of reusing the proof variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/validate_fees_of_event/v0/mod.rs` around lines 257 - 263, The branch currently constructs StateError::InvalidShieldedProofError using InvalidShieldedProofError::new for the case where fees_to_add_to_pool < required_fee; change it to return a fee-specific consensus error (e.g., StateError::InsufficientShieldedFeeError or StateError::InvalidShieldedFeeError) instead of the proof error so “fee below minimum” is distinguishable from proof validation failures—construct the new error with a clear message including fees_to_add_to_pool and required_fee (using the same formatting) and return .into() like the other error variants; update any necessary imports/constructors to use the dedicated Insufficient/InvalidShieldedFeeError type.packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs-79-83 (1)
79-83:⚠️ Potential issue | 🟡 MinorPotential integer overflow when casting
value_balance(u64) to i64.If
v0.value_balanceexceedsi64::MAX(~9.2e18), the cast toi64will overflow and produce a negative value. Subsequently, the check at line 120 (fee as u64) would convert this negative value back to a large positive number, incorrectly passing the fee validation.Proposed defensive check
StateTransition::ShieldedTransfer(st) => match st { dpp::state_transition::shielded_transfer_transition::ShieldedTransferTransition::V0(v0) => { + if v0.value_balance > i64::MAX as u64 { + return Ok(SimpleConsensusValidationResult::new_with_error( + StateError::InsufficientShieldedFeeError( + InsufficientShieldedFeeError::new( + "value_balance exceeds maximum representable fee".to_string(), + ), + ) + .into(), + )); + } (v0.value_balance as i64, v0.actions.len()) } },🤖 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/processor/traits/shielded_proof.rs` around lines 79 - 83, The cast of v0.value_balance to i64 in the StateTransition::ShieldedTransfer match (ShieldedTransferTransition::V0 branch) can overflow; instead validate or convert safely before using fee logic: check that v0.value_balance <= i64::MAX (or use TryFrom/checked conversion) and handle the overflow case by returning a validation error (or rejecting the transition) rather than casting, so subsequent code that uses fee as u64 cannot be tricked by a negative casted value; update the branch that produces (v0.value_balance as i64, v0.actions.len()) to perform a safe conversion and explicit error handling for values > i64::MAX.packages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rs-72-73 (1)
72-73:⚠️ Potential issue | 🟡 MinorRename the new tests to the
should …convention.These are all new test cases under
packages/rs-drive-abci/tests, so they should follow the repo naming rule.As per coding guidelines,
**/tests/**/*.{js,jsx,ts,tsx,rs}: Name tests descriptively, starting with 'should …'.Also applies to: 131-132, 189-190, 276-277, 356-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rs` around lines 72 - 73, Rename the test function names to the repository "should …" convention: change the function name run_chain_shield_transitions (and the other test functions referenced at the other locations) to a descriptive name starting with should_, e.g., should_run_chain_shield_transitions, updating the #[test] functions accordingly; ensure the new names are used where the functions are defined (function identifier run_chain_shield_transitions) and any references in test modules are updated so the tests still compile and run.
🧹 Nitpick comments (23)
packages/rs-drive-abci/src/query/shielded/anchors/v0/mod.rs (2)
26-33: Consider adding a configurable limit to the PathQuery.The query fetches all anchors without a limit. If the anchor tree grows large over time (depending on retention policy), this could impact query performance and response size.
If anchors are periodically pruned or inherently bounded, this is likely acceptable. Otherwise, consider adding pagination or a reasonable default limit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/shielded/anchors/v0/mod.rs` around lines 26 - 33, The PathQuery currently requests the full range (PathQuery with query: SizedQuery { query: Query::new_range_full(), limit: None, offset: None }) which can return all anchors; change the PathQuery construction to accept a configurable limit (and optional offset) instead of None — use a configurable constant or injected config value (e.g., MAX_ANCHORS_PAGE or a runtime parameter) and set SizedQuery.limit = Some(limit) and optionally populate offset for pagination; update the code paths that call shielded_credit_pool_anchors_path_vec()/PathQuery to pass the limit (and support offset) and document the default limit used.
59-64: Silent filtering of non-item elements may hide unexpected data.Using
filter_mapwith.ok()silently ignores any elements that aren't Items. If anchors should always be stored as Items, a failed conversion could indicate data corruption or a bug in the storage layer.Consider either:
- Adding a debug log when elements are skipped, or
- Adding a comment explaining why non-item elements are expected and safe to ignore.
🔍 Option: Add logging for unexpected elements
// Anchors are stored as block_height_be → anchor_bytes; extract values let anchors: Vec<Vec<u8>> = results .to_key_elements() .into_iter() - .filter_map(|(_key, element)| element.into_item_bytes().ok()) + .filter_map(|(key, element)| { + match element.into_item_bytes() { + Ok(bytes) => Some(bytes), + Err(_) => { + tracing::debug!("Skipping non-item element at key {:?} in anchors", key); + None + } + } + }) .collect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/shielded/anchors/v0/mod.rs` around lines 59 - 64, The current silent drop of non-item elements in `results.to_key_elements().into_iter().filter_map(|(_key, element)| element.into_item_bytes().ok()).collect()` can hide data issues; update this code so failed `element.into_item_bytes()` conversions are surfaced: replace the `filter_map` with an explicit match on `element.into_item_bytes()` (or `match element.into_item_bytes()` inside the iterator) and either (a) emit a debug/log message (using the crate logger or `tracing::debug`) when an element fails to convert, or (b) add a clear comment above the `anchors` extraction that explains why non-item elements are expected and safe to ignore; reference the `anchors` variable, `results.to_key_elements()`, and `into_item_bytes()` when making the change.packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs (1)
634-634: Consider propagating the error instead of using.expect().While
chunk_power 11is documented as valid, using.expect()in protocol upgrade code can cause an unrecoverable panic. Propagating the error would be more defensive and consistent with the rest of this function.♻️ Proposed fix to propagate the error
self.drive.grove_insert_if_not_exists( (&shielded_pool_path).into(), &[SHIELDED_NOTES_KEY], - Element::empty_commitment_tree(11).expect("chunk_power 11 is valid"), + Element::empty_commitment_tree(11).map_err(|e| { + Error::Execution(ExecutionError::GroveDBError(format!( + "Failed to create commitment tree: {e}" + ))) + })?, Some(transaction), None, &platform_version.drive, )?;Note: The exact error type mapping may need adjustment based on available error variants in
ExecutionError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/perform_events_on_first_block_of_protocol_change/v0/mod.rs` at line 634, Replace the unconditional panic caused by Element::empty_commitment_tree(11).expect(...) with proper error propagation: call Element::empty_commitment_tree(11) and map or convert its Err into the appropriate ExecutionError variant (or a new variant if needed), then return Err(...) so the surrounding function (which returns a Result) propagates the failure instead of panicking; update any callers/types if necessary to accept that error path.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs (1)
8373-8456: Make this v11 2-in/1-out fixture unambiguous.Line 8408 switches this test to
DeductFromInput(0), while the latest-version fixture with the same shape usesReduceOutput(0). Either mirror that strategy or rename this test to includededuct_from_input, otherwise the cross-version comparison is apples-to-oranges.Possible rename if the strategy change is intentional
-fn test_fee_p2pkh_2_inputs_1_output_on_version_11() { +fn test_fee_p2pkh_2_inputs_1_output_deduct_from_input_on_version_11() {🤖 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/address_funds_transfer/tests.rs` around lines 8373 - 8456, The test test_fee_p2pkh_2_inputs_1_output_on_version_11 is ambiguous because it uses AddressFundsFeeStrategyStep::DeductFromInput(0) while the same-shaped latest-version fixture uses ReduceOutput(0); either make the test mirror the latest-version strategy by replacing DeductFromInput(0) with AddressFundsFeeStrategyStep::ReduceOutput(0) in the AddressFundsTransferTransitionV0::try_from_inputs_with_signer call, or if the different strategy is intentional, rename the test (e.g., append _deduct_from_input) and update any test description/assert messages to reflect the deduct_from_input strategy so cross-version comparisons are unambiguous.packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs (1)
543-565: Align with the established fee-validation pattern in this file.Other branches in this function (lines 47–95, 119–293) validate fees using
is_valid_with_data()and extract the payload viainto_data(), then pass it toSuccessfulPaidExecution. This branch discards the validated data by passingNoneand rebuilds aFeeResultmanually, creating an inconsistency. Refactor to match the established pattern:Suggested approach
let fee_validation_result = maybe_fee_validation_result .expect("fee validation result must exist for PaidFromAssetLockToPool"); - let mut all_errors = fee_validation_result.errors; - all_errors.extend(consensus_errors); - - if all_errors.is_empty() { + if fee_validation_result.is_valid_with_data() && consensus_errors.is_empty() { self.drive .apply_drive_operations( operations, @@ .map_err(Error::Drive)?; Ok(SuccessfulPaidExecution( - None, + Some(fee_validation_result.into_data()?), FeeResult::default_with_fees(0, fees_to_add_to_pool), )) } else { + let mut all_errors = fee_validation_result.errors; + all_errors.extend(consensus_errors); Ok(UnpaidConsensusExecutionError(all_errors)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/state_transition_processing/execute_event/v0/mod.rs` around lines 543 - 565, The branch currently unwraps maybe_fee_validation_result into fee_validation_result, discards the validated payload and constructs a FeeResult manually; instead follow the file's pattern: call is_valid_with_data() on fee_validation_result to ensure it's valid, extract the validated payload with into_data(), and pass that payload into SuccessfulPaidExecution (replacing the None) rather than rebuilding FeeResult by hand; keep the existing all_errors handling and only construct SuccessfulPaidExecution with the extracted fee data when is_valid_with_data() succeeds.packages/rs-drive-abci/src/query/shielded/recent_compacted_nullifier_changes/v0/mod.rs (1)
25-25: Extract the page size into a named constant.Line 25 hard-codes a protocol-facing batch limit. Lifting it into a module constant makes the sync contract explicit and reduces the chance of silent behavior drift later.
♻️ Proposed change
+const RECENT_COMPACTED_NULLIFIER_CHANGES_LIMIT: u16 = 25; + impl<C> Platform<C> { pub(super) fn query_recent_compacted_nullifier_changes_v0( &self, GetRecentCompactedNullifierChangesRequestV0 { start_block_height, @@ ) -> Result<QueryValidationResult<GetRecentCompactedNullifierChangesResponseV0>, Error> { - let limit = Some(25u16); + let limit = Some(RECENT_COMPACTED_NULLIFIER_CHANGES_LIMIT);As per coding guidelines, "Use SCREAMING_SNAKE_CASE for Rust constants".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/shielded/recent_compacted_nullifier_changes/v0/mod.rs` at line 25, Replace the hard-coded batch size assigned to limit with a module-level constant (e.g., declare a SCREAMING_SNAKE_CASE constant like PAGE_SIZE: u16 = 25) and use that constant where limit is set (change let limit = Some(25u16) to let limit = Some(PAGE_SIZE)); add the constant near the top of the module so the protocol-facing page size is explicit and discoverable.packages/rs-drive-abci/src/execution/platform_events/block_processing_end_events/record_shielded_pool_anchor/v0/mod.rs (1)
70-80: Using.last()is semantically correct but potentially confusing with limit 1.With
limit: Some(1), the result will have at most one element. Using.last()works but.next()would be clearer since there's only one possible element.♻️ Consider using `.next()` for clarity
let latest_stored_anchor: Option<[u8; 32]> = results .to_key_elements() .into_iter() - .last() + .next() .and_then(|(_key, element)| { if let Element::Item(value, _) = element { value.try_into().ok() } else { None } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/platform_events/block_processing_end_events/record_shielded_pool_anchor/v0/mod.rs` around lines 70 - 80, The code uses .into_iter().last() to grab the single element from results.to_key_elements() (assigned to latest_stored_anchor), which is correct but confusing given limit: Some(1); replace .into_iter().last() with .into_iter().next() to make intent clear and retain the existing and_then(Element::Item / value.try_into().ok()) logic; locate the expression constructing latest_stored_anchor and change the iterator call from last() to next() (references: latest_stored_anchor, results.to_key_elements(), Element::Item).packages/rs-drive-abci/src/execution/validation/state_transition/processor/v0/mod.rs (1)
222-229: Move the minimum shielded fee check earlier in the pipeline.This block is explicitly stateless, but it currently runs after the identity/address balance and nonce reads. Running it right after basic-structure validation would reject underfunded shielded transitions before the extra Drive work.
🤖 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/processor/v0/mod.rs` around lines 222 - 229, Move the stateless minimum shielded fee check (state_transition.has_shielded_proof_validation() and state_transition.validate_minimum_shielded_fee(platform_version)) to run immediately after the basic-structure validation step so underfunded shielded transitions are rejected before any identity/address balance or nonce reads; extract the existing block that returns Ok(ConsensusValidationResult::<ExecutionEvent>::new_with_errors(result.errors)) when invalid and place it right after the basic-structure validation return point, ensuring the same error handling and platform_version argument are preserved.packages/rs-drive-abci/src/query/shielded/nullifiers/v0/mod.rs (1)
92-109: Consider batching the non-proof lookup path.This does one
grove_has_rawcall per requested nullifier, so latency scales linearly with request size. If Drive exposes a batched existence read or a single-snapshot query path, using that here would keep this endpoint cheaper under load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/shielded/nullifiers/v0/mod.rs` around lines 92 - 109, The current code does a separate self.drive.grove_has_raw call for each nullifier (inside the map creating NullifierStatus using nullifiers_path and DirectQueryType::StatefulDirectQuery), causing linear latency; change this to use Drive's batched existence API or single-snapshot query so all nullifier keys are checked in one call and then map results back to create Vec<NullifierStatus>. Concretely: gather the nullifier keys, call the Drive batch existence method (or single-snapshot read) that accepts multiple keys and returns existence booleans or a map, then build entries by pairing each nullifier with its existence result instead of invoking self.drive.grove_has_raw per item.packages/rs-drive-abci/src/query/shielded/recent_nullifier_changes/mod.rs (1)
1-13: Consider moving module declaration to the top of the file.Conventionally, Rust module declarations (
mod v0;) are placed at the very top of the file before imports. This improves readability and follows common Rust style.Suggested reordering
+mod v0; + use crate::error::query::QueryError; use crate::error::Error; use crate::platform_types::platform::Platform; use crate::platform_types::platform_state::PlatformState; use crate::query::QueryValidationResult; use dapi_grpc::platform::v0::get_recent_nullifier_changes_request::Version as RequestVersion; use dapi_grpc::platform::v0::get_recent_nullifier_changes_response::Version as ResponseVersion; use dapi_grpc::platform::v0::{ GetRecentNullifierChangesRequest, GetRecentNullifierChangesResponse, }; use dpp::version::PlatformVersion; - -mod v0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/shielded/recent_nullifier_changes/mod.rs` around lines 1 - 13, Move the module declaration `mod v0;` to the top of the file before any `use` statements so the module is declared prior to importing symbols from the crate; update the file so `mod v0;` appears before the `use crate::...` and `use dapi_grpc::...` lines, ensuring no change to visibility or other code references (e.g., QueryError, Platform, GetRecentNullifierChangesRequest/Response, RequestVersion/ResponseVersion, PlatformVersion).packages/rs-drive-abci/src/query/shielded/encrypted_notes/v0/mod.rs (1)
62-66: Verify inclusive range boundary calculation.The end index is calculated as
start_index + limit as u64 - 1for an inclusive range query. Ensure this doesn't cause an underflow whenlimitis 0 (though the code above setseffectivetomax_noteswhencountis 0, solimitshould always be ≥1).The current logic is correct since
limitis derived fromeffectivewhich is always ≥1 whencount == 0. Consider adding a debug assertion for clarity:let effective = if count == 0 || count > max_notes { max_notes } else { count }; let limit = effective.min(u16::MAX as u32) as u16; + debug_assert!(limit >= 1, "limit should never be zero");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/shielded/encrypted_notes/v0/mod.rs` around lines 62 - 66, The end_index computation (end_index = start_index + limit as u64 - 1) can underflow if limit is 0; add a debug/assertion before that calculation verifying limit >= 1 (or effective >= 1) to make the invariant explicit — reference the variables start_index, limit, end_index and the upstream values effective, count, max_notes; keep the existing range insertion via Query::insert_range_inclusive as-is but gate it with the assertion so any unexpected zero limit fails fast in debug builds.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/transform_into_action/v0/mod.rs (2)
42-56: Consider consolidating repeated pattern matches.The same
match self { ShieldedTransferTransition::V0(v0) => ... }pattern is repeated three times to extractfee_amount,anchor, andnullifiers. Since there's only one variant currently, this could be consolidated for cleaner code.Proposed consolidation
- // The value_balance is the fee amount extracted from the shielded pool - let fee_amount: Credits = match self { - ShieldedTransferTransition::V0(v0) => v0.value_balance, - }; - - // The anchor from the transition (Merkle root of commitment tree) - let anchor: [u8; 32] = match self { - ShieldedTransferTransition::V0(v0) => v0.anchor, - }; - - // Extract nullifiers from the transition actions - let nullifiers: Vec<[u8; 32]> = match self { - ShieldedTransferTransition::V0(v0) => { - v0.actions.iter().map(|a| a.nullifier).collect() - } - }; + // Extract fields from the transition + let ShieldedTransferTransition::V0(v0) = self; + + // The value_balance is the fee amount extracted from the shielded pool + let fee_amount: Credits = v0.value_balance; + + // The anchor from the transition (Merkle root of commitment tree) + let anchor: [u8; 32] = v0.anchor; + + // Extract nullifiers from the transition actions + let nullifiers: Vec<[u8; 32]> = v0.actions.iter().map(|a| a.nullifier).collect();🤖 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/transform_into_action/v0/mod.rs` around lines 42 - 56, Consolidate the repeated pattern matches by matching once on ShieldedTransferTransition::V0 to bind v0 and then extract fee_amount, anchor, and nullifiers from that single binding; specifically, replace the three separate matches with a single match (or if-let) on ShieldedTransferTransition::V0(v0) and set fee_amount = v0.value_balance, anchor = v0.anchor, and nullifiers = v0.actions.iter().map(|a| a.nullifier).collect() so the code references the same bound v0 for all three variables.
64-76: UseInsufficientShieldedFeeErrorinstead ofInvalidShieldedProofErrorfor balance checks.The check at lines 64-76 validates insufficient pool balance for fees but incorrectly reports it as a proof validation failure.
InvalidShieldedProofErroris semantically meant for cryptographic proof verification failures, not fund availability issues. The dedicatedInsufficientShieldedFeeErroralready exists in the codebase and is the appropriate error type for this validation.Note: The same pattern appears in
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/transform_into_action/v0/mod.rs(lines 103-109) and should be corrected similarly.🤖 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/transform_into_action/v0/mod.rs` around lines 64 - 76, Replace the incorrect InvalidShieldedProofError returned in the balance check inside the block that tests `current_total_balance < fee_amount` (the early return that builds a `ConsensusValidationResult`) with the dedicated `InsufficientShieldedFeeError`; i.e., where the code currently constructs `StateError::InvalidShieldedProofError(dpp::consensus::state::shielded::invalid_shielded_proof_error::InvalidShieldedProofError::new(...)).into()`, construct `StateError::InsufficientShieldedFeeError(dpp::consensus::state::shielded::insufficient_shielded_fee_error::InsufficientShieldedFeeError::new(...)).into()` instead so the error accurately reflects insufficient pool funds; apply the same replacement in the analogous check in the unshield transform (the `current_total_balance < fee_amount` fee check in the unshield v0 module).packages/rs-drive-abci/src/query/shielded/recent_nullifier_changes/v0/mod.rs (1)
24-24: Consider extracting magic number to a named constant or platform version config.The hardcoded limit of
100u16could be made more maintainable by extracting it to a named constant or reading fromplatform_versionconfiguration, similar to howmax_encrypted_notes_per_queryis used in the encrypted notes query.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/shielded/recent_nullifier_changes/v0/mod.rs` at line 24, Replace the magic literal `100u16` used when setting `limit` with a named constant or a configuration value from `platform_version` (e.g., `RECENT_NULLIFIER_CHANGES_LIMIT` or `platform_version.recent_nullifier_changes_limit`) similar to how `max_encrypted_notes_per_query` is obtained; update the assignment to `limit` (the variable currently set with `let limit = Some(100u16);`) to read from the new constant or platform_version field and ensure the constant/field is documented and typed as u16 to match existing usage.packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs (1)
173-184: Clarify the discrepancy between fee validation and proof validation for ShieldedTransfer.In
validate_minimum_shielded_fee(line 79), the comment states "value_balance (u64) IS the fee" and usesv0.value_balanceas the fee. However, invalidate_shielded_proof(line 178), the code passes0asvalue_balancewith the comment "value_balance is 0 for shielded transfers (no net flow)".These seem to serve different purposes (fee vs. ZK proof verification), but the terminology overlap could cause confusion. Consider clarifying the comments to distinguish between:
- The fee amount derived from
value_balance(for fee validation)- The cryptographic
value_balanceparameter for proof verification (which may differ)🤖 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/processor/traits/shielded_proof.rs` around lines 173 - 184, Clarify the overloaded use of "value_balance" by updating comments in validate_minimum_shielded_fee and where reconstruct_and_verify_bundle is called for ShieldedTransfer: explain that v0.value_balance is treated as the fee amount for fee-validation logic (in validate_minimum_shielded_fee) while the value_balance parameter passed into reconstruct_and_verify_bundle (currently 0) is the ZK proof's cryptographic net-balance parameter used during proof verification; explicitly state that these are conceptually different and why we pass 0 into reconstruct_and_verify_bundle for ShieldedTransfer (no net transparent flow), referencing validate_minimum_shielded_fee, validate_shielded_proof (or the proof validation call site that invokes reconstruct_and_verify_bundle), reconstruct_and_verify_bundle, and v0.value_balance so reviewers can locate and update the comments accordingly.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs (1)
396-429: Extract the Orchard bundle fixtures once.
TEST_PROVING_KEY/get_proving_keyandserialize_authorized_bundleare duplicated across three submodules. Keeping one shared helper in this file ortest_helperswill reduce drift in a security-sensitive serialization path.Also applies to: 611-644, 1002-1035
🤖 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_withdrawal/tests.rs` around lines 396 - 429, Duplicate test helpers TEST_PROVING_KEY/get_proving_key and serialize_authorized_bundle should be extracted to a single shared helper so the Orchard bundle fixtures and serialization logic are defined once; move the OnceLock<ProvingKey> TEST_PROVING_KEY and its get_proving_key() function plus the serialize_authorized_bundle(bundle: &Bundle<OrchardAuthorized, i64, DashMemo>) -> (Vec<SerializedAction>, u8, i64, [u8;32], Vec<u8>, [u8;64]) implementation into a common test helpers location (either at the top of this test file or a test_helpers module) and update the three submodules to call that single helper instead of duplicating the code. Ensure the helper is visible to the test modules (pub(crate) or module-level) and preserve the exact serialization behavior (encrypted_note construction, flags/value_balance/anchor/proof/binding_sig extraction) so signatures remain unchanged.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/test_helpers.rs (1)
561-606: Batch dummy note inserts into one transaction.Line 584 opens and commits a GroveDB transaction for every note. These shielded suites call this helper with hundreds of notes, so setup time gets dominated by repeated transaction commits instead of the actual test logic.
Suggested change
pub fn insert_dummy_encrypted_notes(platform: &TempPlatform<MockCoreRPCLike>, count: u64) { use grovedb_commitment_tree::{DashMemo, NoteBytesData, TransmittedNoteCiphertext}; let platform_version = PlatformVersion::latest(); let grove_version = &platform_version.drive.grove_version; let pool_path = shielded_credit_pool_path(); + let transaction = platform.drive.grove.start_transaction(); for i in 0..count { // Generate a deterministic dummy cmx from the index. // Use a valid Pallas base field element (just set it to a small value). let mut cmx = [0u8; 32]; @@ let dummy_rho = [0u8; 32]; // dummy nullifier for rho derivation - let transaction = platform.drive.grove.start_transaction(); platform .drive .grove .commitment_tree_insert( &pool_path, @@ ) .unwrap() .expect("should insert dummy note"); - - platform - .drive - .grove - .commit_transaction(transaction) - .unwrap() - .expect("should commit transaction"); } + + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("should commit transaction"); }🤖 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/test_helpers.rs` around lines 561 - 606, The helper insert_dummy_encrypted_notes opens and commits a GroveDB transaction for each note causing huge overhead; instead, start a single transaction before the loop, call drive.grove.commitment_tree_insert(...) for every generated cmx/ciphertext inside the loop using that same transaction, then after the loop call drive.grove.commit_transaction(transaction) once; update references to transaction (created by platform.drive.grove.start_transaction()) and ensure you still unwrap/expect the insert and final commit results as before (commitment_tree_insert, commit_transaction) so behavior is unchanged but all inserts are batched into one transaction.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/mod.rs (1)
22-30: Keep this transformer trait internal.Line 22 makes a version-dispatch hook part of the public crate API. Unless external crates are expected to implement it, narrow the visibility to
pub(in crate::execution)orpub(crate)to avoid locking this internal validation surface into semver.Suggested change
-pub trait StateTransitionShieldedTransferTransitionActionTransformer { +pub(in crate::execution) trait StateTransitionShieldedTransferTransitionActionTransformer {🤖 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/mod.rs` around lines 22 - 30, The trait StateTransitionShieldedTransferTransitionActionTransformer is currently public but is an internal version-dispatch hook; make it non-public by changing its visibility (e.g., from `pub trait StateTransitionShieldedTransferTransitionActionTransformer` to `pub(crate) trait StateTransitionShieldedTransferTransitionActionTransformer` or `pub(in crate::execution) trait ...`) so external crates cannot implement it; keep the trait name and method transform_into_action_for_shielded_transfer_transition unchanged while updating only the visibility modifier.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs (1)
365-397: Share one Orchard test cache/helper across the file.The file now has three independent
OnceLock<ProvingKey>caches and three copies ofserialize_authorized_bundle(). That can rebuild the proving key multiple times in one test binary and makes future bundle-format changes easy to miss in one module.Also applies to: 565-597, 818-850
🤖 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/unshield/tests.rs` around lines 365 - 397, There are duplicate OnceLock<ProvingKey> caches and three copies of serialize_authorized_bundle; consolidate them into a single shared helper at module scope: define one static TEST_PROVING_KEY: OnceLock<ProvingKey> and one get_proving_key() that calls ProvingKey::build, and replace all other OnceLock and get_proving_key duplicates to reference this single static; likewise extract the serialize_authorized_bundle(bundle: &Bundle<OrchardAuthorized, i64, DashMemo>) implementation into one shared function and update all call sites to use it, removing the duplicate implementations so the proving key is built only once and bundle serialization logic is centralized.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/tests.rs (1)
1079-1118: Reuse the file-level Orchard helpers inreturn_proof.This block re-declares both the proving-key cache and
serialize_authorized_bundle(), so the expensive Orchard key build can happen twice in one test binary and bundle-layout changes have to be kept in sync manually.🤖 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/shield/tests.rs` around lines 1079 - 1118, The test duplicates the file-level Orchard helpers (TEST_PROVING_KEY, get_proving_key, and serialize_authorized_bundle) inside return_proof; remove the local duplicates and call the shared get_proving_key() and serialize_authorized_bundle() helpers instead so the proving key is built once and bundle serialization stays centralized; update return_proof to reference TEST_PROVING_KEY/get_proving_key and the top-level serialize_authorized_bundle function rather than re-declaring them.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/transform_into_action/v0/mod.rs (2)
41-48: Consider extracting common fields via a method on UnshieldTransition.The pattern matching on
UnshieldTransition::V0is repeated multiple times (lines 41-43, 46-48, 99-101). IfUnshieldTransitionexposes accessor methods like.anchor(),.actions(), and.unshielding_amount(), this would reduce duplication and improve maintainability.🤖 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/unshield/transform_into_action/v0/mod.rs` around lines 41 - 48, Introduce accessor methods on the UnshieldTransition enum (e.g., fn anchor(&self) -> [u8;32], fn actions(&self) -> &Vec<ActionType>, fn unshielding_amount(&self) -> u64) that internally match on UnshieldTransition::V0 and return the corresponding fields, then replace the repeated match expressions that extract anchor, nullifiers (from actions), and unshielding_amount with calls to these accessors (use .actions() to map nullifiers). Update any other places that match on UnshieldTransition::V0 to use the new methods to remove duplication and centralize variant handling.
103-115: Semantic mismatch:InvalidShieldedProofErrorused for insufficient balance.Using
InvalidShieldedProofErrorfor an insufficient pool balance condition is semantically misleading. The proof may be perfectly valid, but the pool simply lacks funds. Consider using a dedicated error type likeInsufficientPoolBalanceErrorfor clearer error classification and debugging.If this is intentional (e.g., to avoid leaking pool state information), please add a comment explaining the rationale.
🤖 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/unshield/transform_into_action/v0/mod.rs` around lines 103 - 115, The branch that returns InvalidShieldedProofError for an insufficient pool balance is semantically wrong; replace the error with a dedicated InsufficientPoolBalanceError (e.g. create dpp::consensus::state::shielded::insufficient_pool_balance_error::InsufficientPoolBalanceError and construct it with a message including current_total_balance and amount) and return that via ConsensusValidationResult in the same spot inside transform_into_action v0 (the block comparing current_total_balance < amount), or if the original choice was intentional to avoid leaking pool state, add a clear comment above the check explaining that rationale and why InvalidShieldedProofError is used instead.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_common/mod.rs (1)
232-263: Full table scan for anchor validation may not scale.The function queries all anchors (
Query::new_range_full()) and iterates through them to find a match. This is O(n) where n is the number of stored anchors. If anchors accumulate over time, this could become a performance bottleneck.Consider restructuring the anchor storage to allow direct lookup by anchor value (e.g., storing
anchor_bytes → block_heightinstead ofblock_height → anchor_bytes), enabling O(1) existence checks withgrove_has_raw.🤖 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_common/mod.rs` around lines 232 - 263, The current code builds a PathQuery with shielded_credit_pool_anchors_path and Query::new_range_full then scans results via grove.query_raw to find a matching anchor (O(n)); instead, change the storage/query to allow direct lookup by anchor value and replace the range scan with a direct existence check (use grove_has_raw or a point query). Concretely: migrate or re-index storage from block_height → anchor_bytes to anchor_bytes → block_height (or add a secondary index), update shielded_credit_pool_anchors_path or add a new path builder that includes the anchor bytes, and then call grove_has_raw (or a point query via grove.query_raw for the specific path) to test existence of the anchor instead of iterating results; update any callers/tests relying on the old layout accordingly.
packages/rs-drive-abci/src/execution/engine/run_block_proposal/v0/mod.rs
Show resolved
Hide resolved
.../execution/platform_events/block_processing_end_events/record_shielded_pool_anchor/v0/mod.rs
Outdated
Show resolved
Hide resolved
.../rs-drive-abci/src/execution/validation/state_transition/processor/traits/basic_structure.rs
Outdated
Show resolved
Hide resolved
...rs-drive-abci/src/execution/validation/state_transition/processor/traits/identity_balance.rs
Show resolved
Hide resolved
packages/rs-drive-abci/src/query/shielded/nullifiers_branch_state/v0/mod.rs
Show resolved
Hide resolved
packages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rs
Show resolved
Hide resolved
… bugs - Comment out shielded query services pending dapi-grpc protobuf types - Fix struct field mismatches in shielded tests (flags/value_balance renamed) - Fix proof verification: use correct flags and value_balance for bundle reconstruction (ShieldedTransfer was ignoring fee, Unshield/ShieldedWithdrawal used wrong flags constant) - Comment out obsolete validation tests and shielded strategy tests - Remove unreachable pattern in is_allowed.rs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Nothing ever creates this action. Remove the enum variant, its module, and all match arms referencing it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… version Check the platform version field before calling validate_structure for each shielded transition, matching the pattern used by other gated transitions. Also update has_basic_structure_validation to return .is_some() on the corresponding version field. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…alidation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs (1)
217-305:⚠️ Potential issue | 🟠 MajorThese cases still short-circuit before note/anchor/nullifier validation.
create_default_shielded_withdrawal_transition()withdraws111_549_800, but these tests only seed10_000and keep dummy proof bytes. That meansInvalidShieldedProofErrorcan be produced before pool-note existence, anchor membership, spent-nullifier, or intra-bundle dedup logic runs. Use a valid bundle and fund the pool above the withdrawn amount before mutating the targeted state.Also applies to: 820-857
🤖 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_withdrawal/tests.rs` around lines 217 - 305, Tests short-circuit because create_default_shielded_withdrawal_transition() withdraws 111_549_800 while tests only fund the pool with 10_000 and use dummy proof bytes, so proof verification fails before pool/anchor/nullifier checks; fix each failing test by either (a) using a transition that contains a valid proof and bundle (e.g., create or call a helper like create_valid_shielded_withdrawal_transition()) or (b) reduce the withdrawal amount in the transition to <= the seeded pool balance (or increase the seeded pool via set_pool_total_balance to exceed the transition amount) before mutating state with insert_dummy_encrypted_notes, insert_anchor_into_state, or insert_nullifier_into_state so the intended pool-note/anchor/nullifier validation paths are reached instead of InvalidShieldedProofError.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/tests.rs (1)
163-217:⚠️ Potential issue | 🟠 MajorThese tests still never hit the anchor/nullifier branches.
They keep the dummy bundle from
create_default_shielded_transfer_transition()and never fund the pool above its111_548_800value-balance, soInvalidShieldedProofErrorcan be returned before anchor lookup, already-spent nullifier checks, or intra-bundle dedup runs. Build a valid bundle, callset_pool_total_balance(...), then perturb only the anchor/nullifier state.Also applies to: 944-977
🤖 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` around lines 163 - 217, The tests currently fail to reach anchor/nullifier validation because they use the dummy bundle from create_default_shielded_transfer_transition() and never increase the pool value-balance, so proof verification (InvalidShieldedProofError) triggers first; to fix, construct a valid shielded bundle (or use the helper that builds a valid bundle) and call set_pool_total_balance(...) to raise the pool's total balance above 111_548_800 before calling process_transition, then keep the rest of the setup that perturbs only state (use insert_anchor_into_state(&platform, &anchor) and insert_nullifier_into_state(&platform, &nullifier)) so the transition proceeds past proof verification to hit the anchor/nullifier branches (also replace usage of the dummy serialized action from create_default_shielded_transfer_transition() with the valid bundle builder).packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs (1)
202-280:⚠️ Potential issue | 🟠 MajorThese tests still don't exercise the intended state validators.
create_default_unshield_transition()requires111_549_800from the pool, but these cases either never seed the pool or only seed10_000, and they keep dummy proof bytes.InvalidShieldedProofErrorcan therefore come from the earlier balance/proof gates instead of pool-note, anchor, spent-nullifier, or duplicate-nullifier validation. Use a valid bundle and fund the pool above the unshield amount before mutating state.Also applies to: 681-717
🤖 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/unshield/tests.rs` around lines 202 - 280, Tests are failing to reach the intended state validators because create_default_unshield_transition() uses dummy proof bytes and expects 111_549_800 from the pool, but the tests either don't seed the pool sufficiently (only 10_000 or none) and so the earlier proof/balance checks trigger InvalidShieldedProofError; update the tests to (1) use a valid unshield transition (replace create_default_unshield_transition() with a helper that produces a valid bundle/proof or fix the dummy proof to be accepted) and (2) seed the pool with >=111_549_800 encrypted notes before mutating state (use/adjust insert_dummy_encrypted_notes(&platform, amount) or the pool seeding helper) so the code paths reach anchor/nullifier/duplicate-nullifier validators (while keeping the existing insert_anchor_into_state and insert_nullifier_into_state calls).
🧹 Nitpick comments (6)
packages/rs-drive-abci/src/query/shielded/mod.rs (1)
1-9: Placeholder file with clear dependency blocker documented.The TODO clearly explains the blocker (dapi-grpc shielded protobuf types). This is a reasonable approach for staging code that depends on external components not yet available.
Consider tracking this TODO in an issue to ensure it's not forgotten once the protobuf types become available.
Would you like me to open an issue to track re-enabling these shielded query modules once dapi-grpc protobuf types are available?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/query/shielded/mod.rs` around lines 1 - 9, The file mod.rs currently comments out shielded query modules (anchors, encrypted_notes, nullifiers, nullifiers_branch_state, nullifiers_trunk_state, pool_state, recent_compacted_nullifier_changes, recent_nullifier_changes) with a TODO noting the dependency on dapi-grpc shielded protobuf types; create a tracking issue titled like "Re-enable shielded query modules when dapi-grpc shielded protobuf types available" that lists these exact modules, references this PR, assigns an owner, adds acceptance criteria (protobuf types released, compile + tests passing, remove comments and restore mod declarations in mod.rs), and add it to the appropriate milestone so the TODO isn’t forgotten.packages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rs (1)
77-125: Test naming convention.Per coding guidelines, tests should be named descriptively starting with "should …". Consider renaming to something like
should_shield_funds_from_funded_addressesfor consistency with the naming convention.That said, the
run_chain_*naming pattern is descriptive and common in Rust integration tests, so this is a minor suggestion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rs` around lines 77 - 125, The test function name run_chain_shield_transitions does not follow the project convention of starting tests with "should …"; rename the function to should_shield_funds_from_funded_addresses (i.e., update the fn name for the test defined as fn run_chain_shield_transitions() to fn should_shield_funds_from_funded_addresses()) and keep the body and assertions unchanged so the test runner picks it up with the descriptive naming convention.packages/rs-drive-abci/tests/strategy_tests/strategy.rs (1)
155-175: Hide the parked shielded strategy plumbing until it becomes executable.
ShieldedStateis currently just a stub and every shielded branch/helper in this file is commented out, but the live strategy API still threads&mut Option<ShieldedState>through callers. That leaves inert surface area in the harness and makes it harder to tell whether shielded strategy coverage is actually enabled. Prefer moving the parked shielded code behind#[cfg(...)]or into a separate module, and reintroduce the parameter only onceOperationTypecan exercise it again.Also applies to: 672-673, 1892-1988, 1996-2010, 2611-3041
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/tests/strategy_tests/strategy.rs` around lines 155 - 175, The stubbed ShieldedState and the commented-out shielded helpers are still threaded through the live strategy API (e.g., the ShieldedState type and NetworkStrategy helper methods like create_shield_transition/create_shielded_transfer_transition/create_unshield_transition), leaving inert surface area; hide this parked plumbing behind a compile-time cfg or move it to a separate module and remove the &mut Option<ShieldedState> parameter from public strategy APIs until OperationType supports shielded variants. Concretely: wrap the ShieldedState definition, its original methods (new, record_shielded_note, checkpoint, take_spendable_note, has_spendable_notes), and the serialize_authorized_bundle plus the five NetworkStrategy helper methods in a #[cfg(feature = "shielded")]/#[cfg(feature = "ops_shielded")] or move them into a private module, then update callers to stop accepting or threading &mut Option<ShieldedState> unless the cfg/feature is enabled so the harness surface no longer exposes inert shielded plumbing.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/transform_into_action/v0/mod.rs (1)
41-48: Multiple match statements on the same enum variant.The code matches on
UnshieldTransition::V0three separate times (lines 41-43, 46-48, 99-101) to extractanchor,nullifiers, andunshielding_amount. Consider destructuring once to improve readability.✨ Suggested improvement
+ // Extract all fields from the transition in one match + let (anchor, nullifiers, amount) = match self { + UnshieldTransition::V0(v0) => ( + v0.anchor, + v0.actions.iter().map(|a| a.nullifier).collect::<Vec<_>>(), + v0.unshielding_amount, + ), + }; + - // The anchor from the transition (Merkle root of commitment tree) - let anchor: [u8; 32] = match self { - UnshieldTransition::V0(v0) => v0.anchor, - }; - - // Extract nullifiers from the transition actions - let nullifiers: Vec<[u8; 32]> = match self { - UnshieldTransition::V0(v0) => v0.actions.iter().map(|a| a.nullifier).collect(), - }; // ... later in the function ... - let amount = match self { - UnshieldTransition::V0(v0) => v0.unshielding_amount, - };Also applies to: 99-101
🤖 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/unshield/transform_into_action/v0/mod.rs` around lines 41 - 48, The code repeatedly matches on UnshieldTransition::V0 to extract anchor, nullifiers, and unshielding_amount; refactor by performing a single match/destructure of self (e.g., let UnshieldTransition::V0(v0) = self; or match once and bind v0) at the start of the function/module and then derive anchor (v0.anchor), nullifiers (v0.actions.iter().map(|a| a.nullifier).collect()), and unshielding_amount (v0.unshielding_amount) from that single binding to improve readability and avoid duplicate matches.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rs (2)
836-847: Minor: Prefer slice over&vec![].Line 839 uses
&vec![transition_bytes]which creates a heap allocation. Consider using a slice directly.✨ Suggested improvement
let processing_result = platform .platform .process_raw_state_transitions( - &vec![transition_bytes], + &[transition_bytes], &platform_state, &BlockInfo::default(), &transaction,🤖 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/shield_from_asset_lock/tests.rs` around lines 836 - 847, The code passes an allocated Vec to process_raw_state_transitions using &vec![transition_bytes]; change this to pass a slice to avoid the heap allocation by using a slice reference (e.g. &[transition_bytes] or std::slice::from_ref(&transition_bytes)) when calling platform.platform.process_raw_state_transitions so the argument remains the same type but without creating a Vec; update the call site where transition_bytes is currently wrapped in vec! to use the slice form instead.
732-768: Duplicated helper code within the same test module.
TEST_PROVING_KEY,get_proving_key(), andserialize_authorized_bundle()are duplicated between the outertestsmodule (lines 128-166) and the nestedreturn_proofmodule (lines 732-768). Consider extractingserialize_authorized_bundleto the outer scope and reusing it in both contexts. TheOnceLockstatic can remain in each module if needed, but the serialization logic is identical.🤖 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/shield_from_asset_lock/tests.rs` around lines 732 - 768, The serialize_authorized_bundle helper is duplicated; extract the function serialize_authorized_bundle out of the nested return_proof module into the outer tests module and have both modules call that single implementation, keeping TEST_PROVING_KEY and get_proving_key as module-local OnceLocks if desired; update references to call the moved serialize_authorized_bundle and remove the duplicate definition in the nested module so only one serialization implementation exists.
🤖 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/types/execution_event/mod.rs`:
- Around line 70-79: The PaidFromShieldedPool variant is missing a place to
carry execution_operations/execution_context so precalculated
ValidationOperation::PrecalculatedOperation from
ShieldedTransferStateTransitionTransformIntoActionValidationV0::transform_into_action_v0
is never enforced; update the PaidFromShieldedPool enum to include the same
execution_operations/execution_context field used by PaidFromAssetLockToPool,
populate that field when creating the PaidFromShieldedPool event in
transform_into_action_v0, and then mirror the PaidFromAssetLockToPool handling
in validate_fees_of_event() and execute_event() so the precalculated fee is
consumed and the pool-balance/anchor/nullifier reads for shielded
transfer/unshield/shielded withdrawal are properly charged.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs`:
- Around line 73-97: The check is using unshielding_amount as the fee for
Unshield and ShieldedWithdrawal branches; instead compute the actual fee
(value_balance minus unshielding_amount) or defer to the same fee_amount()
calculation used elsewhere so the minimum_shielded_fee comparison is correct: in
StateTransition::Unshield (unshield_transition::UnshieldTransition::V0) and
StateTransition::ShieldedWithdrawal
(shielded_withdrawal_transition::ShieldedWithdrawalTransition::V0) replace the
tuple’s fee value (currently v0.unshielding_amount as i64) with the true fee
computed as (v0.value_balance as i64 - v0.unshielding_amount as i64) (or move
the minimum_shielded_fee check until after building actions and calling
fee_amount()), leaving the num_actions extraction (v0.actions.len()) unchanged
and keeping the Shield and ShieldedTransfer handling as-is.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rs`:
- Around line 304-316: The test defines an unused variable value_balance (let
value_balance = -(shield_amount as i64)); — remove that unused declaration or
use it consistently; specifically, in the test that builds the transition with
create_signed_shield_from_asset_lock_transition(...) either delete the
value_balance line (since shield_amount is already passed) or replace the
shield_amount argument with value_balance where appropriate if the intent was to
pass a signed/negative balance; update only the variable usage around the
create_signed_shield_from_asset_lock_transition call and remove the unused
binding.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/tests.rs`:
- Around line 269-292: The test currently expects a proof-verification failure
but never funds the pool, so transform_into_action_v0() fails earlier due to
current_total_balance < value_balance; to fix, ensure the pool is funded before
calling process_transition by setting the pool total balance (call
set_pool_total_balance or otherwise update the platform state) for the pool used
by create_default_shielded_transfer_transition (and/or adjust the transition
created by create_default_shielded_transfer_transition to reference a funded
pool), so the code reaches reconstruct_and_verify_bundle() and yields an
InvalidShieldedProofError from proof verification rather than a balance-gate
error; apply the same change to the other test block around lines 377-407.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs`:
- Around line 357-383: The test's proof-verification path is never reached
because set_pool_total_balance(&platform, 10_000) is below the default
unshielding amount (111_549_800) and triggers the balance precondition failure;
increase the seeded pool balance to at least the default unshielding_amount
before calling process_transition so the test exercises proof reconstruction and
encrypted-note checks (update the call to set_pool_total_balance in the test
that uses create_default_shielded_withdrawal_transition and process_transition;
apply the same change to the similar test block around lines 473-509).
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs`:
- Around line 333-357: The test test_invalid_proof_returns_shielded_proof_error
is failing because the pool balance isn't seeded high enough, so
process_transition returns a balance-related error before reaching
reconstruct_and_verify_bundle() to detect the malformed proof; update the test
to fund the pool (e.g., increase whatever setup function or call that seeds pool
balance after insert_dummy_encrypted_notes and before calling
process_transition) so the balance exceeds 111_549_800 and then call
process_transition with the transition created by
create_default_unshield_transition(); this ensures the code reaches
reconstruct_and_verify_bundle()/proof verification and produces the expected
InvalidShieldedProofError.
---
Duplicate comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/tests.rs`:
- Around line 163-217: The tests currently fail to reach anchor/nullifier
validation because they use the dummy bundle from
create_default_shielded_transfer_transition() and never increase the pool
value-balance, so proof verification (InvalidShieldedProofError) triggers first;
to fix, construct a valid shielded bundle (or use the helper that builds a valid
bundle) and call set_pool_total_balance(...) to raise the pool's total balance
above 111_548_800 before calling process_transition, then keep the rest of the
setup that perturbs only state (use insert_anchor_into_state(&platform, &anchor)
and insert_nullifier_into_state(&platform, &nullifier)) so the transition
proceeds past proof verification to hit the anchor/nullifier branches (also
replace usage of the dummy serialized action from
create_default_shielded_transfer_transition() with the valid bundle builder).
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs`:
- Around line 217-305: Tests short-circuit because
create_default_shielded_withdrawal_transition() withdraws 111_549_800 while
tests only fund the pool with 10_000 and use dummy proof bytes, so proof
verification fails before pool/anchor/nullifier checks; fix each failing test by
either (a) using a transition that contains a valid proof and bundle (e.g.,
create or call a helper like create_valid_shielded_withdrawal_transition()) or
(b) reduce the withdrawal amount in the transition to <= the seeded pool balance
(or increase the seeded pool via set_pool_total_balance to exceed the transition
amount) before mutating state with insert_dummy_encrypted_notes,
insert_anchor_into_state, or insert_nullifier_into_state so the intended
pool-note/anchor/nullifier validation paths are reached instead of
InvalidShieldedProofError.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs`:
- Around line 202-280: Tests are failing to reach the intended state validators
because create_default_unshield_transition() uses dummy proof bytes and expects
111_549_800 from the pool, but the tests either don't seed the pool sufficiently
(only 10_000 or none) and so the earlier proof/balance checks trigger
InvalidShieldedProofError; update the tests to (1) use a valid unshield
transition (replace create_default_unshield_transition() with a helper that
produces a valid bundle/proof or fix the dummy proof to be accepted) and (2)
seed the pool with >=111_549_800 encrypted notes before mutating state
(use/adjust insert_dummy_encrypted_notes(&platform, amount) or the pool seeding
helper) so the code paths reach anchor/nullifier/duplicate-nullifier validators
(while keeping the existing insert_anchor_into_state and
insert_nullifier_into_state calls).
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rs`:
- Around line 836-847: The code passes an allocated Vec to
process_raw_state_transitions using &vec![transition_bytes]; change this to pass
a slice to avoid the heap allocation by using a slice reference (e.g.
&[transition_bytes] or std::slice::from_ref(&transition_bytes)) when calling
platform.platform.process_raw_state_transitions so the argument remains the same
type but without creating a Vec; update the call site where transition_bytes is
currently wrapped in vec! to use the slice form instead.
- Around line 732-768: The serialize_authorized_bundle helper is duplicated;
extract the function serialize_authorized_bundle out of the nested return_proof
module into the outer tests module and have both modules call that single
implementation, keeping TEST_PROVING_KEY and get_proving_key as module-local
OnceLocks if desired; update references to call the moved
serialize_authorized_bundle and remove the duplicate definition in the nested
module so only one serialization implementation exists.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/transform_into_action/v0/mod.rs`:
- Around line 41-48: The code repeatedly matches on UnshieldTransition::V0 to
extract anchor, nullifiers, and unshielding_amount; refactor by performing a
single match/destructure of self (e.g., let UnshieldTransition::V0(v0) = self;
or match once and bind v0) at the start of the function/module and then derive
anchor (v0.anchor), nullifiers (v0.actions.iter().map(|a|
a.nullifier).collect()), and unshielding_amount (v0.unshielding_amount) from
that single binding to improve readability and avoid duplicate matches.
In `@packages/rs-drive-abci/src/query/shielded/mod.rs`:
- Around line 1-9: The file mod.rs currently comments out shielded query modules
(anchors, encrypted_notes, nullifiers, nullifiers_branch_state,
nullifiers_trunk_state, pool_state, recent_compacted_nullifier_changes,
recent_nullifier_changes) with a TODO noting the dependency on dapi-grpc
shielded protobuf types; create a tracking issue titled like "Re-enable shielded
query modules when dapi-grpc shielded protobuf types available" that lists these
exact modules, references this PR, assigns an owner, adds acceptance criteria
(protobuf types released, compile + tests passing, remove comments and restore
mod declarations in mod.rs), and add it to the appropriate milestone so the TODO
isn’t forgotten.
In `@packages/rs-drive-abci/tests/strategy_tests/strategy.rs`:
- Around line 155-175: The stubbed ShieldedState and the commented-out shielded
helpers are still threaded through the live strategy API (e.g., the
ShieldedState type and NetworkStrategy helper methods like
create_shield_transition/create_shielded_transfer_transition/create_unshield_transition),
leaving inert surface area; hide this parked plumbing behind a compile-time cfg
or move it to a separate module and remove the &mut Option<ShieldedState>
parameter from public strategy APIs until OperationType supports shielded
variants. Concretely: wrap the ShieldedState definition, its original methods
(new, record_shielded_note, checkpoint, take_spendable_note,
has_spendable_notes), and the serialize_authorized_bundle plus the five
NetworkStrategy helper methods in a #[cfg(feature = "shielded")]/#[cfg(feature =
"ops_shielded")] or move them into a private module, then update callers to stop
accepting or threading &mut Option<ShieldedState> unless the cfg/feature is
enabled so the harness surface no longer exposes inert shielded plumbing.
In `@packages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rs`:
- Around line 77-125: The test function name run_chain_shield_transitions does
not follow the project convention of starting tests with "should …"; rename the
function to should_shield_funds_from_funded_addresses (i.e., update the fn name
for the test defined as fn run_chain_shield_transitions() to fn
should_shield_funds_from_funded_addresses()) and keep the body and assertions
unchanged so the test runner picks it up with the descriptive naming convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc1bb6c7-822d-48c2-bcd0-253d5876d0ce
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
packages/rs-drive-abci/src/execution/types/execution_event/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/is_allowed.rspackages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/tests.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rspackages/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.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/transform_into_action/v0/mod.rspackages/rs-drive-abci/src/query/service.rspackages/rs-drive-abci/src/query/shielded/mod.rspackages/rs-drive-abci/tests/strategy_tests/strategy.rspackages/rs-drive-abci/tests/strategy_tests/test_cases/shielded_tests.rspackages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/mod.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/system/mod.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/system/penalize_shielded_pool.rspackages/rs-drive/src/state_transition_action/mod.rspackages/rs-drive/src/state_transition_action/system/mod.rspackages/rs-drive/src/state_transition_action/system/penalize_shielded_pool_action/mod.rspackages/rs-drive/src/state_transition_action/system/penalize_shielded_pool_action/v0/mod.rs
💤 Files with no reviewable changes (7)
- packages/rs-drive/src/state_transition_action/system/mod.rs
- packages/rs-drive/src/state_transition_action/mod.rs
- packages/rs-drive/src/state_transition_action/system/penalize_shielded_pool_action/v0/mod.rs
- packages/rs-drive/src/state_transition_action/action_convert_to_operations/system/penalize_shielded_pool.rs
- packages/rs-drive/src/state_transition_action/system/penalize_shielded_pool_action/mod.rs
- packages/rs-drive/src/state_transition_action/action_convert_to_operations/mod.rs
- packages/rs-drive/src/state_transition_action/action_convert_to_operations/system/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs
- packages/rs-drive-abci/src/query/service.rs
...s/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs
Show resolved
Hide resolved
.../src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rs
Show resolved
Hide resolved
...-abci/src/execution/validation/state_transition/state_transitions/shielded_transfer/tests.rs
Show resolved
Hide resolved
...bci/src/execution/validation/state_transition/state_transitions/shielded_withdrawal/tests.rs
Show resolved
Hide resolved
.../rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3220 +/- ##
============================================
- Coverage 50.39% 50.38% -0.02%
============================================
Files 3089 3131 +42
Lines 219104 221974 +2870
============================================
+ Hits 110426 111833 +1407
- Misses 108678 110141 +1463
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs (1)
88-101:⚠️ Potential issue | 🟠 MajorFee computation is incorrect for Unshield and ShieldedWithdrawal.
The doc comment at line 55-56 states "fee = value_balance - amount", but the implementation uses
unshielding_amountdirectly as the fee. According to theUnshieldTransitionV0struct,unshielding_amountrepresents "Total credits leaving the shielded pool (recipient amount + fee)"—not the fee itself.Additionally, the inline comment on line 93 states "fee is validated separately", which contradicts using this value for fee validation here.
This allows large unshield/withdrawal transactions to pass the minimum fee check even if the actual platform fee component is below the required minimum.
,
🔧 Suggested approach
Either:
- Compute the actual fee (requires access to the recipient amount), or
- Defer this check until after action transformation and use the
fee_amount()method available onUnshieldAction/ShieldedWithdrawalAction, or- If fee validation truly happens elsewhere for these transitions, return early like
Shielddoes.// Unshield: fee = value_balance - amount. StateTransition::Unshield(st) => match st { dpp::state_transition::unshield_transition::UnshieldTransition::V0( v0, ) => { - // unshielding_amount is the total leaving the pool (fee is validated separately) - (v0.unshielding_amount as i64, v0.actions.len()) + // TODO: Compute actual fee = unshielding_amount - recipient_amount + // For now, skip validation here if handled elsewhere + return Ok(SimpleConsensusValidationResult::new()) } }, - StateTransition::ShieldedWithdrawal(st) => match st { - dpp::state_transition::shielded_withdrawal_transition::ShieldedWithdrawalTransition::V0(v0) => { - (v0.unshielding_amount as i64, v0.actions.len()) - } - }, + StateTransition::ShieldedWithdrawal(st) => { + // TODO: Compute actual fee = unshielding_amount - recipient_amount + // For now, skip validation here if handled elsewhere + return Ok(SimpleConsensusValidationResult::new()) + }🤖 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/processor/traits/shielded_proof.rs` around lines 88 - 101, The Unshield and ShieldedWithdrawal branches are using v0.unshielding_amount as the fee (in StateTransition::Unshield handling and StateTransition::ShieldedWithdrawal handling), but unshielding_amount is total leaving the pool (recipient + fee); change the logic to not treat unshielding_amount as the fee: either compute the fee by subtracting the recipient amount from unshielding_amount (if recipient amount is available in UnshieldTransitionV0/ShieldedWithdrawalTransitionV0), or defer this check until after action transformation and use each action's fee_amount() on UnshieldAction/ShieldedWithdrawalAction, or—if fee validation truly occurs elsewhere—mirror the Shield branch behavior and return/skip fee validation here; update the matching arms in the StateTransition::Unshield and StateTransition::ShieldedWithdrawal handlers accordingly to use the correct fee source.
🤖 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/processor/traits/basic_structure.rs`:
- Around line 380-414: The has_basic_structure_validation() implementation
currently returns false for shielded variants (StateTransition::Shield,
ShieldedTransfer, Unshield, ShieldFromAssetLock, ShieldedWithdrawal) when their
platform_version.drive_abci...basic_structure is None, which causes callers to
skip validate_basic_structure() and avoid returning VersionNotActive; change
those branches to return true (i.e., preserve the guard so
validate_basic_structure() is called) so that validate_basic_structure() can
produce VersionNotActive when basic_structure is None; update the matches for
the listed StateTransition variants in has_basic_structure_validation() to
mirror the behavior of the other transitions and the validate_basic_structure()
path.
---
Duplicate comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs`:
- Around line 88-101: The Unshield and ShieldedWithdrawal branches are using
v0.unshielding_amount as the fee (in StateTransition::Unshield handling and
StateTransition::ShieldedWithdrawal handling), but unshielding_amount is total
leaving the pool (recipient + fee); change the logic to not treat
unshielding_amount as the fee: either compute the fee by subtracting the
recipient amount from unshielding_amount (if recipient amount is available in
UnshieldTransitionV0/ShieldedWithdrawalTransitionV0), or defer this check until
after action transformation and use each action's fee_amount() on
UnshieldAction/ShieldedWithdrawalAction, or—if fee validation truly occurs
elsewhere—mirror the Shield branch behavior and return/skip fee validation here;
update the matching arms in the StateTransition::Unshield and
StateTransition::ShieldedWithdrawal handlers accordingly to use the correct fee
source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49e8047b-be49-4bc2-b562-99607eebd314
📒 Files selected for processing (2)
packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/basic_structure.rspackages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs
.../rs-drive-abci/src/execution/validation/state_transition/processor/traits/basic_structure.rs
Show resolved
Hide resolved
- Add missing i64::MAX bound check on ShieldTransitionV0::amount (H1) - Enable basic_structure validation for shield_from_asset_lock and shielded_withdrawal in platform version v8 config - Fix signable_bytes_len as u16 truncation in shield_from_asset_lock (L1) - Fix unchecked tx_out.value * CREDITS_PER_DUFF overflow (L2) - Remove dead FLAGS_SPENDS_ONLY constant (I1) - Fix clippy warnings (needless borrows) - Add AUDIT_FINDINGS.md with full audit report Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/transform_into_action/v0/mod.rs (3)
80-91:⚠️ Potential issue | 🔴 CriticalAsset lock proof validation only checks fee amount, not total required.
The
validate()call passesrequired_balance(the fee) rather thanshield_amount + required_balance(the total). This allows thehas_data()path to validate against the fee alone while the fresh-RPC path correctly validates against the total. The validation should be consistent across both paths.🐛 Proposed fix
+ // Calculate total required: shield_amount + fee + let required_total = shield_amount + .checked_add(required_balance) + .ok_or(Error::Execution(ExecutionError::Overflow( + "shield amount + fee overflow in shield_from_asset_lock", + )))?; + // Step 4: Validate asset lock proof let asset_lock_proof_validation = if validation_mode != ValidationMode::NoValidation { AssetLockProved::asset_lock_proof(self).validate( platform, &mut signable_bytes_hasher, - required_balance, + required_total, validation_mode, tx, platform_version, )?🤖 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/shield_from_asset_lock/transform_into_action/v0/mod.rs` around lines 80 - 91, The AssetLockProved validation is using only required_balance (fee) instead of the total required amount; update the call to AssetLockProved::asset_lock_proof(self).validate(...) to pass shield_amount + required_balance (compute a total_required_balance variable or inline the sum) so the validator checks the combined shield_amount and fee consistently with the fresh-RPC path; keep all other parameters (platform, &mut signable_bytes_hasher, validation_mode, tx, platform_version) unchanged and use the new total when validation_mode != ValidationMode::NoValidation.
191-208:⚠️ Potential issue | 🔴 CriticalRemaining balance check should include fee in the required amount.
The check at line 193 compares
remaining_credit_valueagainstshield_amountonly, notshield_amount + fee. If the fix above is applied to calculaterequired_total, this comparison should also userequired_totalfor consistency.🐛 Proposed fix (assuming required_total is computed earlier)
// Step 7: Also check that the remaining asset lock balance covers shield_amount let remaining_credit_value = asset_lock_value_to_be_consumed.remaining_credit_value(); - if remaining_credit_value < shield_amount { + if remaining_credit_value < required_total { let asset_lock_proof = AssetLockProved::asset_lock_proof(self); return Ok(ConsensusValidationResult::new_with_error( IdentityAssetLockTransactionOutPointNotEnoughBalanceError::new( ... - shield_amount, + required_total, ) .into(), )); }🤖 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/shield_from_asset_lock/transform_into_action/v0/mod.rs` around lines 191 - 208, The remaining balance check currently compares remaining_credit_value to shield_amount only; change it to compare against required_total (which should be computed earlier as shield_amount + fee) by replacing the condition remaining_credit_value < shield_amount with remaining_credit_value < required_total, and also update the IdentityAssetLockTransactionOutPointNotEnoughBalanceError call (the required amount argument currently using shield_amount) to pass required_total so the error reports the full required amount; locate symbols remaining_credit_value, shield_amount, fee, required_total, AssetLockProved::asset_lock_proof, and IdentityAssetLockTransactionOutPointNotEnoughBalanceError to make these edits.
238-246:⚠️ Potential issue | 🟠 MajorOrchard ZK proof verification is not skipped for
RecheckTx.The ECDSA signature verification is correctly skipped for
RecheckTx(lines 105-106, 145-147), butreconstruct_and_verify_bundle()still runs on every recheck. ZK proof verification is computationally expensive and the proof is immutable, so rechecking it on mempool rechecks is wasteful and creates a CPU hot path.🔧 Proposed fix
// Step 9: Verify Orchard ZK proof via reconstruct_and_verify_bundle() // Use EMPTY extra_sighash_data -- no transparent binding needed since // the asset lock proof authenticates the source of funds. let (actions, anchor, proof, binding_signature) = match self { ShieldFromAssetLockTransition::V0(v0) => ( &v0.actions, &v0.anchor, v0.proof.as_slice(), &v0.binding_signature, ), }; - if let Err(e) = reconstruct_and_verify_bundle( + // Skip expensive ZK verification on RecheckTx - proof is immutable + let zk_verification_result = if validation_mode == ValidationMode::RecheckTx { + Ok(()) + } else { + reconstruct_and_verify_bundle( + actions, + FLAGS_OUTPUTS_ONLY, + -(shield_amount as i64), + anchor, + proof, + binding_signature, + &[], // No transparent fields to bind for shield_from_asset_lock + ) + }; + + if let Err(e) = zk_verification_result {🤖 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/shield_from_asset_lock/transform_into_action/v0/mod.rs` around lines 238 - 246, The Orchard ZK proof verification call reconstruct_and_verify_bundle(...) should be skipped for mempool rechecks; wrap that call in the same RecheckTx check used for ECDSA verification (i.e., detect RecheckTx via the existing request/flag or is_recheck_tx boolean used earlier) and only invoke reconstruct_and_verify_bundle for non-RecheckTx paths so ZK proofs are not re-verified during rechecks.
🧹 Nitpick comments (2)
packages/rs-drive-abci/AUDIT_FINDINGS.md (2)
67-77: Clarify the M1 explanation with a corrected example.Lines 73-74 state: "A withdrawal of 1,000,000 credits with 1 credit fee would pass a minimum fee of 111,548,800 only if
unshielding_amount >= 111,548,800." This example is self-contradictory—if the withdrawal is 1,000,000 credits,unshielding_amountcannot simultaneously be >= 111,548,800.The core issue is clear (the check validates
unshielding_amountinstead of the fee portion), but the example muddles the explanation.✏️ Proposed clarification
Replace lines 73-74 with:
-This means the minimum fee check compares the total withdrawal amount against the minimum fee threshold, which trivially passes for any meaningful withdrawal. A withdrawal of 1,000,000 credits with 1 credit fee would pass a minimum fee of 111,548,800 only if `unshielding_amount >= 111,548,800`, so the check does provide a floor — but it's on the total outflow, not the fee portion. +This means the minimum fee check compares the total withdrawal amount against the minimum fee threshold. For example, a withdrawal with `unshielding_amount = 1,000,000` (including 999,999 to recipient + 1 credit fee) would pass the minimum fee check even though the actual fee is only 1 credit, because the check validates that 1,000,000 >= minimum_fee. The check provides a floor on total outflow, not on the fee portion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/AUDIT_FINDINGS.md` around lines 67 - 77, The explanation's example is incorrect: update the M1 text to state that validate_minimum_shielded_fee currently uses unshielding_amount (total outflow) instead of computing the actual fee; instruct to clarify by showing fee = unshielding_amount - recipient_amount (or fee = value_balance - amount) and replace the contradictory example with a corrected one (e.g., "If unshielding_amount = 1,000,000 and recipient_amount = 999,999 then fee = 1; the current check incorrectly compares 1,000,000 to the minimum fee threshold instead of comparing 1 to the threshold"). Mention the affected symbols validate_minimum_shielded_fee, unshielding_amount, recipient_amount in the clarification.
159-167: Clarify the reasoning in L3.Line 164 states the issue "Works in practice because a Sinsemilla collision between the current and oldest anchor is cryptographically improbable." While correct, this reasoning doesn't clearly explain why the bug is benign.
A clearer explanation: The function checks whether the current anchor already exists in storage to avoid duplicate insertion. Comparing against the oldest anchor still detects uniqueness (if current ≠ oldest and collision probability is negligible, current is likely new), but it's conceptually wrong and inefficient—comparing against the most recent anchor would be the correct approach to detect back-to-back duplicate insertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/AUDIT_FINDINGS.md` around lines 159 - 167, Update the L3 audit note text ("L3: Stale anchor comparison from wrong query direction") to clarify why the bug is benign: explain that the code currently queries ascending with limit: Some(1) and therefore returns the oldest anchor rather than the most recent, and that while this still usually prevents duplicate insertion because a Sinsemilla collision between the current anchor and any stored anchor is negligibly probable, the correct and efficient check is to compare against the most recent anchor (or query descending) to accurately detect back-to-back duplicate insertions; replace the existing single-line rationale with a concise explanation that it “still usually detects uniqueness but is conceptually incorrect and inefficient—compare against the latest anchor or use a descending query to properly detect duplicates.”
🤖 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/AUDIT_FINDINGS.md`:
- Line 5: The audit metadata header currently reads "Base: v2.1-dev" which is
inconsistent with the PR target "v3.1-dev"; update the metadata line to match
the PR target (replace the Base value `v2.1-dev` with `v3.1-dev`) so the audit's
Base field and the PR target branch are consistent.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_common/mod.rs`:
- Around line 229-259: The current code builds an unbounded PathQuery using
Query::new_range_full() over anchors_path and then scans all elements to check
existence (seen in anchors_path, PathQuery, Query::new_range_full(),
drive.grove.query_raw, found), which is O(total anchors); change this to a
direct keyed lookup or a secondary index lookup instead: construct a query or
use GroveDB's direct get/get_key/get_raw API for the specific anchor key (or
maintain/use an anchor->height index) so you only request the single key, then
set found based on that single lookup result rather than iterating the entire
tree.
---
Duplicate comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/transform_into_action/v0/mod.rs`:
- Around line 80-91: The AssetLockProved validation is using only
required_balance (fee) instead of the total required amount; update the call to
AssetLockProved::asset_lock_proof(self).validate(...) to pass shield_amount +
required_balance (compute a total_required_balance variable or inline the sum)
so the validator checks the combined shield_amount and fee consistently with the
fresh-RPC path; keep all other parameters (platform, &mut signable_bytes_hasher,
validation_mode, tx, platform_version) unchanged and use the new total when
validation_mode != ValidationMode::NoValidation.
- Around line 191-208: The remaining balance check currently compares
remaining_credit_value to shield_amount only; change it to compare against
required_total (which should be computed earlier as shield_amount + fee) by
replacing the condition remaining_credit_value < shield_amount with
remaining_credit_value < required_total, and also update the
IdentityAssetLockTransactionOutPointNotEnoughBalanceError call (the required
amount argument currently using shield_amount) to pass required_total so the
error reports the full required amount; locate symbols remaining_credit_value,
shield_amount, fee, required_total, AssetLockProved::asset_lock_proof, and
IdentityAssetLockTransactionOutPointNotEnoughBalanceError to make these edits.
- Around line 238-246: The Orchard ZK proof verification call
reconstruct_and_verify_bundle(...) should be skipped for mempool rechecks; wrap
that call in the same RecheckTx check used for ECDSA verification (i.e., detect
RecheckTx via the existing request/flag or is_recheck_tx boolean used earlier)
and only invoke reconstruct_and_verify_bundle for non-RecheckTx paths so ZK
proofs are not re-verified during rechecks.
---
Nitpick comments:
In `@packages/rs-drive-abci/AUDIT_FINDINGS.md`:
- Around line 67-77: The explanation's example is incorrect: update the M1 text
to state that validate_minimum_shielded_fee currently uses unshielding_amount
(total outflow) instead of computing the actual fee; instruct to clarify by
showing fee = unshielding_amount - recipient_amount (or fee = value_balance -
amount) and replace the contradictory example with a corrected one (e.g., "If
unshielding_amount = 1,000,000 and recipient_amount = 999,999 then fee = 1; the
current check incorrectly compares 1,000,000 to the minimum fee threshold
instead of comparing 1 to the threshold"). Mention the affected symbols
validate_minimum_shielded_fee, unshielding_amount, recipient_amount in the
clarification.
- Around line 159-167: Update the L3 audit note text ("L3: Stale anchor
comparison from wrong query direction") to clarify why the bug is benign:
explain that the code currently queries ascending with limit: Some(1) and
therefore returns the oldest anchor rather than the most recent, and that while
this still usually prevents duplicate insertion because a Sinsemilla collision
between the current anchor and any stored anchor is negligibly probable, the
correct and efficient check is to compare against the most recent anchor (or
query descending) to accurately detect back-to-back duplicate insertions;
replace the existing single-line rationale with a concise explanation that it
“still usually detects uniqueness but is conceptually incorrect and
inefficient—compare against the latest anchor or use a descending query to
properly detect duplicates.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79240acf-af51-4099-af77-8276fb25ea01
📒 Files selected for processing (6)
packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_transition/v0/state_transition_validation.rspackages/rs-drive-abci/AUDIT_FINDINGS.mdpackages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/basic_structure.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/transform_into_action/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shielded_common/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rs
...rive-abci/src/execution/validation/state_transition/state_transitions/shielded_common/mod.rs
Outdated
Show resolved
Hide resolved
… and remove unused sha2 dep Cherry-pick proof verification fix from feat/zk-drive: shielded pool subtrees changed root-level Merk hashes, causing proof size increases and succinctness check failures in strategy tests. Use verify_subset_of_proof=true where proofs contain extra lower layers for sibling subtrees. Also remove unused sha2 dependency from drive-abci Cargo.toml. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-drive/src/drive/initialization/v0/mod.rs (1)
945-1268: Assert decoded proof shape, not serialized byte size.These checks are effectively serialization snapshots even though the test is named around tree heights. The near-uniform
+1churn here is a sign that benign proof-encoding changes can break the test without any actual Merkle-layout regression. Consider decoding the proof and asserting path depth / sibling structure instead, or at least centralizing these byte-size fixtures behind a helper so version-specific updates stay intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/initialization/v0/mod.rs` around lines 945 - 1268, The tests currently assert serialized proof byte lengths (e.g., assert_eq!(proof.len(), ...)) after calling grove_get_proved_path_query (used around Query::new and PathQuery::new blocks), which is fragile; instead decode the returned proof into its structured representation and assert Merkle path properties (path depth, number of siblings/parents, presence of expected sibling nodes) or replace the raw-length checks with a single helper (e.g., assert_proof_shape or decode_and_assert_proof) that centralizes the logic so version-specific serialization size changes won't break tests; update each assertion site to call that helper or perform decoded-structure assertions against expected tree-height/sibling counts for the given RootTree key.
🤖 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-drive/src/drive/initialization/v0/mod.rs`:
- Around line 945-1268: The tests currently assert serialized proof byte lengths
(e.g., assert_eq!(proof.len(), ...)) after calling grove_get_proved_path_query
(used around Query::new and PathQuery::new blocks), which is fragile; instead
decode the returned proof into its structured representation and assert Merkle
path properties (path depth, number of siblings/parents, presence of expected
sibling nodes) or replace the raw-length checks with a single helper (e.g.,
assert_proof_shape or decode_and_assert_proof) that centralizes the logic so
version-specific serialization size changes won't break tests; update each
assertion site to call that helper or perform decoded-structure assertions
against expected tree-height/sibling counts for the given RootTree key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ebb2469-8cde-4af0-a1e1-1710306edaf1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/rs-drive-abci/Cargo.tomlpackages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rspackages/rs-drive/src/drive/initialization/v0/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive-abci/Cargo.toml
- Align grovedb-commitment-tree rev to dd99ed1d (matches dpp/drive), eliminating duplicate compilation from mismatched git revisions - Remove unused `value_balance` variable in shield_from_asset_lock test - Fix base branch in AUDIT_FINDINGS.md (v2.1-dev → v3.1-dev) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…most recent anchor Redesign anchor storage: anchors are now stored as anchor_bytes (key) → block_height_be (value) instead of the reverse, enabling O(1) existence checks via grove_has_raw instead of full table scans. Add a dedicated SHIELDED_MOST_RECENT_ANCHOR_KEY element to track the latest anchor without querying the anchors tree. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…port Add a second anchor tree (key=8) mapping block_height_be → anchor_bytes, the reverse of the existing anchor tree (key=6) which maps anchor_bytes → block_height_be. This enables efficient pruning of old anchors by height range without scanning the primary anchor tree. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add prune_shielded_pool_anchors method that runs at block end to remove anchors older than shielded_anchor_retention_blocks (1000) from both the primary anchors tree (anchor_bytes → height) and the reverse index (height → anchor_bytes). Uses range query on the by-height tree to efficiently find expired entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dd min fee check to check_tx M3: ShieldedTransfer now rejects value_balance == 0 at structure validation since value_balance IS the fee for shielded transfers. M2: Add validate_minimum_shielded_fee to check_tx path before validate_shielded_proof, preventing attackers from triggering expensive ZK proof verification with insufficient fees during check_tx. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… test gating Mark M2, M3, M4, L3 as fixed. Clarify I3 as by-design (long-running tests). Update strategy test comment to explain feature gate rationale. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
QuantumExplorer
commented
Mar 11, 2026
...ation/state_transition/state_transitions/shielded_withdrawal/transform_into_action/v0/mod.rs
Outdated
Show resolved
Hide resolved
.../rs-drive-abci/src/execution/validation/state_transition/state_transitions/unshield/tests.rs
Outdated
Show resolved
Hide resolved
… test - Fix rustfmt formatting across drive-abci and drive packages - Fix clippy lint: use is_multiple_of() instead of manual modulo check - Revert proof size assertions in PV11 init test (PV11 uses init v2 which doesn't include the shielded pool tree, so proof sizes should not have been bumped) - Fix test_too_many_actions tests in unshield and shielded_withdrawal to use direct validate_structure calls (101 actions exceeds 20KB max_state_transition_size before hitting the actions count check) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…_action validators Rename `err` to `consensus_error` in `if let Some(err) = validate_*` patterns across shielded_withdrawal, shielded_transfer, and unshield transform_into_action implementations for clarity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add `shielded_anchor_pruning_interval` (100) to DriveAbciValidationConstants so the pruning cadence is configurable via platform versioning instead of being a magic number. Also remove AUDIT_FINDINGS.md which should not have been committed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
shumkov
requested changes
Mar 11, 2026
.../execution/platform_events/block_processing_end_events/prune_shielded_pool_anchors/v0/mod.rs
Outdated
Show resolved
Hide resolved
.../execution/platform_events/block_processing_end_events/record_shielded_pool_anchor/v0/mod.rs
Outdated
Show resolved
Hide resolved
...es/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield/tests.rs
Show resolved
Hide resolved
...rive-abci/src/execution/validation/state_transition/state_transitions/shielded_common/mod.rs
Show resolved
Hide resolved
…methods Move raw GroveDB operations out of drive-abci into proper Drive methods, following the existing versioned method pattern. This addresses review feedback that drive-abci was directly manipulating drive internals. New Drive methods: - record_shielded_pool_anchor_if_changed: reads commitment tree anchor, writes to anchor trees if changed - prune_shielded_pool_anchors: queries and deletes old anchors below cutoff height - has_shielded_anchor: O(1) key lookup for anchor existence - has_nullifier: O(1) key lookup for nullifier existence - read_shielded_pool_total_balance: reads pool balance sum item - shielded_pool_notes_count: counts notes in commitment tree Drive-abci callers updated to use the new Drive methods instead of direct grove operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
shumkov
previously approved these changes
Mar 12, 2026
…conversions_serde The impl_wasm_conversions macro was never defined. These platform_address transition types are #[serde(transparent)] wrappers, so they should use the existing impl_wasm_conversions_serde macro directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The shielded_tests module references OperationType variants (Shield, ShieldFromAssetLock, ShieldedTransfer, Unshield) that are not yet implemented. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
Out of scope
Test plan
shielded_tests.rs🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Enhancements
Tests
Chores