Conversation
Introduces EncodeKeyV2/DecodeKeyV2 in the new execution/commitment/nibbles package. V2 packs nibbles 2-per-byte high-first and stores the parity flag as a trailing byte (vs V1's leading HP-prefix), so prefix-sorted DB keys preserve trie subtree locality. See issue #17838. Pure additive change: V1 (trie/encoding.go) is untouched. Tests for the new primitives land in follow-up tasks of the same plan.
Table-driven TestDecodeKeyV2_Errors covers all four V2 sentinel errors (length, parity, shape, non-canonical pad) using errors.Is matching.
Asserts EncodeKeyV2 panics on out-of-range nibbles (0x10, 0xff) and on input exceeding MaxPathNibbles (129).
There was a problem hiding this comment.
Pull request overview
Adds an additive “V2” byte encoding for commitment-trie nibble-path DB keys to improve prefix-sort locality (parity flag moved from HP-prefix-at-front to a trailing parity byte), without modifying existing V1 behavior.
Changes:
- Introduces
EncodeKeyV2/DecodeKeyV2with strict canonical decoding and sentinel errors. - Adds comprehensive unit tests (goldens, round-trips, error matrix, panic cases), a locality property test, a fuzz target, and a V1-vs-V2 locality benchmark.
- Adds a design/rollout plan doc describing motivation, algorithm, and follow-ups.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| execution/commitment/nibbles/nibbles_v2.go | Implements V2 key encoder/decoder with parity-suffix format and canonicality checks. |
| execution/commitment/nibbles/nibbles_v2_test.go | Adds vectors, property tests, fuzzing, and benchmark coverage for V2 (and V1 comparison). |
| docs/plans/20260507-nibbles-v2-key-encoding.md | Documents the design rationale, algorithm, and staged implementation plan. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yperbasis
left a comment
yperbasis
left a comment
There was a problem hiding this comment.
Overview
Additive library-only change introducing a V2 byte encoding for commitment-trie nibble paths. V2 packs nibbles 2-per-byte high-first and appends a single parity byte (0x00 even, 0x01 odd) as a suffix instead of V1's HP-prefix-front byte. The motivation (issue #17838) is that V1's front-prefix encoding shards each logical subtree across two disjoint DB regions (even-parity keys vs odd-parity keys), destroying scan locality during fold/unfold.
Scope is deliberately narrow: encoder + decoder + tests + locality benchmark. V1 is untouched; no call sites are migrated. Wiring/migration is explicit follow-up.
I verified the algorithm by tracing all golden vectors against both encoder and decoder, ran the test suite (all pass), ran the fuzz target for 5s (clean), and ran the bench (produces metrics).
Strengths
- Algorithm: All golden vectors are correct. The locality lemma (paths sharing
knibbles → encoded keys share⌊k/2⌋bytes) holds and is well-asserted. - Test depth: golden vectors, round-trip, error matrix with all four sentinels, encoder panic cases, 10K randomized + 10 adversarial property cases, fuzz target with seeded corpus, V1-vs-V2 locality benchmark. The two starred disambiguating vectors (
[2,f,b]→2f b0 01and[2,f,b,3]→2f b3 00) are present. - Strict canonicality: decoder rejects non-zero pad nibbles in odd encoding, giving a clean bijection — catches buggy upstream encoders.
- Failure mode split: encoder panics on programmer errors (matches V1
HexToKeybytesconvention); decoder returns sentinel errors (errors.Is-compatible). Defensible split. - Scope discipline: zero churn outside the new files. Reviewer-friendly.
Issues
Doc-only
nibbles_v2_test.go:303-306— thev1HexToCompactNoTermcomment referencesexecution/commitment/trie/encoding.go's hexToCompact (package-private). That file doesn't exist. The actual V1 source isnibbles.HexToCompactin the same package. Suggest rewording to:// v1HexToCompactNoTerm replicates nibbles.HexToCompact for non-terminated // input, inlined to make this benchmark self-contained ...
Bench evidence is weaker than claimed
- The benchmark produces
v1_neighbor_prefix=3.238vsv2_neighbor_prefix=3.495— a ~8% improvement, not the dramatic ''destroys locality'' effect the issue describes. The uniform[16, 128]length distribution (and uniform-random nibbles) probably dilutes the parity-shard effect because the population is dominated by long paths where the leading bytes already differ. A more representative workload would either (a) weight by depth distribution actually observed in fold/unfold, or (b) measure range-scan miss rate for a subtree query rather than mean neighbor-prefix. Worth flagging in the PR description so reviewers don't read the modest delta as case-closed evidence — the bench is honest-evidence-only per the plan, but the framing should match.
Minor style
commonNibblePrefixandcommonBytePrefixhave identical bodies (nibbles_v2_test.go:221-238). Two names for the same code. The semantic distinction reads well at call sites, but a thin wrapper would remove duplication. Take it or leave it.- Mixed loop styles: production code uses
for i := 0; i < n/2; i++; tests usefor i := range n. Both valid post-Go-1.22, but inconsistent within the same PR.
Possibly missing
- Append-style API.
EncodeKeyV2allocates every call. Once V2 lands in hot paths (PutBranch/Branch, snapshot v2),AppendKeyV2(dst, nibbles []byte) []byteand an in-place decode equivalent will likely be wanted. Not a blocker for this PR — flag for the wiring follow-up. FuzzDecodeKeyV2. The existingFuzzEncodeDecodeKeyV2only round-trips valid inputs. A second fuzz target that feeds arbitrary bytes intoDecodeKeyV2and asserts no panic / no OOB would catch decoder robustness regressions. Cheap to add.- Boundary-length decoder test. Tests cover 67 bytes (over) and 65 (valid, via
max_128_around-trip), but not 66 explicitly. Trivial; the 67 case probably suffices.
Behavioral note (not a defect, worth documenting)
- V2 does not guarantee ''parent sorts before all its children''. E.g.
encode([0x2,0x0]) = {0x20,0x00}sorts beforeencode([0x2]) = {0x20,0x01}. V1 has the analogous property, so this isn't a regression — but if any current V1 caller relies on parent-first scan order, it would break under V2. The follow-up Plan B (caller audit) is the right place to confirm; just noting that the audit needs to cover sort-order assumptions, not only ''where does prefix-front matter''.
Correctness checks I'd want from the author
- Confirm
MaxPathNibbles=128is correct for the commitment trie (Keccak-256 hashed keys → 64 bytes → 128 nibbles ✓). Worth a one-line comment noting ''= 2 × len(Hash) for Keccak-derived paths'' so a future reader doesn't wonder where 128 came from.
Verdict
Library-only, additive, well-tested, correct. The PR is safe to merge on its own merits — it changes zero runtime behavior. Three small asks before un-drafting:
Wire EncodeKeyV2/DecodeKeyV2 (from #21146) into the patricia-hashed trie DB-key path: hex_patricia_hashed (unfold/fold), the concurrent variant, trie_reader, warmuper, plus validatePlainKeys and VerifyBranchHashes on the decode side. trie/* RLP-node-hash paths keep V1 (HexToCompact/CompactToHex) for spec compatibility. Hard cutover: existing V1-encoded datadirs are not readable; fresh sync only. Tests that build branch keys directly are migrated to V2.
V1 vs V2-nibbles A/B benchmark (mainnet)Head-to-head of Timeline (UTC) — for Grafana range selection
Run 1 — no memory limitBoth executed the snapshot range
Unconstrained, V2 ≈ V1 (marginally better) — finishes first, slightly less commitment time and I/O. Run 2 — 32 GB cgroup limit (
|
| metric | A (V1) | B (V2) | B vs A |
|---|---|---|---|
| time to blk 25,099,041 | 22h 06m | 26h 05m | +18% (then OOM 99s later) |
| commitment total | 5292 s (88m) | 11887 s (198m) | 2.25× |
| commitment median / p90 / p99 | 62ms / 39s / 91s | 73ms / 115s / 194s | |
| aggregation total | 3524 s (59m) | 5747 s (96m) | 1.63× |
| exec gas/s (median) | 217 M | 224 M | +3% (exec itself fine) |
| Go alloc median / max | 11.8 / 18.3 GB | 16.9 / 26.8 GB | +43% |
| Go sys median / max | 21.8 / 22.3 GB | 29.6 / 34.3 GB | +36% |
| major page faults | 1281 M | 1782 M | 1.39× |
| minor page faults | 1002 M | 1128 M | 1.13× |
Takeaway
V2's raw execution speed is fine (gas/s +3%). The regression is memory footprint: +43% Go heap / +36% sys at matched block, consistent with the +6.4% larger V2 .kv files. Under a 32 GB cap that pushes the working set past the ceiling → 1.39× major page faults → the memory-heavy commitment/aggregation phases thrash (2.25× commitment time) → +18% wall-clock and eventually OOM. Without a cap (Run 1), the same workload is at parity.
A re-run at a 48 GB cap is in progress to check whether the footprint fits with headroom and commitment returns to Run-1 parity.
|
Question: commitment.kv had compress-keys - V2 does same or different? |
|
But where is ram spent? Maybe just a bug (buf capacity growing, or something like this) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
execution/commitment/hex_patricia_hashed.go:2263
- Changing updateKey to V2 encoding changes the DB keys written/read for folded branches, effectively altering the commitment storage format. Without explicit version gating/migration, this will make the new binary incompatible with existing chaindata and commitment snapshots.
updateKey := nibbles.EncodeKeyV2(hph.currentKey[:updateKeyLen])
| depth := startDepth | ||
| for depth <= len(hashedKey) && depth <= w.maxDepth { | ||
| prefix := nibbles.HexToCompact(hashedKey[:depth]) | ||
| prefix := nibbles.EncodeKeyV2(hashedKey[:depth]) |
| } | ||
|
|
||
| prefix := nibbles.HexToCompact(hashedKey[:depth]) | ||
| prefix := nibbles.EncodeKeyV2(hashedKey[:depth]) |
| key := nibbles.EncodeKeyV2(hph.currentKey[:hph.currentKeyLen]) | ||
| hph.metrics.BranchLoad(hph.currentKey[:hph.currentKeyLen]) |
| func (p *ConcurrentPatriciaHashed) CanDoConcurrentNext() (bool, error) { | ||
| if p.root.root.extLen == 0 { | ||
| zeroPrefixBranch, _, err := p.root.ctx.Branch(nibbles.HexToCompact([]byte{0})) | ||
| zeroPrefixBranch, _, err := p.root.ctx.Branch(nibbles.EncodeKeyV2([]byte{0})) |
| nib, err := nibbles.DecodeKeyV2(branchKey) | ||
| if err != nil { | ||
| return fmt.Errorf("decode branch key: %w", err) | ||
| } |
| uncompactedBranchKey, err := nibbles.DecodeKeyV2(branchKey) | ||
| if err != nil { | ||
| return fmt.Errorf("decode branch key: %w", err) | ||
| } |
|
|
||
| Motivation is GitHub issue [erigontech/erigon#17838](https://github.com/erigontech/erigon/issues/17838): the current HP-prefix-front encoding shards each logical subtree across two unrelated DB regions (one for even-parity keys, one for odd), destroying data locality during commitment fold/unfold operations. Moving the parity flag to a suffix makes the DB layout mirror the trie shape — keys sharing a path prefix cluster together regardless of parity. | ||
|
|
||
| This plan delivers **only the library primitives** (encoder, decoder, tests). Wiring V2 into `PutBranch` / `Branch`, snapshot v2, and migration are explicitly out of scope and will come in follow-up plans. The change is purely additive — V1 is untouched. |
## Problem `HashSort` streams each batch's hashed/plain keys into a reused `byteArena` bump buffer and hands every key to the async warmuper as a sub-slice for MDBX prefetch. At each 10k batch boundary it reset that single arena while warmup workers were still reading earlier keys — the next batch's `arenaAlloc` overwrote bytes a worker was mid-read on. Data race. Latent on main: `HexToCompact` tolerates the garbage (at worst a wasted prefetch). On nibblesv2 (erigontech#21146) `EncodeKeyV2` validates nibbles and panics on the corrupted byte: `panic: nibbles v2: nibble at index 68 is 0xff`, mainnet ~blk 24.83M, mid commitment. ## Fix Replace the single arena with a 2-slot ring (`arenaRingSize`) keyed by a generation counter. Each warmed key is tagged with the current `gen`; the warmuper keeps a per-slot in-flight count (`outstanding[gen % ringSize]`). Before a batch boundary reuses a slot, the producer calls `WaitBufferFree(slot)`, which blocks until that slot's previous-generation warm items have drained — so no worker still references the bytes about to be overwritten. Workers decrement on completion and broadcast on drain-to-zero; a waker goroutine releases any waiter on ctx cancellation. Zero-copy, no per-key allocation: the arena is pre-sized once per batch (`arenaEnsureCap`), and an over-capacity key falls back to an independent allocation rather than reallocating the buffer (which would invalidate live sub-slices). Wired at both `HashSort` batch boundaries for `ModeDirect` and `ModeUpdate`; the `nil`-warmuper path is unchanged. ## Tests - `TestHashSort_WarmupArenaNoRace` — `-race` repro; DATA RACE in `HexToCompact` on the old single-arena wiring, green after. Covers `ModeDirect` and `ModeUpdate`. - `WaitBufferFree` behaviour: blocks until a straggler drains, fast-path when the slot is already empty, unblocks on ctx cancel. - Slot-reuse invariant `curArena == gen % arenaRingSize` survives a cancel landing inside a boundary wait; `arenaAlloc` returns non-overlapping sub-slices and falls back cleanly on over-capacity. `make lint` clean, `make erigon integration` builds, commitment package green under `-race`. --------- Co-authored-by: Alex Sharov <AskAlexSharov@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
The library half of this PR is already on
What is left unique here is exactly the wiring the description calls out of scope, and it is not in good shape:
On the open RAM question: the file-size half is structural, not a bug. Suggest closing. The deliverable is already merged in a cleaner form, and the remainder is an unversioned on-disk format change against code that has moved out from under it. If a V2 rollout is still wanted it is a fresh PR on today's Separately, |
|
change behind this branch become a part of v3 commitment design #23904 |
Summary
Add a V2 byte encoding for commitment-trie nibble paths, alongside existing V1 (
HexToCompact/CompactToHex). V2 packs nibbles 2-per-byte high-first and appends a trailing parity byte (0x00even,0x01odd) instead of an HP-prefix at the front.Motivation: erigontech/erigon#17838 — the HP-prefix-front encoding shards each logical subtree across two unrelated DB regions (one even-parity, one odd), destroying locality during commitment fold/unfold. A suffix parity flag makes prefix-sorted DB keys preserve trie subtree locality.
This PR delivers library primitives only — encoder, decoder, tests, fuzz, property test, and a V1-vs-V2 locality benchmark. Wiring into
PutBranch/Branch, snapshot v2, and migration are explicit out-of-scope follow-ups. V1 is untouched; the change is purely additive.execution/commitment/nibbles/nibbles_v2.go—EncodeKeyV2/DecodeKeyV2,MaxPathNibbles=128, four sentinel errors (length, parity, shape, non-canonical pad).execution/commitment/nibbles/nibbles_v2_test.go— golden vectors, round-trip, decoder error matrix, encoder panic cases, subtree-locality property test, round-trip fuzz target, V1-vs-V2 locality benchmark.docs/plans/20260507-nibbles-v2-key-encoding.md— design + task plan.Test plan
go test ./execution/commitment/nibbles/...passesmake lintcleantrie/hasher.go,trie/witness_marshalling.go,trie/keybytes.go) remain on V1 — they're RLP-node-hash paths, not DB-key paths