Repository navigation
feat(felt): size-based compact CBOR codec on Typed[K] - #4230
Conversation
|
Claude finished @EgeCaner's task in 14m 54s —— View job Reviewing PR #4230
SummaryThis PR is clean — I didn't find any correctness, concurrency, or security issues worth flagging. Specifics I checked closely given the risk profile of a wire-format change like this:
One non-blocking observation (not filed inline since it's not a bug): |
Title(Describe updated until commit 94363b8)feat(felt): size-based compact CBOR codec on Typed[K] PR TypeEnhancement, Tests Description
|
| Relevant files | |||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Tests | 23 files
| ||||||||||||||||||||||||||||||||||||||||||||||
| Enhancement | 11 files
| ||||||||||||||||||||||||||||||||||||||||||||||
| Miscellaneous | |||||||||||||||||||||||||||||||||||||||||||||||
| Refactoring | 8 files
| ||||||||||||||||||||||||||||||||||||||||||||||
| Configuration changes |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4230 +/- ##
==========================================
+ Coverage 81.56% 81.61% +0.05%
==========================================
Files 505 507 +2
Lines 36877 37005 +128
==========================================
+ Hits 30078 30202 +124
- Misses 6793 6797 +4
Partials 6 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude finished @EgeCaner's task in 9m 44s —— View job Reviewing PR #4230
SummaryClean — no correctness, concurrency, or resource-leak issues found in the two new commits since the last review ( What I checked:
Nit-level (not blocking, no inline comment since it's not a bug): |
Reviewing PR #4230
|
|
Update since the description: the write side is now behind the |
|
Claude finished @EgeCaner's task in 12m 22s —— View job Reviewing PR #4230
SummaryClean — nothing new to flag in the commits added since the last review pass. What I verified:
No blockers or nits found in this round. |
781c2b8 to
6896899
Compare
danielntmd
left a comment
There was a problem hiding this comment.
lgtm, found no correctness issues, won't block on this since it is gated by build tag anyways.
6896899 to
82b4a7d
Compare
741a497 to
b5a080a
Compare
|
Reviewed PR #4230 (size-based compact CBOR felt codec). Posted 2 inline comments on GitHub:
Core codec logic (compact/legacy CBOR encode/decode, MinimalBytes/SetMinimalBytes, trie leaf trimming, migration bookkeeping, canonical-form validation, modulus checks, truncation handling, legacy/compact interop, and extensive test/golden vectors) was reviewed in depth and found correct. |
b5a080a to
ed493a6
Compare
|
Launched three parallel review forks (core felt codec, migration/db code, tests) for PR 4230; awaiting their completion notifications before compiling the final review. |
…read both widths everywhere
…y builds from a compact DB
ed493a6 to
94363b8
Compare
|
Launched four parallel review forks for PR 4230 (NethermindEth/juno, size-based compact CBOR felt codec): (1) core/felt CBOR encode/decode correctness and panic-safety, (2) migration/bucket wiring for the compact-felt switch and legacy-build gating, (3) backward-compatibility of deprecatedstate/state call sites using the new StoredBytes/SetMinimalBytes/FromMinimalBytes APIs against pre-existing DB rows, and (4) test quality/coverage across the new felt codec and migration tests. All four are still running; I scheduled a wakeup to compile their findings into inline PR comments plus a severity-grouped summary once they finish. No results are available yet to report. |

What changes
Every felt stored through CBOR was written as a CBOR array of its four Montgomery limbs: 37 bytes for any
non-zero value, whether it was a nonce of 1 or a 32-byte hash. This PR replaces that with a size-based encoding on
Typed[K], so every felt kind encodes identically:The length alone identifies the form, and each value has exactly one valid encoding: non-minimal headers, leading
zeros, byte strings below 24, 31-byte bodies and values at or above the modulus are rejected, and a rejected input
never modifies the destination. Hashes keep their limbs so decoding them is a copy plus a modulus check; small values
pay one canonical-to-Montgomery multiplication on decode, except values below 24, which come from a precomputed table.
Legacy rows still decode. The decoder recognises the old array shape and takes the limb path, and anything that shape
check does not cover falls back to the generic decoder as before, so a node reads a mixed database. New writes are
compact from the first block after upgrade. Rewriting existing rows is a separate PR (
feat/felt/compact-felt-migration);the size numbers below come from that migration, run on a full mainnet node with this codec.
Also in this PR:
Slice[F]encodes and decodes element by element with the same codec and the same legacy fallback.MinimalBytes/SetMinimalBytesonTyped[K]: the trimmed big-endian value without CBOR framing, for buckets whosewhole value is one felt (nonces, class hashes, storage values). Strict like
SetBytesCanonicalbut accepting 1 to 32bytes. Used by the migration PR; no callers here.
on 100+ values and 20 malformed legacy shapes (carried over from the old fast-path tests), mixed legacy and compact
elements in one slice, all eight kinds encoding identically, map key ordering, and two fuzzers. The CBOR compatibility
goldens are regenerated for the new form and the old file is kept verbatim as
golden_bytes_legacy.json, decode-only.The typed-bucket test keeps its real mainnet rows as the read side and adds the compact bytes as the write side.
library, and a struct-level read benchmark on the mainnet fixtures.
Disk size
Full mainnet DB at height 14,733,615. Per-bucket bytes are the live SST sizes from a
pebble manifest dump,attributed to buckets by key prefix (the totals match
du). "Before" is the pristine snapshot as synced bymain(26,064 tables, all L6, median 4 MiB). "After" is after the migration rewrote every felt-bearing bucket and
juno db compactran with the node's zstd3 options (9,283 tables, median 63 MiB): the rewrite alone leaves old and newSSTs overlapping until compaction (504 → 496 GiB), so only the compacted number is meaningful. The base was not
re-compacted the same way, so the total includes whatever a full compaction would have reclaimed on its own; buckets
the migration did not touch moved 0 to -1.5%, which bounds that share for the large buckets.
Raw CBOR shrinks more than that (BlockTransactions -48%, Class -54%, StateUpdates -38% per record); zstd was already
recovering part of the padding, so the on-disk gain is the compressed one.
RPC throughput (k6)
Interleaved A/B on the same laptop (M3 Max, 36 GiB, NVMe):
mainbinary on the pristine snapshot vs this branch onthe migrated DB. Each arm's DB is restored from a verified copy before every block and the page cache is purged before
each profile. sequential = 1 VU, high = 500 VUs.
Warm corpus, 5,000 requests per method, 3 trials, medians:
Cold corpus, 1,000,000 distinct requests per method drawn uniformly over the whole chain (mostly NVMe reads):
No failed requests in any run. A golden replay of the warm corpus (241.5k requests) returned the same responses from
both arms, modulo map ordering and L1 finality state.
Micro benchmarks
Felt level (
BenchmarkMarshalCBOR,BenchmarkUnmarshalCBOR, 5 runs):Slice of 5,000 felts (
BenchmarkSliceCBOR), decoded directly and through fxamacker, which is how struct fields aredecoded. "short" = values 0..4095, the worst case for compact; "long" = random hashes:
Direct, small values are slower to decode by the one multiplication. Through the library compact wins both cases: a
compact felt is one CBOR item the library delimits in one header read, a legacy felt is a nested five-item array it has
to walk first.
Struct level on the mainnet fixtures,
cbor.Marshalp50 (p95): Transactions 26.1 → 19.2 µs (150.8 → 92.1), Receipts41.0 → 30.5 µs (141.9 → 98.2), StateUpdate 14.1 → 12.9 µs, Header 1.7 → 1.5 µs. Decode per record on a read-only
mainnet sample: BlockTransactions 268 → 191 µs (-29%), transaction hashes only 88 → 33 µs (-63%), Class 327 → 205 µs
(-37%), Headers 2.1 → 1.8 µs. Decoding an unmigrated row with the new decoder is within ±3% of the old decoder.