Experimental/optimizations - #870
Conversation
|
/bench |
Benchmark — ethrex 20 transfers (median of 3)Table parallelism: auto (cores / 3)
Commit: 4b1e8c9 · Baseline: cached · Runner: self-hosted bench |
diegokingston
left a comment
There was a problem hiding this comment.
Nice work — the design is sound (the HINT table's rationale for being sound is well documented, executor and prover share compute_hint, the allocator story is well thought out, and the hint guests cover multi-row + read-back chaining). A set of concrete improvement proposals, roughly ordered:
1. Make the hint ABI big-endian (drop ~8 byte-reversal loops). Every hint call currently does 4 per-byte reversals: 2 guest-side (each of scalar_inv/decompress_r/field_inv converts BE→LE on input and LE→BE on output = 6 loops) + 2 host-side in compute_hint. k256 is BE-native (to_bytes/from_bytes), so a BE ABI deletes all of them: guests pass x.to_bytes() and read the result with from_bytes directly; the host stops reversing too. Saves ~1-2k cycles per ecrecover (3 hints) and simplifies the docs. Trade-off: diverges from the ECSM ecall's LE convention — worth it since the hint's consumers are all k256-native, and an LE variant can always be added later if needed. Touch points: syscalls::hint doc, compute_hint, the three consumers, and the test guests' expectations if byte order is asserted.
2. Missing address validation in the Hint dispatch. The handler does load_u256_le(in_addr) / store_u256_le(out_addr) with no alignment/overflow checks, unlike ECSM (ecsm_addr_ok) and Keccak (8-alignment + range). The HINT table sends four doublewords at out_addr +0/8/16/24; an unaligned or low-limb-overflowing out_addr breaks the table's assumptions. Suggest the same guard (out_addr % 8 == 0, (addr % 2^32) + max_offset < 2^32 for both addresses) plus a negative test guest — the current hint_min/hint_multi don't exercise rejection.
3. RecoveryHint vs hint-ecall: pick one. There's parallel unmerged work (preloaded hints via the private input, with a [len][rkyv][n×97B hints] fixture frame) that this PR's on-demand ecall supersedes. The on-demand design is strictly simpler (no fixture format change, no host precompute plumbing). Suggest documenting the decision in the PR and dropping the preloaded path + fixture tail + the fixture-tooling changes; also note the guest's rkyv::from_bytes will reject framed fixtures, so the two formats can't coexist.
4. Use ct_eq instead of to_bytes comparisons. field_inv ((*x * inv).to_bytes() == ONE.to_bytes()) and decompress_r (y2.to_bytes() != rhs.to_bytes()) each pay 2 full normalizations + a byte compare; scalar_inv already uses ct_eq. A few hundred cycles per call, free.
5. Stale import. crypto/ethrex-crypto/src/lib.rs:19 use ethrex_crypto::keccak::keccak_hash is unused on riscv64 builds (warning); cfg-gate or delete.
6. collect_hint_ops reads the hint input byte-by-byte (32 map reads) — switch to 4 × read_bytes(addr, 8) (symmetry with the write loop, fewer map ops).
Process/minor:
- "BENCH ONLY" markers are everywhere; if hint-then-verify is here to stay (the numbers say yes), decide before merging whether to keep them or document the pattern as permanent.
- Consider documenting an allocation convention for future hint ids (e.g. per-curve ranges) in the enum docs.
- The red "Run benchmarks for modified programs" check is a workflow script bug (
cmpmisuse + missinghead_programs/*.elfbecause the branch adds new guests without a base counterpart) — not a code problem; everything else is green.
Verified as good (no action): the table's mu gating/padding, not modeling the input read (the documented soundness argument holds), the shared compute_hint between executor and prover, the sqrt parity + from_encoded_point backstop, the tiny-keccak vendoring approach, and the dlmalloc-backed-by-bump design.
|
Benchmark Results for modified programs 🚀
|
|
/bench |
|
Follow-up on my earlier review — item 4 (use The rest of the review items are implemented on branch |
|
/bench |
|
/bench |
This branch bundles the guest-side performance work that was measured together on
ethrex blocks. It is not meant to be merged as one PR — it mixes four unrelated
changes with very different review surfaces, and it is being split into separate PRs
against main. It stays open as the reference for the numbers and as the base for the
branches that hang off it.
What is in it:
a value that is expensive to compute and cheap to check — modular inverse, square root
— and the guest verifies it in ordinary constrained instructions. The single call site
is ecrecover's inversions and square root. This is the largest single win here and the
one with an open decision: the table deliberately does not constrain the hinted value,
so soundness rests on the guest's verify rather than on the AIR.
change —
keccakfcalls thekeccak_permutesyscall — so ethrex's trie and RLPhashing reaches the accelerator instead of running the permutation in software.
allocator behind a feature. Superseded by Perf/dlmalloc guest allocator #869, which splits the same work better.
copying it into a Vec.
The last five commits are correctness fixes to the hint ecall: big-endian ABI, operand
address validation, and verifying hints by difference instead of comparing bytes.