fix: reduce notes limits to 100#1464
Conversation
SantiagoPittella
added
the
no changelog
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
LGTM. Something to keep in mind with the endpoint in terms of optimizing the bandwidth for the 4 MB limit is that the user could be querying for private notes, in which case the response size would be quite smaller since it would not contain note details (compared to the same amount of public notes), but I'm not sure we want to optimize it that much.
Co-authored-by: igamigo <ignacio.amigo@lambdaclass.com>
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left one small comment inline.
| /// The approximate maximum size for notes is around 32KB. | ||
| const LIMIT: usize = 100; |
There was a problem hiding this comment.
I would maybe move the comment into the struct doc comments, and maybe expand it - e.g., "The limit is set to 100 notes to make sure that response size does not exceed 4 MB as note sizes are limited to 32 KB".
It may be nice to add similar comments to other structs here to make size assumptions explicit - but that could be done in a follow-up PR.
There was a problem hiding this comment.
I did those changes in this PR
| /// Capped at 1000 prefixes to keep queries and responses comfortably within the 4 MB payload | ||
| /// budget and to avoid unbounded prefix scans. | ||
| pub struct QueryParamNullifierPrefixLimit; |
There was a problem hiding this comment.
It would be great to add the underlying assumptions to the comments explaining why 1000 is the right number. For example, 1000 prefixes could return a large number of nullifiers - but I think there is some pagination enforced where whatever is returned would still fit into 4 MB, right?
There was a problem hiding this comment.
That's true for some of those endpoints, but does not apply to all of them since some are not paginated. I can add the approximation that was made to determine the limits
There was a problem hiding this comment.
For not-paginated ones we should also try to ensure that the response size does not exceed 4 MB. I think it would good to have documentation for every endpoint explaining how this limit is satisfied. This may highlight that we are not satisfying this limit somewhere.
The only endpoints that don't need that are the ones that are used for testing only (though, not sure if we have such user-facing endpoints).
4 participants
This is a followup of #1443
In that PR the limits were generalized to 1000, but for notes it can exceed the maximum payload size.
Related to the docs:
There is a draft to get the limits through a RPC call, so it could be documented in that endpoint. Should I add it anyways here? cc @bobbinth