Skip to content

perf(l1): give the RLP encoder's scratch buffer an initial capacity - #7161

Open
diegokingston wants to merge 1 commit into
mainfrom
perf/rlp-encoder-scratch-capacity
Open

perf(l1): give the RLP encoder's scratch buffer an initial capacity#7161
diegokingston wants to merge 1 commit into
mainfrom
perf/rlp-encoder-scratch-capacity

Conversation

@diegokingston

Copy link
Copy Markdown
Contributor

Motivation

Encoder stages a struct's fields in temp_buf so the list header can be written once the payload length is known. That buffer starts empty, so every encode pays a chain of doubling reallocations: an RLP list of a few hundred bytes grows 8 → 16 → … → 512, which is seven allocations and six copies to encode one node.

The PERF note already in Encoder::new asks for exactly this.

Description

Start temp_buf at 512 bytes. That covers a trie node — the hashing path already sizes its own buffers at 512 as the maximum encoded node size — and the common transaction and receipt shapes, which is where the encoder is hottest. Larger payloads still grow from there; the point is to skip the small doublings, not to bound the size.

Encoding output is unchanged: capacity only affects how many times the buffer is reallocated on the way there.

Measurements

Taken on the LambdaVM zkVM guest, which runs ethrex's block execution as a RISC-V program, so every allocation is executed work rather than something a host allocator can hide:

block before after delta
empty 609,404 586,983 −3.68%
1 transfer 1,097,816 1,059,997 −3.44%
10 transfers 3,702,726 3,604,757 −2.65%

A guest profile attributes it: Encoder::encode_bytes, previously 7–10% of all allocator cycles, disappears from the profile entirely, and total allocator cycles fall 23%.

The capacity was picked by sweeping it — 64 gave −1.0%/−1.8%, 128 −1.7%/−2.7%, 512 −2.3%/−3.0%, 1024 −2.3%/−3.4%. Returns flatten after 512, which is also the bound the trie code already assumes.

Validation

cargo test -p ethrex-rlp and cargo test -p ethrex-common (171 tests, which exercise RLP round-trips) pass; cargo clippy clean.

`Encoder` stages a struct's fields in `temp_buf` so the list header can be
written once the payload length is known. That buffer started empty, so every
encode paid a chain of doubling reallocations: an RLP list of a few hundred
bytes grows 8 -> 16 -> ... -> 512, which is seven allocations and six copies to
encode one node.

Start it at 512 instead. That covers a trie node — the hashing path already
sizes its own buffers at 512 as the maximum encoded node size — and the common
transaction and receipt shapes, which is where the encoder is hottest. Larger
payloads still grow from there; the point is to skip the small doublings, not
to bound the size. The `PERF` note asking for exactly this is now addressed, so
it goes away.

Encoding is unchanged: capacity does not affect output, only how many times the
buffer is reallocated on the way there.

Measured on the LambdaVM zkVM guest, which runs ethrex's block execution as a
RISC-V program, so every allocation is executed work rather than something the
allocator can hide:

  block             before      after     delta
  empty            609,404    586,983    -3.68%
  1 transfer     1,097,816  1,059,997    -3.44%
  10 transfers   3,702,726  3,604,757    -2.65%

A guest profile attributes the win: `Encoder::encode_bytes`, which was 7-10% of
all allocator cycles, disappears from the profile entirely, and total allocator
cycles fall 23%.
@diegokingston
diegokingston requested a review from a team as a code owner August 19, 2026 18:05
@github-actions

Copy link
Copy Markdown

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

rpc-compat log-bearing cases excluded

Where: KNOWN_EXCLUDED_TESTS in .github/scripts/check-hive-results.sh counts out
eight hive rpc-compat cases — the four eth_getLogs cases, eth_getBlockReceipts/get-block-receipts-latest,
and three eth_getTransactionReceipt cases. They are exactly the cases whose recorded
response contains at least one log object; every case with an empty log array still runs.
Note this leaves eth_getLogs with no rpc-compat coverage at all, since all four of its
cases are in the set.

Why: ethrex populates blockTimestamp on log objects, as geth, besu, nethermind, reth
and erigon all do. hive's rpc-compat compares responses byte-exactly (jsondiff.FullMatch;
the lenient checkJSONStructure path applies only to cases upstream marks speconly), and
the corpus is pinned to execution-apis d08382ae (2025-02-10), whose recordings predate the
field — it entered the schema in execution-apis#639 and the fixtures in #846 (2026-07-22).
So the extra key cannot match, and this is a property of the pin rather than of the response.

The pin cannot move, and this is not temporary. The pin sits one commit before
execution-apis#627, which moved the test chain to a pre-merge genesis: the current corpus has
~36 proof-of-work blocks before its terminal total difficulty. ethrex does not support
pre-merge chains and will not, so importing that chain.rlp fails at block 1 —
validate_block_header has no pre-London base-fee path. Every revision carrying
blockTimestamp in its fixtures also carries that chain, so there is no revision that
satisfies both. Nor can the corpus be patched locally: rpc-compat's Dockerfile clones
ethereum/execution-apis by hard-coded URL, so the branch buildarg cannot point at a fork.

Coverage: the field itself is pinned by
block_timestamp_is_on_the_log_and_not_on_the_receipt in
crates/networking/rpc/types/receipt.rs, which asserts it is present on each log and absent
from the receipt level.

Removal: delete the entries if ethrex ever gains pre-merge chain import, or if upstream
marks these cases speconly so they are type-checked instead of compared byte-for-byte.


The stateless schema id does not identify the encoding

Where: STATELESS_INPUT_SCHEMA_ID in crates/common/types/stateless_ssz.rs.

Upstream keeps the stateless input schema id at 0x1501
(fork_index 0x15 << 8 | revision 0x01) across incompatible body changes. Three
encodings have now shipped under it: tests-zkevm@v0.6.2, then #3248 + #3278,
then #3356, which moved state, codes and public_keys from SszList to
ProgressiveList. ethrex speaks the last one.

The consequence is that the 2-byte prefix cannot be used to detect a stale or
mismatched bundle. A wrong-dialect input is accepted by the id check and then
fails later — in SSZ decode, or on a root that does not match — rather than being
rejected up front for what it is. only_amsterdam_schema_id_decodes therefore
proves less than its name suggests.

Worth raising upstream: a revision field that does not move across a body change
provides no version negotiation at all.


ZisK guest program hash changes with the unsync_cell gate

Where: crates/common/types/block.rs, transaction.rs.

The gate on the single-threaded unsync_cell::OnceCell moved from
all(feature = "eip-8025", target_arch = "riscv64") to
all(feature = "zisk", target_arch = "riscv64") when the eip-8025 feature was removed.

The guest ELFs were previously built --features "<zkvm>-build-elf,ci", which never enabled
eip-8025, so they compiled the atomic once_cell variant. bin/zisk/Cargo.toml does enable
ethrex-common/zisk, so the ZisK guest now compiles the unsafe impl Sync cell instead.
That changes the ELF bytes and therefore the program hash and verification key.

This is intended (the guest is single-threaded, so the unsync cell is sound and cheaper), but it
is a VK change rather than a no-op refactor, and the diffstat presents it as a file rename
(eip8025_cell.rsunsync_cell.rs). Anyone pinning a ZisK VK across this change must
re-register it. The stateless-validator crate now forwards ethrex-common/zisk from its own
zisk feature so the two ZisK guests do not disagree on the cell type.


Release signing key is an unprotected repository secret

Where: .github/workflows/tag_release.yaml.

MINISIGN_SECRET_KEY is a plain repository secret. There is no environment: on
finalize-release or dry-run-release-assets, and gh api repos/lambdaclass/ethrex/rulesets
shows only branch-targeted rulesets, so the github.ref_type == 'tag' condition is a workflow
check rather than an enforced boundary: anyone who can push a tag can reach the signing key.

This is a repository-settings change, not a code change, so it is recorded here rather than
fixed in the tree. Recommended:

  1. Move MINISIGN_SECRET_KEY / MINISIGN_PASSWORD into a GitHub Environment with required
    reviewers, and add environment: to the two jobs that sign.
  2. Add a ruleset targeting refs/tags/v* restricting who may create release tags.

Until then, the compromise of that key is silent and durable: signatures would still verify
against the committed .github/minisign.pub.

@github-actions github-actions Bot added L1 Ethereum client performance Block execution throughput and performance in general labels Aug 19, 2026
@github-actions

Copy link
Copy Markdown

🤖 Kimi Code Review

This is a well-crafted performance optimization. The change correctly addresses the reallocation overhead in RLP encoding without introducing any correctness or safety issues.

Highlights:

  • Correctness: Switching from Default::default() to Vec::with_capacity() is semantically equivalent (both create an empty vector), only differing in initial allocation. The encoding logic remains unchanged.
  • Documentation: The constant documentation (lines 184-196) is excellent—it explains the problem (reallocation chains), the specific value choice (512 bytes for trie nodes), and acknowledges that larger payloads gracefully degrade through standard vector growth.
  • Performance: Avoiding the 8→16→32→64→128→256→512 byte doubling chain for common Ethereum data structures (transactions, receipts, trie nodes) will reduce allocator pressure significantly in hot paths.
  • Maintenance: Removing the stale PERF comment (line 186-187 in the old code) appropriately resolves that TODO.

Minor suggestion:

Consider whether ENCODER_SCRATCH_CAPACITY should be pub if other modules tune their buffers to match this size (e.g., the comment mentions trie hashing already uses 512). However, keeping it private is correct for now—visibility can be expanded later if needed.

Verdict: LGTM. The 512-byte pre-allocation is a sensible trade-off given Ethereum's data structure sizes, and the comprehensive comment ensures future maintainers understand the rationale.


Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Codex Code Review

  1. crates/common/rlp/structs.rs:199-203 introduces an unconditional 512-byte heap allocation on every Encoder::new() via Vec::with_capacity(512). That is a behavioral change from the previous lazy allocation path and is likely a net performance regression for the many small RLP lists encoded across the codebase. Examples of tiny/frequent callers include crates/common/types/block_access_list.rs:64-69, crates/common/types/receipt.rs:29-35, and crates/networking/p2p/rlpx/eth/status.rs:76-86. In those paths, payloads are often far below 512 bytes, so this trades a few occasional growth reallocations for guaranteed allocator traffic and higher peak RSS on every encode. I’d keep the default lazy and add a targeted pre-sized constructor or reserve() only at the proven large/hot call sites (for example trie node hashing), or at minimum back this with benchmarks before applying it globally.

I did not find correctness, consensus, gas-accounting, or security issues in the diff itself. The concern here is that the optimization is too global for a utility used pervasively in small nested encodes.


Automated review by OpenAI Codex · gpt-5.4 · custom prompt

@github-actions

Copy link
Copy Markdown

🤖 Claude Code Review

Good — confirms the PR touches only crates/common/rlp/structs.rs.

Review: PR 7161 — RLP encoder scratch buffer initial capacity

Correctness

The change is behavior-preserving: temp_buf capacity has no effect on encoded output, only allocation count. Tests (ethrex-rlp, ethrex-common) and clippy are reported clean, and nothing here changes RLP semantics, gas accounting, or consensus logic. No correctness issues found.

Performance — the main point worth scrutinizing

Encoder::new is a shared, generic constructor used by every RLPEncode impl that goes through structs::Encoder — not just trie nodes, transactions, and receipts. A quick survey of call sites turns up several that encode payloads far smaller than 512 bytes:

  • Nibbles::encode (crates/common/trie/nibbles.rs:693)
  • ForkId::encode (crates/common/types/fork_id.rs:167) — 2 small fields
  • discv4 UDP messages — PingMessage, FindNodeMessage, ENRRequestMessage, etc. (crates/networking/p2p/discv4/messages.rs) — all well under 100 bytes
  • AccountInfo/AccountState (crates/common/types/account.rs:307-338) — ~100 bytes, encoded once per touched account per block, which is a much higher call frequency than one-per-trie-node

For these, the old Vec::default() paid nothing until the first push and grew only as far as actually needed (e.g., ~8–32 bytes for a ForkId or discv4 ping). This PR makes every single one of them eagerly commit a 512-byte heap allocation regardless of what they'll actually use — trading "several small reallocations" for "one allocation that's ~5-50x oversized" at these call sites.

The measurements in the PR description are all from block-execution workloads (empty/1-transfer/10-transfer blocks) in a zkVM guest, which is dominated by trie-node, transaction, and receipt encoding — exactly the cases 512 was tuned for. That's a solid result, but it doesn't tell us what this does to:

  1. Memory pressure on the P2P discovery/handshake hot path, where many tiny messages get encoded per second.
  2. Peak memory footprint in memory-constrained environments (the zkVM guest itself, or many concurrent trie-commit workers each staging their own oversized temp_buf), since bump-style allocators in a guest may not reclaim the wasted capacity as cheaply as a "cycles" metric alone would suggest.

This may still be a net win overall (a single allocation call might cost roughly the same regardless of size on the profiled allocator, which would explain the results), but that's an assumption the PR doesn't validate for the small-payload call sites. Worth either:

  • Confirming with a quick benchmark/profile on a P2P-heavy workload (sync, discovery) before merging, or
  • Scoping the larger capacity to just the call sites it was measured for (e.g., a with_capacity-style constructor used by trie/tx/receipt encoders, leaving the default Encoder::new untouched for everything else).

Documentation

The new doc comment on ENCODER_SCRATCH_CAPACITY (lines 184-194) is a good, honest explanation of the trade-off and correctly cites where the 512 bound comes from (the hashing path's max encoded node size). Nothing to fix there — just noting it doesn't mention the small-payload call sites discussed above, which a future reader might not think to check.

Minor

  • The removed PERF comment is appropriately cleaned up now that it's addressed.
  • No CHANGELOG.md entry — check if this repo's convention requires one for perf-labeled PRs (some ethrex PRs do, some skip it for internal-only changes).

Automated review by Claude (Anthropic) · sonnet · custom prompt

@github-actions

Copy link
Copy Markdown

Lines of code report

Total lines added: 1
Total lines removed: 0
Total lines changed: 1

Detailed view
+-------------------------------------+-------+------+
| File                                | Lines | Diff |
+-------------------------------------+-------+------+
| ethrex/crates/common/rlp/structs.rs | 129   | +1   |
+-------------------------------------+-------+------+

@diegokingston

Copy link
Copy Markdown
Contributor Author

Confirmed on a real block. Running the same guest against mainnet block 25368371 (45M cycles, where signature recovery is only 18.9% of the total rather than the ~65% it is on the synthetic transfer fixtures):

before after delta
mainnet 25368371 45,075,241 44,073,776 −2.22% (1,001,465 cycles)

Consistent with the −2.65% measured on the 10-transfer fixture, so the win holds at real block size and isn't an artifact of the small fixtures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client performance Block execution throughput and performance in general

Projects

Status: No status
Status: Todo

Development

Successfully merging this pull request may close these issues.

1 participant