chore: unify and revisit query limits#1443
Conversation
SantiagoPittella
force-pushed
the
santiagopittella-unify-limits
branch
from
00af4bc to
c1d22df
Compare
SantiagoPittella
force-pushed
the
santiagopittella-unify-limits
branch
from
c1d22df to
8d7b031
Compare
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left just a few small comments inline.
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
LGTM! As we talked offline, if this gets merged we should update the limits on the client, so we should open an issue or a PR directly since it would be a very simple change anyway.
Also, one question: if get_notes_by_id returns 1000 notes (the limit), could it not potentially be above the maximum current response payload size?
CHANGELOG.md
Outdated
| - [BREAKING] Added block signing capabilities to Validator component and updated gensis bootstrap to sign blocks with configured signer ([#1426](https://github.com/0xMiden/miden-node/pull/1426)). | ||
| - Reduced default block interval from `5s` to `2s` ([#1438](https://github.com/0xMiden/miden-node/pull/1438)). | ||
| - Increased retained account tree history from 33 to 100 blocks to account for the reduced block interval ([#1438](https://github.com/0xMiden/miden-node/pull/1438)). | ||
| - Increased the maximum query limit for the store ([#1443](https://github.com/0xMiden/miden-node/pull/1443)) |
There was a problem hiding this comment.
nit for consistency:
| - Increased the maximum query limit for the store ([#1443](https://github.com/0xMiden/miden-node/pull/1443)) | |
| - Increased the maximum query limit for the store ([#1443](https://github.com/0xMiden/miden-node/pull/1443)). |
crates/utils/src/limiter.rs
Outdated
| //! 1. the external facing RPC | ||
| //! 2. limiting SQL statements not exceeding parameter limits | ||
| //! # Rationale | ||
| //! - Parameter limits are kept at 1000 items across all multi-value RPC parameters. This caps |
There was a problem hiding this comment.
Is there a way to reference the 1000 const here? So that the doc comment does not accidentally drift if it gets updated
Yes! I'm planning to open a PR for that when this gets merged.
You could request for up to 1000 notes but if the limit is reached you will need to request the "next page" |
SantiagoPittella
force-pushed
the
santiagopittella-unify-limits
branch
from
ba4b31d to
d7d7320
Compare
| /// Maximum payload size (in bytes) for paginated responses returned by the | ||
| /// store. | ||
| pub const MAX_RESPONSE_PAYLOAD_BYTES: usize = 4 * 1024 * 1024; |
There was a problem hiding this comment.
I think the comment here needs to be updated as well.
A single note shouldn't end up being that big. Right now, a note may have up to 1024 input elements (8KB), and we'll probably need to impose script size limit on a note, and we'll also soon have "note attachments", which we'll also probably limit to 8KB or 16KB. Overall, maybe we impose a general limit on the size of a note to be something like 32KB (@PhilippGackstatter - let's create an issue for this if we don't have one yet). At this limit, we could allow up to 100 notes. So, let's reduce the limit on this endpoint to 100. |
|
Separately, it may be good to also include limits in the RPC endpoint docs. Let's create another issue for this (unless we already have one). |
3 participants
closes #1389
Centralized limits for the store and rpc queries.