Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions packages/rs-drive-abci/src/query/shielded/encrypted_notes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ mod tests {
fn test_query_shielded_encrypted_notes_non_aligned_start_index_rejected() {
let (platform, state, version) = setup_platform(None, Network::Testnet, None);

// chunk size is `max_encrypted_notes_per_query` (2048 in v1). Anything
// that isn't a multiple of that should be rejected as unaligned.
// Alignment is to the MMR chunk size (`1 << SHIELDED_NOTES_CHUNK_POWER`
// = 2048). Anything that isn't a multiple of that is rejected.
let request = GetShieldedEncryptedNotesRequest {
version: Some(RequestVersion::V0(GetShieldedEncryptedNotesRequestV0 {
start_index: 1,
Expand Down Expand Up @@ -151,9 +151,10 @@ mod tests {

#[test]
fn test_query_shielded_encrypted_notes_count_zero_uses_max() {
// When count == 0, the limit falls back to `max_encrypted_notes_per_query`.
// This exercises the "count == 0 || count > max" branch of the limit
// derivation without needing notes to be stored.
// When count == 0, the limit falls back to the per-query max
// (`max_query_chunks × MMR_CHUNK_SIZE`). This exercises the
// "count == 0 || count > max" branch of the limit derivation
// without needing notes to be stored.
let (platform, state, version) = setup_platform(None, Network::Testnet, None);

let request = GetShieldedEncryptedNotesRequest {
Expand Down Expand Up @@ -188,11 +189,8 @@ mod tests {
#[test]
fn test_query_shielded_encrypted_notes_count_exceeds_max_clamped() {
let (platform, state, version) = setup_platform(None, Network::Testnet, None);
let max = version
.drive_abci
.query
.shielded_queries
.max_encrypted_notes_per_query as u32;
let max = version.drive_abci.query.shielded_queries.max_query_chunks as u32
* (1u32 << drive::drive::shielded::paths::SHIELDED_NOTES_CHUNK_POWER);

let request = GetShieldedEncryptedNotesRequest {
version: Some(RequestVersion::V0(GetShieldedEncryptedNotesRequestV0 {
Expand Down
58 changes: 35 additions & 23 deletions packages/rs-drive-abci/src/query/shielded/encrypted_notes/v0/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ use dpp::check_validation_result_with_data;
use dpp::validation::ValidationResult;
use dpp::version::PlatformVersion;
use drive::drive::shielded::paths::{
shielded_credit_pool_path, shielded_credit_pool_path_vec, SHIELDED_NOTES_KEY,
shielded_credit_pool_path, shielded_credit_pool_path_vec, SHIELDED_NOTES_CHUNK_POWER,
SHIELDED_NOTES_KEY,
};
use drive::grovedb::{PathQuery, Query, QueryItem, SizedQuery, SubqueryBranch};
use drive::grovedb_path::SubtreePath;
Expand All @@ -32,20 +33,31 @@ impl<C> Platform<C> {
platform_state: &PlatformState,
platform_version: &PlatformVersion,
) -> Result<QueryValidationResult<GetShieldedEncryptedNotesResponseV0>, Error> {
let max_notes = platform_version
// Two distinct quantities:
// * `mmr_chunk_size` — the on-chain MMR chunk size
// (`1 << SHIELDED_NOTES_CHUNK_POWER` = 2048 today). This is the
// alignment unit: `start_index` MUST be a multiple of this so
// every query begins at an MMR chunk boundary.
// * `max_query_chunks` — the per-query CAP, expressed in chunks.
// One query may span up to this many adjacent MMR chunks, so
// the wire-level note limit is `max_query_chunks × mmr_chunk_size`.
// Decoupling the cap from the chunk size is what lets us bump
// throughput without touching the on-chain tree shape.
let mmr_chunk_size: u64 = 1u64 << SHIELDED_NOTES_CHUNK_POWER;
let max_query_chunks = platform_version
.drive_abci
.query
.shielded_queries
.max_encrypted_notes_per_query as u32;
.max_query_chunks as u32;
// `saturating_mul` on u32 already caps at u32::MAX — no extra
// clamp needed.
let max_notes = max_query_chunks.saturating_mul(mmr_chunk_size as u32);

Comment on lines +47 to 55

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard max_query_chunks > 0 before deriving max_notes.

If max_query_chunks is ever 0 (e.g., misconfigured/defaulted platform version), limit becomes 0, and the prove path underflows at Line 74 (start_index + limit - 1). Add an explicit non-zero validation early.

Suggested fix
         let max_query_chunks = platform_version
             .drive_abci
             .query
             .shielded_queries
             .max_query_chunks as u32;
+        if max_query_chunks == 0 {
+            return Ok(QueryValidationResult::new_with_error(
+                QueryError::InvalidArgument(
+                    "platform version misconfigured: max_query_chunks must be greater than 0"
+                        .to_string(),
+                ),
+            ));
+        }
         let max_notes = max_query_chunks
             .saturating_mul(mmr_chunk_size as u32)
             .min(u32::MAX);
📝 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 max_query_chunks = platform_version
.drive_abci
.query
.shielded_queries
.max_encrypted_notes_per_query as u32;
.max_query_chunks as u32;
let max_notes = max_query_chunks
.saturating_mul(mmr_chunk_size as u32)
.min(u32::MAX);
let max_query_chunks = platform_version
.drive_abci
.query
.shielded_queries
.max_query_chunks as u32;
if max_query_chunks == 0 {
return Ok(QueryValidationResult::new_with_error(
QueryError::InvalidArgument(
"platform version misconfigured: max_query_chunks must be greater than 0"
.to_string(),
),
));
}
let max_notes = max_query_chunks
.saturating_mul(mmr_chunk_size as u32)
.min(u32::MAX);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-drive-abci/src/query/shielded/encrypted_notes/v0/mod.rs` around
lines 47 - 55, Guard against max_query_chunks being zero before computing
max_notes: after you compute max_query_chunks (variable max_query_chunks) add an
explicit check that it is > 0 and return a validation/error early (or set a safe
minimum) rather than continuing; this prevents limit/max_notes from becoming 0
and avoids the underflow in the prove path calculation (start_index + limit -
1). Update the logic around max_notes/limit calculation in mod.rs so the
function returns an error (or a clear Result::Err) when max_query_chunks == 0,
referencing the variables max_query_chunks, max_notes, and the downstream use of
limit/start_index + limit - 1 to locate the change.

// start_index must be chunk-aligned (multiple of max_notes) so each
// query touches exactly one MMR chunk or the buffer.
let chunk_size = max_notes as u64;
if start_index % chunk_size != 0 {
if start_index % mmr_chunk_size != 0 {
return Ok(QueryValidationResult::new_with_error(
QueryError::InvalidArgument(format!(
"start_index {} is not chunk-aligned; must be a multiple of {}",
start_index, chunk_size
start_index, mmr_chunk_size
)),
));
}
Expand Down Expand Up @@ -153,12 +165,16 @@ mod tests {
use crate::query::tests::setup_platform;
use dpp::dashcore::Network;

fn max_chunk_size(version: &PlatformVersion) -> u64 {
version
.drive_abci
.query
.shielded_queries
.max_encrypted_notes_per_query as u64
/// MMR chunk size used for alignment. Derived from
/// `SHIELDED_NOTES_CHUNK_POWER`; independent of `max_query_chunks`.
fn mmr_chunk_size() -> u64 {
1u64 << SHIELDED_NOTES_CHUNK_POWER
}

/// Per-query cap on returned notes: `max_query_chunks × mmr_chunk_size`.
fn max_notes(version: &PlatformVersion) -> u32 {
let chunks = version.drive_abci.query.shielded_queries.max_query_chunks as u32;
chunks.saturating_mul(mmr_chunk_size() as u32)
}

#[test]
Expand All @@ -168,7 +184,7 @@ mod tests {
// test never degrades into a vacuous check if the constant is later
// tuned to 1 or 5.
let (platform, state, version) = setup_platform(None, Network::Testnet, None);
let chunk = max_chunk_size(version);
let chunk = mmr_chunk_size();
assert!(
chunk > 1,
"test requires a chunk size > 1 so an unaligned start_index exists"
Expand All @@ -194,7 +210,7 @@ mod tests {
fn test_v0_non_aligned_large_start_index_errors() {
// An almost-aligned value (chunk_size + 1) must still be rejected.
let (platform, state, version) = setup_platform(None, Network::Testnet, None);
let chunk = max_chunk_size(version);
let chunk = mmr_chunk_size();

let request = GetShieldedEncryptedNotesRequestV0 {
start_index: chunk + 1,
Expand All @@ -217,7 +233,7 @@ mod tests {
// An aligned start_index equal to exactly chunk_size should succeed
// (fresh pool → empty result set).
let (platform, state, version) = setup_platform(None, Network::Testnet, None);
let chunk = max_chunk_size(version);
let chunk = mmr_chunk_size();

let request = GetShieldedEncryptedNotesRequestV0 {
start_index: chunk,
Expand All @@ -243,7 +259,7 @@ mod tests {
fn test_v0_aligned_start_at_multiple_of_chunk_size_ok() {
// start_index = 2 * chunk_size must also be accepted.
let (platform, state, version) = setup_platform(None, Network::Testnet, None);
let chunk = max_chunk_size(version);
let chunk = mmr_chunk_size();

let request = GetShieldedEncryptedNotesRequestV0 {
start_index: chunk * 2,
Expand Down Expand Up @@ -337,11 +353,7 @@ mod tests {
// count == max is neither `0` nor `> max`, so it falls through the
// inner `else` that keeps count as-is. Covers that fallthrough.
let (platform, state, version) = setup_platform(None, Network::Testnet, None);
let max = version
.drive_abci
.query
.shielded_queries
.max_encrypted_notes_per_query as u32;
let max = max_notes(version);

let request = GetShieldedEncryptedNotesRequestV0 {
start_index: 0,
Expand Down
3 changes: 2 additions & 1 deletion packages/rs-drive-proof-verifier/src/proof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2550,7 +2550,8 @@ impl FromProof<platform::GetShieldedEncryptedNotesRequest> for ShieldedEncrypted
.drive_abci
.query
.shielded_queries
.max_encrypted_notes_per_query as u32;
.max_query_chunks as u32
* (1u32 << drive::drive::shielded::paths::SHIELDED_NOTES_CHUNK_POWER);
Comment on lines 2550 to +2554

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Carried-forward (STILL VALID): verifier uses plain *; server uses saturating_mul — same invariant, two computations

After the latest delta the server computes max_notes with saturating_mul. The verifier still computes the same quantity with plain * and a non-saturating 1u32 << SHIELDED_NOTES_CHUNK_POWER. Today's constants (max_query_chunks ≤ 4, SHIELDED_NOTES_CHUNK_POWER = 11) cannot overflow either form, so this is purely a consistency / future-proofing point: a future bump of SHIELDED_NOTES_CHUNK_POWER past ~22 with a sizeable max_query_chunks would make the server clamp to u32::MAX and the verifier panic in debug / wrap in release — a verification mismatch from an arithmetic asymmetry. Mirror the server-side formula. Independent of the alignment-unit bug above; this is just about making both sides compute the same value the same way.

Suggested change
.drive_abci
.query
.shielded_queries
.max_encrypted_notes_per_query as u32;
.max_query_chunks as u32
* (1u32 << drive::drive::shielded::paths::SHIELDED_NOTES_CHUNK_POWER);
let max_elements = (platform_version
.drive_abci
.query
.shielded_queries
.max_query_chunks as u32)
.saturating_mul(1u32 << drive::drive::shielded::paths::SHIELDED_NOTES_CHUNK_POWER);

source: ['claude']


let (root_hash, notes) = Drive::verify_shielded_encrypted_notes(
&proof.grovedb_proof,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,21 @@ pub struct DriveAbciQueryShieldedVersions {
pub nullifiers_branch_state: FeatureVersionBounds,
pub recent_nullifier_changes: FeatureVersionBounds,
pub recent_compacted_nullifier_changes: FeatureVersionBounds,
/// Maximum number of encrypted notes returned per query.
/// Should match the BulkAppendTree buffer capacity (2^chunk_power).
pub max_encrypted_notes_per_query: u16,
/// Maximum number of MMR chunks a single `getShieldedEncryptedNotes`
/// query may span.
///
/// The wire-level cap on notes returned per query is therefore
/// `max_query_chunks × (1 << SHIELDED_NOTES_CHUNK_POWER)` — today
/// `chunk_power = 11` so each chunk holds 2048 notes. `start_index`
/// must still be chunk-aligned (2048-note boundary); this cap only
/// controls how many adjacent chunks one proof may cover.
///
/// Expressed in chunks (not raw notes) so the MMR-shape coupling
/// is explicit and the cap can be bumped independently of the
/// chunk size. v0 = 1 (legacy single-chunk-per-query behaviour);
/// v1 = 4 (8192-note responses, ~4× fewer round-trips on a cold
/// 1M-note sync).
pub max_query_chunks: u8,
}

#[derive(Clone, Debug, Default)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ pub const DRIVE_ABCI_QUERY_VERSIONS_V0: DriveAbciQueryVersions = DriveAbciQueryV
max_version: 0,
default_current_version: 0,
},
max_encrypted_notes_per_query: 2048,
max_query_chunks: 1,
},
address_funds_queries: DriveAbciQueryAddressFundsVersions {
addresses_infos: FeatureVersionBounds {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ pub const DRIVE_ABCI_QUERY_VERSIONS_V1: DriveAbciQueryVersions = DriveAbciQueryV
max_version: 0,
default_current_version: 0,
},
max_encrypted_notes_per_query: 2048,
max_query_chunks: 4,
},
address_funds_queries: DriveAbciQueryAddressFundsVersions {
addresses_infos: FeatureVersionBounds {
Expand Down
2 changes: 1 addition & 1 deletion packages/rs-platform-version/src/version/mocks/v2_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ pub const TEST_PLATFORM_V2: PlatformVersion = PlatformVersion {
max_version: 0,
default_current_version: 0,
},
max_encrypted_notes_per_query: 2048,
max_query_chunks: 1,
},
address_funds_queries: DriveAbciQueryAddressFundsVersions {
addresses_infos: FeatureVersionBounds {
Expand Down
10 changes: 9 additions & 1 deletion packages/rs-platform-wallet/src/wallet/shielded/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,15 @@ use super::store::{ShieldedStore, SubwalletId};
use crate::changeset::ShieldedChangeSet;
use crate::error::PlatformWalletError;

/// Server-enforced chunk size — start_index must be a multiple of this.
/// On-chain MMR chunk size for the shielded notes tree — must stay
/// in lock-step with `SHIELDED_NOTES_CHUNK_POWER` in
/// `rs-drive/src/drive/shielded/paths.rs` (`1 << 11 = 2048`).
/// `start_index` aligns to this regardless of how many chunks one
/// query spans; multi-chunk fetches still have to begin on a chunk
/// boundary because that's what the server-side range proof
/// bisects against. Hardcoded rather than imported because
/// `rs-platform-wallet` doesn't depend on `drive` directly — bump
/// here and there together if the chunk power ever changes.
const CHUNK_SIZE: u64 = 2048;

/// Result of one note-sync pass.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,21 +40,39 @@ pub async fn sync_shielded_notes(
) -> Result<ShieldedSyncResult, Error> {
let config = config.unwrap_or_default();

let chunk_size = sdk
// `mmr_chunk_size` is the on-chain MMR chunk size — the unit that
// `start_index` must align to. `fetch_size` is how many notes we
// pull per request; under `max_query_chunks=4` the server packs 4
// MMR chunks into one proof, so each request advances by 4× the
// MMR chunk size. Decoupling the two means the SDK can opportunistically
// request larger spans without touching the on-chain tree shape.
let mmr_chunk_size: u64 = 1u64 << drive::drive::shielded::paths::SHIELDED_NOTES_CHUNK_POWER;
let max_query_chunks = sdk
.version()
.drive_abci
.query
.shielded_queries
.max_encrypted_notes_per_query as u64;

// Validate alignment
if chunk_size > 0 && !start_index.is_multiple_of(chunk_size) {
.max_query_chunks as u64;
let fetch_size = mmr_chunk_size
.saturating_mul(max_query_chunks)
.max(mmr_chunk_size);

// Validate alignment against the MMR chunk size (NOT the multi-chunk
// fetch size). The server only requires per-MMR-chunk alignment; any
// multiple of `mmr_chunk_size` is a legal start. A `start_index`
// produced by a previous sync pass will be a multiple of `fetch_size`
// (and therefore of `mmr_chunk_size`) so this check is normally a
// no-op — but a hand-built resume point could land on a non-fetch
// boundary and still be valid.
if mmr_chunk_size > 0 && !start_index.is_multiple_of(mmr_chunk_size) {
return Err(Error::Generic(format!(
"start_index {} is not chunk-aligned; must be a multiple of {}",
start_index, chunk_size
start_index, mmr_chunk_size
)));
}

let chunk_size = fetch_size;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Carried-forward (STILL VALID): redundant let chunk_size = fetch_size rebinding

After the PR there are two legitimate 'chunk sizes' in this function — the MMR chunk size (alignment unit) and the per-request fetch window. The variable still named chunk_size is the fetch-window one, kept only so that downstream uses didn't need to be renamed. This is a magnet for future confusion (and is implicated in the is_partial / rewind findings, where the wrong 'chunk size' is being used). Either rename the downstream uses to fetch_size and drop the alias, or rename fetch_size to chunk_size and drop the alias — pick one term per quantity.

source: ['claude']


let max_concurrent = config.max_concurrent.max(1);
let settings = config.request_settings;

Expand Down
Loading