Skip to content

rpc/store: align list-limit metadata and document query limits#1986

Open
namedfarouk wants to merge 6 commits into0xMiden:nextfrom
namedfarouk:issue-1080-list-limits
Open

rpc/store: align list-limit metadata and document query limits#1986
namedfarouk wants to merge 6 commits into0xMiden:nextfrom
namedfarouk:issue-1080-list-limits

Conversation

@namedfarouk
Copy link
Copy Markdown

@namedfarouk namedfarouk commented Apr 22, 2026

Summary

  • align SyncNullifiers list-limit validation in RPC and store with the parameter semantics (nullifier_prefix)
  • expose the same SyncNullifiers parameter key via GetLimits
  • extend GetLimits test coverage to assert SyncNullifiers and SyncNotes limits
  • expand internal RPC docs with a concrete query-limits table and rationale

Why

Issue #1080 asks to revisit/document list parameter limits.

This keeps limit handling consistent across:

  • runtime validation
  • GetLimits response metadata
  • tests
  • internal docs

Validation

  • cargo test -p miden-node-rpc get_limits_endpoint
  • cargo check -p miden-node-store -p miden-node-rpc

Partially addresses #1080

Copy link
Copy Markdown
Collaborator

@SantiagoPittella SantiagoPittella left a comment

Choose a reason for hiding this comment

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

We need to update the documentation in crates/utils/src/limiter.rs too.

Comment thread docs/internal/src/rpc.md Outdated
Comment on lines +32 to +46
| Endpoint | Parameter | Limit | Rationale |
|---|---:|---:|---|
| `CheckNullifiers` | `nullifier` | `1000` | Bounds `IN`-style lookups and keeps responses under payload budget |
| `SyncNullifiers` | `nullifier_prefix` | `1000` | Bounds prefix-based nullifier scans |
| `SyncNotes` | `note_tag` | `1000` | Keeps note sync responses within payload budget |
| `GetNotesById` | `note_id` | `100` | Notes can be large (~32 KiB), so this is intentionally tighter |
| `SyncTransactions` | `account_id` | `1000` | Bounds account filter fan-out and response size |
| `GetAccount` | `storage_map_key` | `64` | SMT proof generation for storage map keys is comparatively expensive |

Additional internal-only limits in `miden_node_utils::limiter` (not surfaced by `GetLimits`) include:

| Parameter | Limit | Used by |
|---:|---:|---|
| `note_commitment` | `1000` | Internal note proof lookups |
| `block_header` | `1000` | Internal batch/block header operations |
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.

nits:

  • We don't use right-alignment in ours tables, lets remove that.
  • convention in this repo is to pad table cells and use full-width --- separators so the markdown is readable in raw form too. You can take a look at docs/external/src/rpc.md:115 for comparison

@namedfarouk
Copy link
Copy Markdown
Author

namedfarouk commented Apr 23, 2026

Addressed, thanks for the review.

Updated in follow-up commit f475201:

crates/utils/src/limiter.rs: Endpoint docs now reflect current limit usage (sync_nullifiers for nullifier_prefix, check_nullifiers for nullifier).

docs/internal/src/rpc.md: Normalized table formatting to repo style (no right-alignment markers, padded cells, full-width separators).

Validation rerun: cargo test -p miden-node-rpc get_limits_endpoint (pass).

Comment thread docs/internal/src/rpc.md Outdated
Comment on lines +34 to +39
| `CheckNullifiers` | `nullifier` | `1000` | Bounds `IN`-style lookups and keeps responses under payload budget |
| `SyncNullifiers` | `nullifier_prefix` | `1000` | Bounds prefix-based nullifier scans |
| `SyncNotes` | `note_tag` | `1000` | Keeps note sync responses within payload budget |
| `GetNotesById` | `note_id` | `100` | Notes can be large (~32 KiB), so this is intentionally tighter |
| `SyncTransactions` | `account_id` | `1000` | Bounds account filter fan-out and response size |
| `GetAccount` | `storage_map_key` | `64` | SMT proof generation for storage map keys is comparatively expensive |
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.

Let use alphabetical order

Updated CHANGELOG with recent changes and breaking updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants