feat(drive): add shielded pool storage, actions, and verification (Medusa part 2)#3198
Merged
QuantumExplorer merged 30 commits intov3.1-devfrom Mar 10, 2026
Merged
feat(drive): add shielded pool storage, actions, and verification (Medusa part 2)#3198QuantumExplorer merged 30 commits intov3.1-devfrom
QuantumExplorer merged 30 commits intov3.1-devfrom
Conversation
Adds the rs-drive layer for Orchard shielded transactions (Medusa): - Shielded pool GroveDB trees: commitment tree for encrypted notes, nullifier storage, anchor tracking, and pool balance - State transition actions for all 5 shielded transition types (shield, shield-from-asset-lock, shielded-transfer, unshield, shielded-withdrawal) plus penalize_shielded_pool system action - Drive operations: store/fetch/compact/cleanup nullifiers, batch insert into commitment trees, count tree operations - Proof generation and verification for shielded state (nullifiers, anchors, encrypted notes, pool balance) - Estimated costs for shielded operations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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: "471d6dd04433400842ffec199f41eb63570e177023b25a114737fcef34b59af1"
)Xcode manual integration:
|
QuantumExplorer
changed the title
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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:
📝 WalkthroughWalkthroughThis PR introduces comprehensive shielded pool functionality to the drive system, including operations for note management, nullifier handling with compaction and cleanup, proof generation and verification, and corresponding state transition actions (Shield, Unshield, ShieldedTransfer, ShieldedWithdrawal, ShieldFromAssetLock, PenalizeShieldedPool). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StateTransition
participant HighLevelOps
participant ShieldedOps
participant Drive
participant GroveDB
Client->>StateTransition: Create Shield/Unshield/Transfer
StateTransition->>HighLevelOps: into_high_level_drive_operations()
HighLevelOps->>ShieldedOps: insert_notes(), insert_nullifiers(), update_balance()
ShieldedOps->>Drive: insert_note_op(), insert_nullifiers(), update_total_balance_op()
Drive->>GroveDB: QualifiedGroveDbOp (commitment_tree, nullifiers_tree, total_balance)
GroveDB-->>Drive: Operation queued
Drive-->>ShieldedOps: LowLevelDriveOperation
ShieldedOps-->>HighLevelOps: Operations collected
HighLevelOps-->>StateTransition: DriveOperation vector
StateTransition-->>Client: State updated
sequenceDiagram
participant Prover
participant Drive
participant ShieldedProof
participant PathQuery
participant GroveDB
participant Verifier
Prover->>Drive: prove_nullifiers_trunk_query()
Drive->>ShieldedProof: nullifiers_path_for_pool()
ShieldedProof-->>Drive: Path constructed
Drive->>PathQuery: PathTrunkChunkQuery (min/max depth)
PathQuery->>GroveDB: grove_get_proved_trunk_chunk_query()
GroveDB-->>PathQuery: Proof bytes
PathQuery-->>Drive: Vec<u8>
Drive-->>Prover: Proof
Prover->>Verifier: Send proof
Verifier->>Drive: verify_nullifiers_trunk_query()
Drive->>GroveDB: verify_trunk_chunk_proof()
GroveDB-->>Drive: (RootHash, GroveTrunkQueryResult)
Drive-->>Verifier: Verification result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
…ion (#3204) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…n code (#3203) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…3202) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…3199) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
- Allow clippy::too_many_arguments on prove_nullifiers_branch_query - Allow clippy::type_complexity on verify_shielded_encrypted_notes and verify_shielded_nullifiers - Fix clippy::clone_on_copy on PlatformAddress in unshield transformer - Fix clippy::manual_is_multiple_of in verify_shielded_encrypted_notes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Define NULLIFIERS_KEY_U8 and COMPACTED_NULLIFIERS_KEY_U8 locally in verify modules instead of importing from server-only saved_block_transactions module (fixes verify-only feature builds) - Add missing shielded action match arms in drive-abci strategy tests - Run cargo fmt on affected files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-drive/src/state_transition_action/system/partially_use_asset_lock_action/v0/mod.rs (2)
40-42:⚠️ Potential issue | 🟡 MinorTypo in documentation: "very" should be "verify".
📝 Proposed fix
- /// asset lock script used to very that the asset lock can be used + /// asset lock script used to verify that the asset lock can be used fn asset_lock_script(&self) -> &Vec<u8>; - /// asset lock script used to very that the asset lock can be used, this consumes the action + /// asset lock script used to verify that the asset lock can be used, this consumes the action fn asset_lock_script_owned(self) -> Vec<u8>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/system/partially_use_asset_lock_action/v0/mod.rs` around lines 40 - 42, Fix the typo in the doc comments by replacing "very" with "verify" for the asset lock documentation; update the comment on the asset_lock_script method (and the following doc line that describes the consuming action) so both read "verify" instead of "very" to correctly describe that the asset lock script verifies use of the asset lock.
34-34:⚠️ Potential issue | 🟡 MinorIncorrect documentation comment.
The trait documentation refers to "document base transition action" which appears to be a copy-paste error from another module. It should reference "partially use asset lock action".
📝 Proposed fix
-/// document base transition action accessors v0 +/// Partially use asset lock action accessors v0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/system/partially_use_asset_lock_action/v0/mod.rs` at line 34, The trait/module doc comment currently says "document base transition action" (a copy-paste error); update the doc comment in the v0 module for partially_use_asset_lock_action to correctly reference "partially use asset lock action" (and any related trait name in this file such as the PartiallyUseAssetLockAction trait or implementation) so the documentation accurately describes this module's purpose.
🧹 Nitpick comments (25)
packages/rs-drive/src/util/grove_operations/grove_get_proved_path_query_v1/mod.rs (1)
1-10: Consider consistent import source forDriveVersion.This file imports
DriveVersionfromdpp::version::drive_versions, while the v0 submodule imports it fromplatform_version::version::drive_versions. While these are likely the same re-exported type, using a consistent import source across related modules improves maintainability.♻️ Suggested change for consistency
Either change this file to match v0:
-use dpp::version::drive_versions::DriveVersion; +use platform_version::version::drive_versions::DriveVersion;Or update v0/mod.rs to use the
dppre-export (whichever is the project convention).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/util/grove_operations/grove_get_proved_path_query_v1/mod.rs` around lines 1 - 10, The import for DriveVersion in this module is inconsistent with the v0 submodule; update the use of DriveVersion in this file to import from the same source as v0 (platform_version::version::drive_versions) so both modules reference the same re-export source; locate the top-level use statement that currently brings in DriveVersion from dpp::version::drive_versions and change it to platform_version::version::drive_versions to match v0 and keep imports consistent across grove_get_proved_path_query_v1 and its v0 submodule.packages/rs-drive/src/state_transition_action/system/partially_use_asset_lock_action/v0/mod.rs (1)
41-41: Prefer slice references over&Vec<T>in trait signatures.Returning
&[u8]and&[Bytes32]is more idiomatic Rust since it allows callers to pass any contiguous sequence, not just aVec. This is a common Clippy lint (clippy::ptr_arg).♻️ Proposed refactor
- fn asset_lock_script(&self) -> &Vec<u8>; + fn asset_lock_script(&self) -> &[u8];- fn previous_transaction_hashes_ref(&self) -> &Vec<Bytes32>; + fn previous_transaction_hashes_ref(&self) -> &[Bytes32];Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/system/partially_use_asset_lock_action/v0/mod.rs` at line 41, The trait uses `&Vec` return types which is non-idiomatic; update the signatures to return slices instead — change the `asset_lock_script(&self) -> &Vec<u8>` to `asset_lock_script(&self) -> &[u8]` and likewise replace any `-> &Vec<Bytes32>` signature (the other occurrence around line 52) with `-> &[Bytes32]`; adjust any implementing structs' method signatures to match these slice returns so callers can use any contiguous sequence (Vec or slice).packages/rs-drive/src/state_transition_action/system/partially_use_asset_lock_action/mod.rs (1)
33-36: Accessor implementations return&Vec<T>— update if trait signatures change.If the optional refactor to use slice references (
&[u8],&[Bytes32]) is adopted in the trait definition, these implementations would need corresponding updates to return slices.♻️ Proposed refactor (if trait is updated)
fn asset_lock_script(&self) -> &Vec<u8> { match self { - PartiallyUseAssetLockAction::V0(transition) => &transition.asset_lock_script, + PartiallyUseAssetLockAction::V0(transition) => transition.asset_lock_script.as_slice(), } }fn previous_transaction_hashes_ref(&self) -> &Vec<Bytes32> { match self { - PartiallyUseAssetLockAction::V0(transition) => &transition.previous_transaction_hashes, + PartiallyUseAssetLockAction::V0(transition) => transition.previous_transaction_hashes.as_slice(), } }Also applies to: 63-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/system/partially_use_asset_lock_action/mod.rs` around lines 33 - 36, The accessors on PartiallyUseAssetLockAction currently return &Vec<T> (e.g. asset_lock_script(&self) -> &Vec<u8> and the similar accessor at lines 63-66); if the trait signatures are refactored to return slices instead, update these methods to match by changing their return types to slices (e.g. -> &[u8] or -> &[Bytes32]) and return a slice of the inner Vec from the V0 variant (e.g. return &transition.asset_lock_script[..] or &transition.some_vec_field[..]); ensure you update all accessor signatures and all match arms in PartiallyUseAssetLockAction::V0 accordingly so the implementation matches the new trait types.packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs (2)
308-309: Usetry_fromfor the integer cast to avoid silent truncation.The PR commits mention using
try_fromfor integer casts, but this line usesas u16which silently truncates ifnullifier_keys.len()exceedsu16::MAX. Consider usingu16::try_fromwith proper error handling for consistency with the stated fix pattern.Proposed fix
- let nullifier_keys: Vec<Vec<u8>> = st.nullifiers(); - let count = nullifier_keys.len() as u16; + let nullifier_keys: Vec<Vec<u8>> = st.nullifiers(); + let count = u16::try_from(nullifier_keys.len()).map_err(|_| { + Error::Proof(ProofError::InvalidTransition( + "shielded transfer has too many nullifiers".to_string(), + )) + })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs` around lines 308 - 309, Replace the silent cast "let count = nullifier_keys.len() as u16;" with a fallible conversion using u16::try_from(nullifier_keys.len()) and propagate or return an error on failure so the code doesn't truncate large lengths; update the code around st.nullifiers() and the count variable (e.g., where count is used later) to handle the Result (using ? or explicit error mapping) so an out-of-range length yields a clear error instead of silent truncation.
350-366: Avoid unnecessary clone ofnullifier_keys.The vector is cloned on line 352 but only
first()is accessed afterward on line 359. Extract the first nullifier before consuming the keys to avoid the allocation.Proposed fix
let nullifier_keys: Vec<Vec<u8>> = st.nullifiers(); + let first_nullifier = nullifier_keys.first().ok_or_else(|| { + Error::Proof(ProofError::InvalidTransition( + "shielded withdrawal has no nullifiers".to_string(), + )) + })?; + let mut nf_query = grovedb::Query::new(); - nf_query.insert_keys(nullifier_keys.clone()); + nf_query.insert_keys(nullifier_keys); let nullifier_pq = PathQuery::new( shielded_credit_pool_nullifiers_path_vec(), grovedb::SizedQuery::new(nf_query, None, None), ); // Compute withdrawal document ID deterministically - let first_nullifier = nullifier_keys.first().ok_or_else(|| { - Error::Proof(ProofError::InvalidTransition( - "shielded withdrawal has no nullifiers".to_string(), - )) - })?; let mut entropy = Vec::new();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs` around lines 350 - 366, The code currently clones nullifier_keys unnecessarily; instead, bind nullifier_keys = st.nullifiers() once, call first() on that owned Vec to obtain first_nullifier (using first().ok_or_else(...) and cloning the single first element if needed), then pass the original nullifier_keys into nf_query.insert_keys(nullifier_keys) (no clone). Update the sequence around nullifier_keys, nf_query.insert_keys(...), PathQuery, and first_nullifier so you consume the owned Vec once rather than cloning it.packages/rs-drive/src/fees/op.rs (1)
638-638: Consider usingchecked_mulfor consistency and overflow safety.The
sinsemilla_costcalculation uses unchecked multiplication, unlike the other cost calculations in this function (e.g.,seek_cost,storage_added_bytes_ephemeral_cost) which usechecked_mul. While line 637 has a comment justifying whyhash_node_costcan't overflow, no similar rationale is provided for sinsemilla operations.If
sinsemilla_baseorsinsemilla_hash_callscan be large (e.g., for batch shielded operations), this could potentially overflow.♻️ Proposed fix for consistency with other cost calculations
- let sinsemilla_cost = fee_version.hashing.sinsemilla_base * (*sinsemilla_hash_calls as u64); + let sinsemilla_cost = fee_version + .hashing + .sinsemilla_base + .checked_mul(*sinsemilla_hash_calls as u64) + .ok_or_else(|| get_overflow_error("sinsemilla cost overflow"))?;If overflow is truly impossible, consider adding a comment explaining why (similar to the
hash_node_costcomment on line 636).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/fees/op.rs` at line 638, Replace the unchecked multiplication computing sinsemilla_cost (sinsemilla_cost = fee_version.hashing.sinsemilla_base * (*sinsemilla_hash_calls as u64)) with a checked multiplication like other cost calculations (e.g., seek_cost, storage_added_bytes_ephemeral_cost); call .checked_mul(...) on fee_version.hashing.sinsemilla_base and handle the None case the same way this function handles other overflow cases (propagate an error or produce a clear panic/log message), or if overflow is provably impossible add a comment explaining why (similar to the hash_node_cost comment).packages/rs-drive/src/util/grove_operations/grove_commitment_tree_count/v0/mod.rs (1)
21-25: Avoid recording empty cost operations here.Line 24 pushes a
CalculatedCostOperationunconditionally. The standard non-optional helper inpackages/rs-drive/src/util/grove_operations/mod.rsskipscost.is_nothing(), so this path can accumulate no-op entries and drift from the rest of the fee-accounting flow.♻️ Suggested change
let CostContext { value, cost } = self.grove .commitment_tree_count(path, key, transaction, &drive_version.grove_version); - drive_operations.push(CalculatedCostOperation(cost)); + if !cost.is_nothing() { + drive_operations.push(CalculatedCostOperation(cost)); + } value.map_err(Error::from)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/util/grove_operations/grove_commitment_tree_count/v0/mod.rs` around lines 21 - 25, The code unconditionally pushes a CalculatedCostOperation from the CostContext returned by self.grove.commitment_tree_count, which can add no-op entries; update the block in the commitment_tree_count handling so you only push CalculatedCostOperation(cost) into drive_operations when the cost is non-empty (e.g., check cost.is_nothing()/is_some() or equivalent) and leave the value.map_err(Error::from) behavior unchanged; refer to CostContext, commitment_tree_count, drive_operations, and CalculatedCostOperation to locate and modify the code.packages/rs-drive/src/util/grove_operations/grove_commitment_tree_count/mod.rs (1)
8-8: NormalizeDriveVersionimports to useplatform_versiondirectly across both the facade and v0 implementation.Currently, the facade imports
DriveVersionfromdpp(line 8) whilepackages/rs-drive/src/util/grove_operations/grove_commitment_tree_count/v0/mod.rsimports it fromplatform_version. Although this works becausedppre-exportsplatform_version::version::*, the inconsistency introduces unnecessary coupling to thedppcrate's re-export mechanism. Using the canonical source (platform_version) directly in both files improves clarity and reduces fragility to future dependency changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/util/grove_operations/grove_commitment_tree_count/mod.rs` at line 8, Replace the non-canonical import of DriveVersion in the facade by importing it directly from the platform_version crate to match the v0 implementation; specifically change the use of dpp::version::drive_versions::DriveVersion to platform_version::version::drive_versions::DriveVersion (so both the facade and grove_commitment_tree_count::v0::mod.rs use platform_version::version::drive_versions::DriveVersion).packages/rs-drive/src/state_transition_action/shielded/shield_from_asset_lock/v0/transformer.rs (1)
9-25: Rename or narrow this constructor.
try_from_transitionreads like the action is derived fromvalue, butvalueis ignored and every invariant comes from side inputs. That makes this public API easy to misuse and also makes future transition-field drift harder to spot. Consider making itpub(crate)or renaming it to something likefrom_prevalidated_parts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/shielded/shield_from_asset_lock/v0/transformer.rs` around lines 9 - 25, The public constructor try_from_transition on ShieldFromAssetLockTransitionActionV0 should be narrowed or renamed because it ignores the provided ShieldFromAssetLockTransitionV0 value; change its visibility to pub(crate) or rename it to a clearer name like from_prevalidated_parts to signal that inputs are pre-validated and not derived from the transition value, and update any call sites accordingly (search for try_from_transition and ShieldFromAssetLockTransitionActionV0 to locate usages).packages/rs-drive/src/verify/shielded/verify_shielded_encrypted_notes/v0/mod.rs (1)
95-97: Silent skip on absent elements may warrant logging or explicit handling.When
maybe_elementisNone, the code silently continues. While the comment indicates this is expected for non-existent keys, consider whether callers need to distinguish between "no notes in range" vs "some notes missing."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/shielded/verify_shielded_encrypted_notes/v0/mod.rs` around lines 95 - 97, The code silently ignores None cases for maybe_element in the loop inside verify_shielded_encrypted_notes (v0 mod.rs); update the handling to either log a debug/info line identifying the absent key (including the range/index or note identifier) or return/accumulate a distinct result flag so callers can distinguish "no notes in range" versus "some notes missing." Locate the loop that matches on maybe_element and replace the empty-arm comment with a call to the logger (e.g., trace/debug with the note index) or push an enum/status into the function's result type (or both) so absence is explicitly recorded.packages/rs-drive/src/drive/initialization/v3/mod.rs (1)
6-6: Consider explicit imports instead of wildcard.While the wildcard import from
shielded::pathsis acceptable for internal modules, explicit imports improve readability and make dependencies clearer.Suggested explicit imports
-use crate::drive::shielded::paths::*; +use crate::drive::shielded::paths::{ + shielded_credit_pool_path_vec, SHIELDED_ANCHORS_IN_POOL_KEY, SHIELDED_CREDIT_POOL_KEY_U8, + SHIELDED_NOTES_CHUNK_POWER, SHIELDED_NOTES_KEY, SHIELDED_NULLIFIERS_KEY, + SHIELDED_TOTAL_BALANCE_KEY, +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/initialization/v3/mod.rs` at line 6, Replace the wildcard import "use crate::drive::shielded::paths::*;" with explicit imports from the shielded::paths module: list only the specific items this file uses (e.g., structs, functions, constants) by name in a `use crate::drive::shielded::paths::{ItemA, ItemB, ...};` statement in mod.rs; locate the wildcard in the mod.rs top-level imports and replace it with the concrete identifiers used in this module (adjust the list until the compiler reports no unresolved names).packages/rs-drive/src/drive/saved_block_transactions/compact_nullifiers/v0/mod.rs (1)
67-71: Unnecessaryclone()beforetry_into().The
keyis moved into the loop iteration, soclone()is redundant here sincetry_into()consumes the value.Suggested fix
- let height_bytes: [u8; 8] = key.clone().try_into().map_err(|_| { + let height_bytes: [u8; 8] = key.try_into().map_err(|_| {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/saved_block_transactions/compact_nullifiers/v0/mod.rs` around lines 67 - 71, Remove the unnecessary clone by calling try_into() directly on the moved `key` instead of `key.clone()`, i.e. replace `key.clone().try_into()` with `key.try_into()` in the block that constructs `height_bytes: [u8; 8]` (the place that currently maps errors into `Error::Protocol(Box::new(ProtocolError::CorruptedSerialization(...)))`).packages/rs-drive/src/verify/shielded/verify_shielded_nullifiers/mod.rs (1)
12-17: Use a fixed-size nullifier type in this new API.Shielded nullifiers are fixed-width, but this public signature accepts arbitrary-length
Vec<u8>values and returns them the same way. That pushes length validation to runtime and bakes a looser contract into the API surface.Possible tightening
- pub fn verify_shielded_nullifiers( - proof: &[u8], - nullifiers: &[Vec<u8>], - verify_subset_of_proof: bool, - platform_version: &PlatformVersion, - ) -> Result<(RootHash, Vec<(Vec<u8>, bool)>), Error> { + pub fn verify_shielded_nullifiers( + proof: &[u8], + nullifiers: &[[u8; 32]], + verify_subset_of_proof: bool, + platform_version: &PlatformVersion, + ) -> Result<(RootHash, Vec<([u8; 32], bool)>), Error> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/shielded/verify_shielded_nullifiers/mod.rs` around lines 12 - 17, The public function verify_shielded_nullifiers currently accepts and returns variable-length Vec<u8>, which weakens the API; change the signature of verify_shielded_nullifiers to accept a slice of a fixed-size nullifier type (e.g., &[Nullifier] where Nullifier is a newtype over [u8; N] or use &[ [u8; N] ]) and return Vec<(Nullifier, bool)> instead of Vec<(Vec<u8>, bool)>; update any callers to construct/validate Nullifier instances (or convert from Vec<u8> with a single centralized length check/From/TryFrom impl) so length validation is enforced at the type level rather than at runtime inside this function.packages/rs-drive/src/state_transition_action/shielded/unshield/v0/transformer.rs (1)
20-20: Reminder: Address the TODO for fee calculation.The fee amount is hardcoded to
0. Ensure fee calculation logic is implemented before this feature goes to production to prevent potential economic exploits or DoS vectors.Would you like me to open an issue to track the unshield fee calculation implementation?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/shielded/unshield/v0/transformer.rs` at line 20, The hardcoded fee_amount: 0 in the unshield transformer must be replaced with a real fee calculation: implement or call a calculate_unshield_fee(...) function (or add it inside the unshield transformation logic in transformer.rs) that takes relevant inputs (unshield amount, asset metadata, gas/weight estimates, current fee rate or fee schedule, and any protocol constants) and returns the fee; set fee_amount to that computed value, validate it does not exceed the unshield amount, and add unit tests for edge cases (zero amount, tiny amounts, max allowed, and overflow) plus clear error handling if fee computation fails.packages/rs-drive/src/state_transition_action/shielded/mod.rs (1)
12-21: Consider addingPartialEqandEqderives if comparison is needed.If
ShieldedActionNoteinstances need to be compared (e.g., in tests or deduplication logic), consider adding these derives:-#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct ShieldedActionNote {This is optional if comparisons are not required for the current use cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/shielded/mod.rs` around lines 12 - 21, Add PartialEq and Eq derives to the ShieldedActionNote struct so instances can be compared or used in equality-based collections; update the derive attribute on the ShieldedActionNote definition (currently #[derive(Debug, Clone)]) to include PartialEq and Eq, keeping Debug and Clone as-is.packages/rs-drive/src/drive/saved_block_transactions/cleanup_expired_nullifiers/v0/mod.rs (1)
54-87: Consider avoiding repeated path cloning in the loop.
compacted_path.clone()andexpiration_path.clone()are called for each iteration. If the batch API supports borrowing paths or if this is a hot path, consider extracting slices or using references.♻️ Potential optimization
If
GroveDbOpBatch::add_deletecan accept owned paths, you could convert tointo_iter()on the last iteration or restructure. However, if performance here is not critical (cleanup runs infrequently), the current approach is acceptable for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/saved_block_transactions/cleanup_expired_nullifiers/v0/mod.rs` around lines 54 - 87, The loop repeatedly clones compacted_path and expiration_path for each deletion; to avoid that allocate or borrow the paths once outside the for (expiration_key, element) in key_elements loop and reuse references when calling batch.add_delete (use &compacted_path and &expiration_path or move an owned path into the loop if add_delete takes ownership), updating calls to batch.add_delete to pass the borrowed/owned path variable instead of calling compacted_path.clone() and expiration_path.clone() each iteration; locate usages around compacted_path, expiration_path, and batch.add_delete in the cleanup_expired_nullifiers code to apply the change.packages/rs-drive/src/drive/saved_block_transactions/store_nullifiers/v0/mod.rs (1)
140-141: Inconsistent integer cast compared to line 63.Line 63 uses
i64::try_from()with proper error handling, but line 141 usesas i64which could silently overflow for extremely large slices. For consistency and safety, consider usingtry_fromhere as well.♻️ Suggested change
- let new_sum = sum + nullifiers.len() as i64; + let new_sum = sum + i64::try_from(nullifiers.len()).unwrap_or(i64::MAX);In practice, this is unlikely to overflow given realistic nullifier counts, but consistency with line 63 would be cleaner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/saved_block_transactions/store_nullifiers/v0/mod.rs` around lines 140 - 141, Replace the silent cast "nullifiers.len() as i64" used when computing new_sum with a fallible conversion like i64::try_from(nullifiers.len()) and propagate or handle the potential error the same way line 63 does; update the computation of new_sum (and any dependent new_count/new_sum assignments) to use the converted i64 value (or return the same error type) so overflow is detected consistently with the earlier conversion logic.packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs (2)
163-178: Redundant error handling after length check.Lines 169-178 use
try_into().map_err()for the slice conversions, but the length was already validated at line 163. While harmless, usingexpect()(as done at lines 117-119) would be more concise here.♻️ Suggested simplification
- let range_start = u64::from_be_bytes(key[0..8].try_into().map_err(|_| { - Error::Proof(ProofError::CorruptedProof( - "invalid key slice length for block height".to_string(), - )) - })?); - let range_end = u64::from_be_bytes(key[8..16].try_into().map_err(|_| { - Error::Proof(ProofError::CorruptedProof( - "invalid key slice length for block height".to_string(), - )) - })?); + // Safety: length verified to be 16 at line 163 + let range_start = u64::from_be_bytes(key[0..8].try_into().expect("len checked")); + let range_end = u64::from_be_bytes(key[8..16].try_into().expect("len checked"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs` around lines 163 - 178, The code performs a manual length check on key (key.len() != 16) then uses try_into().map_err(...) to convert slices for range_start and range_end; replace the redundant map_err error handling with the concise expect(...) pattern used elsewhere (e.g., the earlier slice conversions) so after the length check you call try_into().expect("validated slice length") for both key[0..8] and key[8..16] when computing range_start and range_end.
1-2: Consider importing the constant instead of duplicating it.The constant
COMPACTED_NULLIFIERS_KEY_U8is already exported fromsaved_block_transactions. Importing it here prevents potential divergence.♻️ Suggested change
-/// The subtree key for compacted nullifiers storage as u8 (must match saved_block_transactions::COMPACTED_NULLIFIERS_KEY_U8) -const COMPACTED_NULLIFIERS_KEY_U8: u8 = b'o'; +use crate::drive::saved_block_transactions::COMPACTED_NULLIFIERS_KEY_U8;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs` around lines 1 - 2, Replace the duplicated local constant by importing the existing COMPACTED_NULLIFIERS_KEY_U8 from the saved_block_transactions module: remove the local const COMPACTED_NULLIFIERS_KEY_U8 declaration in verify_compacted_nullifier_changes::v0 and add a use (or equivalent) to bring saved_block_transactions::COMPACTED_NULLIFIERS_KEY_U8 into scope so the module reuses the exported constant rather than redefining it.packages/rs-drive/src/verify/shielded/verify_recent_nullifier_changes/v0/mod.rs (1)
1-2: Consider importing the constant instead of duplicating it.The constant
NULLIFIERS_KEY_U8is already publicly exported fromcrate::drive::saved_block_transactions(viapub use queries::*). Importing it instead of duplicating would eliminate the risk of them diverging.♻️ Suggested change
-/// The subtree key for nullifiers storage as u8 (must match saved_block_transactions::NULLIFIERS_KEY_U8) -const NULLIFIERS_KEY_U8: u8 = b'n'; +use crate::drive::saved_block_transactions::NULLIFIERS_KEY_U8;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/shielded/verify_recent_nullifier_changes/v0/mod.rs` around lines 1 - 2, Remove the duplicated constant declaration of NULLIFIERS_KEY_U8 in verify_recent_nullifier_changes::v0::mod.rs and instead import the existing public constant from crate::drive::saved_block_transactions (or the re-exported path used in the crate) so the module uses that single source of truth; update any references in the file to use the imported NULLIFIERS_KEY_U8 and ensure no local const remains to avoid divergence.packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs (2)
45-50: Consider replacingexpect()with proper error handling.While the
platform_value!macro does produce a map, usingexpect()in library code is inconsistent with the error propagation patterns used elsewhere in this PR (per commit message: "Replace expect/unwrap with error propagation"). Consider usingmap_errto convert the error.♻️ Proposed fix
let withdrawal_document = DocumentV0 { id: document_id, owner_id, properties: document_data .into_btree_string_map() - .expect("platform_value macro produces a map"), + .map_err(|e| { + ConsensusError::BasicError(BasicError::InvalidDocumentPropertyValueError( + InvalidDocumentPropertyValueError::new(format!("platform_value map error: {e}")) + )) + })?, revision: Some(1),Note: You'll need to adjust the error type to match your consensus error handling patterns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs` around lines 45 - 50, Replace the panic-causing expect on document_data.into_btree_string_map() in the DocumentV0 construction with proper error propagation: call into_btree_string_map().map_err(...) (or ? after mapping) to convert the conversion failure into the module's consensus/error type and return it from the transformer function (the code constructing DocumentV0 in transformer.rs for shielded_withdrawal v0). Ensure the converted error matches the surrounding error enum/Result signature so the function compiles without using expect().
72-72: Reminder: Address the TODO for fee calculation.The
fee_amountis hardcoded to0. This needs to be implemented before the feature is production-ready.Do you want me to help design the fee calculation logic or open an issue to track this task?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rs` at line 72, The fee_amount in the shielded withdrawal transformer is currently hardcoded to 0; implement the real fee calculation in the function/struct that builds the withdrawal output (look for fee_amount assignment in transformer.rs within the ShieldedWithdrawal v0 transformer) by computing the required fee based on the protocol's fee schedule (gas/weight or per-byte rules), any network/consensus fee parameters available in the build context, and the withdrawal value; replace the 0 with a call to a new helper (e.g., calculate_shielded_withdrawal_fee) that accepts the withdrawal details and fee params and returns the fee_amount, and write unit tests exercising typical/edge cases and fallback behavior when fee parameters are missing.packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/mod.rs (1)
21-79: Missing accessor forcurrent_total_balance.The
ShieldedWithdrawalTransitionActionV0struct contains acurrent_total_balancefield (per the AI summary), but there's no accessor method defined for it onShieldedWithdrawalTransitionAction. If this field needs to be accessed externally, consider adding the accessor for API completeness.♻️ Proposed accessor
/// Fee amount (value_balance - amount), paid to proposers pub fn fee_amount(&self) -> Credits { match self { ShieldedWithdrawalTransitionAction::V0(transition) => transition.fee_amount, } } + /// Get current total balance of the shielded pool + pub fn current_total_balance(&self) -> Credits { + match self { + ShieldedWithdrawalTransitionAction::V0(transition) => transition.current_total_balance, + } + } /// Get prepared withdrawal document🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/mod.rs` around lines 21 - 79, Add a missing accessor for the current_total_balance field on ShieldedWithdrawalTransitionAction: implement a pub fn current_total_balance(&self) -> Credits that matches ShieldedWithdrawalTransitionAction::V0(transition) and returns transition.current_total_balance; ensure the method is placed alongside the other accessors (e.g., near amount(), fee_amount()) and uses the same return type (Credits) and borrowing semantics as the other getters.packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_nullifiers/v0/mod.rs (1)
230-261: Reduce code duplication in fallback key construction.The key construction
(start_block_height, start_block_height)is repeated three times with identical code. Consider extracting this to a helper closure or variable.♻️ Proposed refactor
+ // Default key when no containing range is found + let default_key = || { + let mut key = Vec::with_capacity(16); + key.extend_from_slice(&start_block_height.to_be_bytes()); + key.extend_from_slice(&start_block_height.to_be_bytes()); + key + }; + // Determine the actual start key for the proved query // If we found a containing range, use its exact key // Otherwise use (start_block_height, start_block_height) since end_block >= start_block always let start_key = if let Some((key, _)) = desc_results.to_key_elements().into_iter().next() { if key.len() == 16 { let end_block = u64::from_be_bytes(key[8..16].try_into().map_err(|_| { Error::Protocol(Box::new(ProtocolError::CorruptedSerialization( "invalid compacted key slice".to_string(), ))) })?); // If this range contains start_block_height, use its exact key if end_block >= start_block_height { key } else { - // No containing range, use (start_block_height, start_block_height) - let mut key = Vec::with_capacity(16); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key + default_key() } } else { - let mut key = Vec::with_capacity(16); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key + default_key() } } else { - let mut key = Vec::with_capacity(16); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key + default_key() };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_nullifiers/v0/mod.rs` around lines 230 - 261, The fallback key (the Vec 16-byte pair of start_block_height twice) is duplicated; extract a small helper/closure (e.g., let make_fallback_key = || { ... }) and use it in the start_key computation wherever the fallback is needed (inside the else branches and the non-containing-range branch) to replace the repeated key construction; reference symbols: start_key, desc_results.to_key_elements(), start_block_height.packages/rs-drive/src/util/batch/drive_op_batch/shielded.rs (1)
102-116: Side-effect operation pattern documentation is clear.The
StoreNullifiersForBlockvariant directly callsdrive.store_nullifiers_for_blockas a side effect rather than returning low-level ops. This deviation from the converter pattern is well-documented in the comment. However, note that this means the operation is executed immediately during batch conversion rather than being deferred with other batch operations, which could affect atomicity guarantees.Consider whether this should instead return an operation that gets executed in the same transaction as other batch operations to maintain atomicity. If the current behavior is intentional (e.g., for performance or because it operates on a separate tree), this is acceptable but worth documenting at the call site.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/util/batch/drive_op_batch/shielded.rs` around lines 102 - 116, StoreNullifiersForBlock currently executes drive.store_nullifiers_for_block immediately during batch conversion (in ShieldedPoolOperationType::StoreNullifiersForBlock) which breaks the normal converter pattern and may affect atomicity; either refactor to return a deferred operation that yields grove ops executed with the same transaction as other batch ops (e.g., add a variant that produces a DriveOperation/grove op applied later) or, if immediate execution is intentional, add a clear doc comment at the StoreNullifiersForBlock handling and at drive.store_nullifiers_for_block explaining why it must run now (separate tree, performance, or acceptable atomicity trade-off) so callers know this side-effect is deliberate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d175aa1a-3fde-43d3-b563-44afe8f09202
📒 Files selected for processing (98)
packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rspackages/rs-drive/src/drive/credit_pools/mod.rspackages/rs-drive/src/drive/credit_pools/pending_epoch_refunds/methods/add_delete_pending_epoch_refunds_except_specified/v0/mod.rspackages/rs-drive/src/drive/initialization/v3/mod.rspackages/rs-drive/src/drive/mod.rspackages/rs-drive/src/drive/saved_block_transactions/cleanup_expired_nullifiers/mod.rspackages/rs-drive/src/drive/saved_block_transactions/cleanup_expired_nullifiers/v0/mod.rspackages/rs-drive/src/drive/saved_block_transactions/compact_nullifiers/mod.rspackages/rs-drive/src/drive/saved_block_transactions/compact_nullifiers/v0/mod.rspackages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_nullifiers/mod.rspackages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_nullifiers/v0/mod.rspackages/rs-drive/src/drive/saved_block_transactions/fetch_nullifiers/mod.rspackages/rs-drive/src/drive/saved_block_transactions/fetch_nullifiers/v0/mod.rspackages/rs-drive/src/drive/saved_block_transactions/mod.rspackages/rs-drive/src/drive/saved_block_transactions/queries.rspackages/rs-drive/src/drive/saved_block_transactions/store_nullifiers/mod.rspackages/rs-drive/src/drive/saved_block_transactions/store_nullifiers/v0/mod.rspackages/rs-drive/src/drive/shielded/estimated_costs.rspackages/rs-drive/src/drive/shielded/mod.rspackages/rs-drive/src/drive/shielded/paths.rspackages/rs-drive/src/drive/shielded/prove/mod.rspackages/rs-drive/src/drive/shielded/prove/prove_nullifiers_branch_query/mod.rspackages/rs-drive/src/drive/shielded/prove/prove_nullifiers_branch_query/v0/mod.rspackages/rs-drive/src/drive/shielded/prove/prove_nullifiers_trunk_query/mod.rspackages/rs-drive/src/drive/shielded/prove/prove_nullifiers_trunk_query/v0/mod.rspackages/rs-drive/src/fees/op.rspackages/rs-drive/src/prove/prove_state_transition/v0/mod.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/mod.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/mod.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shield_from_asset_lock_transition.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shield_transition.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_transfer_transition.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_withdrawal_transition.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.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/shielded/mod.rspackages/rs-drive/src/state_transition_action/shielded/shield/mod.rspackages/rs-drive/src/state_transition_action/shielded/shield/transformer.rspackages/rs-drive/src/state_transition_action/shielded/shield/v0/mod.rspackages/rs-drive/src/state_transition_action/shielded/shield/v0/transformer.rspackages/rs-drive/src/state_transition_action/shielded/shield_from_asset_lock/mod.rspackages/rs-drive/src/state_transition_action/shielded/shield_from_asset_lock/transformer.rspackages/rs-drive/src/state_transition_action/shielded/shield_from_asset_lock/v0/mod.rspackages/rs-drive/src/state_transition_action/shielded/shield_from_asset_lock/v0/transformer.rspackages/rs-drive/src/state_transition_action/shielded/shielded_transfer/mod.rspackages/rs-drive/src/state_transition_action/shielded/shielded_transfer/transformer.rspackages/rs-drive/src/state_transition_action/shielded/shielded_transfer/v0/mod.rspackages/rs-drive/src/state_transition_action/shielded/shielded_transfer/v0/transformer.rspackages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/mod.rspackages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/transformer.rspackages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/mod.rspackages/rs-drive/src/state_transition_action/shielded/shielded_withdrawal/v0/transformer.rspackages/rs-drive/src/state_transition_action/shielded/unshield/mod.rspackages/rs-drive/src/state_transition_action/shielded/unshield/transformer.rspackages/rs-drive/src/state_transition_action/shielded/unshield/v0/mod.rspackages/rs-drive/src/state_transition_action/shielded/unshield/v0/transformer.rspackages/rs-drive/src/state_transition_action/system/bump_address_input_nonces_action/transformer.rspackages/rs-drive/src/state_transition_action/system/bump_address_input_nonces_action/v0/transformer.rspackages/rs-drive/src/state_transition_action/system/mod.rspackages/rs-drive/src/state_transition_action/system/partially_use_asset_lock_action/mod.rspackages/rs-drive/src/state_transition_action/system/partially_use_asset_lock_action/v0/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.rspackages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v0/mod.rspackages/rs-drive/src/util/batch/drive_op_batch/finalize_task.rspackages/rs-drive/src/util/batch/drive_op_batch/mod.rspackages/rs-drive/src/util/batch/drive_op_batch/shielded.rspackages/rs-drive/src/util/batch/grovedb_op_batch/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_auto_incremented_items_in_count_tree/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_auto_incremented_items_in_count_tree/v0/mod.rspackages/rs-drive/src/util/grove_operations/grove_commitment_tree_count/mod.rspackages/rs-drive/src/util/grove_operations/grove_commitment_tree_count/v0/mod.rspackages/rs-drive/src/util/grove_operations/grove_get_proved_path_query_v1/mod.rspackages/rs-drive/src/util/grove_operations/grove_get_proved_path_query_v1/v0/mod.rspackages/rs-drive/src/util/grove_operations/grove_insert_empty_tree/v0/mod.rspackages/rs-drive/src/util/grove_operations/mod.rspackages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rspackages/rs-drive/src/verify/mod.rspackages/rs-drive/src/verify/shielded/mod.rspackages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/mod.rspackages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_nullifiers_branch_query/mod.rspackages/rs-drive/src/verify/shielded/verify_nullifiers_branch_query/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_nullifiers_trunk_query/mod.rspackages/rs-drive/src/verify/shielded/verify_nullifiers_trunk_query/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_recent_nullifier_changes/mod.rspackages/rs-drive/src/verify/shielded/verify_recent_nullifier_changes/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_shielded_anchors/mod.rspackages/rs-drive/src/verify/shielded/verify_shielded_anchors/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_shielded_encrypted_notes/mod.rspackages/rs-drive/src/verify/shielded/verify_shielded_encrypted_notes/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_shielded_nullifiers/mod.rspackages/rs-drive/src/verify/shielded/verify_shielded_nullifiers/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_shielded_pool_state/mod.rspackages/rs-drive/src/verify/shielded/verify_shielded_pool_state/v0/mod.rspackages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
Comment on lines
+1448
to
+1453
| StateTransitionAction::ShieldAction(_) => {} | ||
| StateTransitionAction::ShieldedTransferAction(_) => {} | ||
| StateTransitionAction::UnshieldAction(_) => {} | ||
| StateTransitionAction::ShieldFromAssetLockAction(_) => {} | ||
| StateTransitionAction::ShieldedWithdrawalAction(_) => {} | ||
| StateTransitionAction::PenalizeShieldedPoolAction(_) => {} |
Contributor
There was a problem hiding this comment.
Don't silently skip proof assertions for shielded transitions.
These no-op arms let every new shielded action pass this helper without checking execution outcome, proof contents, or even the returned root hash. That means a broken shielded proof path can still go green here. Please verify these variants explicitly, or make the test fail until shielded verification is implemented.
🤖 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/verify_state_transitions.rs`
around lines 1448 - 1453, The match arms for
StateTransitionAction::ShieldAction, ::ShieldedTransferAction, ::UnshieldAction,
::ShieldFromAssetLockAction, ::ShieldedWithdrawalAction, and
::PenalizeShieldedPoolAction are no-ops and must not silently accept shielded
transitions; update each arm in the verify_state_transitions helper to either
call the appropriate shielded verification helper (e.g., verify_shielded_action
or the existing shield proof verifier) with assertions on execution outcome and
returned root hash, or if verification is not yet implemented, make the test
fail immediately by panicking or returning an Err with a clear message like
"unimplemented: shielded action verification" so missing verification cannot
pass silently.
Member
Author
There was a problem hiding this comment.
this is just temporary
Contributor
There was a problem hiding this comment.
@QuantumExplorer, understood — thanks for the clarification. Since these are intentional placeholders pending a future PR, it would be helpful to add a brief comment (e.g. // TODO: implement shielded proof verification) to each arm so that it's clear to anyone reading the code that this is a known gap rather than an oversight. This also makes it easier to grep for when the follow-up implementation comes around.
✏️ Learnings added
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 3198
File: packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs:1448-1453
Timestamp: 2026-03-09T09:49:28.771Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/verify_state_transitions.rs`, the empty no-op match arms for `StateTransitionAction::ShieldAction`, `ShieldedTransferAction`, `UnshieldAction`, `ShieldFromAssetLockAction`, `ShieldedWithdrawalAction`, and `PenalizeShieldedPoolAction` in `verify_state_transitions_were_or_were_not_executed` are intentional temporary placeholders. Full shielded proof verification for these variants will be implemented in a future PR (related to the Medusa/ZK shielded pool feature).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lklimek
Repo: dashpay/platform PR: 3032
File: packages/rs-drive-abci/tests/strategy_tests/test_cases/identity_and_document_tests.rs:670-672
Timestamp: 2026-01-30T15:22:20.469Z
Learning: Enforce descriptive naming for new tests in test files with names starting with 'should …'. Do not rename existing tests that already follow a different convention. This applies to all test files under any directory named tests, for JS/JSX/TS/TSX/.RS test files as well.
packages/rs-drive/src/drive/shielded/nullifiers/fetch_compacted_nullifiers/v0/mod.rs
Show resolved
Hide resolved
...ve/src/state_transition_action/action_convert_to_operations/system/penalize_shielded_pool.rs
Show resolved
Hide resolved
Comment on lines
+85
to
+87
| // Execute drive operation callbacks after updating state | ||
| for task in finalize_tasks { | ||
| task.execute(self, platform_version); | ||
| task.execute(self, transaction, platform_version)?; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Definitions around apply + finalize:"
rg -n -C4 'apply_drive_operations_v0|apply_batch_low_level_drive_operations|task\.execute\(' --type rust
echo
echo "Call sites of apply_drive_operations_v0:"
rg -n -C3 'apply_drive_operations_v0\s*\(' --type rust
echo
echo "Direct uses of None with drive operation application APIs:"
rg -n -C3 'apply_drive_operations(_v0)?\([^;\n]*None' --type rust
echo
echo "Finalize task implementations:"
fd 'finalize_task.*\.rs$' packages/rs-drive/src -x sed -n '1,240p' {}Repository: dashpay/platform
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full implementation of apply_drive_operations_v0
echo "=== Full implementation of apply_drive_operations_v0 ==="
sed -n '40,95p' packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v0/mod.rs
echo ""
echo "=== Direct call sites with context (limit output) ==="
rg 'apply_drive_operations_v0\s*\(' -B2 -A5 --max-count 10 --type rust
echo ""
echo "=== Check for None transaction patterns with apply_batch_low_level_drive_operations ==="
rg 'apply_batch_low_level_drive_operations\s*\([^)]*None[^)]*\)' --type rust --max-count 20
echo ""
echo "=== Transaction parameter definition ==="
rg 'fn apply_drive_operations_v0|fn apply_batch_low_level_drive_operations' -A15 --type rust | head -60Repository: dashpay/platform
Length of output: 4168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== TransactionArg definition ==="
rg 'type TransactionArg|pub.*TransactionArg' --type rust | head -20
echo ""
echo "=== Search for TransactionArg import/definition ==="
rg -l 'TransactionArg' packages/rs-drive/src/util/batch --type rust
echo ""
echo "=== Get the mod.rs file that imports TransactionArg ==="
head -50 packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v0/mod.rs
echo ""
echo "=== Callers of apply_drive_operations (the wrapper) ==="
rg 'apply_drive_operations\s*\(' --type rust -B3 -A3 | grep -v apply_drive_operations_v0 | grep -v apply_batch | head -80Repository: dashpay/platform
Length of output: 9433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check grovedb TransactionArg definition (likely type alias) ==="
rg 'TransactionArg' packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/mod.rs -B5 -A5
echo ""
echo "=== Complete apply_drive_operations wrapper signature and implementation ==="
sed -n '30,70p' packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/mod.rs
echo ""
echo "=== Actual call to apply_drive_operations with transaction details ==="
rg 'apply_drive_operations\(' packages/rs-drive-abci/src -B5 -A5 --type rust | head -100Repository: dashpay/platform
Length of output: 13281
🏁 Script executed:
#!/bin/bash
set -euo pipeflight
echo "=== Find all call sites passing None to apply_drive_operations ==="
rg 'apply_drive_operations\(' packages/rs-drive-abci/src -B3 -A8 --type rust | grep -A8 'apply_drive_operations' | grep -B3 'None'
echo ""
echo "=== Check what TransactionArg is from grovedb ==="
# Look at how it's used - if None is passed, it must be Option-like
rg 'transaction\s*:\s*TransactionArg|TransactionArg<' packages/rs-drive/src --type rust -A2 | head -40
echo ""
echo "=== Finalize task execute signature ==="
rg 'fn execute' packages/rs-drive/src/util/batch/drive_op_batch/finalize_task.rs -A8 | head -40
echo ""
echo "=== Check grovedb for TransactionArg type definition in Cargo.lock or dependencies ==="
grep -A5 'name = "grovedb"' Cargo.lock | head -10Repository: dashpay/platform
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find all call sites passing None to apply_drive_operations ==="
rg 'apply_drive_operations\(' packages/rs-drive-abci/src -B3 -A8 --type rust | grep -A8 'apply_drive_operations' | grep -B3 'None'
echo ""
echo "=== Check what TransactionArg is from grovedb ==="
# Look at how it's used - if None is passed, it must be Option-like
rg 'transaction\s*:\s*TransactionArg|TransactionArg<' packages/rs-drive/src --type rust -A2 | head -40
echo ""
echo "=== Finalize task execute signature ==="
rg 'fn execute' packages/rs-drive/src/util/batch/drive_op_batch/finalize_task.rs -A8 | head -40
echo ""
echo "=== Check grovedb for TransactionArg type definition in Cargo.lock or dependencies ==="
grep -A5 'name = "grovedb"' Cargo.lock | head -10Repository: dashpay/platform
Length of output: 24075
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finalize task implementations ==="
cat packages/rs-drive/src/util/batch/drive_op_batch/finalize_task.rs
echo ""
echo "=== Example of finalize task that makes external calls ==="
rg 'RemoveDataContractFromCache|DriveOperationFinalizeTask' packages/rs-drive/src -A10 --type rust | grep -A10 'enum DriveOperationFinalizeTask'Repository: dashpay/platform
Length of output: 2672
Finalize errors can escape after batch operations are already committed.
The task.execute(...)? call on line 87 runs after apply_batch_low_level_drive_operations() has already executed. When this function is called with transaction: None (which occurs in query code paths), any error from a finalize task will be returned to the caller after state modifications are irreversibly applied.
While the current RemoveDataContractFromCache implementation cannot fail, the execute() signature returns Result<(), Error>, allowing future finalize tasks to fail. Ensure either:
- Finalize tasks are documented to never fail, or
- Both the apply phase and finalization are always wrapped in a single transaction boundary at the call site
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-drive/src/util/batch/drive_op_batch/drive_methods/apply_drive_operations/v0/mod.rs`
around lines 85 - 87, The finalize task errors can escape after state has been
committed because finalize tasks (task.execute) run after
apply_batch_low_level_drive_operations when transaction may be None; change the
call site so finalization runs inside the same transaction boundary as the apply
phase (i.e., ensure the same Transaction object is passed into both
apply_batch_low_level_drive_operations and the subsequent task.execute calls and
that any commit/abort happens only after all task.execute() results are
handled), or alternatively change the finalize task contract by making execute
non-fallible (remove Result return) and update implementations like
RemoveDataContractFromCache accordingly; locate task.execute,
apply_batch_low_level_drive_operations, and RemoveDataContractFromCache in this
module to implement the chosen fix.
...-drive/src/util/grove_operations/batch_insert_auto_incremented_items_in_count_tree/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs
Outdated
Show resolved
Hide resolved
Comment on lines
+17
to
+29
| /// - `pool_type`: The shielded pool type (0 = credit, 1 = main token, 2 = individual token). | ||
| /// - `pool_identifier`: Optional 32-byte identifier for individual token pools. | ||
| /// - `platform_version`: A reference to the platform version. | ||
| /// | ||
| /// # Returns | ||
| /// - `Ok((RootHash, GroveTrunkQueryResult))`: The root hash and verified trunk result. | ||
| /// - `Err(Error)`: If verification fails. | ||
| pub fn verify_nullifiers_trunk_query( | ||
| proof: &[u8], | ||
| pool_type: u32, | ||
| pool_identifier: Option<&[u8]>, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<(RootHash, GroveTrunkQueryResult), Error> { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect the verifier entry point and v0 implementation:"
fd 'mod.rs' packages/rs-drive/src/verify/shielded/verify_nullifiers_trunk_query -x sh -c 'echo "--- $1 ---"; sed -n "1,220p" "$1"' sh {}
echo
echo "Search for pool_identifier validation or fixed-size conversions:"
rg -n -C3 '\bpool_identifier\b|len\(\)\s*==\s*32|try_into\(\)|\[u8;\s*32\]' packages/rs-drive/src/verify/shielded/verify_nullifiers_trunk_query packages/rs-drive/src -g '!**/target/**'
echo
echo "Inspect current call sites:"
rg -nP -C2 '\bverify_nullifiers_trunk_query\s*\(' packages/rs-drive/src -g '!**/target/**'Repository: dashpay/platform
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find nullifiers_path_for_pool definition ==="
rg -n 'fn nullifiers_path_for_pool' packages/rs-drive/src -g '!**/target/**' -A 20
echo
echo "=== Check for uses of nullifiers_path_for_pool ==="
rg -n 'nullifiers_path_for_pool' packages/rs-drive/src -g '!**/target/**' | head -20Repository: dashpay/platform
Length of output: 2936
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect call sites with context ==="
rg -n 'verify_nullifiers.*query\s*\(' packages/rs-drive/src -g '!**/target/**' -B 2 -A 5 | head -80
echo
echo "=== Check prove_nullifiers functions (similar pattern) ==="
rg -n 'prove_nullifiers.*query' packages/rs-drive/src -g '!**/target/**' -B 2 -A 3 | head -60Repository: dashpay/platform
Length of output: 8322
Enforce the documented 32-byte pool_identifier contract before token pool support is added.
The public verify_nullifiers_trunk_query API (and verify_nullifiers_branch_query) accepts pool_identifier: Option<&[u8]> but documents it as "Optional 32-byte identifier for individual token pools." The underlying nullifiers_path_for_pool function currently discards the parameter (prefixed with _) and returns NotSupported for pool types 1 and 2. However, when those pool types are implemented, callers will silently pass identifiers of any length without validation, risking silent failures or incorrect pool routing.
Prefer Option<&[u8; 32]> in the public signature, or add explicit len() == 32 validation at the entry point before those pool types are supported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-drive/src/verify/shielded/verify_nullifiers_trunk_query/mod.rs`
around lines 17 - 29, The public API verify_nullifiers_trunk_query currently
accepts pool_identifier: Option<&[u8]> but promises a 32-byte identifier; update
the function to enforce that contract by either changing the signature to
Option<&[u8; 32]> or by validating pool_identifier.map(|b| b.len() == 32) at the
start and returning an Error if length != 32; apply the same enforcement to
verify_nullifiers_branch_query and ensure callers and any uses of
nullifiers_path_for_pool expect/receive a validated 32-byte identifier so future
token-pool implementations cannot receive arbitrary-length slices.
...-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
Show resolved
Hide resolved
...-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
Show resolved
Hide resolved
- Add checked_add for auto-incremented count tree index to prevent overflow - Require withdrawal document to be present in executed ShieldedWithdrawal proof - Reject NotPresent asset lock info in executed ShieldFromAssetLock proof - Fix compacted nullifier dedup to compare full (start_block, end_block) tuple - Error on unsupported V1 proof byte variants instead of silently returning empty Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs (1)
1-2: Consider importing the constant instead of duplicating it.The comment acknowledges this constant must match
saved_block_transactions::COMPACTED_NULLIFIERS_KEY_U8. Duplicating constants creates a maintenance risk if one is updated without the other. Consider importing the constant from its canonical location to ensure consistency.♻️ Suggested refactor
-/// The subtree key for compacted nullifiers storage as u8 (must match saved_block_transactions::COMPACTED_NULLIFIERS_KEY_U8) -const COMPACTED_NULLIFIERS_KEY_U8: u8 = b'o'; +use crate::drive::saved_block_transactions::COMPACTED_NULLIFIERS_KEY_U8;This requires ensuring
COMPACTED_NULLIFIERS_KEY_U8ispubinsaved_block_transactions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs` around lines 1 - 2, You are duplicating the constant COMPACTED_NULLIFIERS_KEY_U8 in this module; instead import the canonical constant from saved_block_transactions to avoid drift—make sure saved_block_transactions::COMPACTED_NULLIFIERS_KEY_U8 is declared pub, remove the local const, and replace usages with the imported symbol (e.g., refer to saved_block_transactions::COMPACTED_NULLIFIERS_KEY_U8 or bring it into scope with use) so both modules share the single source of truth.packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_nullifiers/v0/mod.rs (1)
235-263: Consider extracting fallback key construction to reduce repetition.The fallback key
(start_block_height, start_block_height)is constructed identically in three branches (lines 247-250, 253-256, 259-262). Extracting this to a closure or computing it once before the match would improve maintainability.♻️ Proposed refactor
+ // Precompute fallback key + let fallback_key = || { + let mut key = Vec::with_capacity(16); + key.extend_from_slice(&start_block_height.to_be_bytes()); + key.extend_from_slice(&start_block_height.to_be_bytes()); + key + }; + let start_key = if let Some((key, _)) = desc_results.to_key_elements().into_iter().next() { if key.len() == 16 { let end_block = u64::from_be_bytes(key[8..16].try_into().map_err(|_| { Error::Protocol(Box::new(ProtocolError::CorruptedSerialization( "invalid compacted key slice".to_string(), ))) })?); // If this range contains start_block_height, use its exact key if end_block >= start_block_height { key } else { - // No containing range, use (start_block_height, start_block_height) - let mut key = Vec::with_capacity(16); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key + fallback_key() } } else { - let mut key = Vec::with_capacity(16); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key + fallback_key() } } else { - let mut key = Vec::with_capacity(16); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key.extend_from_slice(&start_block_height.to_be_bytes()); - key + fallback_key() };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_nullifiers/v0/mod.rs` around lines 235 - 263, The code builds the same fallback key (start_block_height,start_block_height) in three places when computing start_key; extract this into a single reusable value or closure (e.g., let fallback_key = || { ... } or let fallback_key = make_fallback_bytes(start_block_height)) and replace the repeated Vec::with_capacity/extend_from_slice blocks in the branches of the desc_results.to_key_elements() handling and the key.len() != 16 branch so all fallbacks return the shared fallback_key, keeping the existing logic that returns the parsed key when end_block >= start_block_height and preserving the current ProtocolError handling for the corrupted slice case.packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs (1)
1307-1310: UseRootTree::SpentAssetLockTransactions as u8instead of the hardcoded literal.The codebase already uses enum-based path construction elsewhere (e.g.,
packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs:404). Consistent use ofvec![vec![RootTree::SpentAssetLockTransactions as u8]]improves maintainability and aligns with existing patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs` around lines 1307 - 1310, Replace the hardcoded path byte vec![vec![72u8]] with the enum-backed value to match existing patterns: use vec![vec![RootTree::SpentAssetLockTransactions as u8]] when constructing the grovedb::PathQuery in the verify_state_transition_was_executed_with_proof/v0 code (the block creating grovedb::PathQuery with grovedb::SizedQuery). This ensures you reference RootTree::SpentAssetLockTransactions instead of the magic number and keeps path construction consistent across PathQuery usage.packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs (1)
97-112: Add direct regression coverage for the new V1 branch.This change now has a distinct
GroveDBProof::V1fail-closed path, but this module still only verifies a single opaque fixture and never asserts the non-Merkerror case. Please add a focused V1 test for bothProofBytes::Merkand an unsupported variant returningCorruptedProof.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs` around lines 97 - 112, Add a focused unit test that directly constructs a GroveDBProof::V1 value exercising the new branch: (1) a case where root_layer.lower_layers contains a layer with ProofBytes::Merk bytes and assert that extract_kv_entries_from_merk_proof is used and returns the expected entries; and (2) a case where the compacted layer contains a non-Merk ProofBytes variant and assert the function returns an Error::Proof(ProofError::CorruptedProof(...)). Locate the V1 branch handling (GroveDBProof::V1, root_layer.lower_layers, compacted_key) and write tests that build minimal GroveDBProof::V1 fixtures or builders to trigger both the Merk and non-Merk paths, asserting success for the Merk case and the specific CorruptedProof error for the unsupported variant.
🤖 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/saved_block_transactions/fetch_compacted_nullifiers/v0/mod.rs`:
- Around line 235-263: The code builds the same fallback key
(start_block_height,start_block_height) in three places when computing
start_key; extract this into a single reusable value or closure (e.g., let
fallback_key = || { ... } or let fallback_key =
make_fallback_bytes(start_block_height)) and replace the repeated
Vec::with_capacity/extend_from_slice blocks in the branches of the
desc_results.to_key_elements() handling and the key.len() != 16 branch so all
fallbacks return the shared fallback_key, keeping the existing logic that
returns the parsed key when end_block >= start_block_height and preserving the
current ProtocolError handling for the corrupted slice case.
In
`@packages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rs`:
- Around line 97-112: Add a focused unit test that directly constructs a
GroveDBProof::V1 value exercising the new branch: (1) a case where
root_layer.lower_layers contains a layer with ProofBytes::Merk bytes and assert
that extract_kv_entries_from_merk_proof is used and returns the expected
entries; and (2) a case where the compacted layer contains a non-Merk ProofBytes
variant and assert the function returns an
Error::Proof(ProofError::CorruptedProof(...)). Locate the V1 branch handling
(GroveDBProof::V1, root_layer.lower_layers, compacted_key) and write tests that
build minimal GroveDBProof::V1 fixtures or builders to trigger both the Merk and
non-Merk paths, asserting success for the Merk case and the specific
CorruptedProof error for the unsupported variant.
In
`@packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs`:
- Around line 1-2: You are duplicating the constant COMPACTED_NULLIFIERS_KEY_U8
in this module; instead import the canonical constant from
saved_block_transactions to avoid drift—make sure
saved_block_transactions::COMPACTED_NULLIFIERS_KEY_U8 is declared pub, remove
the local const, and replace usages with the imported symbol (e.g., refer to
saved_block_transactions::COMPACTED_NULLIFIERS_KEY_U8 or bring it into scope
with use) so both modules share the single source of truth.
In
`@packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- Around line 1307-1310: Replace the hardcoded path byte vec![vec![72u8]] with
the enum-backed value to match existing patterns: use
vec![vec![RootTree::SpentAssetLockTransactions as u8]] when constructing the
grovedb::PathQuery in the verify_state_transition_was_executed_with_proof/v0
code (the block creating grovedb::PathQuery with grovedb::SizedQuery). This
ensures you reference RootTree::SpentAssetLockTransactions instead of the magic
number and keeps path construction consistent across PathQuery usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbf3e203-c599-4654-afa9-f30442e5df93
📒 Files selected for processing (5)
packages/rs-drive/src/drive/saved_block_transactions/fetch_compacted_nullifiers/v0/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_auto_incremented_items_in_count_tree/v0/mod.rspackages/rs-drive/src/verify/address_funds/verify_compacted_address_balance_changes/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rspackages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive/src/util/grove_operations/batch_insert_auto_incremented_items_in_count_tree/v0/mod.rs
Add AddressWitnessError and ShieldedBuildError variants to ProtocolError to replace Generic usage in address witness verification and shielded transaction building code. Also fix IdentifierError usage in wallet FFI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs (1)
53-58:⚠️ Potential issue | 🟠 MajorValidate
withdrawal_amount + fee, not justwithdrawal_amount.The Orchard bundle carries the total value leaving the pool in an
i64.withdrawal_amount == i64::MAX as u64with any positive fee still passes the first guard here, but the resultingvalue_balancecan no longer be represented and only fails later in the builder.Suggested fix
let required = withdrawal_amount .checked_add(effective_fee) .ok_or_else(|| { ProtocolError::ShieldedBuildError("fee + withdrawal_amount overflows u64".to_string()) })?; + if required > i64::MAX as u64 { + return Err(ProtocolError::ShieldedBuildError(format!( + "withdrawal amount {} + fee {} exceeds maximum allowed value {}", + withdrawal_amount, + effective_fee, + i64::MAX as u64 + ))); + } if required > total_spent { return Err(ProtocolError::ShieldedBuildError(format!( "withdrawal amount {} + fee {} = {} exceeds total spendable value {}",Also applies to: 77-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs` around lines 53 - 58, The guard currently only checks withdrawal_amount against i64::MAX, but you must validate the sum withdrawal_amount + fee fits in an i64 because the Orchard bundle encodes total value leaving the pool; update the checks (the one around withdrawal_amount and the analogous block at 77-86) to compute and validate total_out = withdrawal_amount.checked_add(fee) (or equivalent) and error if total_out > i64::MAX as u64 or if addition overflows; ensure you update the error message to mention the combined amount and reference the same ProtocolError::ShieldedBuildError path used today so downstream value_balance calculations won't overflow.packages/rs-dpp/src/shielded/builder/unshield.rs (1)
47-52:⚠️ Potential issue | 🟠 MajorValidate
unshield_amount + fee, not justunshield_amount.The Orchard bundle encodes the total value leaving the pool as an
i64. Anunshield_amountat the current ceiling still slips through here wheneffective_fee > 0, even though the finalvalue_balanceis no longer representable and will only fail deeper in the build path.Suggested fix
let required = unshield_amount .checked_add(effective_fee) .ok_or_else(|| ProtocolError::ShieldedBuildError("fee + unshield_amount overflows u64".to_string()))?; + if required > i64::MAX as u64 { + return Err(ProtocolError::ShieldedBuildError(format!( + "unshield amount {} + fee {} exceeds maximum allowed value {}", + unshield_amount, + effective_fee, + i64::MAX as u64 + ))); + } if required > total_spent { return Err(ProtocolError::ShieldedBuildError(format!( "unshield amount {} + fee {} = {} exceeds total spendable value {}",Also applies to: 71-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/shielded/builder/unshield.rs` around lines 47 - 52, The existing checks validate only unshield_amount against i64::MAX but must validate the total leaving the pool (unshield_amount + effective_fee); update both validation sites (the check around unshield_amount and the similar block at lines 71-78) to compute the sum using checked_add (e.g., total_out = unshield_amount.checked_add(effective_fee)) to avoid overflow and then compare total_out against i64::MAX as u64, returning ProtocolError::ShieldedBuildError with the same style message if it exceeds the limit or if checked_add returns None.packages/rs-dpp/src/shielded/builder/shielded_transfer.rs (1)
52-75:⚠️ Potential issue | 🔴 CriticalDon't force the two-output minimum fee when no change output is emitted.
spends.len().max(2)assumes every transfer needs both the recipient output and a change output, but the builder only adds change whenchange_amount > 0. For a single-note exact-spend transfer, this overstates the minimum fee and can reject a transaction that would be valid with the one-output fee schedule.Choose the fee floor from the actual output count that will be serialized, or retry the single-output path before returning
exceeds total spendable value.
🤖 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-dpp/src/errors/protocol_error.rs`:
- Around line 143-147: The new variants AddressWitnessError and
ShieldedBuildError expand the public ProtocolError enum and break exhaustive
matches; to avoid a breaking change, mark the ProtocolError enum with
#[non_exhaustive] (i.e., add the #[non_exhaustive] attribute above the enum
declaration of ProtocolError) so downstream code cannot exhaustively match it,
and update any public docs/changelog to note the change; ensure the added
attribute is applied where ProtocolError is defined and re-run cargo
build/tests.
---
Outside diff comments:
In `@packages/rs-dpp/src/shielded/builder/shielded_withdrawal.rs`:
- Around line 53-58: The guard currently only checks withdrawal_amount against
i64::MAX, but you must validate the sum withdrawal_amount + fee fits in an i64
because the Orchard bundle encodes total value leaving the pool; update the
checks (the one around withdrawal_amount and the analogous block at 77-86) to
compute and validate total_out = withdrawal_amount.checked_add(fee) (or
equivalent) and error if total_out > i64::MAX as u64 or if addition overflows;
ensure you update the error message to mention the combined amount and reference
the same ProtocolError::ShieldedBuildError path used today so downstream
value_balance calculations won't overflow.
In `@packages/rs-dpp/src/shielded/builder/unshield.rs`:
- Around line 47-52: The existing checks validate only unshield_amount against
i64::MAX but must validate the total leaving the pool (unshield_amount +
effective_fee); update both validation sites (the check around unshield_amount
and the similar block at lines 71-78) to compute the sum using checked_add
(e.g., total_out = unshield_amount.checked_add(effective_fee)) to avoid overflow
and then compare total_out against i64::MAX as u64, returning
ProtocolError::ShieldedBuildError with the same style message if it exceeds the
limit or if checked_add returns None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28f7b25b-b4dd-4266-9908-e773a88f09bb
📒 Files selected for processing (9)
packages/rs-dpp/src/address_funds/platform_address.rspackages/rs-dpp/src/errors/protocol_error.rspackages/rs-dpp/src/shielded/builder/mod.rspackages/rs-dpp/src/shielded/builder/shield.rspackages/rs-dpp/src/shielded/builder/shield_from_asset_lock.rspackages/rs-dpp/src/shielded/builder/shielded_transfer.rspackages/rs-dpp/src/shielded/builder/shielded_withdrawal.rspackages/rs-dpp/src/shielded/builder/unshield.rspackages/rs-platform-wallet-ffi/src/types.rs
Comment on lines
+143
to
+147
| #[error("Address witness verification error: {0}")] | ||
| AddressWitnessError(String), | ||
|
|
||
| #[error("Shielded transaction build error: {0}")] | ||
| ShieldedBuildError(String), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rust semver, is adding a variant to a public enum without #[non_exhaustive] considered a breaking change for downstream crates?
💡 Result:
Yes. In Rust, adding a new variant to a public enum that is not marked #[non_exhaustive] is considered a SemVer-breaking change for downstream crates, because downstream code can (and often does) exhaustively match on the enum, and a new variant makes those matches fail to compile. [1], [2]
If the enum is #[non_exhaustive], downstream crates are required to include a wildcard arm (or otherwise handle unknown variants), so adding variants is intended to be non-breaking. [1]
Sources:
[1] Rust Cargo Book — SemVer Compatibility
[2] Effective Rust — SemVer discussion of adding enum variants
🏁 Script executed:
cat -n packages/rs-dpp/src/errors/protocol_error.rs | head -160Repository: dashpay/platform
Length of output: 6248
🏁 Script executed:
rg "AddressWitnessError|ShieldedBuildError" --type rust -B 2 -A 2Repository: dashpay/platform
Length of output: 22729
This is a breaking expansion of the public ProtocolError surface.
ProtocolError is public and not #[non_exhaustive], so adding AddressWitnessError and ShieldedBuildError breaks downstream exhaustive matches. These variants are returned from public builder and witness-verification functions, so callers that key off Generic specifically will miss them unless this release is treated as breaking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-dpp/src/errors/protocol_error.rs` around lines 143 - 147, The new
variants AddressWitnessError and ShieldedBuildError expand the public
ProtocolError enum and break exhaustive matches; to avoid a breaking change,
mark the ProtocolError enum with #[non_exhaustive] (i.e., add the
#[non_exhaustive] attribute above the enum declaration of ProtocolError) so
downstream code cannot exhaustively match it, and update any public
docs/changelog to note the change; ensure the added attribute is applied where
ProtocolError is defined and re-run cargo build/tests.
- Update grovedb to 99a2ab01 which splits InsertOnly into InsertWithKnownToNotAlreadyExist and InsertIfNotExists - Adapt API changes: empty_commitment_tree/empty_bulk_append_tree now return Result, prove_query_v1 renamed to prove_query - Use GROVE_V3 in drive version v7 for non-Merk tree proof support - Update fee regression tests for protocol v12 shielded pool tree costs (+51,300 processing credits due to larger Merk propagation) - Add 3 protocol v11 fee regression tests to verify pre-shielded fees Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-drive/src/util/batch/grovedb_op_batch/mod.rs (1)
696-708:⚠️ Potential issue | 🟠 MajorCaller does not handle new insert variants — elements will be discarded.
The
remove_if_insertmethod now removesInsertWithKnownToNotAlreadyExistandInsertIfNotExistsoperations from the batch (lines 700-701), but the only caller inbatch_remove_raw/v0/mod.rsdoes not have match arms for these variants. They fall into the catch-all_case at line 49, which setsneeds_removal_from_state = trueinstead of returning the element like other insert operations do.This causes incorrect behavior: the element is silently discarded rather than returned to the caller.
Update the caller's match statement to include these variants:
Some(GroveOp::InsertOrReplace { element }) | Some(GroveOp::Replace { element }) | Some(GroveOp::Patch { element, .. }) | Some(GroveOp::InsertWithKnownToNotAlreadyExist { element }) | Some(GroveOp::InsertIfNotExists { element, .. }) => return Ok(Some(element)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/util/batch/grovedb_op_batch/mod.rs` around lines 696 - 708, The caller in batch_remove_raw/v0/mod.rs does not handle the new GroveOp variants InsertWithKnownToNotAlreadyExist and InsertIfNotExists, causing removed inserts to be treated as generic removals; update the match in the caller (the match that currently returns elements for InsertOrReplace, Replace, Patch but falls through to `_`) to include the two new variants so they return the element as well (i.e., add patterns for Some(GroveOp::InsertWithKnownToNotAlreadyExist { element }) and Some(GroveOp::InsertIfNotExists { element, .. }) alongside the existing InsertOrReplace/Replace/Patch arms).
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs (1)
8291-8534: Consider table-driving the protocol-v11 fee cases.These tests duplicate the same setup/execution path as the latest-protocol regressions, which makes future fee fixture updates easy to miss in one matrix. A small helper that takes the protocol version, fee strategy, fee increase, and expected fees would keep the v11 and latest baselines in sync.
🤖 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 8291 - 8534, The three protocol-v11 fee tests duplicate setup and execution; refactor by creating a small table-driven helper (e.g., run_fee_regression_test) that accepts platform_version, fee_strategy (Vec<AddressFundsFeeStrategyStep>), user_fee_increase, input/output amounts, and expected (processing_fee, storage_fee, total_fee), and performs the common steps using TestPlatformBuilder, setup_address_with_balance, AddressFundsTransferTransitionV0::try_from_inputs_with_signer, transition.serialize_to_bytes, platform.platform.process_raw_state_transitions, and extract_fees; then replace the three tests (test_fee_simple_p2pkh_1_input_1_output_deduct_from_input_in_protocol_v11, test_fee_simple_p2pkh_1_input_1_output_reduce_output_in_protocol_v11, test_fee_with_user_fee_increase_in_protocol_v11) with table entries that call the helper with the appropriate fee strategy and expected values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs`:
- Around line 8276-8288: The test currently only asserts total_fee_new and
total_fee_existing; change it to also assert the individual fee components so
regressions that move cost between processing_fee and storage_fee are caught.
Locate the variables used in the test (e.g., processing_fee_new, storage_fee_new
and processing_fee_existing, storage_fee_existing alongside
total_fee_new/total_fee_existing) and add explicit assert_eq! checks for each
component with the expected literal values matching the intended processing-fee
delta, keeping the existing total assertions as well.
In `@packages/rs-drive/Cargo.toml`:
- Around line 55-60: The rs-dpp manifest still pins the grovedb-commitment-tree
crate to an older revision; update the grovedb-commitment-tree dependency in
packages/rs-dpp/Cargo.toml to use the same rev
"99a2ab0187b6a5f05c5bed7b4882646bf652f05f" used by the other GroveDB crates so
all grovedb-related crates (grovedb, grovedb-costs, grovedb-path,
grovedb-storage, grovedb-version, grovedb-epoch-based-storage-flags) share the
identical commit and avoid type/trait mismatches.
In `@packages/rs-drive/src/fees/op.rs`:
- Around line 638-645: The sinsemilla_cost is currently computed with unchecked
multiplication which can overflow before the checked_add chain; change the
computation to use checked multiplication (call checked_mul on
fee_version.hashing.sinsemilla_base with *sinsemilla_hash_calls as u64) and
propagate the Option result into the existing addition chain (i.e., if
checked_mul returns None, the whole expression should return None), updating the
chain that references sinsemilla_cost so it uses the Option from checked_mul
instead of a plain u64.
---
Outside diff comments:
In `@packages/rs-drive/src/util/batch/grovedb_op_batch/mod.rs`:
- Around line 696-708: The caller in batch_remove_raw/v0/mod.rs does not handle
the new GroveOp variants InsertWithKnownToNotAlreadyExist and InsertIfNotExists,
causing removed inserts to be treated as generic removals; update the match in
the caller (the match that currently returns elements for InsertOrReplace,
Replace, Patch but falls through to `_`) to include the two new variants so they
return the element as well (i.e., add patterns for
Some(GroveOp::InsertWithKnownToNotAlreadyExist { element }) and
Some(GroveOp::InsertIfNotExists { element, .. }) alongside the existing
InsertOrReplace/Replace/Patch arms).
---
Nitpick comments:
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs`:
- Around line 8291-8534: The three protocol-v11 fee tests duplicate setup and
execution; refactor by creating a small table-driven helper (e.g.,
run_fee_regression_test) that accepts platform_version, fee_strategy
(Vec<AddressFundsFeeStrategyStep>), user_fee_increase, input/output amounts, and
expected (processing_fee, storage_fee, total_fee), and performs the common steps
using TestPlatformBuilder, setup_address_with_balance,
AddressFundsTransferTransitionV0::try_from_inputs_with_signer,
transition.serialize_to_bytes, platform.platform.process_raw_state_transitions,
and extract_fees; then replace the three tests
(test_fee_simple_p2pkh_1_input_1_output_deduct_from_input_in_protocol_v11,
test_fee_simple_p2pkh_1_input_1_output_reduce_output_in_protocol_v11,
test_fee_with_user_fee_increase_in_protocol_v11) with table entries that call
the helper with the appropriate fee strategy and expected values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79c26907-0168-4ebd-88e6-c3d1b8af4ef7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rspackages/rs-drive/Cargo.tomlpackages/rs-drive/src/drive/initialization/v3/mod.rspackages/rs-drive/src/drive/tokens/system/add_to_token_total_supply/v0/mod.rspackages/rs-drive/src/fees/op.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/mod.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_transfer_transition.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.rspackages/rs-drive/src/util/batch/drive_op_batch/shielded.rspackages/rs-drive/src/util/batch/grovedb_op_batch/mod.rspackages/rs-drive/src/util/grove_operations/batch_insert_empty_tree_if_not_exists/v0/mod.rspackages/rs-drive/src/util/grove_operations/grove_get_proved_path_query_v1/v0/mod.rspackages/rs-drive/src/util/grove_operations/grove_insert_empty_tree/v0/mod.rspackages/rs-platform-version/Cargo.tomlpackages/rs-platform-version/src/version/drive_versions/v7.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.rs
- packages/rs-drive/src/util/batch/drive_op_batch/shielded.rs
- packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/mod.rs
Comment on lines
8276
to
8288
| // Assert exact values for new address | ||
| assert_eq!( | ||
| total_fee_new, 6532440, | ||
| "Total fee to new address changed! Was 6532440, now {}", | ||
| total_fee_new, 6583740, | ||
| "Total fee to new address changed! Was 6583740, now {}", | ||
| total_fee_new | ||
| ); | ||
|
|
||
| // Assert exact values for existing address (much cheaper - only updates balance) | ||
| assert_eq!( | ||
| total_fee_existing, 445920, | ||
| "Total fee to existing address changed! Was 445920, now {}", | ||
| total_fee_existing, 497220, | ||
| "Total fee to existing address changed! Was 497220, now {}", | ||
| total_fee_existing | ||
| ); |
Contributor
There was a problem hiding this comment.
Pin the fee breakdown here, not just the total.
This case only asserts total_fee_*, so a regression that shifts cost between processing_fee and storage_fee while preserving the same total would still pass. Since the change being tracked is specifically a processing-fee delta, this test should assert the components too.
🤖 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 8276 - 8288, The test currently only asserts total_fee_new and
total_fee_existing; change it to also assert the individual fee components so
regressions that move cost between processing_fee and storage_fee are caught.
Locate the variables used in the test (e.g., processing_fee_new, storage_fee_new
and processing_fee_existing, storage_fee_existing alongside
total_fee_new/total_fee_existing) and add explicit assert_eq! checks for each
component with the expected literal values matching the intended processing-fee
delta, keeping the existing total assertions as well.
packages/rs-drive/Cargo.toml
Outdated
Comment on lines
+55
to
+60
| grovedb = { git = "https://github.com/dashpay/grovedb", rev = "99a2ab0187b6a5f05c5bed7b4882646bf652f05f", optional = true, default-features = false } | ||
| grovedb-costs = { git = "https://github.com/dashpay/grovedb", rev = "99a2ab0187b6a5f05c5bed7b4882646bf652f05f", optional = true } | ||
| grovedb-path = { git = "https://github.com/dashpay/grovedb", rev = "99a2ab0187b6a5f05c5bed7b4882646bf652f05f" } | ||
| grovedb-storage = { git = "https://github.com/dashpay/grovedb", rev = "99a2ab0187b6a5f05c5bed7b4882646bf652f05f", optional = true } | ||
| grovedb-version = { git = "https://github.com/dashpay/grovedb", rev = "99a2ab0187b6a5f05c5bed7b4882646bf652f05f" } | ||
| grovedb-epoch-based-storage-flags = { git = "https://github.com/dashpay/grovedb", rev = "99a2ab0187b6a5f05c5bed7b4882646bf652f05f" } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List all dashpay/grovedb pins across workspace manifests.
rg -n --glob 'Cargo.toml' 'https://github\.com/dashpay/grovedb|rev = "'
# Expected: every dependency from dashpay/grovedb should use
# 99a2ab0187b6a5f05c5bed7b4882646bf652f05f after this PR.
# Any remaining 7ecb8465fad750c7cddd5332adb6f97fcceb498b entries need review.
rg -n --glob 'Cargo.toml' '7ecb8465fad750c7cddd5332adb6f97fcceb498b|99a2ab0187b6a5f05c5bed7b4882646bf652f05f'Repository: dashpay/platform
Length of output: 4231
Update packages/rs-dpp/Cargo.toml to pin grovedb-commitment-tree to the same revision.
packages/rs-dpp/Cargo.toml line 74 still pins grovedb-commitment-tree to 7ecb8465fad750c7cddd5332adb6f97fcceb498b while this manifest updates other GroveDB crates to 99a2ab0187b6a5f05c5bed7b4882646bf652f05f. Since rs-drive depends on rs-dpp, this creates a version split across the same monorepo—different snapshots can introduce type/trait incompatibilities when those crate APIs meet between the two packages. Update line 74 in packages/rs-dpp/Cargo.toml to use the new revision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-drive/Cargo.toml` around lines 55 - 60, The rs-dpp manifest still
pins the grovedb-commitment-tree crate to an older revision; update the
grovedb-commitment-tree dependency in packages/rs-dpp/Cargo.toml to use the same
rev "99a2ab0187b6a5f05c5bed7b4882646bf652f05f" used by the other GroveDB crates
so all grovedb-related crates (grovedb, grovedb-costs, grovedb-path,
grovedb-storage, grovedb-version, grovedb-epoch-based-storage-flags) share the
identical commit and avoid type/trait mismatches.
Comment on lines
+638
to
+645
| let sinsemilla_cost = fee_version.hashing.sinsemilla_base * (*sinsemilla_hash_calls as u64); | ||
| seek_cost | ||
| .checked_add(storage_added_bytes_ephemeral_cost) | ||
| .and_then(|c| c.checked_add(storage_replaced_bytes_ephemeral_cost)) | ||
| .and_then(|c| c.checked_add(storage_loaded_bytes_cost)) | ||
| .and_then(|c| c.checked_add(storage_removed_bytes_ephemeral_cost)) | ||
| .and_then(|c| c.checked_add(hash_node_cost)) | ||
| .and_then(|c| c.checked_add(sinsemilla_cost)) |
Contributor
There was a problem hiding this comment.
Use checked_mul for the new Sinsemilla fee term.
Line 638 reintroduces unchecked multiplication into an otherwise overflow-checked fee path. If sinsemilla_hash_calls spikes, this can overflow before the checked_add chain runs.
Suggested fix
- let sinsemilla_cost = fee_version.hashing.sinsemilla_base * (*sinsemilla_hash_calls as u64);
+ let sinsemilla_cost = fee_version
+ .hashing
+ .sinsemilla_base
+ .checked_mul(*sinsemilla_hash_calls as u64)
+ .ok_or_else(|| get_overflow_error("sinsemilla cost overflow"))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let sinsemilla_cost = fee_version.hashing.sinsemilla_base * (*sinsemilla_hash_calls as u64); | |
| seek_cost | |
| .checked_add(storage_added_bytes_ephemeral_cost) | |
| .and_then(|c| c.checked_add(storage_replaced_bytes_ephemeral_cost)) | |
| .and_then(|c| c.checked_add(storage_loaded_bytes_cost)) | |
| .and_then(|c| c.checked_add(storage_removed_bytes_ephemeral_cost)) | |
| .and_then(|c| c.checked_add(hash_node_cost)) | |
| .and_then(|c| c.checked_add(sinsemilla_cost)) | |
| let sinsemilla_cost = fee_version | |
| .hashing | |
| .sinsemilla_base | |
| .checked_mul(*sinsemilla_hash_calls as u64) | |
| .ok_or_else(|| get_overflow_error("sinsemilla cost overflow"))?; | |
| seek_cost | |
| .checked_add(storage_added_bytes_ephemeral_cost) | |
| .and_then(|c| c.checked_add(storage_replaced_bytes_ephemeral_cost)) | |
| .and_then(|c| c.checked_add(storage_loaded_bytes_cost)) | |
| .and_then(|c| c.checked_add(storage_removed_bytes_ephemeral_cost)) | |
| .and_then(|c| c.checked_add(hash_node_cost)) | |
| .and_then(|c| c.checked_add(sinsemilla_cost)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-drive/src/fees/op.rs` around lines 638 - 645, The sinsemilla_cost
is currently computed with unchecked multiplication which can overflow before
the checked_add chain; change the computation to use checked multiplication
(call checked_mul on fee_version.hashing.sinsemilla_base with
*sinsemilla_hash_calls as u64) and propagate the Option result into the existing
addition chain (i.e., if checked_mul returns None, the whole expression should
return None), updating the chain that references sinsemilla_cost so it uses the
Option from checked_mul instead of a plain u64.
Per-block nullifier storage (recent, compacted, expiration) was incorrectly under SavedBlockTransactions. Move paths and code to shielded/nullifiers/ under [AddressBalances, "s", ...] where the shielded pool lives. Also fix feature gates, cost estimation layer count (4→7), and stale doc comments. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nd improve naming - Rename cleanup_expired_nullifiers to cleanup_expired_nullifier_compactions (the function purges expired sync data, not the permanent nullifiers) - Extract inline grove ops (InsertNote, InsertNullifier, UpdateTotalBalance) into versioned Drive methods: insert_note_op, insert_nullifiers, update_total_balance_op - Merge InsertNullifier + StoreNullifiersForBlock into single InsertNullifiers operation that handles both permanent tree insertion and per-block sync storage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…transition Remove redundant `notes` and `anchor` parameters from all five shielded `try_from_transition` methods. These values are already available on the transition structs (`value.actions` and `value.anchor`), so passing them separately was unnecessary indirection. Add `From<&SerializedAction> for ShieldedActionNote` conversion to cleanly map transition actions to action notes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
packages/rs-drive/src/util/grove_operations/batch_insert_auto_incremented_items_in_count_tree/v0/mod.rs (1)
35-46:⚠️ Potential issue | 🟠 MajorValidate that the element is a CountTree before using its count value.
The
count_value_or_default()method returns1for all non-CountTree element types, not just0for missing elements. If a wrong path is provided pointing to a non-CountTree element, the code silently seeds inserts from key1instead of failing, potentially causing key collisions or breaking the count tree invariant.Consider explicitly matching on
Element::CountTreeand returning an error for unexpected element types:🛠️ Suggested fix
// Read the current count from the count tree element - let current_count = self + let maybe_element = self .grove_get_raw_optional( parent_path.as_slice().into(), count_tree_key, DirectQueryType::StatefulDirectQuery, transaction, drive_operations, drive_version, - )? - .map(|element| element.count_value_or_default()) - .unwrap_or(0); + )?; + + let current_count = match maybe_element { + None => 0, + Some(Element::CountTree(_, count, _)) => count, + Some(_) => { + return Err(Error::Drive(DriveError::CorruptedCodeExecution( + "expected a CountTree element for auto-incremented batch insert", + ))); + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/util/grove_operations/batch_insert_auto_incremented_items_in_count_tree/v0/mod.rs` around lines 35 - 46, The code currently uses element.count_value_or_default() after grove_get_raw_optional, which returns 1 for non-CountTree elements and can silently seed inserts incorrectly; update the logic in the batch_insert_auto_incremented_items_in_count_tree v0 flow to explicitly match the returned element variant (from self.grove_get_raw_optional) and extract the count only when it is Element::CountTree, otherwise return an explicit error (e.g., InvalidElementType or a descriptive error) instead of falling back to the count_value_or_default; ensure the error is propagated through the surrounding function so wrong paths fail fast and do not start inserting from key 1.packages/rs-drive/src/fees/op.rs (1)
640-647:⚠️ Potential issue | 🟠 MajorUse
checked_mulforsinsemilla_cost.Line 640 can overflow before the checked-add chain runs, which can undercount fees in release builds and panic in debug builds.
Suggested fix
- let sinsemilla_cost = fee_version.hashing.sinsemilla_base * (*sinsemilla_hash_calls as u64); + let sinsemilla_cost = fee_version + .hashing + .sinsemilla_base + .checked_mul(*sinsemilla_hash_calls as u64) + .ok_or_else(|| get_overflow_error("sinsemilla cost overflow"))?; seek_cost .checked_add(storage_added_bytes_ephemeral_cost) .and_then(|c| c.checked_add(storage_replaced_bytes_ephemeral_cost)) .and_then(|c| c.checked_add(storage_loaded_bytes_cost)) .and_then(|c| c.checked_add(storage_removed_bytes_ephemeral_cost)) .and_then(|c| c.checked_add(hash_node_cost)) .and_then(|c| c.checked_add(sinsemilla_cost))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/fees/op.rs` around lines 640 - 647, Compute sinsemilla_cost using a checked multiplication instead of a raw multiply to avoid overflow: replace the direct multiplication of fee_version.hashing.sinsemilla_base * (*sinsemilla_hash_calls as u64) with a checked_mul on fee_version.hashing.sinsemilla_base (e.g., fee_version.hashing.sinsemilla_base.checked_mul(*sinsemilla_hash_calls as u64)), then thread that Option into the existing checked-add chain (use and_then or map to add it to c) so the whole sequence fails safely on overflow; update the reference to sinsemilla_cost in the checked-add chain accordingly (look for sinsemilla_cost, fee_version.hashing.sinsemilla_base, sinsemilla_hash_calls and the existing checked_add chain).packages/rs-drive/src/state_transition_action/action_convert_to_operations/system/penalize_shielded_pool.rs (1)
28-32:⚠️ Potential issue | 🟠 MajorUse
checked_subwith error handling for penalty deduction.The use of
saturating_subsilently clamps to zero whenpenalty_amount > current_total_balance. Other balance operations in this codebase (unshield, shielded_transfer, shield, etc.) usechecked_subwith explicit error handling. This inconsistency in consensus-critical code allows the action to succeed even when the penalty exceeds the pool balance, masking potential accounting invariant violations.Suggested fix
- // 2. Deduct penalty from pool total balance - let new_total_balance = v0.current_total_balance.saturating_sub(v0.penalty_amount); + // 2. Deduct penalty from pool total balance + let new_total_balance = v0 + .current_total_balance + .checked_sub(v0.penalty_amount) + .ok_or_else(|| { + Error::Drive(crate::error::drive::DriveError::CorruptedDriveState( + "penalty amount exceeds current total balance".to_string(), + )) + })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/state_transition_action/action_convert_to_operations/system/penalize_shielded_pool.rs` around lines 28 - 32, Replace the silent saturating subtraction on v0.current_total_balance with a checked subtraction and return an explicit error when the penalty exceeds the pool balance: use v0.current_total_balance.checked_sub(v0.penalty_amount) and, if it returns None, propagate/return a descriptive error (consistent with other balance ops like unshield/shielded_transfer) instead of creating the UpdateTotalBalance DriveOperation; update the code around the ShieldedPoolOperation::UpdateTotalBalance construction in penalize_shielded_pool.rs to perform this check and only push the operation when the subtraction succeeds.
🧹 Nitpick comments (8)
packages/rs-drive/src/drive/shielded/nullifiers/compact_nullifiers/v0/mod.rs (2)
149-157: Consider removing unuseddrive_operationsor documenting its purpose.The
drive_operationsvector is created and passed togrove_get_raw_optional, but the operations collected in it are never applied or used afterward. If this is intentionally for cost-tracking that isn't needed here, consider either:
- Using
&mut vec![]inline if the operations aren't needed- Adding a brief comment explaining why the operations are discarded
♻️ Suggested simplification if operations aren't needed
- let mut drive_operations = vec![]; let existing_ranges = self.grove_get_raw_optional( SubtreePath::from(expiration_path.as_ref()), &expiration_key, DirectQueryType::StatefulDirectQuery, transaction, - &mut drive_operations, + &mut vec![], &platform_version.drive, )?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/shielded/nullifiers/compact_nullifiers/v0/mod.rs` around lines 149 - 157, The temporary vector drive_operations passed into grove_get_raw_optional is created but never used later; either replace it with an inline empty mutable vec (e.g., pass &mut vec![]) when calling grove_get_raw_optional to avoid the unused variable, or keep drive_operations and add a one-line comment near its declaration explaining that it intentionally collects operations from grove_get_raw_optional for cost/accounting reasons even though they are discarded; locate the call site using grove_get_raw_optional, SubtreePath::from(expiration_path.as_ref()), expiration_key, DirectQueryType::StatefulDirectQuery, transaction, and platform_version.drive to make the change.
69-76: Unnecessaryclone()beforetry_into().The
clone()is not needed here sincetry_into()will consume theVec<u8>anyway. Sincekeyis only used inkeys_to_delete.push(key)at the end of the loop, you can reorder operations to avoid the clone.♻️ Suggested fix to avoid unnecessary clone
for (key, element) in key_elements { // Parse block height from key (8 bytes, big-endian) - let height_bytes: [u8; 8] = key.clone().try_into().map_err(|_| { + let height_bytes: [u8; 8] = key.as_slice().try_into().map_err(|_| { Error::Protocol(Box::new(ProtocolError::CorruptedSerialization( "invalid block height key length".to_string(), ))) })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/shielded/nullifiers/compact_nullifiers/v0/mod.rs` around lines 69 - 76, Avoid cloning `key` when parsing the block height: replace `key.clone().try_into()` with a slice conversion like `key[..8].try_into()` (or `key.as_slice()[..8].try_into()`), validating the length first to produce the same `ProtocolError` if too short, so you can still call `keys_to_delete.push(key)` later without cloning; update the parsing around the `block_height` assignment (in the loop over `key_elements`) to borrow the first 8 bytes instead of consuming a cloned Vec.packages/rs-dpp/src/shielded/builder/unshield.rs (1)
60-69: Missing upper-bound fee validation present in sibling builder.
build_shielded_transfer_transitionvalidates that fee does not exceed 1000x the minimum (to prevent accidental overpayment), but this function only validates the lower bound. If this is intentional, consider adding a comment explaining why; otherwise, add the same guard for consistency.Some(f) if f < min_fee => { return Err(ProtocolError::ShieldedBuildError(format!( "fee {} is below minimum required fee {}", f, min_fee ))); } + Some(f) if f > min_fee.saturating_mul(1000) => { + return Err(ProtocolError::ShieldedBuildError(format!( + "fee {} exceeds 1000x the minimum fee {}", + f, min_fee + ))); + } Some(f) => f,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/shielded/builder/unshield.rs` around lines 60 - 69, The fee validation here only enforces the lower bound; add the same upper-bound guard used in build_shielded_transfer_transition by checking that a provided fee does not exceed 1000 * min_fee and returning a ProtocolError::ShieldedBuildError (with a clear message) if it does; keep the existing logic for None => min_fee and ensure you reference the same symbols (fee, min_fee, effective_fee) and error variant when implementing the check so behavior matches the sibling builder.packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs (2)
1277-1281: Consider usingIncorrectProoffor consistency with other verification branches.The document existence check is correctly implemented. However, for consistency with similar patterns throughout this file (e.g., lines 88, 186, 218, 257),
IncorrectProofis more appropriate when proof verification succeeded but expected data is absent.♻️ Suggested consistency fix
let doc = maybe_doc.ok_or_else(|| { - Error::Proof(ProofError::CorruptedProof( - "shielded withdrawal was executed but withdrawal document is missing from proof".to_string(), + Error::Proof(ProofError::IncorrectProof(format!( + "proof did not contain withdrawal document {} expected to exist because of state transition (shielded withdrawal)", + document_id - )) + ))) })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs` around lines 1277 - 1281, The current check converts a missing withdrawal document into ProofError::CorruptedProof; change it to use ProofError::IncorrectProof for consistency with other verification paths: update the closure that returns Error::Proof(...) for the maybe_doc None case (the block creating the CorruptedProof with the message "shielded withdrawal was executed but withdrawal document is missing from proof") to instead construct ProofError::IncorrectProof with the same message so the maybe_doc existence check aligns with the other branches.
1305-1310: Use theRootTree::SpentAssetLockTransactionsenum constant instead of hardcoded72u8.Line 1308 hardcodes the magic number
72u8with only a comment reference. The rest of the codebase (e.g.,packages/rs-drive/src/drive/initialization/v0/mod.rsandpackages/rs-drive/src/prove/prove_state_transition/v0/mod.rs) consistently usesRootTree::SpentAssetLockTransactions as u8, which is more maintainable and self-documenting.♻️ Suggested improvement
+ use crate::drive::RootTree; + let mut query = grovedb::Query::new(); query.insert_key(outpoint_bytes.to_vec()); let path_query = grovedb::PathQuery::new( - vec![vec![72u8]], // RootTree::SpentAssetLockTransactions + vec![vec![RootTree::SpentAssetLockTransactions as u8]], grovedb::SizedQuery::new(query, Some(1), None), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs` around lines 1305 - 1310, Replace the hardcoded magic byte 72u8 in the PathQuery construction with the enum constant RootTree::SpentAssetLockTransactions as u8 to improve clarity and consistency; update the vec![vec![72u8]] used in grovedb::PathQuery::new (near the grovedb::Query and path_query variables) to use RootTree::SpentAssetLockTransactions as u8 so it matches other usages in the codebase.packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs (2)
19-20: Consider removing the unnecessary clippy allow.The return type
Vec<(Vec<u8>, Vec<u8>)>is not particularly complex and shouldn't triggerclippy::type_complexity. This attribute may be leftover from an earlier iteration.Suggested fix
/// Extract KV entries from merk proof bytes using the proper decoder. -#[allow(clippy::type_complexity)] fn extract_kv_entries_from_merk_proof(merk_proof: &[u8]) -> Result<Vec<(Vec<u8>, Vec<u8>)>, Error> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs` around lines 19 - 20, Remove the unnecessary clippy suppression above the function extract_kv_entries_from_merk_proof: delete the #[allow(clippy::type_complexity)] attribute so the function declaration begins directly with fn extract_kv_entries_from_merk_proof(merk_proof: &[u8]) -> Result<Vec<(Vec<u8>, Vec<u8>)>, Error> { and then run cargo build/tests to confirm no clippy warnings/errors remain.
174-183: Redundant error mapping after length validation.After confirming
key.len() == 16at line 168, thetry_into()calls for 8-byte slices cannot fail. This contrasts with lines 124-127 where.expect()is used for the same pattern.Consider using
.expect()for consistency, or keepmap_errif you prefer defensive coding—but be consistent throughout.Option 1: Use expect for consistency with lines 124-127
- let range_start = u64::from_be_bytes(key[0..8].try_into().map_err(|_| { - Error::Proof(ProofError::CorruptedProof( - "invalid key slice length for block height".to_string(), - )) - })?); - let range_end = u64::from_be_bytes(key[8..16].try_into().map_err(|_| { - Error::Proof(ProofError::CorruptedProof( - "invalid key slice length for block height".to_string(), - )) - })?); + let range_start = + u64::from_be_bytes(key[0..8].try_into().expect("len checked to be 16")); + let range_end = + u64::from_be_bytes(key[8..16].try_into().expect("len checked to be 16"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs` around lines 174 - 183, The redundant map_err on converting the 8-byte slices to u64 (for range_start and range_end) should be simplified to match the earlier pattern: since you already checked key.len() == 16, replace the try_into().map_err(...) pattern with try_into().expect("slice has correct length") (or an equivalent .expect message) for both range_start and range_end so the conversion is consistent with lines 124–127 and removes the impossible error mapping.packages/rs-drive/src/drive/shielded/nullifiers/store_nullifiers/v0/mod.rs (1)
143-145: Use safe conversion fornullifiers.len()toi64.Line 144 uses
nullifiers.len() as i64which performs an unchecked cast. While practically the length is unlikely to exceedi64::MAX, lines 66-70 already demonstrate the correct pattern withtry_from. For consistency and defensive coding:Suggested fix
- let new_sum = sum + nullifiers.len() as i64; + let new_nullifier_count = i64::try_from(nullifiers.len()).map_err(|_| { + Error::Drive(crate::error::drive::DriveError::CorruptedDriveState( + "nullifier count exceeds i64::MAX".to_string(), + )) + })?; + let new_sum = sum + new_nullifier_count;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/drive/shielded/nullifiers/store_nullifiers/v0/mod.rs` around lines 143 - 145, Replace the unchecked cast `nullifiers.len() as i64` with a safe conversion using `i64::try_from(nullifiers.len())` (or `TryInto::<i64>::try_into`) and handle the Result the same way other code does (e.g., propagate the error or map it to a descriptive error) when computing `new_sum`; update the assignment that sets `new_sum` (and any surrounding code that assumes the conversion cannot fail) to use the try_from result so the conversion is checked and consistent with the pattern used earlier.
🤖 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/src/drive/shielded/insert_note/v0/mod.rs`:
- Around line 14-26: The insert_note_op_v0 function currently accepts any
encrypted_note Vec<u8> but must enforce the documented 216-byte payload size;
add a guard at the start of insert_note_op_v0 that checks encrypted_note.len()
== 216 and returns an Err(Error::...) (use the crate's appropriate error variant
for invalid/malformed input) when the length differs, before computing
notes_path or emitting the GroveOperation
(QualifiedGroveDbOp::commitment_tree_insert_op), so only well-formed v0 notes
are persisted.
In
`@packages/rs-drive/src/drive/shielded/nullifiers/cleanup_expired_nullifier_compactions/v0/mod.rs`:
- Around line 27-35: The current cleanup builds an unbounded query (Query +
QueryItem::RangeToInclusive on current_block_time_ms) and calls
PathQuery::new(expiration_path.clone(), SizedQuery::new(query, None, None)),
then materializes all expired buckets and enqueues deletes in one giant batch;
change this to page the expiration query and cap how many ranges you process per
pass by using SizedQuery's size/offset (replace the None, None with a page size
and optional cursor/offset) or otherwise limit results to a fixed page_size,
iterate processing one page at a time (materialize, enqueue deletes,
commit/flush the batch), and loop until either no more expired items or you hit
a maximum per-run limit to bound memory and write size (use identifiers like
current_block_time_ms, expiration_path, Query, QueryItem::RangeToInclusive,
PathQuery::new, SizedQuery in your changes).
In
`@packages/rs-drive/src/drive/shielded/nullifiers/fetch_compacted_nullifiers/v0/mod.rs`:
- Around line 45-58: The probe that looks for a containing range (built via
Query::new_with_direction, desc_query and desc_path_query using PathQuery::new
and SizedQuery::new with Some(1)) must not return results when the caller
requested limit == Some(0); either short-circuit and return an empty
compacted_changes when limit == Some(0) before performing the descending probe,
or ensure the SizedQuery used for the probe uses the same limit (Some(0)) so no
entries are returned; apply the same guard to the later logic around the
range-return (the code path referred to at lines 107-115) so the API never emits
an entry when limit == Some(0).
- Around line 236-258: The code currently treats a non-16-byte probe key inside
the start_key computation (when iterating desc_results.to_key_elements()) as "no
containing range" and synthesizes a (start_block_height, start_block_height)
key; instead, detect malformed keys (key.len() != 16) and return an
Error::Protocol(ProtocolError::CorruptedSerialization(...)) so the probe fails
like the fetch path; specifically update the branch in the start_key
construction that checks key.len() to return the CorruptedSerialization error
(using the same error message style used when u64::from_be_bytes fails)
referencing desc_results.to_key_elements(), start_block_height, Error::Protocol
and ProtocolError::CorruptedSerialization.
In
`@packages/rs-drive/src/verify/shielded/verify_recent_nullifier_changes/v0/mod.rs`:
- Around line 25-27: The bincode config currently uses .with_no_limit() which
permits unbounded allocations and the decoder ignores trailing bytes; update the
decoder in the v0 verifier (where config is created) to use .with_limit::<N>()
with a sensible max N for the expected serialized payload and capture the
returned bytes_read when decoding serialized_data (e.g., in the proof
deserialization call), then explicitly verify bytes_read ==
serialized_data.len() and return an error if not; ensure you apply the same
change to the other occurrences noted (lines 64–70) so untrusted proof payloads
cannot cause over-allocation or accept trailing data.
In `@packages/rs-platform-version/src/version/drive_versions/mod.rs`:
- Line 94: The public field rename is a breaking change; restore the original
field name cleanup_expired_nullifiers on the public struct
DriveSavedBlockTransactionsMethodVersions and add the new
cleanup_expired_nullifier_compactions field alongside it to preserve
compatibility; update any constructors/impls (e.g., new() or Default/From
implementations that set these fields) to populate both fields consistently
(copy the value or call a single helper) and add a short doc comment indicating
the new name is preferred going forward so downstream consumers continue to
compile while you prepare a proper semver bump.
In `@packages/rs-platform-version/src/version/drive_versions/v1.rs`:
- Around line 107-110: DRIVE_VERSION_V1 (and other pre-v7 constants) incorrectly
advertise shielded write methods via DriveShieldedMethodVersions (insert_note,
insert_nullifiers, update_total_balance) despite create_initial_state_structure
being pre-shielded; remove or mark these fields as unavailable for
DRIVE_VERSION_V1 (and other pre-v7 constants) so they do not advertise support,
or alternatively add a guard in the dispatch path that checks DRIVE_VERSION_V7
before allowing insert_note/insert_nullifiers/update_total_balance; locate the
constants named DRIVE_VERSION_V1 and the struct DriveShieldedMethodVersions in
the diff and either set those shielded method entries to the unavailable state
used elsewhere for pre-shielded versions or gate their invocation behind a
DRIVE_VERSION_V7 check.
---
Duplicate comments:
In `@packages/rs-drive/src/fees/op.rs`:
- Around line 640-647: Compute sinsemilla_cost using a checked multiplication
instead of a raw multiply to avoid overflow: replace the direct multiplication
of fee_version.hashing.sinsemilla_base * (*sinsemilla_hash_calls as u64) with a
checked_mul on fee_version.hashing.sinsemilla_base (e.g.,
fee_version.hashing.sinsemilla_base.checked_mul(*sinsemilla_hash_calls as u64)),
then thread that Option into the existing checked-add chain (use and_then or map
to add it to c) so the whole sequence fails safely on overflow; update the
reference to sinsemilla_cost in the checked-add chain accordingly (look for
sinsemilla_cost, fee_version.hashing.sinsemilla_base, sinsemilla_hash_calls and
the existing checked_add chain).
In
`@packages/rs-drive/src/state_transition_action/action_convert_to_operations/system/penalize_shielded_pool.rs`:
- Around line 28-32: Replace the silent saturating subtraction on
v0.current_total_balance with a checked subtraction and return an explicit error
when the penalty exceeds the pool balance: use
v0.current_total_balance.checked_sub(v0.penalty_amount) and, if it returns None,
propagate/return a descriptive error (consistent with other balance ops like
unshield/shielded_transfer) instead of creating the UpdateTotalBalance
DriveOperation; update the code around the
ShieldedPoolOperation::UpdateTotalBalance construction in
penalize_shielded_pool.rs to perform this check and only push the operation when
the subtraction succeeds.
In
`@packages/rs-drive/src/util/grove_operations/batch_insert_auto_incremented_items_in_count_tree/v0/mod.rs`:
- Around line 35-46: The code currently uses element.count_value_or_default()
after grove_get_raw_optional, which returns 1 for non-CountTree elements and can
silently seed inserts incorrectly; update the logic in the
batch_insert_auto_incremented_items_in_count_tree v0 flow to explicitly match
the returned element variant (from self.grove_get_raw_optional) and extract the
count only when it is Element::CountTree, otherwise return an explicit error
(e.g., InvalidElementType or a descriptive error) instead of falling back to the
count_value_or_default; ensure the error is propagated through the surrounding
function so wrong paths fail fast and do not start inserting from key 1.
---
Nitpick comments:
In `@packages/rs-dpp/src/shielded/builder/unshield.rs`:
- Around line 60-69: The fee validation here only enforces the lower bound; add
the same upper-bound guard used in build_shielded_transfer_transition by
checking that a provided fee does not exceed 1000 * min_fee and returning a
ProtocolError::ShieldedBuildError (with a clear message) if it does; keep the
existing logic for None => min_fee and ensure you reference the same symbols
(fee, min_fee, effective_fee) and error variant when implementing the check so
behavior matches the sibling builder.
In
`@packages/rs-drive/src/drive/shielded/nullifiers/compact_nullifiers/v0/mod.rs`:
- Around line 149-157: The temporary vector drive_operations passed into
grove_get_raw_optional is created but never used later; either replace it with
an inline empty mutable vec (e.g., pass &mut vec![]) when calling
grove_get_raw_optional to avoid the unused variable, or keep drive_operations
and add a one-line comment near its declaration explaining that it intentionally
collects operations from grove_get_raw_optional for cost/accounting reasons even
though they are discarded; locate the call site using grove_get_raw_optional,
SubtreePath::from(expiration_path.as_ref()), expiration_key,
DirectQueryType::StatefulDirectQuery, transaction, and platform_version.drive to
make the change.
- Around line 69-76: Avoid cloning `key` when parsing the block height: replace
`key.clone().try_into()` with a slice conversion like `key[..8].try_into()` (or
`key.as_slice()[..8].try_into()`), validating the length first to produce the
same `ProtocolError` if too short, so you can still call
`keys_to_delete.push(key)` later without cloning; update the parsing around the
`block_height` assignment (in the loop over `key_elements`) to borrow the first
8 bytes instead of consuming a cloned Vec.
In `@packages/rs-drive/src/drive/shielded/nullifiers/store_nullifiers/v0/mod.rs`:
- Around line 143-145: Replace the unchecked cast `nullifiers.len() as i64` with
a safe conversion using `i64::try_from(nullifiers.len())` (or
`TryInto::<i64>::try_into`) and handle the Result the same way other code does
(e.g., propagate the error or map it to a descriptive error) when computing
`new_sum`; update the assignment that sets `new_sum` (and any surrounding code
that assumes the conversion cannot fail) to use the try_from result so the
conversion is checked and consistent with the pattern used earlier.
In
`@packages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rs`:
- Around line 19-20: Remove the unnecessary clippy suppression above the
function extract_kv_entries_from_merk_proof: delete the
#[allow(clippy::type_complexity)] attribute so the function declaration begins
directly with fn extract_kv_entries_from_merk_proof(merk_proof: &[u8]) ->
Result<Vec<(Vec<u8>, Vec<u8>)>, Error> { and then run cargo build/tests to
confirm no clippy warnings/errors remain.
- Around line 174-183: The redundant map_err on converting the 8-byte slices to
u64 (for range_start and range_end) should be simplified to match the earlier
pattern: since you already checked key.len() == 16, replace the
try_into().map_err(...) pattern with try_into().expect("slice has correct
length") (or an equivalent .expect message) for both range_start and range_end
so the conversion is consistent with lines 124–127 and removes the impossible
error mapping.
In
`@packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- Around line 1277-1281: The current check converts a missing withdrawal
document into ProofError::CorruptedProof; change it to use
ProofError::IncorrectProof for consistency with other verification paths: update
the closure that returns Error::Proof(...) for the maybe_doc None case (the
block creating the CorruptedProof with the message "shielded withdrawal was
executed but withdrawal document is missing from proof") to instead construct
ProofError::IncorrectProof with the same message so the maybe_doc existence
check aligns with the other branches.
- Around line 1305-1310: Replace the hardcoded magic byte 72u8 in the PathQuery
construction with the enum constant RootTree::SpentAssetLockTransactions as u8
to improve clarity and consistency; update the vec![vec![72u8]] used in
grovedb::PathQuery::new (near the grovedb::Query and path_query variables) to
use RootTree::SpentAssetLockTransactions as u8 so it matches other usages in the
codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2735a20b-242c-4d15-a46d-fdc74b411151
📒 Files selected for processing (47)
packages/rs-dpp/src/address_funds/platform_address.rspackages/rs-dpp/src/shielded/builder/mod.rspackages/rs-dpp/src/shielded/builder/shielded_transfer.rspackages/rs-dpp/src/shielded/builder/unshield.rspackages/rs-drive/src/drive/initialization/v3/mod.rspackages/rs-drive/src/drive/shielded/estimated_costs.rspackages/rs-drive/src/drive/shielded/insert_note/mod.rspackages/rs-drive/src/drive/shielded/insert_note/v0/mod.rspackages/rs-drive/src/drive/shielded/insert_nullifiers/mod.rspackages/rs-drive/src/drive/shielded/insert_nullifiers/v0/mod.rspackages/rs-drive/src/drive/shielded/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/cleanup_expired_nullifier_compactions/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/cleanup_expired_nullifier_compactions/v0/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/compact_nullifiers/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/compact_nullifiers/v0/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/fetch_compacted_nullifiers/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/fetch_compacted_nullifiers/v0/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/fetch_nullifiers/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/fetch_nullifiers/v0/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/queries.rspackages/rs-drive/src/drive/shielded/nullifiers/store_nullifiers/mod.rspackages/rs-drive/src/drive/shielded/nullifiers/store_nullifiers/v0/mod.rspackages/rs-drive/src/drive/shielded/update_total_balance/mod.rspackages/rs-drive/src/drive/shielded/update_total_balance/v0/mod.rspackages/rs-drive/src/fees/op.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/mod.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_transfer_transition.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_withdrawal_transition.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/unshield_transition.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/system/penalize_shielded_pool.rspackages/rs-drive/src/util/batch/drive_op_batch/shielded.rspackages/rs-drive/src/util/grove_operations/batch_insert_auto_incremented_items_in_count_tree/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_compacted_nullifier_changes/v0/mod.rspackages/rs-drive/src/verify/shielded/verify_recent_nullifier_changes/v0/mod.rspackages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_group_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/v2.rspackages/rs-platform-version/src/version/drive_versions/v3.rspackages/rs-platform-version/src/version/drive_versions/v4.rspackages/rs-platform-version/src/version/drive_versions/v5.rspackages/rs-platform-version/src/version/drive_versions/v6.rspackages/rs-platform-version/src/version/drive_versions/v7.rspackages/rs-platform-version/src/version/mocks/v2_test.rspackages/rs-platform-wallet-ffi/src/types.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_transfer_transition.rs
- packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/shielded_withdrawal_transition.rs
- packages/rs-drive/src/state_transition_action/action_convert_to_operations/shielded/mod.rs
Comment on lines
+27
to
+35
| let mut query = Query::new(); | ||
| // Range from 0 to current_block_time_ms (inclusive) | ||
| query.insert_item(QueryItem::RangeToInclusive( | ||
| ..=current_block_time_ms.to_be_bytes().to_vec(), | ||
| )); | ||
|
|
||
| let path_query = | ||
| PathQuery::new(expiration_path.clone(), SizedQuery::new(query, None, None)); | ||
|
|
Contributor
There was a problem hiding this comment.
Bound each cleanup pass instead of draining the full backlog in one transaction.
Line 44 materializes every expired bucket, and Lines 54-94 enqueue deletes for every referenced range into a single batch. After downtime or a long catch-up, one call can grow without bound in memory and write size, which is risky if this runs on the block-processing path. Please page the expiration query or cap the number of expired ranges processed per pass.
Also applies to: 44-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-drive/src/drive/shielded/nullifiers/cleanup_expired_nullifier_compactions/v0/mod.rs`
around lines 27 - 35, The current cleanup builds an unbounded query (Query +
QueryItem::RangeToInclusive on current_block_time_ms) and calls
PathQuery::new(expiration_path.clone(), SizedQuery::new(query, None, None)),
then materializes all expired buckets and enqueues deletes in one giant batch;
change this to page the expiration query and cap how many ranges you process per
pass by using SizedQuery's size/offset (replace the None, None with a page size
and optional cursor/offset) or otherwise limit results to a fixed page_size,
iterate processing one page at a time (materialize, enqueue deletes,
commit/flush the batch), and loop until either no more expired items or you hit
a maximum per-run limit to bound memory and write size (use identifiers like
current_block_time_ms, expiration_path, Query, QueryItem::RangeToInclusive,
PathQuery::new, SizedQuery in your changes).
packages/rs-drive/src/drive/shielded/nullifiers/fetch_compacted_nullifiers/v0/mod.rs
Show resolved
Hide resolved
packages/rs-drive/src/drive/shielded/nullifiers/fetch_compacted_nullifiers/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/verify/shielded/verify_recent_nullifier_changes/v0/mod.rs
Outdated
Show resolved
Hide resolved
…elded nullifiers - Replace with_no_limit() with with_limit() in verify_recent and verify_compacted nullifier verifiers (128 KiB for nullifier lists, 4 MiB for GroveDBProof) - Capture bytes_read and reject trailing bytes after decode - Short-circuit fetch_compacted_nullifiers when limit == Some(0) - Return CorruptedSerialization error on malformed probe key in prove_compacted_nullifier_changes instead of silently synthesizing a default key Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
shumkov
requested changes
Mar 9, 2026
| let path_query = | ||
| PathQuery::new(expiration_path.clone(), SizedQuery::new(query, None, None)); | ||
|
|
||
| let (results, _) = self.grove_get_path_query( |
Member
Author
There was a problem hiding this comment.
No limit is needed here — the data model naturally bounds the number of entries, and even under extreme load it's not a problem.
Math:
- Compaction triggers every 64 blocks or 2048 nullifiers (whichever comes first)
- Each compaction creates 1 expiration entry with a unique timestamp key
- Expiration is 1 week, so the query returns at most 1 week's worth of entries
With 3s blocks (201,600 blocks/week):
- Normal case (~1 shielded TPS): block threshold dominates →
201,600 / 64= 3,150 entries/week - Extreme case (nullifier threshold dominates, compaction every block): needs ≥2048 nullifiers/block =
2048 / 3s≈ 683 shielded TPS sustained for an entire week → 201,600 entries
Even at 683 shielded TPS sustained for a full week (which is far beyond any realistic scenario), you get ~200K entries — well within what GroveDB handles.
Added a test that creates 5,000 entries (exceeding the normal worst case) and verifies cleanup handles them fine in ~3.6s:
packages/rs-drive/src/drive/shielded/nullifiers/cleanup_expired_nullifier_compactions/v0/mod.rs — test_cleanup_handles_5000_expired_entries
Collaborator
There was a problem hiding this comment.
I think it will be much slower on machine what masternodes are running but I don't think it'll ever happend. Could you ask him what would be a cost for such event?
Member
Author
There was a problem hiding this comment.
you have to understand it's just for the cleanup... so platform would have need to stalled, and there be heavy load when it stalled, it's possible... but in this case... we can fix it, because platform would have stalled :)
Collaborator
There was a problem hiding this comment.
So it will increase chain halt duration in this case :). This is a risk, I just highlighted it. Actually, rabbit aslo has concern about it.
packages/rs-drive/src/drive/shielded/nullifiers/cleanup_expired_nullifier_compactions/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/drive/shielded/nullifiers/compact_nullifiers/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/drive/shielded/prove/prove_nullifiers_branch_query/mod.rs
Show resolved
Hide resolved
QuantumExplorer
commented
Mar 9, 2026
Member
Author
QuantumExplorer
left a comment
QuantumExplorer
left a comment
There was a problem hiding this comment.
Review comments
Comment on lines
+1448
to
+1453
| StateTransitionAction::ShieldAction(_) => {} | ||
| StateTransitionAction::ShieldedTransferAction(_) => {} | ||
| StateTransitionAction::UnshieldAction(_) => {} | ||
| StateTransitionAction::ShieldFromAssetLockAction(_) => {} | ||
| StateTransitionAction::ShieldedWithdrawalAction(_) => {} | ||
| StateTransitionAction::PenalizeShieldedPoolAction(_) => {} |
Member
Author
There was a problem hiding this comment.
this is just temporary
- Centralize bincode serialization into CompactedNullifiers and NullifierExpirationRanges wrapper structs in nullifiers/types.rs - Move prove functions under shielded/prove/ for consistent organization - Add test proving unbounded cleanup query handles 5000 entries safely - Document TPS analysis justifying unbounded query as code comment Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… changes Shielded pool subtrees under AddressBalances changed root-level Merk hashes, causing proof size increases (+1 byte) and succinctness check failures. Use verify_subset_query/verify_subset_of_proof where proofs contain extra lower layers for sibling subtrees. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
shumkov
requested changes
Mar 9, 2026
| let path_query = | ||
| PathQuery::new(expiration_path.clone(), SizedQuery::new(query, None, None)); | ||
|
|
||
| let (results, _) = self.grove_get_path_query( |
Collaborator
There was a problem hiding this comment.
I think it will be much slower on machine what masternodes are running but I don't think it'll ever happend. Could you ask him what would be a cost for such event?
packages/rs-drive/src/drive/shielded/nullifiers/fetch_compacted_nullifiers/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/drive/shielded/nullifiers/fetch_compacted_nullifiers/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/drive/shielded/nullifiers/fetch_nullifiers/v0/mod.rs
Outdated
Show resolved
Hide resolved
…ead code - Upgrade grovedb to a7b878f72279a4893a7e9dc0ab2973ca0b09ce69 - Add protocol version 11 proof size test - Fix shielded pool estimated costs: EstimatedLevel(7→3) for fixed 7-element tree, include CommitmentTree in non_sum_trees_weight - Use checked_sub instead of saturating_sub in penalize_shielded_pool to fail on balance underflow instead of silently clamping - Remove unused batch_insert_auto_incremented_items_in_count_tree - Remove unused transaction param from DriveOperationFinalizeTask::execute - Add doc comments to drive version v7 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
shumkov
requested changes
Mar 9, 2026
packages/rs-drive/src/drive/shielded/nullifiers/store_nullifiers/v0/mod.rs
Show resolved
Hide resolved
Replace raw tuple type aliases (CompactedNullifierChanges, NullifierChangesPerBlock, etc.) with proper structs (CompactedNullifierChange, NullifierChangePerBlock) that wrap CompactedNullifiers instead of Vec<[u8; 32]>. Centralizes the duplicated 16-byte key parsing logic into CompactedNullifierChange::parse_key() and uses CompactedNullifiers::decode() in verify modules instead of inline bincode deserialization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…#3217) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Updates grovedb to include dashpay/grovedb#639 which renames DontCheck to DontCheckWithNoCleanup, eliminating unnecessary find_subtrees + storage.clear() I/O costs for known-empty tree deletions during document transfers and deletions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n cleanup The test expected paid-out epoch proposers to still be readable, but this relied on the old grovedb behavior where DeleteTree silently failed to clean up child storage. Now that DeleteChildren properly removes the proposers tree contents, paid epochs correctly have 0 proposers. The proposer data is preserved in FinalizedEpochInfo in newer protocol versions (v6+), but the v0 fee distribution path used by this test does not persist it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # .github/grpc-queries-cache.json
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (18.26%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3198 +/- ##
=============================================
+ Coverage 30.67% 50.04% +19.37%
=============================================
Files 105 3110 +3005
Lines 9356 219919 +210563
=============================================
+ Hits 2870 110068 +107198
- Misses 6486 109851 +103365
🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
Code coverage will be in a future PR, this one is big enough. |
5 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Drive-layer implementation for Orchard shielded transactions (Medusa / ZK shielded pool). This is the
rs-driveportion extracted from the fullfeat/zkbranch for easier review.What was done?
Shielded pool GroveDB storage
State transition actions
ShieldTransitionAction— funds entering the pool from platform addressesShieldFromAssetLockTransitionAction— funds entering the pool from asset locksShieldedTransferTransitionAction— private transfers within the poolUnshieldTransitionAction— funds leaving the pool to a platform addressShieldedWithdrawalTransitionAction— funds leaving the pool to a core address (withdrawal)PenalizeShieldedPoolAction— system action for invalid proof penaltiesDrive operations
Proof generation & verification
Other
grove_get_proved_path_query_v1utility for newer proof formatStateTransitionActionenum and action-to-operations pipelineHow Has This Been Tested?
cargo check -p drive— compiles cleanly with no warningscargo check -p drive-abci— downstream compiles cleanly (no drive-abci changes in this PR)cargo clippy -p drive— no new lintsBreaking Changes
None. All changes are additive — new modules, new enum variants, new trait impls. Existing code paths are unchanged.
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements