Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a configurable soft response-size cap for eth_simulateV1 by threading a max_output_size parameter from the Rust RPC layer into the C++ executor and enforcing it via a pre-construction size estimator, with accompanying test updates.
Changes:
- Add
max_output_sizeparameter to the Rusteth_simulate_v1API and pass it through the FFI boundary. - Extend
monad_executor_eth_simulate_submitC API and C++ executor plumbing to accept the limit and enforce it during trace construction. - Update/extend executor tests to supply the new argument and add a new output-size enforcement test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| rust/crates/monad-ethcall/src/lib.rs | Adds max_output_size to the Rust entrypoint and forwards it into the executor FFI call. |
| category/rpc/monad_executor.h | Extends the C API signature with a max_output_size argument. |
| category/rpc/monad_executor.cpp | Implements a soft max-size guardrail for eth_simulate output construction using a carried-size estimator. |
| category/rpc/monad_executor_test.cpp | Updates existing tests for the new parameter and adds a new output-size enforcement test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a636f55 to
1615ad5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
| uint8_t const *rlp_block_id, size_t rlp_block_id_len, | ||
| uint8_t const *rlp_grandparent_block_id, | ||
| size_t rlp_grandparent_block_id_len, uint64_t gas_limit, size_t max_calls, | ||
| size_t max_output_size, |
There was a problem hiding this comment.
[P3] The new max_output_size parameter lacks any header documentation, and its actual semantics are surprising: it bounds the in-memory JSON size estimate (with a 12.5% slack, see get_simulate_soft_max_size in the .cpp), not the CBOR-encoded response length that the C API returns to callers via encoded_trace_len. Since the JSON representation carries per-value/object type overhead absent from the CBOR encoding, the ratio between the two isn't 1:1, and a Rust caller sizing this against a target wire budget will pick the wrong value. Please add a comment on this declaration stating (a) what value it bounds and (b) that 0 rejects everything.
There was a problem hiding this comment.
Rust receives the actual length + data. It never tries to allocate a suitably sized container a priori. I will consider documenting the semantics later, once we have settled on the precise details.
1615ad5 to
7fe9095
Compare
This comment was marked as outdated.
This comment was marked as outdated.
7fe9095 to
a4ffb9a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
a4ffb9a to
3bd1858
Compare
This comment was marked as outdated.
This comment was marked as outdated.
5931ead to
6c51405
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
7767d23 to
cc7caf2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
cc7caf2 to
ec0f169
Compare
|
@claude review please. I've removed the extra |
There was a problem hiding this comment.
Re-reviewed the full diff at ec0f169, with focus on the calls size calculation and all previously raised threads.
The calls fix is correct. log_entry_size now counts the calls field as calls.size() + sizeof(array_t) + sizeof(object_t) * n_txs with no sizeof(value_t) term, which matches the convention used by every sibling array field (logs, topics, uncles, transactions, withdrawals): array wrappers carry no value_t, and each key/value pair carries exactly one value_t at the call site now that the value_size overloads no longer add it internally. The double-count flagged in the previous round is gone.
Previous findings — all verified fixed in the current tree:
value_sizeoverloads moved intoMONAD_ANONYMOUS_NAMESPACE(no exported symbols), and the redundantusing namespace monad;is gone.- The top-level entry object is now seeded with
sizeof(json::object_t)as the first term oflog_entry_size. withdrawalsRootis estimated viavalue_size(block.header.withdrawals_root.value_or(NULL_HASH)), matching the emitter's unconditional 66-char output.- East-const is now on the definition (
padded_max_size(size_t const max_size)) and off the declaration, as required. - Leftover
<tuple>/<array>includes and the unused header includes inresponse_size.hppare removed;#pragma onceis the first directive. - The
padded_max_sizecomment now matches the formula: withM = 25'000'000,slack_bps = 1 + (4999·4096)/25'004'096 = 1, so slack = 2500 bytes — I verified the arithmetic, including theceildecomposition and both overflow guards (max_size + kandmax_size + slacksaturate correctly, covering theSIZE_MAXlimit used in the test). soft_max_sizeis computed once inexecute_eth_simulateand threaded through;carried_sizeaccumulates across blocks against it.- The test's second submission now actually passes
actual_cbor_output_sizeas the limit, and the stray+ 768is gone;parallel_callsis down to 10.
Regression check: I cross-checked the estimator in response_size.cpp field-by-field against the emitter in save_eth_simulate_log_entry — all 19 header fields, the 3 per-call scalars, the 9 per-log fields plus per-topic accounting, the error branch (object_t + 2·value_t + "error" + "message" + "execution reverted"), and the 4 withdrawal fields all line up, including types (nonce is byte_string_fixed<8> → 18; logs_bloom is byte_string_fixed<256> → 514; the size field is upper-bounded by value_size(SIZE_MAX) = 18). The forward declarations in response_size.hpp are valid (all five types are real structs, not aliases). The FFI parameter insertion is mirrored in the header, the in-tree Rust wrapper (bindgen regenerates the extern declaration at build time), and there are no other in-tree callers; the companion monad-bft PR #3156 covers downstream. MONAD_ASSERT_THROW (not MONAD_ASSERT) is used on the user-facing paths, and no temporal-language or dead-identifier comment issues remain after the refactors.
No new findings. One reminder for merge: the branch is at 11 commits ("Address latest comments", "Remove unused import", etc.) — per the repo convention this should be squashed to a single commit and rebased on origin/main before merging. I could not run a local build in this environment (submodules uninitialized), so the compile/test gate rests on CI.
Verdict: CORRECT
🤖 Generated with Claude Code
This patch enforces a soft maximum size on the output of
eth_simulateV1inside the C++ executor.Companion BFT patch: category-labs/monad-bft#3156