Skip to content

perf(felt): add CBOR fast path for encode and decode#3792

Open
ongyimeng wants to merge 7 commits into
mainfrom
perf/felt-decode
Open

perf(felt): add CBOR fast path for encode and decode#3792
ongyimeng wants to merge 7 commits into
mainfrom
perf/felt-decode

Conversation

@ongyimeng

@ongyimeng ongyimeng commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Felt.MarshalCBOR/UnmarshalCBOR used to go through a generic reflection-based CBOR codec.
A Felt is always 4 uint64 limbs, so this PR encodes/decodes that exact shape directly, and falls back to the generic codec for anything else.

Benchmark

Single Felt, no DB:

  before after Δ
Marshal ~100-110ns ~27-36ns -70%
Unmarshal ~175-195ns ~8-19ns -93%

Real mainnet blocks through pebble:
Benchmark before after Δ
StateUpdate p50 51.4µs 32.8µs -36.2%
StateUpdate p95 415.1µs 230.5µs -44.5%
StateUpdate p99 630.9µs 354.1µs -43.9%
Block p50 516.3µs 366.9µs -28.9%
Block p99 1.10ms 686.2µs -37.6%
Block p95 2.10ms 1.08ms -48.6%

n=8, p≤0.01 everywhere.

Allocation count unchanged (1 for Marshal, 0 for Unmarshal). Byte size unchanged except tiny Marshal inputs, which now allocate a fixed 48-byte buffer instead of one sized to the value.

Testing

  • Fuzz + 1B random values against the generic codec: 0 mismatches.
  • Replayed from block 0 to ~2.1M real mainnet blocks (state diffs, receipts, classes) against a synced DB copy: 0 mismatches.

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.30%. Comparing base (92a5693) to head (6cecada).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3792      +/-   ##
==========================================
+ Coverage   75.17%   75.30%   +0.12%     
==========================================
  Files         438      439       +1     
  Lines       39376    39397      +21     
==========================================
+ Hits        29602    29669      +67     
+ Misses       7699     7653      -46     
  Partials     2075     2075              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@RafaelGranza RafaelGranza force-pushed the perf/felt-decode branch 2 times, most recently from 08ce659 to 5c42e80 Compare July 7, 2026 19:28
@RafaelGranza RafaelGranza changed the title wip: add CBOR fast path decode perf(felt): add CBOR fast path for encode and decode Jul 7, 2026
@RafaelGranza RafaelGranza marked this pull request as ready for review July 7, 2026 20:42
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 2m 38s —— View job


Review: CBOR fast path for Felt

  • Gather context (diff, changed files)
  • Review core/felt/cbor.go (encode/decode fast path)
  • Review removed code in core/felt/felt.go
  • Review tests & benchmarks
  • Post summary

Verdict: clean. This is a well-scoped, correct optimization with unusually strong test coverage. I found no blockers or correctness issues, so I'm skipping inline comments and summarizing below.

What I verified

Byte compatibility with the old generic codec. The old path was cbor.Marshal((*fp.Element)(z)), i.e. a definite-length CBOR array (0x84) of 4 minimally-encoded uint64s. encodeFeltLimbs reproduces exactly that shape, and appendCBORUint emits canonical shortest-form ints at every boundary (<24, ≤u8, ≤u16, ≤u32, else u64). Old DB data was written by the generic encoder in canonical form, so it decodes fine on the fast path — consistent with the "replayed from block 0 → 0 mismatches" claim.

Decoder safety. decodeFeltLimbs is defensive in the right places:

  • Rejects wrong header (!= 0x84), so short/long arrays (0x80, 0x83, 0x85), indefinite-length (0x9f), and non-array types fall through to the generic codec.
  • headerByte > cborUint64AdditionalInfo (0x1b) rejects every non-uint major type (negatives 0x20+, floats 0xfa/0xfb, null) and the reserved size codes 0x1c–0x1f — all covered by decodeCornerCases.
  • Every multi-byte read is length-checked (byteOffset+N > len(data)) before indexing → no OOB panic on truncated input.
  • Trailing bytes rejected via the final byteOffset != len(data) check.

Crucially, on any ok=false the fast path leaves z untouched and defers to cbor.Unmarshal, so there's no partial-write corruption, and the only way errFast==nil while generic errors would be the fast path accepting something generic rejects — which the fuzz/stress equivalence harness is specifically built to catch.

Tests. cbor_fastpath_test.go is thorough: boundary values per limb width, the field modulus / modulus-1, corner-case malformed inputs, plus two fuzz targets and an opt-in 1B-value stress sweep — all asserting fast-vs-generic equivalence rather than hardcoded bytes. The feltFromLimbs helper (bypassing Montgomery conversion so small values actually land in a limb) is a nice touch. Benchmarks use b.Loop() + sinks correctly, and the accessor benchmarks exercise realistic p50/p95/p99 mainnet fixtures through pebble.

Nits (non-blocking, no action required)

  • core/felt/cbor.go:30 — buffer capacity is 1 + 4*9 = 37 bytes; the PR description's "fixed 48-byte buffer" is just Go's size-class rounding of that 37. Purely a description detail, not a code issue.
  • The three testdata/*.json mainnet fixtures are single-line and moderately large; realistic-benchmark value likely justifies the repo size, just flagging.

Nice work — the encoder is canonical-by-construction and the decoder degrades gracefully to the generic path on anything it doesn't recognize.
· perf/felt-decode

@RafaelGranza RafaelGranza force-pushed the perf/felt-decode branch 2 times, most recently from 450370d to 4897dbf Compare July 8, 2026 02:10
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 1m 29s —— View job


Review: CBOR fast path for Felt

  • Gather context (diff, changed files)
  • Review core/felt/cbor.go (encode/decode fast path)
  • Review tests & benchmarks
  • Post summary

Verdict: clean — no blockers, important issues, or nits worth acting on. This is a well-scoped, correct optimization on a genuine hot path (sync serialization), backed by unusually strong equivalence testing. I re-reviewed the full diff independently and confirm the earlier assessment.

What I verified

Encoder is canonical-by-construction. appendCBORUint (core/felt/cbor.go:44) emits shortest-form ints at every boundary (<24 inline → u8 → u16 → u32 → u64), reproducing exactly the definite-length 4-element array (0x84) that cbor.Marshal((*fp.Element)(z)) produced. encodeFeltLimbs allocates a fresh buffer per call — no shared state, so it's safe under concurrent sync goroutines.

Decoder degrades gracefully and can't corrupt on failure. decodeFeltLimbs writes into a local limbs and only assigns *element after full success, so any rejected input falls through to cbor.Unmarshal with z untouched — no partial-write corruption. It rejects wrong headers (!= 0x84), non-uint major types and reserved size codes (headerByte > 0x1b), and length-checks every multi-byte read before indexing (no OOB panic on truncated input). Trailing bytes are rejected via the final byteOffset != len(data) check — matching cbor.Unmarshal's default extraneous-data behavior.

Non-canonical acceptance is intentional and consistent. The decoder accepts non-shortest encodings (e.g. 5 as 0x18 0x05), which mirrors the lenient default fxamacker/cbor decode mode. Since the fast path only ever replaces the generic decoder when it succeeds and produces the identical value, this can't diverge — and the fuzz + 1B-value sweep against the generic codec as oracle is built precisely to catch any such divergence (0 mismatches).

Tests & benchmarks. cbor_fastpath_test.go asserts fast-vs-generic equivalence rather than hardcoded bytes: per-width limb boundaries, modulus / modulus−1, malformed corner cases, two fuzz targets, and an opt-in 1B stress sweep. The feltFromLimbs helper correctly bypasses Montgomery conversion so small values actually land in a limb. accessors_benchmark_test.go exercises realistic p50/p95/p99 mainnet fixtures through pebble with b.Loop() — no flaky time.Sleep, proper b.Cleanup for DB close.

Nice work — nothing to change.
· perf/felt-decode

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 1m 48s —— View job


Review: CBOR fast path for Felt

  • Gather context (diff, changed files)
  • Review core/felt/cbor.go (encode/decode fast path)
  • Review tests & benchmarks (incl. new mainnet-fixture accessor benchmark)
  • Post summary

Verdict: clean — no blockers, important issues, or actionable nits. I re-reviewed the full diff after the latest push (the accessor-benchmark fixture commit) and confirm the earlier assessments still hold. go build and go vet on core/felt/ pass. (Running the test suite is blocked by the sandbox in this CI environment, so I verified by inspection + build/vet.)

What I verified

Encoder is canonical-by-construction (core/felt/cbor.go:33). appendCBORUint emits shortest-form ints at every boundary (<24 inline → u8 → u16 → u32 → u64), reproducing exactly the definite-length 4-element array (0x84) that the old cbor.Marshal((*fp.Element)(z)) produced. cborArrayHeader4 = 0x80 | Limbs resolves to 0x84 since Limbs == fp.Limbs == 4. Each call allocates a fresh buffer — no shared state, safe under concurrent sync goroutines.

Decoder degrades gracefully and can't corrupt on failure (core/felt/cbor.go:68). Writes into a local limbs and only assigns *element on full success, so any rejected input falls through to cbor.Unmarshal with z untouched. It rejects wrong headers (!= 0x84), non-uint major types / reserved size codes (headerByte > 0x1b, covering 0x1c–0x1f and all negative/float/null majors), and length-checks every multi-byte read before indexing (no OOB panic on truncated input). The switch covers all valid additional-info values (<24, 24, 25, 26, default=27) with no gap. Trailing bytes are rejected via the final byteOffset != len(data) check, matching cbor.Unmarshal's default extraneous-data behavior.

Correctness of the fast/generic equivalence. The fast path only ever replaces the generic decoder when it succeeds; on any false it defers. Non-shortest encodings (e.g. 5 as 0x18 0x05) are still accepted, mirroring fxamacker/cbor's lenient default decode mode, so the two paths can't diverge on real DB data — consistent with the fuzz + 1B-value + block-0→2.1M-replay results (0 mismatches).

felt.go cleanup (-11) correctly removes the old reflection-based MarshalCBOR/UnmarshalCBOR and the now-unused fxamacker/cbor import; the new implementations live in cbor.go.

Tests & benchmarks. cbor_fastpath_test.go asserts fast-vs-generic equivalence rather than hardcoded bytes (per-width limb boundaries, modulus / modulus−1, malformed corner cases, two fuzz targets, opt-in 1B sweep). The new accessors_benchmark_test.go uses realistic p50/p95/p99 mainnet fixtures through pebble with b.Loop(), b.ReportAllocs(), and proper b.Cleanup for DB close — no flaky time.Sleep. felt_benchmark_test.go adds CBOR marshal/unmarshal benchmarks with correct sinks; the benchJSONInputsbenchFeltInputs rename is purely cosmetic.

Nice work — nothing to change.
· perf/felt-decode

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @thiagodeev's task in 1m 42s —— View job


Review: CBOR fast path for Felt

  • Gather context (diff, changed files)
  • Review core/felt/cbor.go (encode/decode fast path)
  • Review tests & benchmarks
  • Post summary

Verdict: clean — no blockers, important issues, or actionable nits. go build and go vet on core/felt/ and core/ pass. (The test suite itself is blocked by the sandbox in this CI environment, so I verified by inspection + build/vet.) This is a well-scoped, correct optimization on a real hot path (sync serialization), backed by unusually strong equivalence testing.

What I verified

Encoder is canonical-by-construction (core/felt/cbor.go:33). appendCBORUint emits shortest-form ints at every boundary (<24 inline → u8 → u16 → u32 → u64), reproducing exactly the definite-length 4-element array (0x84) that the old cbor.Marshal((*fp.Element)(z)) produced. cborArrayHeader4 = 0x80 | Limbs resolves to 0x84 since Limbs == 4. Each call allocates a fresh buffer (make([]byte, 1, 1+Limbs*maxCBORUintLen)) — no shared state, safe under concurrent sync goroutines.

Decoder degrades gracefully and can't corrupt on failure (core/felt/cbor.go:68). It writes into a local limbs and only assigns *element on full success, so any rejected input falls through to cbor.Unmarshal with z untouched — no partial-write corruption. It:

  • rejects wrong headers (!= 0x84) → short/long/indefinite arrays and non-array types defer to generic;
  • rejects non-uint major types and reserved size codes via headerByte > 0x1b (covers 0x1c–0x1f, all negative/float/null majors);
  • length-checks every multi-byte read before indexing → no OOB panic on truncated input;
  • rejects trailing bytes via the final byteOffset != len(data) check, matching cbor.Unmarshal's default extraneous-data behavior.

The switch covers all valid additional-info values with no gap (<24, 24, 25, 26, default 27).

Fast/generic equivalence is sound. The fast path only replaces the generic decoder when it succeeds and produces the identical value; on any false it defers. Non-shortest encodings (e.g. 5 as 0x18 0x05) are still accepted, mirroring fxamacker/cbor's lenient default decode mode, so the two paths can't diverge on real DB data — consistent with the fuzz + 1B-value + block-0→2.1M-replay results (0 mismatches). Since DB data is only ever written by the canonical encoder, the "fast accepts what generic rejects" case can't arise for stored data, and the fuzz harness guards against it regardless.

felt.go cleanup (-11) correctly removes the old reflection-based MarshalCBOR/UnmarshalCBOR and the now-unused fxamacker/cbor import; the new implementations live in cbor.go.

Tests & benchmarks. cbor_fastpath_test.go asserts fast-vs-generic equivalence rather than hardcoded bytes (per-width limb boundaries, modulus / modulus−1, malformed corner cases, two fuzz targets, opt-in 1B sweep). feltFromLimbs correctly bypasses Montgomery conversion so small values actually land in a limb. accessors_benchmark_test.go uses realistic p50/p95/p99 mainnet fixtures through pebble with b.Loop(), b.ReportAllocs(), and proper b.Cleanup for DB close — no flaky time.Sleep.

Nice work — nothing to change.
· perf/felt-decode

@thiagodeev thiagodeev self-requested a review July 9, 2026 21:11

@thiagodeev thiagodeev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!
Partial review with some probable improvements

Comment thread core/felt/cbor.go Outdated
Comment thread core/felt/cbor.go Outdated
Comment thread core/felt/cbor.go Outdated
@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 1m 38s —— View job


Review: CBOR fast path for Felt

  • Gather context (diff, changed files)
  • Review core/felt/cbor.go (encode/decode fast path, post-review changes)
  • Review tests & benchmarks
  • Post summary

Verdict: clean — no blockers, important issues, or actionable nits. go build ./core/... and go vet ./core/felt/ pass. (The test suite itself is blocked by the sandbox in this CI environment, so I verified by inspection + build/vet.) I re-reviewed the full diff after the latest push, which reorders the encoder switch per @thiagodeev's hot-path suggestion.

What I verified in the latest change

The reordered encoder switch is still canonical-by-construction (core/felt/cbor.go:44). Cases now run widest-first (> MaxUint32 → u64, > MaxUint16 → u32, > MaxUint8 → u16, >= 24 → u8, else inline) so real (non-zero, Montgomery-form) felts hit the u64 branch immediately. The ranges are mutually exclusive, so reordering doesn't change which branch matches — output is byte-identical to the prior order and to the old cbor.Marshal((*fp.Element)(z)). The benchmark deltas you posted (Marshal/nonzero -27%, at the cost of a slower zero case that's ~never hit in practice) bear this out. cborArrayHeader4 = 0x80 | Limbs resolves to 0x84 since Limbs == 4.

Decoder is unchanged and remains correct/defensive (core/felt/cbor.go:68). It writes into a local limbs and only assigns *element on full success, so any rejected input defers to cbor.Unmarshal with z untouched — no partial-write corruption. It rejects wrong headers (!= 0x84), non-uint majors / reserved size codes (headerByte > 0x1b, covering 0x1c–0x1f and all negative/float/null majors), length-checks every multi-byte read before indexing (no OOB on truncated input), and rejects trailing bytes via the final byteOffset != len(data) check. The equality-based switch means decode order is irrelevant to perf, so leaving it as-is (vs. also reordering) is fine.

Fast/generic equivalence holds. The fast path only replaces the generic decoder when it succeeds and produces the identical value; on any false it defers. Non-shortest encodings are still accepted, mirroring fxamacker/cbor's lenient default — consistent with the fuzz + 1B-value + block-0→2.1M-replay results (0 mismatches).

On the reviewer suggestions (already resolved in-thread)

  • Buffer pool (cbor.go:34): correctly declined — the output size is known up front, so a sync.Pool scratch buffer only adds indirection; @RafaelGranza's benchmark confirms it's +22–32% slower with identical alloc count.
  • append()copy()/PutUint*: correctly declined — measured 8–20% slower; append with a small constant arg count is a pattern the Go compiler optimizes well.

Tests & benchmarks

cbor_fastpath_test.go asserts fast-vs-generic equivalence rather than hardcoded bytes (per-width limb boundaries, modulus / modulus−1, malformed corner cases, two fuzz targets, opt-in 1B sweep). feltFromLimbs correctly bypasses Montgomery conversion so small values land in a limb. accessors_benchmark_test.go uses realistic p50/p95/p99 mainnet fixtures through pebble with b.Loop(), b.ReportAllocs(), and b.Cleanup for DB close — no flaky time.Sleep.

Nice work — nothing to change.
· perf/felt-decode

@thiagodeev thiagodeev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Only a few comments with nitpicks

Comment thread core/felt/cbor.go Outdated
Comment thread core/felt/cbor_fastpath_test.go Outdated
Comment thread core/felt/cbor_fastpath_test.go Outdated
Comment thread core/felt/cbor_fastpath_test.go Outdated
Comment thread core/felt/cbor.go Outdated
Comment thread core/felt/cbor.go Outdated
Comment thread core/felt/cbor.go
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 2m 18s —— View job


Review: CBOR fast path for Felt

  • Gather context (diff vs origin/main, changed files)
  • Review core/felt/cbor.go (encode/decode fast path)
  • Review core/felt/felt.go cleanup
  • Review tests & benchmarks
  • go build ./core/... + go vet ./core/felt/
  • Post summary

Verdict: clean — no blockers, no important issues, no actionable nits. go build ./core/... and go vet ./core/felt/ pass. I re-reviewed the full diff independently against origin/main and confirm the earlier assessments hold. Skipping inline comments per the clean result.

What I verified

Encoder is canonical-by-construction (core/felt/cbor.go:57). appendCBORUint emits shortest-form ints at every boundary, now ordered widest-first (> MaxUint32 → u64, > MaxUint16 → u32, > MaxUint8 → u16, >= 24 → u8, else inline). The ranges are mutually exclusive, so the reordering is byte-identical to the old cbor.Marshal((*fp.Element)(z)) output while letting real (non-zero, Montgomery-form) felts hit the u64 branch first. cborArrayHeader4 = (1<<7) | Limbs resolves to 0x84 since Limbs == fp.Limbs == 4, and the comment documents why. Each call allocates a fresh buffer (cap 1 + 4*9) — no shared state, safe under concurrent sync goroutines.

Decoder degrades gracefully and can't corrupt on failure (core/felt/cbor.go:81). It decodes into a local limbs and only assigns *element after full success, so any rejected input defers to cbor.Unmarshal with z untouched. It rejects wrong headers (!= 0x84), non-uint majors / reserved size codes (headerByte > 0x1b, covering 0x1c–0x1f and all negative/float/null majors), length-checks every multi-byte read before indexing (no OOB on truncated input), and rejects trailing bytes via the final byteOffset != len(data) check — matching cbor.Unmarshal's default extraneous-data behavior. The switch covers all valid additional-info values with no gap.

Fast/generic equivalence is sound. The fast path only replaces the generic decoder when it succeeds and produces the identical value; on any false it defers. Since stored DB data is only ever written by the canonical encoder, the "fast accepts what generic rejects" divergence can't arise for real data, and the fuzz harness guards it regardless — consistent with the fuzz + 1B-value + block-0→2.1M-replay results (0 mismatches).

felt.go cleanup (-11) correctly removes the old reflection-based MarshalCBOR/UnmarshalCBOR; the new implementations live in cbor.go.

Tests & benchmarks. cbor_fastpath_test.go asserts fast-vs-generic equivalence rather than hardcoded bytes (per-width limb boundaries, modulus / modulus−1, malformed corner cases, two fuzz targets, opt-in 1B sweep), and uses require.NoError/require.Equal/require.True throughout. feltFromLimbs correctly bypasses Montgomery conversion so small values land in a limb. accessors_benchmark_test.go exercises realistic p50/p95/p99 mainnet fixtures through pebble with b.Loop(), b.ReportAllocs(), and b.Cleanup for DB close — no flaky time.Sleep.

The remaining review-thread items (cborArrayHeader4 readability, dropping single-use test helpers, extra doc comments) are cosmetic nits already being handled in-thread — none affect correctness.

Nice work — nothing blocking from my side.
· perf/felt-decode

Comment thread core/felt/cbor.go Outdated
Comment thread core/felt/cbor.go Outdated
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 1m 41s —— View job


Review: CBOR fast path for Felt

  • Gather context (diff vs origin/main, changed files)
  • Review core/felt/cbor.go (latest: inlined limb encoding)
  • Review tests & benchmarks
  • go build ./core/... + go vet ./core/felt/
  • Post summary

Verdict: clean — no blockers, important issues, or actionable nits. go build ./core/... and go vet ./core/felt/ pass. (The test suite itself is blocked by the sandbox in this CI environment, so I verified by inspection + build/vet.) I re-reviewed the full diff after the latest push (perf(felt): inline CBOR limb encoding), which applies @rodrodros's hot-path suggestion.

What I verified in the latest change

The append → indexed-write refactor is byte-identical and correct (core/felt/cbor.go:45). encodeFeltLimbs now allocates a fixed-size zero buffer (make([]byte, 1+Limbs*maxCBORUintLen) = 37 B), writes cborArrayHeader4 at [0], and advances an index cursor per limb — with the width switch inlined into the loop rather than a separate putCBORUint call. Two things make this the right call and safe:

  • Inlining is what actually recovered the win. As documented in @RafaelGranza's reinvestigation, a non-inlined put helper was slower than append (+9–12%); inlining the switch flips it to −7% geomean. Good instinct to check the assembly rather than assume the bounds-check story.
  • No OOB risk. The buffer is sized for the worst case (every limb full-width, 1 + 4*9), and each branch writes at most 1 + 8 bytes from index, so binary.BigEndian.PutUint64(buffer[index+1:], …) always has room. Cursor arithmetic (index += 1 + N) matches each branch's byte count exactly, and buffer[:index] trims to the true length. Widest-first case order (> MaxUint32 → u64 first) keeps real Montgomery-form felts on the fast branch.

Decoder is unchanged and remains correct/defensive (core/felt/cbor.go:85). Decodes into a local limbs, assigns *element only on full success (no partial-write corruption on fallback), rejects wrong headers (!= 0x84), rejects non-uint majors / reserved size codes (headerByte > 0x1b, covering 0x1c–0x1f and all negative/float/null majors), length-checks every multi-byte read before indexing, and rejects trailing bytes via the final byteOffset != len(data) check. The switch covers all valid additional-info values with no gap.

Fast/generic equivalence still holds. The output is byte-identical to the prior append-based encoder (and to the original cbor.Marshal((*fp.Element)(z))), so the fuzz + 1B-value + block-0→2.1M-replay evidence (0 mismatches) carries over. The fast path only replaces the generic path on success; on any false it defers with z untouched.

Review-thread items — all resolved

  • Reorder switch widest-first (@thiagodeev) — applied, benchmarked (+net win on the common nonzero case).
  • Buffer pool — correctly declined (output size known up front; measured +22–32% slower, identical allocs). @thiagodeev agreed in-thread once he saw the buffer is returned directly.
  • appendcopy — withdrawn by @thiagodeev (only one slice + a value, so copy doesn't apply).
  • append → indexed Put (@rodrodros) — applied via inlining, with data.
  • Docs / cborArrayHeader4 readability / drop single-use test helpers — addressed (the header const is now documented, tests use require.NoError/require.Equal/require.True).

Tests & benchmarks

cbor_fastpath_test.go asserts fast-vs-generic equivalence rather than hardcoded bytes: per-width limb boundaries, modulus / modulus−1, malformed corner cases (decodeCornerCases), two fuzz targets, and an opt-in 1B sweep. feltFromLimbs correctly bypasses Montgomery conversion so small values land in a limb. accessors_benchmark_test.go exercises realistic p50/p95/p99 mainnet fixtures through pebble with b.Loop(), b.ReportAllocs(), and b.Cleanup for DB close — no flaky time.Sleep.

Nice work — nothing blocking from my side.
· perf/felt-decode

@rodrodros

Copy link
Copy Markdown
Contributor

@RafaelGranza, initially the PR only added Felt decoding because it showed a clear improvement on the benchmarks. Can you please add benchmarks showing Felt encoding benefits?

Comment thread core/felt/cbor.go Outdated
Comment thread core/felt/cbor_fastpath_test.go
Comment thread core/felt/cbor.go Outdated

@rodrodros rodrodros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @RafaelGranza's task in 2m 53s —— View job


Task: add encode benchmarks showing Felt CBOR fast-path benefit

  • Review existing CBOR benchmarks
  • Add generic-vs-fast baseline benchmarks so the encode win is measurable
  • go vet ./core/felt/ (compiles the new benchmark code) — passes
  • Commit locally
  • ⚠️ Push failed — github-actions[bot] lacks write permission (HTTP 403)

@rodrodros — the existing BenchmarkMarshalCBOR/BenchmarkUnmarshalCBOR only exercise the fast path, so on their own they don't show a benefit — there's no baseline to compare against. I added the missing generic baselines.

The trick (thanks to your catch on cbor_fastpath_test.go:61): benchmarking felt.Felt dispatches to the new fast method, so the baseline has to encode/decode the underlying *fp.Element to route through the reflection-based generic codec — exactly what the old code did.

Added to core/felt/felt_benchmark_test.go:

// BenchmarkMarshalCBORGeneric is the pre-fast-path baseline: it routes through
// the reflection-based generic codec by encoding the underlying *fp.Element
// (Felt would dispatch to the fast MarshalCBOR). Compare against
// BenchmarkMarshalCBOR to see the encode speedup.
func BenchmarkMarshalCBORGeneric(b *testing.B) {
	for _, tc := range benchCBORInputs {
		value := felt.UnsafeFromString[felt.Felt](tc.hex)
		element := (*fp.Element)(&value)
		b.Run(tc.name, func(b *testing.B) {
			b.ReportAllocs()
			var out []byte
			for b.Loop() {
				out, _ = cbor.Marshal(element)
			}
			benchBytesSink = out
		})
	}
}

plus the symmetric BenchmarkUnmarshalCBORGeneric (decodes into (*fp.Element)(&out)), and the fp/cbor imports.

Now the encode benefit is a direct A/B — e.g.:

go test ./core/felt/ -run '^$' -bench 'MarshalCBOR' -benchmem -count 10 | benchstat ...

BenchmarkMarshalCBOR/nonzero (fast) vs BenchmarkMarshalCBORGeneric/nonzero (baseline) gives the ~70% encode figure from the PR description directly, rather than only citing external microbenchmarks.

Note: I couldn't run the benchmarks here — this CI sandbox blocks go test/go test -bench (only go build/go vet are permitted), the same limitation earlier review runs hit. go vet ./core/felt/ (which compiles the test files) passes, so the code is valid; please run the sweep locally for the actual numbers.

⚠️ Push blocked: I committed this locally (test(felt): add generic CBOR baseline benchmarks for encode/decode) but the push was denied — Permission to NethermindEth/juno.git denied to github-actions[bot] (HTTP 403). The diff is above if you'd like to apply it directly, or re-trigger once the bot has write access to this branch.
· perf/felt-decode

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants