feat: add memory limit for RPC value deserialization - #7440
feat: add memory limit for RPC value deserialization#7440francesco-stacks wants to merge 2 commits into
Conversation
Coverage Report for CI Build 30926002638Coverage decreased (-0.02%) to 86.598%Details
Uncovered Changes
Coverage Regressions163 previously-covered lines in 27 files lost coverage.
Coverage Stats
💛 - Coveralls |
c9301db to
a874438
Compare
a874438 to
478b5b2
Compare
benjamin-stacks
left a comment
benjamin-stacks
left a comment
There was a problem hiding this comment.
LGTM, couple nits. Didn't pick "approve" because you've enabled automerge, and I'd like you to consider some of my notes at least. Happy to approve as-is though.
| "TrackingAllocator is not installed as the global allocator; any miner or signer configured memory limits will never trigger" | ||
| ); | ||
| } | ||
| tracking_allocator_installed(); |
There was a problem hiding this comment.
Not sure I agree with this change. tracking_allocator_installed() only outputs a warning the first time it's called (which is hopefully in a sport where it prevents node startup with a panic).
I think it's appropriate for this to log an error every time this assumption is incorrectly relied on.
| read_only_call_limit: ExecutionCost, | ||
| read_only_max_execution_time: Duration, | ||
| read_only_call_max_mem_bytes: u64, | ||
| pub read_only_call_max_mem_bytes: u64, |
There was a problem hiding this comment.
Nit: I don't have much context on how CallReadOnly and FastCallReadOnly relate to each other, but this feels like a little bit of a smell. (Pre-existing smell, to be clear -- I know this didn't start here in this PR, and in fact you've improved the situation a little)
| @@ -0,0 +1,279 @@ | |||
| // Copyright (C) 2026 Stacks Open Internet Foundation | |||
There was a problem hiding this comment.
Nit: Can we move this file to a subfolder? I've always liked the fact that net/api/ basically has a 1:1 correspondence between files and endpoints (with mod.rs being the reasonable exception).
| use crate::chainstate::stacks::MAX_TRANSACTION_LEN; | ||
| use crate::net::http::Error; | ||
|
|
||
| /// Shortest Clarity parameter declaration, `(a int)`, plus a separating space. |
There was a problem hiding this comment.
Nit: Technically only a very limited number of parameters can be single characters (they type checker seems to let it slide if you have duplicates, but execution fails). So the true number would be less.
That said a) it doesn't matter in practice, and b), I think it would be totally okay to set the cap at a more reasonable upper limit. If somebody has really deployed a function with 200k parameters, they deserver their fate 😅 (at least for a non-consensus-critical API call).
| /// Parse a JSON body, charging its wire size up front and re-measuring | ||
| /// retention afterwards. | ||
| fn parse_json_body<T: DeserializeOwned>(body: &[u8], budget: &ParseBudget) -> Result<T, Error> { | ||
| budget.preflight(body.len() as u64)?; |
There was a problem hiding this comment.
If the purpose of this is to charge the caller for the memory usage of the serialized arguments in the request, and make up for the fact that that memory has already been allocated before taking the baseline measurement? If so, shouldn't this modify the baseline instead?
| tracker: MemoryTracker, | ||
| } | ||
|
|
||
| impl ParseBudget { |
There was a problem hiding this comment.
Some nits on this. Nothing to block this PR over, so feel free to go ahead with it, just take it as me saying "I might go refactor this" and speak out if you disagree 😁
First, this is conflating the terminology I deliberately chose in #7432: A budget only defines the upper bounds, while a limiter is responsible for measuring the baseline and enforcing the budget. In that terminology, ParseBudget is a limiter.
Second, one of the purposes of #7432 was to encapsulate all this in dedicated structures for a general "resource" concept, so we have one way of achieving this instead of the previous multitudes. I still think that architectural choice was correct, and I don't think opening it up again here achieves much that would make up for the disadvantage.
The preflight thing sounds like a good enhancement for the ResourceLimiter, and it looks like that's the only thing that makes the pub-lication of the MemoryTracker necessary (and maybe not even that, given that it's just a thin wrapper around Aaron's thread_allocated() API).
4 participants
Description
This extend our existing memory heap allocator guards to also cover clarity value deserialization in read-only RPC functions
Applicable issues
Additional info (benefits, drawbacks, caveats)
Checklist
docs/property-testing.md)changelog.d/README.md)rpc/openapi.yamlfor RPC endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepo