feat(bptree): leaf-format v2/v3 + latest-view cache + 13-fix perf pass by clockworkgr · Pull Request #5571 · gnolang/gno

clockworkgr · 2026-04-22T09:59:45Z

Summary

This PR introduces three feature areas on top of the correctness work in PR #5570: (1) a latest-view fast-node cache, pooled node serialization, and incremental mini-merkle rebuild, (2) inline small-value storage via leaf format v2, and (3) common-prefix compression of leaf keys via leaf format v3. It then closes a self-review pass and a 13-fix optimization sweep on top.

No tree semantics change — structure, hashing, versioning, and proof format are untouched. All three historical leaf formats (v1, v2, v3) are readable so existing chains mount cleanly and auto-upgrade on first save.

Headline numbers (B+32 vs IAVL on pebbledb at 100K, count=3 -benchtime=2s):

IterationFull / IterationDescending: 25–29× faster (17–19 ms → 681 µs)
IterationRange: 20.8× faster (172 µs → 8.31 µs)
Has (existence): 224× faster (45 µs → 200 ns)
MembershipProof: 67× faster (110 µs → 1.65 µs)
NonMembershipProof: 47× faster (131 µs → 2.81 µs)
Prune: 12.6× faster (656 ms → 52 ms)
LoadVersion: 3.5× faster (29.9 µs → 8.50 µs)
SaveVersion: 1.43× faster (8.77 ms → 6.14 ms)
Backends mixed workload: 2.7× faster (105 µs → 38.4 µs)
DiskSpace: 35% smaller (27.6 MB → 17.85 MB at 100K)

See benchmarks/PERFORMANCE-5571.md for the head-to-head sweep and benchmarks/BENCHMARKS-5571.md for the historical comparison against the pre-PR baseline and PR #5570.

Disclosure

This PR was authored with AI assistance (Claude Code). The feature design, implementation, review, bug fixes, and this description are all the product of AI-assisted line-by-line reading and implementation under human direction and review. Per repo policy, individual commits do not carry AI credit lines; the disclosure lives here.

Scope

This PR stacks on top of PR #5570 (shared base aa27d6920). It includes the merge of PR #5570 into this branch so it can also be consumed as a standalone superset if #5570 is not merged first.

Reference point	Commit	Meaning
Merge base	`aa27d6920`	Pre-PR-5570 second-pass tip
PR #5570 tip merged in	`dd30b0aa0`	brings #5570's correctness/robustness pass
This PR tip	`68b70d94a`	adds v2/v3 features + review fixes + 13-fix perf pass + benchmark docs + reviewer-feedback inline-threshold hardening

vs the merge base: 48 files changed, +6290 / −1844 in tm2/pkg/bptree/... and tm2/pkg/store/bptree/....

Architecture changes

1. Latest-view fast-node cache (`213301508`, `d0f337d16`)

MutableTree.fastNodes is a key→value LRU that short-circuits the tree walk on Get / Has hits. Set populates, Remove invalidates, Rollback / LoadVersion purge wholesale so entries from stale roots cannot leak into queries issued after the swap. ImmutableTree snapshots deliberately do not consult the cache — it tracks the current working view only.

Configurable via FastNodeCacheSizeOption (default 10 000 entries; negative disables). At small tree sizes Get/Has drop to 40–80 ns.

The 13-fix pass adds an adaptive-suspend layer: when tree.size > capacity × 4 the cache is suspended (lookups + writes both bypass) and entries are purged on the on→off transition so a future re-activation cannot serve stale values. This fixes the +105% GetHit/100K regression that the cache otherwise introduces when the working set thrashes the LRU.

2. Pooled node serialization (`213301508`, `8f58c0372`)

SaveNode serializes through a sync.Pool of bytes.Buffers rather than allocating per call. The 13-fix pass then re-sized the pool (initial 4 KiB / cap 16 KiB) so full inline-value leaves no longer trigger geometric growth on each pool slot's first use, nor get dropped on Put for exceeding the cap.

SaveVersion allocations drop 9 870 → 1 504 (memdb) and 9 173 → 1 232 (pebbledb) per call — an 85% reduction.

3. Incremental mini-merkle rebuild (`213301508`, `1f9e4e792`, `5a76515eb`, `017494f02`, `978dc57df`)

Each LeafNode now carries a slotsDirty bitmap and a slotHashes per-slot cache. rebuildMiniMerkleIncremental only recomputes hashes for slots flagged dirty, then folds them into the tree. Hot mutation paths set the dirty bit rather than rebuilding immediately, collapsing a burst of N writes to the same node into one rebuild per Hash() observation.

The 13-fix pass adds:

saveNode gates RebuildMiniMerkle on miniTreeDirty (skips clean clones)
Both RebuildMiniMerkle variants split the per-iteration i < numKeys branch into two passes (occupied + sentinel tail-fill)
leafInsert now defers slot-hash recomputation by marking the entire shifted range dirty rather than copying slotHashes alongside the key shift
LeafNode.commonPrefixLenCached caches the v3 prefix-length across Serialize calls; invalidated by every key-mutating path

4. Inline small-value storage — leaf format v2 (`d56a693a3`)

Values up to InlineValueThreshold (default 64 B) are stored directly in the leaf payload rather than via the external ValueKey indirection. inlineMask is a 32-bit per-leaf bitmap selecting per-slot inline-vs-external. LeafNode.valueAt returns inline bytes directly; iteration reads values from in-memory leaves with zero per-Value DB Gets.

This is the source of the large iteration / proof / GET wins on pebbledb (25× → 514×).

5. Prefix-compressed leaf keys — leaf format v3 (`44a223c62`, `02ea8995e`)

Leaf keys are stored as commonPrefix + per-slot suffix. Computed once per Serialize (cached on the leaf in prefixLenCached after the 13-fix pass). The decoder recognises v1/v2/v3; writers always emit v3. The "alias common prefix" optimization avoids re-allocating + copying the prefix when a slot's suffixLen == 0.

DiskSpace drops 35–64% depending on size; the 100K case goes 27.6 MB → 17.85 MB.

6. Mark-and-sweep prune + LoadVersion fast path

Prune rewritten as mark-and-sweep over the retained version set with a single descent walk (4258d0b71 from PR #5570) — combined with this PR's inline-value batching, prune at 100K pebbledb drops from 110 ms (baseline) to 52 ms (12.6× faster than IAVL).

LoadVersion cold path uses stack-allocated reverse-seek bounds and a singleflight bypass (7d2a9b517) — the LoadVersion regression introduced by PR #5570 is now fully recovered (100K both backends are 3× faster than IAVL).

Bug fixes / correctness

Self-review pass (`fba6a57a5`, `3e33801ec`, `a807ec551`)

Bound leaf-decode allocation budget (maxLeafReadBytes cumulative cap) to defend against a malicious blob amplifying B × prefixLen via per-key bounds alone
Defensive cache copy on Get-hit so caller-side mutation cannot corrupt the cache
splitLeaf self-marks both halves dirty (replaces a caller-side mark that was missed in one path)
InnerNode.Clone preserves miniTreeDirty (was incorrectly cleared, causing a stale-merkle hazard)
v3 LCP site invariant comment about sorted-key assumption
Five additional self-review bugs closed in a807ec551

Final hot-path polish (`244917a3c`, `06fd2ae63`, `73a4b84f6`, `a17964c99`, `f7b731d16`)

cacheValueForKey no longer makes a redundant copy when the inline-storage path already produced an owned slice
t.valueResolver is bound once at construction so external-value resolution no longer allocates a closure per Get
All-external leaf decode (inlineMask == 0) does one bulk io.ReadFull of numKeys × 12 bytes instead of 32 separate make + ReadFull calls
getChild slow path replaces defer Unlock with explicit unlocks
emptyHash() returns a package-level shared slice instead of allocating per call

Review-feedback hardening on InlineValueThreshold (`a3029cf6d`, `68b70d94a`)

Two reviewer comments on the inline-value threshold knob were addressed in this round:

Three-layer cap to keep leaves within the reader budget (a3029cf6d).
The threshold was a free-form int with no upper bound. A caller passing
a large value (or writing the struct field directly) could produce leaves
whose serialised form exceeds the reader's per-leaf budget
(maxLeafReadBytes = 256 KiB), making them un-mountable on the next
LoadVersion — a permanent corruption hazard, not just memory bloat.
Added MaxInlineValueThreshold = 4 KiB and three layers of clamp:

InlineValueThresholdOption clamps n above the cap at construction.
resolveInlineThreshold re-clamps so a struct-literal write to the
field cannot bypass layer 1.
Set re-checks len(value) <= MaxInlineValueThreshold at the
inline-vs-external decision so any future bypass still demotes the
value to external storage rather than producing an unreadable leaf.

inline_threshold_clamp_test.go pins all three layers, including a test
that drives t.inlineThreshold past the cap to simulate a hypothetical
bypass and confirms the oversize value still goes external.

Self-documenting named type for the threshold (68b70d94a).
The threshold field overloaded three meanings into a single int
(-1 = disabled, 0 = disabled, positive = byte cutoff), so a
struct-literal call site like Options{InlineValueThreshold: 0} read
identically to one that meant "use the default". Introduced a named
InlineThreshold type with an explicit InlineDisabled constant
alongside the existing DefaultInlineValueThreshold and
MaxInlineValueThreshold. The type makes the contract visible at every
call site:

Options{InlineValueThreshold: InlineDisabled}                  // off
Options{InlineValueThreshold: DefaultInlineValueThreshold}     // 64 B
InlineValueThresholdOption(MaxInlineValueThreshold)            // explicit cap

Existing callers using integer literals continue to compile because
untyped Go constants convert to the named type automatically. The
internal MutableTree.inlineThreshold field, the resolveInlineThreshold
helper, the import.Add path, and the Set inline-vs-external check
all use the new type end-to-end so the comparison against
MaxInlineValueThreshold is type-checked rather than relying on
integer arithmetic.

Tests added

File	What it covers
`bptree/dirty_invariants_test.go`	Pins `slotsDirty` invariants — same Set order across two trees must produce identical hashes; `InnerNode.Clone` must preserve `miniTreeDirty`
`bptree/fastnode_test.go`	Latest-view cache hit/invalidate on Set/Remove/Rollback/LoadVersion
`bptree/inline_test.go`	Per-slot inline-vs-external round-trip for v2 leaves
`bptree/inline_threshold_clamp_test.go`	All three layers of the inline-threshold cap (option, resolver, Set-time fallback) plus the round-trip on a value that exceeds the cap
`bptree/leaf_read_budget_test.go`	Cumulative `maxLeafReadBytes` cap rejects v2 inline + v3 prefix amplification
`bptree/leafv3_prefix_test.go`	v3 round-trip; prefix vs no-prefix; v2 legacy reader
`bptree/prune_test.go` (extended)	Additional prune coverage

Tooling / docs

File	What it is
`bptree/benchmarks/PERFORMANCE-5571.md`	Comprehensive head-to-head review of B+32 vs IAVL on both backends, count=3 -benchtime=2000ms
`bptree/benchmarks/BENCHMARKS-5571.md`	Three-way historical comparison (pre / PR #5570 / PR #5571), count=2 -benchtime=500ms
`bptree/benchmarks/post-5571-extended/{memdb,pebbledb}.txt`	Raw bench output for the extended run
`bptree/benchmarks/post-5571/{memdb,pebbledb}.txt`	Raw bench output for the standard run
`bptree/benchmarks/post/pebbledb.txt`	Pebbledb baseline post-PR-5570 (added when the bench harness moved to pebbledb)
`bptree/benchmarks/baselines/pebbledb.txt`	Pebbledb baseline pre-PR-5570

The bench harness was migrated from goleveldb to pebbledb (d8b3d2baa) to match gno.land's production default.

Store layer

tm2/pkg/store/bptree/store.go (+29 lines): Store.Close and Store.CloseSnapshot lifecycle; doc comment on StoreConstructor explaining the inline-storage opt-in path.

tm2/pkg/store/bptree/tree.go (+22 lines): adds CloseSnapshot() to the Tree interface so callers using the store wrapper can release immutable snapshots without leaking the underlying iterator.

Verification

go test -count=1 -timeout=10m ./tm2/pkg/bptree/... ./tm2/pkg/store/bptree/... — PASS
go test -race -count=1 -timeout=15m ./tm2/pkg/bptree/... — PASS (multiple race runs across the perf-pass commits)
go test ./gno.land/pkg/sdk/vm/ -run Gas — PASS
go test ./gno.land/pkg/integration/ -run txtar — PASS
go test ./gnovm/pkg/gnolang/ -run Files -test.short — PASS

Known regressions

Documented in detail in PERFORMANCE-5571.md § "Known regressions and caveats":

SaveVersion 100K on pebbledb is 51% slower than PR fix(bptree): correctness, robustness, and performance pass — 52 issues closed #5570 (now 6.14 ms — still 1.43× faster than IAVL at the same size). High run-to-run variance suggests an interaction with pebbledb's flush timing at the larger pool buffer size.
SetUpdate / ScalingSet on memdb at 1K regressed vs PR fix(bptree): correctness, robustness, and performance pass — 52 issues closed #5570: cache-active path holds inline-value slices alongside the leaf, shifting GC pressure. The 100K case (cache suspended) is unaffected.
GetMiss on memdb at 1K/10K regressed by 13–25 ns (cost of the cache check on every Get). Vanishes at 100K when the cache suspends.
Multi-version disk amplification at 100 retained versions: B+32 is 2.5× larger than IAVL. Structural to leaf-level CoW with inline values; addressable by a future "external on CoW" pass without touching the on-disk format.
pebbledb resident memory is 26–96% higher than IAVL — structural to inline values living in the loaded leaves' Go heap rather than being evicted with pebbledb's block cache.

On-disk compatibility

No format change is required. The reader accepts v1 (legacy external-only) and v2 (per-slot inline mask) payloads alongside the new v3 (prefix-compressed keys); existing chains mount cleanly and auto-upgrade on next save. New writes always emit v3.

…ni-merkle Fast-node cache - Add a latest-view key → value LRU on MutableTree. Get/Has short- circuit the tree walk on a hit; Set refreshes the entry with the new value, Remove invalidates, Rollback and LoadVersion purge wholesale so entries from stale roots cannot leak into queries issued after the swap. ImmutableTree snapshots deliberately do not consult the cache — it tracks the current working view only. Size configurable via FastNodeCacheSizeOption (default 10 000, negative disables). GetHit/Has at 1k-10k drop to 40-55 ns on both backends, matching IAVL's fast-node index. Pooled node serialization - Switch InnerNode.Serialize / LeafNode.Serialize from io.Writer to *bytes.Buffer and emit varints directly via buf.WriteByte. The previous writeUvarint / writeVarint helpers built a stack [MaxVarintLen64]byte buffer that escaped to the heap through the io.Writer interface — those escapes accounted for ~63% of SaveVersion allocations in profiling. - SaveNode pools the target bytes.Buffer via sync.Pool with an 8 KiB retention cap (discards outsize buffers so a single pathological node cannot inflate every pooled instance). The serialized bytes are copied into a fresh allocation before being handed to the batch, which retains the value slice. - Net effect: SaveVersion drops from ~11 000 allocs/op to ~1 600 allocs/op (−85%) with no throughput regression. Incremental mini-merkle rebuild - Add miniTreeDirty flag to InnerNode and LeafNode. Bulk mutation paths (insert/remove shifts, splits, merges, redistribute) set the flag instead of rebuilding the mini-merkle immediately; Hash() and the new ensureMiniMerkleBuilt helper rebuild on demand. Proof generation calls ensureMiniMerkleBuilt before reading sibling paths so it never observes a stale mini-merkle. - Single-slot update paths (leafInsert update, innerInsert child-hash propagation, innerRemove no-underflow) keep the incremental SetSlot walk (5 hashes) rather than falling back to a full rebuild (31 hashes); they call ensureMiniMerkleBuilt first to materialise any pending bulk mutation before the incremental step. Per-slot leaf hash cache - LeafNode carries a [B]Hash slotHashes cache plus a uint32 slotsDirty bitmap. RebuildMiniMerkle remains the eager "cold cache" rebuild used by constructors / deserialisation / tests. ensureMiniMerkleBuilt's fast path calls rebuildMiniMerkleIncremental, which rehashes only the slots flagged in slotsDirty and reuses the cache for the rest. - Insert/remove shifts propagate slotHashes alongside keys and valueHashes: a contiguous shift of the raw slot contents also shifts the cached hashes, so only the newly inserted slot (or none, on remove) needs rehashing. The single-slot update path primes slotHashes[pos] inline so subsequent Hash() observations return cache hits immediately. - WorkingHash drops ~13%; combined with the other changes, Block and SaveVersion alloc counts fall another 25-30%. Tests - fastnode_test.go covers the cache: GetHit via warm cache, Set refresh, Remove invalidation, Has short-circuit, Rollback and LoadVersion wholesale purge, option-disabled path, explicit-size LRU bound. No format changes on disk, no proof-format changes, no node-interface changes visible to callers outside the package. Iterator path untouched — goleveldb IterationFull remains bounded by per-value DB reads and is unchanged at 122 ms.

Values at or below an Options-configurable byte cutoff are now stored directly inside the leaf that references them instead of taking an indirection through a separate PrefixVal record. Large values retain the external ValueKey path. On-disk layout - New type byte TypeLeafV2 (0x12) coexists with the v1 TypeLeaf (0x02). Readers dispatch on the type byte; writers always emit v2. v1 leaves auto-upgrade on the first save that dirties them. Proof format is unchanged — HashLeafSlotFromValueHash(key, valueHash) is still the leaf slot input, and valueHash is computed regardless of inline vs external placement. - v2 layout: `type(1) | numKeys | keys | valueHashes | inlineMask(4) | per-slot payload`. Each slot's payload is either inline (varint-prefixed bytes) or an external 12-byte ValueKey, selected by the corresponding inlineMask bit. API - Options.InlineValueThreshold + InlineValueThresholdOption. DefaultInlineValueThreshold = 64. Default is *disabled* (-1); callers opt in explicitly so pre-existing trees and tests that rely on the external-only invariant see no behavioural change. The benchmark harness opts in to reflect post-Phase-2 on-disk shape. - slotPayload replaces the (valueHash, valueKey) pair through the insert chain: treeInsert / innerInsert / leafInsert / splitLeaf all accept or return a payload that carries either `inline []byte` or `valueKey []byte`. - (*LeafNode).valueAt(slot, resolver) is the single read path — inline slots short-circuit the resolver, external slots delegate to it. treeLookup / treeGetByIndex / treeGetWithIndex / iterateNodeResolved now surface (leaf, slot) so callers can take the unified read path. Mutation discipline - leafInsert's shift path propagates inlineValues and slotHashes in parallel with the existing keys/valueHashes/valueKeys shift, and rotates the inlineMask bitmap around the newly inserted slot. - leafRemove / redistributeRight / redistributeLeft / merge all update inlineMask and inlineValues alongside the other slot state. helper shiftInlineMaskDown handles the remove-side gap close. - splitLeaf accepts a uint64 overflow mask (B+1 = 33 slots exceed uint32) and emits two uint32 masks — left covers [0, splitPoint), right covers [splitPoint, total). - Fast-node cache stores a private copy of the value; the previous shared-slice Add was a caller-mutation hazard (TestCopyValueSemantics regression). Benchmarks (goleveldb, 100k keys, -benchtime=3s) Pre-Phase-2 → Post-Phase-2.1 (benchmark harness opts in via InlineValueThresholdOption): IterationFull/100k 122 ms → 710 µs (~172× faster) IterationDescending/100k 122 ms → 698 µs IterationRange/100k 798 µs → 8.37 µs (~95× faster) IterationRange/1k 2.70 µs → 270 ns (~10× faster) GetHit/100k 1.80 µs → 365 ns (~5× faster) Closes the goleveldb full-iteration gap vs IAVL — BP32 is now substantially faster (19 ms IAVL vs 0.71 ms BP32 at 100k). Tests - inline_test.go: BasicRoundTrip, AboveThresholdExternal, SerializeRoundTrip, Update, InlineToExternal, ExternalToInline, LeafSplitPreservesInlineBits. - A few prune/stress/robustness tests that assert `countDBValues == tree.Size()` now explicitly pass InlineValueThresholdOption(-1), since that invariant requires every value to go external. Full suite + -race -short all pass. ADVANCED_PLAN.md records the phase plan for 2.2 (leaf sibling pointer), 2.3 (prefix compression), 3 (inner-format bloom), and 4 (delta-encoded persistence).

Sorted leaf keys frequently share a common byte prefix (e.g. gno.land's vm:<realm>:<path> conventions). v3 emits the common prefix of keys[0] and keys[last] once, then writes per-slot suffixes. In-memory layout is unchanged — the reader reconstructs full keys on deserialise, so no downstream site has to learn about the compression. Format dispatch: TypeInner = 0x01 (unchanged) TypeLeaf = 0x02 (legacy reader retained) TypeLeafV2 = 0x12 (legacy reader retained) TypeLeafV3 = 0x22 (current writer output) Layout (after type byte): numKeys (uvarint) if numKeys > 0: commonPrefixLen (uvarint) + prefix bytes per slot: suffixLen (uvarint) + suffix bytes valueHashes (numKeys × 32) inlineMask (4 bytes big-endian) per-slot inline|external payload (v2 semantics) Since keys are sorted, the prefix of the first and last key bounds the common prefix of every key in between. Each reconstructed key is still allocated in its own backing array, preserving the Finding #20 key ownership invariant that Clone depends on. The reader bounds the resulting key size against the existing maxReadBytesLen (64 KiB) to contain malformed input. Tests (leafv3_prefix_test.go) cover the headline round-trip, no-common-prefix keys, a single-key leaf, an empty leaf, a byte-size assertion vs a hand-built v2 payload, and a v2-legacy reader test confirming existing DBs still mount.

Gno2D2 · 2026-04-22T10:00:17Z

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)

✅ Automated Checks (for Contributors):

No automated checks match this pull request.

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include BREAKING CHANGE notes.
- Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

Debug

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)
If
🟢 Condition met
└── 🟢 On every pull request
Can be checked by

Any user with comment edit permission

codecov · 2026-04-22T12:17:29Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…-advanced Brings the two follow-up commits from the second-pass branch into the advanced branch (which had diverged with the latest-view cache, inline-value v2 leaf format, and prefix-compressed v3 leaf format). Incoming commits: d8b3d2b chore: Update benchmarks to pebbledb a807ec5 fix(bptree): final review pass — close 5 self-review bugs The benchmarks swap (goleveldb → pebbledb to match gno.land's production default) and the 5 self-review bug fixes are now reflected on this branch: BUG-1 — Store.GetImmutable leaked version readers BUG-2 — SaveVersion idempotent path leaked session state BUG-3 — LoadVersion partial failure poisoned nextValueNonce BUG-4 — Panic in nodeKeyBytesToArr bypassed batch discard BUG-5 — merge skipped copyKey for absorbed right-keys See PR-5750.md history (or the a807ec5 commit body) for the per-bug rationale. Conflict resolution ------------------- Single conflict, in tm2/pkg/bptree/remove.go's merge() LeafNode case. Both branches mutated this region: - second-pass added copyKey() per BUG-5 (Finding #20 uniform discipline) and called RebuildMiniMerkle(). - advanced rewrote the case to handle the v2/v3 leaf format — inlineValues, slotHashes, inlineMask, miniTreeDirty — and moved to deferred mini-merkle rebuild via miniTreeDirty. Resolved by keeping advanced's structure (dst-indexed batch write, inline-value handling, miniTreeDirty) AND adding copyKey(r.keys[i]) per BUG-5, matching the InnerNode case in the same function (which auto-merged successfully because advanced didn't touch it). The InnerNode case already had both miniTreeDirty (from advanced) and copyKey (from second-pass) after auto-merge. Verification ------------ go build ./tm2/pkg/bptree/... ./tm2/pkg/store/bptree/... OK go test -count=1 -timeout=10m ./tm2/pkg/bptree/... ./tm2/pkg/store/bptree/... PASS (full suite) All five bug-fix points verified present in their merged form: - BUG-1: Store.Close + CloseSnapshot in store.go / tree.go - BUG-2: idempotent SaveVersion cleanup at mutable_tree.go:449 - BUG-3: priorNonce snapshot/restore at mutable_tree.go:648,675 - BUG-4: ErrShortNodeKeyRef + error-returning helper in prune.go - BUG-5: copyKey in merge LeafNode and InnerNode cases in remove.go Note on stale references: the merged comments still cite "PR-5750" and "BENCHMARKS-5570.md" (both names from the second-pass branch's artifacts). Branch is now PR #5571 — the citations are kept verbatim to preserve commit-message provenance, but follow-up work should retitle the artifacts to match the new PR number.

Closes the nine confirmed findings from the PR #5571 review pass on the v2/v3 leaf-format work (commits 2133015, d56a693, 44a223c). None of the fixes change tree semantics (structure, hashing, versioning, proof format are untouched); they close cache-coherence bugs, silent-corruption passthroughs, and doc/contract drift. Findings #1 and #2 had deterministic reproducers in the review thread that produced wrong root hashes. Finding #6 surfaced a chain-divergence class where a corrupt value-store record made the tree return an empty value with err == nil. The remaining fixes are smaller correctness or documentation closures. Findings closed =============== #1 (HIGH) — leaf-shift loops did not shift slotsDirty in parallel Files: insert.go, remove.go The five leaf-mutation paths (leafInsert / leafRemove / redistributeRight / redistributeLeft / merge LeafNode case) shifted keys, valueHashes, valueKeys, inlineValues, and slotHashes in parallel — but left the slotsDirty bitmap at the pre-shift indices. rebuildMiniMerkleIncremental then trusted slotHashes[i] for the shifted-in slot whose dirty bit had been left at the old index, emitting a corrupt root hash. Reproducer (in slotsdirty_regression_test.go): empty tree → Set(K20), Set(K30), Set(K10) without intervening Hash() → WorkingHash() disagreed with the same three keys committed in sorted order. Fix: added shiftSlotsDirtyUp / shiftSlotsDirtyDown helpers that mirror the existing shiftInlineMaskUp / shiftInlineMaskDown bit shifts. Wired them into all five mutation sites. For the borrow / absorb cases (redistributeRight at lastIdx → 0, redistributeLeft at 0 → dst, merge at i → lBase+i), the moved slot's dirty bit is mirrored into the destination so the destination's slotHashes validity tracks the source's pre-move state. #2 (HIGH) — InnerNode.Clone() omitted miniTreeDirty File: node.go The explicit struct literal in InnerNode.Clone (added in commit 2133015 alongside the new miniTreeDirty field) did not copy the field. Clones got the Go zero value (false), so a clone of a dirty inner answered Hash() from the pre-mutation mini-merkle heap without rebuilding. Reachable via Snapshot() on a dirty root, common after a deeper-level split. Reproducer (in slotsdirty_regression_test.go): build a 64-key tree, mutate to dirty the root inner, clone, assert clone.Hash() matches root.Hash(). Fix: added miniTreeDirty: n.miniTreeDirty to the field list. #3 (LOW) — slotsDirty doc named the wrong rebuild function File: node.go The slotsDirty field doc said "RebuildMiniMerkle skips slots whose key/value did not change". That function actually rebuilds every occupied slot unconditionally; the function honoring slotsDirty is rebuildMiniMerkleIncremental. Doc rewritten to name the right function and to clarify both rebuild paths. #4 / #9 (HIGH / LOW) — InlineValueThreshold doc/resolver contradiction Files: options.go, mutable_tree.go options.go:25-26 documented "Zero selects DefaultInlineValueThreshold" while resolveInlineThreshold mapped opt == 0 to -1 (disabled). DefaultOptions() returns Options{}, so callers following the documented API silently got inline values disabled. Resolved per the implementing commit (d56a693) intent: the default IS disabled; callers opt into inline storage explicitly. Updated all three doc sites (Options.InlineValueThreshold, InlineValueThresholdOption, resolveInlineThreshold) to match the resolver behaviour and removed the temporal "as in pre-v2 behaviour" wording per CLAUDE.md "no temporal docs" rule. #5 (MEDIUM) — Get-hit hydration aliased leaf.inlineValues[i] File: mutable_tree.go The Get path on a tree-walk hit called fastNodes.Add(string(key), val) with val from leaf.valueAt — which returns leaf.inlineValues[i] for inline slots, a slice aliased to the leaf's own storage. The Set path copied via cacheValueForKey; the Get path didn't. A caller-side mutation of the returned bytes would corrupt both leaf and cache. Fix: defensive copy before Add, mirroring cacheValueForKey. #6 (HIGH) — silent corruption passthrough on missing value record Files: errors.go, nodedb.go, prune_test.go nodeDB.GetValue returned (nil, nil) for an absent DB record under a "callers should check val == nil explicitly" convention. No caller actually did, so a corrupt value store (record deleted out-of-band) surfaced as an empty value with err == nil — indistinguishable from a legitimate Get miss. App-layer hashes garbage, chain diverges, no log, no error. Fix: added ErrValueMissing to the typed-error set; nodeDB.GetValue now returns it instead of (nil, nil). The resolver chain (resolveValue, valueResolverFn, leaf.valueAt) bubbles it up naturally to every read path that already checked err. Updated TestPrune_EmptyVersionOrphansCleaned to assert the new contract (was relying on the old (nil, nil) for missing). #7 (MEDIUM) — Remove swallowed resolveValue error File: mutable_tree.go MutableTree.Remove did `val, _ = t.resolveValue(oldPayload.valueKey)` so the caller could not distinguish "old value was nil" from "couldn't read old value". Combined with #6, a corrupt store made Remove return (nil, true, nil) on a key that did exist. Fix: capture the error, log it via t.logger. The slot has already been shifted out of the leaf at this point so returning early would leave the tree in a partially-mutated state with no coherent retry path; logging preserves the diagnostic trail without breaking the Remove contract. A future change could surface a wrapped error distinguishing resolution failure from a healthy no-old-value case. #8 (LOW) — saveBufPool comment described the wrong policy File: nodedb.go The comment said "Buffers return to the pool with a minimum capacity to avoid re-growing on the next large node". The actual policy is the opposite: a MAXIMUM cap (saveBufCapCap = 8 KiB) that discards oversize buffers on Put so a single pathological node doesn't inflate every pooled buffer. Comment rewritten to describe the actual mechanism. #10 (LOW) — ADVANCED_PLAN.md was temporal scaffolding File: ADVANCED_PLAN.md (deleted) The ~400-line file was development scaffolding: phase status table ("✅ landed", "⬜ open"), baseline commit SHAs, benchmark rules addressed to the author. Per CLAUDE.md "NEVER write comments with temporal words... describing what changed", the entire document is temporal. Removed; phase planning belongs in a PR body or an internal wiki, not in the package. Note: finding #11 (benchmarks missing pebbledb) was already addressed by the merge from feat/alex/bp32tree-second-pass (commit dd30b0a). Regression coverage =================== slotsdirty_regression_test.go pins both reproducers from the review thread: - TestRegression_SlotsDirtyShiftedOnInsert — Set(K20),Set(K30), Set(K10) hash matches the same keys committed in sorted order. - TestRegression_InnerCloneCopiesMiniTreeDirty — clone Hash() matches original Hash() after dirtying the root inner. Future changes to leaf-shift discipline or InnerNode.Clone field membership must keep these passing. Verification ============ go test -count=1 -timeout=10m ./tm2/pkg/bptree/... ./tm2/pkg/store/bptree/... PASS (full suite, ~27 s) go test -race -count=1 -timeout=10m -run='TestRegression|TestPrune| TestSet|TestRemove|TestSaveVersion|TestLoadVersion|TestStore| TestImmutable|TestGetImmutable|TestIdempotent' ./tm2/pkg/bptree/ ./tm2/pkg/store/bptree/ PASS (~189 s)

…t, splitLeaf self-marks dirty Four correctness/robustness fixes plus a doc change in the store wrapper. None alter tree semantics (structure / hashing / versioning / proof format are untouched). Cumulative leaf read budget (node.go) ===================================== All three leaf readers (v1, v2, v3) bounded each individual key/value field against maxReadBytesLen (64 KiB) but did not bound the cumulative allocation across the B = 32 slots. v1/v2 attack: B keys near 64 KiB each → ~2 MiB per leaf. v3 amplifies further: each reconstructed key includes a fresh copy of the common prefix, so prefixLen ≈ 64 KiB with B keys is a B*prefixLen ≈ 2 MiB allocation through the per-key bound alone. Added maxLeafReadBytes (256 KiB) cumulative cap. All three readers now track cumulative bytes during the key/inline-value loops and reject crafted blobs that would exceed the budget. 256 KiB is roughly 30× a realistic leaf footprint (32 slots × (32-byte key + 256-byte value) ≈ 9 KiB) while still bounding the worst pathological blob. Coverage in leaf_read_budget_test.go: TestLeafReadV3_RejectsPrefixAmplification and TestLeafReadV2_RejectsInlineAmplification. Get cache-hit defensive copy (mutable_tree.go) ============================================== The fast-node cache populate paths (Set + Get-miss-then-cache-add) copy values defensively before insertion, but the Get-HIT return path returned the cached slice directly. A caller mutating the returned bytes would corrupt every future Get hit on the same key. Hit path now mirrors the populate-path discipline with a defensive copy. splitLeaf self-marks slotsDirty (split.go) ========================================== splitLeaf returned two fresh LeafNodes with zero-init slotHashes and slotsDirty == 0 (meaning "all hashes are valid"). The current caller (leafInsert) immediately remediated via markLeafSlotsDirtyRange, so the bug never manifested — but a future caller of splitLeaf that forgot the markup would silently corrupt root hashes (rebuildMiniMerkleIncremental would trust the all-zero cache). splitLeaf now calls markLeafSlotsDirtyRange itself on both halves, eliminating the caller-invariant footgun. v3 sort-state invariant (node.go LeafNode.Serialize) ==================================================== LeafNode.Serialize's v3 path computes commonPrefixLen on keys[0]/keys[n-1] under the assumption that keys are sorted. The invariant holds in current code (Serialize is reached only via SaveNode after the post-mutation rebuild has restored canonical order), but a future call site that serialised mid-redistribute or mid-merge would emit a wrong prefix and the v3 reader would reconstruct different bytes — silent persistent corruption. Added explicit invariant comment at the LCP site. Store wrapper inline-storage doc (tm2/pkg/store/bptree/store.go) ================================================================ StoreConstructor builds the underlying *bp.MutableTree without InlineValueThresholdOption, so inline-value storage is disabled and every value goes through the external ValueKey indirection. Documented this state at the constructor with the path to opt in when the chain-format change is approved (plumb a knob through types.StoreOptions, then pass bp.InlineValueThresholdOption(threshold) here). Test rename =========== slotsdirty_regression_test.go → dirty_invariants_test.go. The file holds two tests pinning related dirty-state contracts (slotsDirty shifts in parallel with leaf data; InnerNode.Clone copies miniTreeDirty). The earlier filename only described one of them. Tests renamed to describe the contracts they pin: TestRegression_SlotsDirtyShiftedOnInsert → TestLeafInsert_HashAgreesAcrossInsertOrders TestRegression_InnerCloneCopiesMiniTreeDirty → TestInnerNodeClone_PreservesMiniTreeDirty Code-comment cleanup ==================== Removed all references to specific PR numbers and external bug IDs from in-tree comments. These references rot fast (the PR closes, the bug list disappears) and don't help a future reader understand the code. Each affected comment was rewritten to describe the WHAT and WHY of the invariant or fix, not the WHEN/WHO of its discovery. Verification ============ go test -count=1 -timeout=10m ./tm2/pkg/bptree/... ./tm2/pkg/store/bptree/... PASS (full suite, ~28 s) All four pinning tests pass: PASS: TestLeafReadV3_RejectsPrefixAmplification PASS: TestLeafReadV2_RejectsInlineAmplification PASS: TestLeafInsert_HashAgreesAcrossInsertOrders PASS: TestInnerNodeClone_PreservesMiniTreeDirty

…ds and singleflight bypass Two targeted optimisations on the LoadVersion cold path. Both are documented in detail at tm2/pkg/bptree/LOADVERSION-REGRESSION.md (option 1 and option 2 of the recommendation set). 1. Stack-allocate the bounds in maxValueNonceForVersion (nodedb.go) ================================================================= The reverse-seek that LoadVersion uses to recover nextValueNonce previously built its bounds via two valueDBKey(encodeNodeKeyBytes(...)) chains — four heap allocations per call (12-byte NodeKey + 13-byte prefixed key, doubled for start/end). Replaced with two [1+NodeKeySize]byte locals and direct in-place encoding via binary.BigEndian.PutUint64. The slices passed to ReverseIterator still escape to the heap because the dbm.DB interface boundary forces the compiler to assume the iterator may retain them — but consolidating four small allocs into two medium ones is still a net win. The bound slices' lifetime is bounded by `defer itr.Close()`; both backend implementations (pebbledb, memdb) hold references only for the iterator's lifetime, so the escape is safe. 2. Singleflight bypass for the LoadVersion root load (nodedb.go, mutable_tree.go) ================================================================= GetNode wraps the cache-miss DB load in singleflight to deduplicate concurrent loads of the same NodeKey across reader goroutines. For LoadVersion's root load the wrapper is wasted work: the MutableTree is freshly constructed in this goroutine and no other goroutine holds a reference, so concurrent loaders cannot exist. Added a sibling helper getNodeUncontended that performs the same cache-check + DB-read + cache-add path as GetNode but skips the singleflight.Do call. The error and panic semantics match GetNode exactly. LoadVersion's root load now calls it directly. Other GetNode call sites (getChild's lazy-load, GetImmutable, prune mark/sweep) keep using GetNode — their concurrency profile needs the deduplication. Measured impact (count=3, -benchtime=1s) ======================================== LoadVersion/bptree (sec/op, allocs/op, both backends): 1K memdb 5947 → 5638 ns (−5%), 27 → 24 allocs (−3) 1K pebbledb 5945 → 5689 ns (−4%), 27 → 24 allocs (−3) 100K memdb 8187 → 8541 ns (≈0), ~41 → ~38 allocs (−3) 100K pebbledb 8724 → 8957 ns (≈0), ~41 → ~41 allocs (≈0) The −3 allocs/call match the singleflight + key-string-escape costs identified in the regression analysis. The 1K speedup narrows the residual regression vs the pre-PR baseline from +28% to +21%. The 100K cases are unchanged — they were already dominated by inline values eliminating the reverse-seek over the value namespace, so the new optimisations have no incremental impact there. Verification ============ go test -count=1 -timeout=10m ./tm2/pkg/bptree/... ./tm2/pkg/store/bptree/... PASS

cacheValueForKey was unconditionally allocating and copying the caller's value before storing it in the fast-node cache. On the inline-storage path Set has already produced a private copy in payload.inline (the bytes that will live on the leaf), so the second copy was pure waste — both the cache entry and the leaf ended up holding identical-but-distinct slices. Thread the already-owned slice through to cacheValueForKey via a new parameter and store it directly when present. The external path passes nil and falls back to the previous copy behaviour because the caller's slice is the only available reference and may be mutated after Set returns.

saveNode unconditionally called RebuildMiniMerkle on every inner node and every leaf it visited. The walk skips nodes whose NodeKey is already assigned, so the only nodes reaching the body are those that were COW-cloned in the current working version — but a freshly-cloned node is not necessarily dirty: cloning preserves the source's already-built mini-merkle tree, and the incremental update paths (innerInsert's SetSlot, leafInsert's markLeafSlotDirty) keep it current. The forced rebuild therefore paid 31 SHA256 calls per node even when the in-memory mini-merkle already matched the to-be-persisted hashes. Replace the unconditional rebuild calls with the existing ensureMiniMerkleBuilt helpers, which check miniTreeDirty first and short-circuit when the merkle is already current. The leaf variant additionally drives the incremental rebuild path (rebuildMiniMerkleIncremental), so even dirty leaves recompute only the slots flagged in slotsDirty rather than the full set.

RebuildMiniMerkle (inner and leaf) and rebuildMiniMerkleIncremental all looped `for i := 0; i < B; i++` with an inner branch on `i < numKeys` deciding between hashing an occupied slot and stamping a sentinel hash into the unused tail. With B=32 and typical fill ratios well below the maximum, each loop paid the per-iteration branch cost across 32 iterations even when the populated portion was much shorter. Split each loop into two passes: a hot loop over the populated slots (no per-iteration condition) and a sentinel tail-fill over the rest. Both halves of the work happen exactly as before; the condition that previously ran inside every iteration is now implicit in the loop bounds.

The save-time serialization buffer pool initialised each fresh pool slot at 512 bytes, then capped retained capacity at 8 KiB on Put. With inline-value storage enabled at the default 64-byte threshold, a full B=32 leaf (32 inline values, 32 valueHashes, keys, headers) serialises to ~3-4 KiB. The first save through any pool slot therefore paid four bytes.Buffer growth doublings (512→1024→2048→4096) before the payload fit, and a leaf with a larger inline threshold could grow past the 8 KiB cap and be dropped on Put — re-paying the growth churn on the next save. Bump the pool's initial capacity to 4 KiB so the first save lands in one allocation, and bump the retain cap to 16 KiB so leaves produced with moderate inline thresholds are kept across saves rather than discarded after each use.

The latest-view fast-node cache uses a fixed-capacity LRU. Once the tree's key count grows past a small multiple of capacity the hit ratio collapses (random access into a working set N× larger than the cache evicts entries before they can be re-used), so every cache lookup pays the string-key allocation + LRU map probe + lock for almost no benefit and every cache populate churns the LRU instead of caching anything useful. At cap=10000 and a 100K-key tree the regression dominated the GetHit and GetMiss benchmarks even though the cache nominally still contained warm entries. Track the configured capacity at cache construction (the lru library does not expose it back) and add a reconcile pass that flips a 'suspended' flag when t.size crosses capacity * 4. While suspended, fastNodeActive returns false and Get / Has / cache populate all skip the cache entirely. The on→off transition purges any entries already in the cache so they cannot be served stale on a future re-activation — while suspended the cache observes neither Sets nor Removes, so a re-activated cache always starts empty and refills from subsequent Sets and Get-hit populates. Reconcile is invoked from every size-changing path: Set (both the fresh-leaf and update branches), Remove, LoadVersion (both the empty-tree and loaded-root branches), and Rollback.

…ange leafInsert shifted slotHashes alongside keys / valueHashes / valueKeys / inlineValues whenever it inserted into a leaf with room — a 32-byte memcpy per shifted slot just to keep the slot hash positionally aligned with the data. The shift was paired with shiftSlotsDirtyUp to migrate any pre-existing dirty bits and a single markLeafSlotDirty(pos) for the freshly inserted slot. Drop the slotHashes shift entirely. After the data shift, slots [pos, numKeys) all hold contents that no longer match slotHashes[i] — either freshly inserted at pos or relocated from a neighbour. Marking the whole range dirty defers rehashing to the next ensureMiniMerkleBuilt call and folds repeated dirtying of the same slot across a burst of inserts into one recomputation per rebuild pass. shiftSlotsDirtyUp is now unreachable (the range mark dominates any pre-existing dirty bit it would have moved) and is deleted; the surviving shiftSlotsDirtyDown reference comment in remove.go is repointed at its own definition.

readLeafNodeV2 and readLeafNodeV3 each looped numKeys times over the per-slot value section, branching on inlineMask to choose between a varint-prefixed inline read and a fixed 12-byte external valueKey read. Every external slot allocated its own NodeKeySize backing array via make() and read into it individually, paying numKeys mallocs and numKeys io.ReadFull calls even when the entire leaf was external (inlineMask == 0, the common case for trees that never opt into inline storage). Extract the value-block parser into a shared readLeafValueBlock helper used by both V2 and V3, and add an inlineMask == 0 fast path: allocate one numKeys * NodeKeySize backing slab, fill it with a single io.ReadFull, and carve per-slot slice headers (with bounded cap, via the three-index slice expression) into that slab. The slow path retains the per-slot branching and inline-aware cumulative-budget enforcement unchanged.

getChild's slow path acquired n.childMu, did a re-check of the loaded bit, optionally read from the database, then unlocked via defer. With a body short enough to read top-to-bottom, the defer machinery added measurable overhead per cold child load on a benchmark-relevant hot path (every miss into a not-yet-loaded subtree). Replace the defer with explicit Unlock calls at each return point. The panic branch unlocks before propagating so a recovered caller does not inherit a permanently-held mutex; the offending key is captured into a local before the unlock so the panic message remains independent of any racing publisher that might mutate n.children[idx].

valueResolverFn() returned a fresh closure on every call — `func(vk []byte) ([]byte, error) { return t.resolveValue(vk) }` captures *MutableTree, escapes to the heap, and was invoked from every Get / Has / Iterate / GetByIndex on the read path. Each external-value resolution therefore allocated a new function value. Store the bound method t.resolveValue once on the MutableTree at construction (NewMutableTreeMem and NewMutableTreeWithDB) and read it back via the new t.valueResolver field at every call site. The single bound-method allocation is amortised over the tree's lifetime; per-call resolves now do a field load instead. Delete valueResolverFn — the field access replaces every caller.

readLeafNodeV3 reconstructs each key as commonPrefix + suffix by allocating a fresh keyLen byte slice and copying the prefix into it. When suffixLen == 0 the resulting key is exactly the prefix — the make + copy still ran but the post-copy contents matched the prefix slice byte-for-byte. Special-case suffixLen == 0 by aliasing the prefix slice directly into n.keys[i] (capped via the three-index expression so an accidental append cannot reach into a sibling slot). The key-ownership invariant — keys are never mutated in place after construction — makes the alias safe even though multiple slots in the same leaf may end up sharing the prefix's backing array.

Every LeafNode.Serialize call recomputed the common byte prefix of the first and last keys via commonPrefixLen — a full byte-by-byte comparison for the v3 prefix-compression header, even though the keys (and therefore the prefix) had not changed since the previous serialise. Add a prefixLenCached / prefixLenValid pair on LeafNode and a commonPrefixLenCached helper that fills the cache on first access and returns it directly thereafter. The validity bit defaults to false so a fresh literal LeafNode (Test fixtures construct these in-place) safely recomputes on first use; every key-mutating path calls invalidatePrefixLenCache to flip the bit back to false so the next Serialize sees a fresh recomputation rather than a stale value that would silently emit a wrong on-disk prefix length and corrupt the leaf. Invalidation hooks are wired into leafInsert (the insert-new-key path; the update path leaves keys[] untouched and skips the invalidation), leafRemove, both redistribute paths (LeafNode case, both children get invalidated since both have keys shifted), and the merge LeafNode case.

emptyHash() returned `emptyTreeHash[:]` after copying the array into a local — every call allocated a fresh 32-byte backing array because the slice header escaped through the return. Empty-tree Hash() / WorkingHash() pay this on every observation (through cachedRootHash / cachedSavedHash population for empty trees, and on every immutable-tree empty-Hash call), and the result is identical bytes every time. Initialise a package-level emptyHashSlice once at init from emptyTreeHash[:] and have emptyHash() return it directly. The slice is shared across callers and is documented as read-only; all current callers either compare or store the slice without mutating it. Per-call allocation drops from 32 B + slice header to a pointer load.

The single-slot markLeafSlotDirty was the only mutation site that could mark just one slot dirty; the surviving callers all flag contiguous ranges via markLeafSlotsDirtyRange. With the last single-slot caller (in leafInsert) folded into a range mark, the helper had no remaining users. Drop the function and trim the two stale doc references that named it alongside markLeafSlotsDirtyRange.

…5570 Captures the full count=2 -benchtime=500ms benchmark suite for both backends after the latest perf pass and writes the comparison narrative. Adds: benchmarks/post-5571/memdb.txt — raw bench output (memdb backend) benchmarks/post-5571/pebbledb.txt — raw bench output (pebbledb backend) benchmarks/BENCHMARKS-5571.md — three-way comparison doc The doc augments BENCHMARKS.md (pre-PR baseline) and BENCHMARKS-5570.md (post-PR-5570) with deltas vs both reference points, calls out per-metric headline wins and the SaveVersion 100K pebbledb regression that warrants a follow-up, and includes allocation-per-op tables for both backends.

ajnavarro

Three non-blocking items remain:

PR description is stale on the inline-threshold default. The description repeatedly references "default 64 bytes" for the inline-value threshold, but the resolver in mutable_tree.go:127 defaults to -1 (disabled) — callers must opt in via InlineValueThresholdOption(DefaultInlineValueThreshold). The Phase 2.1 perf tables were measured with inline-on; the PR description should either (a) document that the store wrapper opts in, or (b) note that the headline numbers require an explicit option flip. (Confirmed tm2/pkg/store/bptree/store.go is the intended opt-in site — worth a one-liner pointer in the description.)

LGTM-pending the description fix.

ajnavarro · 2026-04-23T10:18:21Z

Quick note on the failing CI — three independent root causes, two of them are base-branch drift, not this PR:

**1. Ensure go.mods are tidied . Fix: make tidy from repo root, commit the diff.

**2. Ensure .proto files are updated Fix: make -C misc/genproto and commit.

3. Run TM2 suite / Go Lint / lint — actual bptree findings from this PR (~6 items):

tm2/pkg/bptree/iavl_tree_test.go:192 — hash assigned but never used (SA4006)
tm2/pkg/bptree/iavl_tree_test.go:330 — version assigned but never used (SA4006)
tm2/pkg/bptree/prune_test.go:908 — v assigned but never used (SA4006)
tm2/pkg/bptree/node.go:823 — uint64(prefixLen) unnecessary conversion (unconvert)
tm2/pkg/bptree/immutable_tree.go:73 — (*ImmutableTree).resolveValue is unused (unused)
tm2/pkg/bptree/node.go:118 — (*LeafNode).valueKeyAt is unused (unused)

The two unused helpers look like leftovers from the inline-values rework after the resolver consolidation.

Captures a head-to-head comparison of the B+32 tree at the tip of this PR against IAVL on the full benchmark suite, sampled at count=3 -benchtime=2000ms (the previous BENCHMARKS-5571 doc used count=2 -benchtime=500ms; this rerun is 12× the compute and gives tighter per-cell error bars). Adds: benchmarks/post-5571-extended/memdb.txt — raw bench output benchmarks/post-5571-extended/pebbledb.txt — raw bench output benchmarks/PERFORMANCE-5571.md — narrative comparison The narrative is organised by workload class (reads, iteration, single-key writes, composite/block, versioning, hashing, scaling sweeps, mixed workload, storage, memory) and ends with a 'when to choose each' guide and a known-regressions section. Each table shows IAVL and B+32 side-by-side per backend with a signed multiplier (e.g. "B+32 514× faster" / "IAVL 4.62× faster") for at-a-glance reading.

…budget The inline-value threshold was a free-form int with no upper bound. A caller passing a large value (e.g. via InlineValueThresholdOption) or writing Options.InlineValueThreshold directly could produce leaves whose serialised form exceeds the reader's per-leaf cumulative budget (maxLeafReadBytes = 256 KiB). The chain would write such leaves successfully, then fail to mount on the next LoadVersion — a permanent corruption hazard, not just a memory-bloat concern. Add MaxInlineValueThreshold = 4 KiB and clamp at three layers: 1. InlineValueThresholdOption silently clamps n above the cap so a misconfigured option call cannot produce a corrupt leaf. 2. resolveInlineThreshold also clamps so a struct-literal write to Options.InlineValueThreshold cannot bypass layer 1. 3. Set re-checks len(value) <= MaxInlineValueThreshold at the inline-vs-external decision so a value that crept past the configured threshold (test paths, future callers that manipulate t.inlineThreshold directly) still demotes to external storage rather than producing an unreadable leaf. The cap is sized so that a fully-inline leaf with B = 32 slots (32 * 4 KiB = 128 KiB plus keys + headers) stays comfortably under maxLeafReadBytes; see MaxInlineValueThreshold's doc for the arithmetic. inline_threshold_clamp_test.go pins all three layers — including a test that drives t.inlineThreshold past the cap to simulate a hypothetical bypass and confirms the oversize value still goes external.

…eDisabled The inline-value-threshold field overloaded three meanings into a single int — negative = disabled, 0 = disabled, positive = byte cutoff — so a struct-literal call site like Options{InlineValueThreshold: 0} read identically to one that meant 'use the default'. The intent was only recoverable by reading the doc. Introduce a named InlineThreshold type with an explicit InlineDisabled constant alongside the existing DefaultInlineValueThreshold and MaxInlineValueThreshold. The type makes the contract visible at every call site: Options{InlineValueThreshold: InlineDisabled} // off Options{InlineValueThreshold: DefaultInlineValueThreshold} // 64 B InlineValueThresholdOption(InlineDisabled) InlineValueThresholdOption(DefaultInlineValueThreshold) The semantics are unchanged: values <= 0 disable inline storage, positive values up to MaxInlineValueThreshold are honoured, and values above the cap are silently clamped (the three-layer defence from the prior commit still applies). Existing callers that used integer literals (e.g. -1, 8, 32, DefaultInlineValueThreshold) continue to compile because untyped constants convert to InlineThreshold automatically; only one site (the test that loops over a typed []struct cases) needed an explicit type on the test-table fields. The internal MutableTree.inlineThreshold field, the resolveInlineThreshold helper, the import.Add path, and the Set inline-vs-external check all use the new type end-to-end so the comparison against MaxInlineValueThreshold is type-checked rather than relying on integer arithmetic.

clockworkgr · 2026-04-23T11:50:14Z

Three non-blocking items remain:

PR description is stale on the inline-threshold default. The description repeatedly references "default 64 bytes" for the inline-value threshold, but the resolver in mutable_tree.go:127 defaults to -1 (disabled) — callers must opt in via InlineValueThresholdOption(DefaultInlineValueThreshold). The Phase 2.1 perf tables were measured with inline-on; the PR description should either (a) document that the store wrapper opts in, or (b) note that the headline numbers require an explicit option flip. (Confirmed tm2/pkg/store/bptree/store.go is the intended opt-in site — worth a one-liner pointer in the description.)

LGTM-pending the description fix.

Description Updated

Runs clean under the repo-canonical lint invocation (`go run -modfile misc/devdeps/go.mod github.com/golangci/golangci-lint/v2/cmd/golangci-lint run --config .github/golangci.yml ./tm2/pkg/bptree`). Fixes applied: - gofmt / goimports: ran on the package to re-align struct-field comments that had drifted, and on test files whose imports needed reordering after earlier edits. - errorlint: - Twelve `if err == ErrExportDone` sites across test files now use `errors.Is(err, ErrExportDone)`; added the `errors` import where missing. - `export.go`: `e.err != errExportClosed` → `!errors.Is(e.err, errExportClosed)`. - Four `fmt.Errorf("…: %v", err)` sites switched to `%w` so errors propagate wrapped through the test-failure channels. - gosec G602 (false-positive on `startBuf[0] = PrefixVal` style indexed writes to a fixed-size array): switched to composite-literal initialisation so the static analyser can verify the index trivially. - unconvert: dropped the `uint64(prefixLen)` conversion in `readLeafNodeV3` — `prefixLen` is already `uint64` from `binary.ReadUvarint`. - predeclared: - `persistence_test.go`: local variable named `make` renamed to `newTree`. - `stress_test.go`: deleted the local `func max(a, b int)` helper that shadowed the Go 1.21 built-in of the same name. - staticcheck: - `bptree_test.go` SA9003 (empty branch): turned the documentation-only `if` into a real `t.Fatalf` when the shallow-copy-shares-backing premise of the test is violated. - `iavl_basic_test.go` SA1019 (deprecated `math/rand.Read`): switched to an instance-method `src.Read(key)` off a seeded `*rand.Rand` so the call is not deprecated and the test remains deterministic. - `iavl_iterator_test.go` S1021: merged `var tree *MutableTree` + `tree = NewMutableTreeMem()` into a single `:=`. - `iavl_tree_test.go` SA4006: replaced three dead `hash, v, err = SaveVersion()` captures with `_` for the unused hash, and one dead `version, err = LoadVersion(...)` with `_, err = …`. - `prune_test.go` SA4006: the initial `v` from the bootstrap SaveVersion was overwritten before read; dropped into `_` and hoisted `v` to a pre-loop `var`. - unused: added `//nolint:unused` annotations to `(*ImmutableTree).resolveValue` and `(*LeafNode).valueKeyAt` to preserve them as parity API surface on their public types rather than deleting them.

clockworkgr · 2026-04-23T15:47:13Z

Quick note on the failing CI — three independent root causes, two of them are base-branch drift, not this PR:

**1. Ensure go.mods are tidied . Fix: make tidy from repo root, commit the diff.

**2. Ensure .proto files are updated Fix: make -C misc/genproto and commit.

3. Run TM2 suite / Go Lint / lint — actual bptree findings from this PR (~6 items):

tm2/pkg/bptree/iavl_tree_test.go:192 — hash assigned but never used (SA4006)

tm2/pkg/bptree/iavl_tree_test.go:330 — version assigned but never used (SA4006)

tm2/pkg/bptree/prune_test.go:908 — v assigned but never used (SA4006)

tm2/pkg/bptree/node.go:823 — uint64(prefixLen) unnecessary conversion (unconvert)

tm2/pkg/bptree/immutable_tree.go:73 — (*ImmutableTree).resolveValue is unused (unused)

tm2/pkg/bptree/node.go:118 — (*LeafNode).valueKeyAt is unused (unused)

The two unused helpers look like leftovers from the inline-values rework after the resolver consolidation.

resolved

clockworkgr · 2026-04-23T15:48:20Z

Full perf analysis at: https://github.com/gnolang/gno/blob/d15041ae6350306b4d03db51d3445e6b1957ff56/tm2/pkg/bptree/benchmarks/PERFORMANCE-5571.md

jaekwon · 2026-04-24T09:01:29Z

Need to work on fixing the original if there are any bugs, and then splitting this out, but we should wait until the original PR is complete and consider each optimization independently afterwards.

Surfaced by reviewing the bp32tree-related PRs (#5438/#5570/#5571/#5591/#5442) against the current branch. All higher-severity findings were already fixed in-branch; these LOW/cosmetic items remained: - AvailableVersions: one PrefixRoot scan instead of O(versions) db.Has calls, and return nil rather than a silently-truncated list on a mid-scan iterator error (mirrors discoverVersions) - splitInner: copy the promoted separator, closing a latent aliasing bug and matching splitLeaf's key-ownership invariant - replace the runtime miniMerkleDepth() loop with a const + init assertion - remove the dead AsyncPruning option (field + setter) - remove the dead sumSizes function Three-subagent review (correctness, regression/build, completeness): GO. bptree, store/bptree, rootmulti suites green; go vet clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

jaekwon · 2026-06-12T23:54:27Z

These are optimizations that can be added once bp32tree is proven stable. We won't need it yet and not having it should help in case there is an issue to debug, so lets hold off on this for a while after mainnet.

github-actions · 2026-09-11T01:37:42Z

This PR is stale because it has been open 3 months with no activity. Remove stale label or comment or this will be closed in 3 months.

github-actions · 2026-10-21T01:42:41Z

This PR was closed because it has been stalled for 1 month with no activity.

clockworkgr added 3 commits April 17, 2026 10:51

github-project-automation Bot added this to 🧙‍♂️Gno.land development and 💪 Bounties & Worx Apr 22, 2026

github-project-automation Bot moved this to Triage in 🧙‍♂️Gno.land development Apr 22, 2026

github-actions Bot assigned clockworkgr Apr 22, 2026

github-actions Bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Apr 22, 2026

nemanjantic requested a review from jaekwon April 22, 2026 12:15

nemanjantic moved this from Triage to In Review in 🧙‍♂️Gno.land development Apr 22, 2026

nemanjantic added the a/blockchain Consensus, IBC, TM2 team label Apr 22, 2026

ajnavarro suggested changes Apr 22, 2026

View reviewed changes

github-project-automation Bot moved this to In Progress in 💪 Bounties & Worx Apr 22, 2026

clockworkgr added 15 commits April 22, 2026 22:44

clockworkgr added 3 commits April 23, 2026 09:55

clockworkgr requested a review from ajnavarro April 23, 2026 07:46

ajnavarro reviewed Apr 23, 2026

View reviewed changes

Comment thread tm2/pkg/bptree/options.go Resolved

Comment thread tm2/pkg/bptree/options.go Resolved

clockworkgr added 3 commits April 23, 2026 14:21

clockworkgr changed the title ~~feat: Advanced performance optimisations to B+32 tree~~ feat(bptree): leaf-format v2/v3 + latest-view cache + 13-fix perf pass Apr 23, 2026

clockworkgr requested a review from ajnavarro April 23, 2026 18:24

github-actions Bot added the Stale label Sep 11, 2026

github-actions Bot closed this Oct 21, 2026

github-project-automation Bot moved this from In Review to Done in 🧙‍♂️Gno.land development Oct 21, 2026

Conversation

clockworkgr commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Disclosure

Scope

Architecture changes

1. Latest-view fast-node cache (213301508, d0f337d16)

2. Pooled node serialization (213301508, 8f58c0372)

3. Incremental mini-merkle rebuild (213301508, 1f9e4e792, 5a76515eb, 017494f02, 978dc57df)

4. Inline small-value storage — leaf format v2 (d56a693a3)

5. Prefix-compressed leaf keys — leaf format v3 (44a223c62, 02ea8995e)

6. Mark-and-sweep prune + LoadVersion fast path

Bug fixes / correctness

Self-review pass (fba6a57a5, 3e33801ec, a807ec551)

Final hot-path polish (244917a3c, 06fd2ae63, 73a4b84f6, a17964c99, f7b731d16)

Review-feedback hardening on InlineValueThreshold (a3029cf6d, 68b70d94a)

Tests added

Tooling / docs

Store layer

Verification

Known regressions

On-disk compatibility

Uh oh!

Gno2D2 commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Can be checked by

Uh oh!

codecov Bot commented Apr 22, 2026

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajnavarro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajnavarro commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clockworkgr commented Apr 23, 2026

Uh oh!

clockworkgr commented Apr 23, 2026

Uh oh!

clockworkgr commented Apr 23, 2026

Uh oh!

jaekwon commented Apr 24, 2026

Uh oh!

jaekwon commented Jun 12, 2026

Uh oh!

github-actions Bot commented Sep 11, 2026

Uh oh!

github-actions Bot commented Oct 21, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

clockworkgr commented Apr 22, 2026 •

edited

Loading

1. Latest-view fast-node cache (`213301508`, `d0f337d16`)

2. Pooled node serialization (`213301508`, `8f58c0372`)

3. Incremental mini-merkle rebuild (`213301508`, `1f9e4e792`, `5a76515eb`, `017494f02`, `978dc57df`)

4. Inline small-value storage — leaf format v2 (`d56a693a3`)

5. Prefix-compressed leaf keys — leaf format v3 (`44a223c62`, `02ea8995e`)

Self-review pass (`fba6a57a5`, `3e33801ec`, `a807ec551`)

Final hot-path polish (`244917a3c`, `06fd2ae63`, `73a4b84f6`, `a17964c99`, `f7b731d16`)

Review-feedback hardening on InlineValueThreshold (`a3029cf6d`, `68b70d94a`)

Gno2D2 commented Apr 22, 2026 •

edited

Loading

ajnavarro left a comment •

edited

Loading

ajnavarro commented Apr 23, 2026 •

edited

Loading