feat(FlatDB): long finality support with persisted snapshots

[asdacap]

Replaces #11663 (the original long-finality PR, built on the HSST format) and supersedes the draft #12100 (the HSST→SortedTable split). This branch is the long-finality work reimplemented on a simpler, single-level SortedTable persisted-snapshot format. Block numbers were migrated to ulong to track master's #11937 "Unify types with Geth".

Summary

Adds a long-finality storage tier to Nethermind.State.Flat so the in-memory
snapshot ring (today bounded by MaxReorgDepth, default 256) can be extended to
~90,000 blocks (configurable via LongFinalityMaxReorgDepth) without the
per-snapshot RAM cost growing linearly. Snapshots that fall off the in-memory
tail are converted into immutable, mmap-backed persisted snapshots stored in
arena files, then logarithmically merged ("Linked" compaction) so the snapshot
count stays bounded.

The new tier is opt-in (FlatDb.EnableLongFinality = false by default) and sits
underneath the existing in-memory snapshot ring; the persistence pipeline,
RPC/sync surface and consensus paths are unchanged.

Motivation

• Today's flat DB caps reorg depth at MaxReorgDepth (~256). Every in-memory
  snapshot keeps its diff in RAM, so raising that ceiling is proportionally
  expensive in heap.
• Use cases that need to serve historical-tip queries or replay across a large
  reorg window (long-finality CL designs, archive-lite serving) need a way to
  keep many more snapshots cheap to retain and cheap to read.
• The persisted tier is an mmap-backed format: paged access on the read path
  means cold snapshots cost only file-descriptor + index metadata until queried,
  and a PageResidencyTracker per arena drives madvise(DONTNEED) so the
  resident set is bounded by a configured page-cache budget rather than by total
  snapshot bytes on disk.

High-level design

Mirrors the in-memory Snapshot pipeline

A PersistedSnapshot is the on-disk twin of an in-memory Snapshot: same
(From, To) semantics, same StateId-keyed lookups, same hierarchical
compaction policy. The in-memory compactor keys merge decisions off the
lowest-set-bit of the block number, giving 1×, 2×, 4×, …, CompactSize× layers;
the persisted-tier compactor uses the same lowest-set-bit rule
(CompactionSchedule) so the persisted-snapshot stack is just a continuation of
the in-memory snapshot stack across the tier boundary. The number of live
persisted-snapshot layers is therefore logarithmic in the configured reorg
depth rather than linear. Reads fall through from the in-memory Snapshot to
the PersistedSnapshot at the same block/root once the in-memory window has
rotated past it; eviction and compaction follow the same mirror.

Single-level SortedTable on-disk format

Persisted snapshots are not RocksDB-shaped. The original #11663 used a
purpose-built HSST (Hierarchical Static Sorted Table) with nested index
variants. This branch replaces HSST with a single-level SortedTable: a
flat, two-level block layout (fixed-size data blocks + a tail index block,
located via a fixed footer). Lookups are LevelDB-style — the index block
locates the data block, then DataBlockReader.SeekCeiling binary-searches the
data block's restart table and scans forward to the first key ≥ target. It is
written by a streaming builder that enforces globally-ascending key order, so
it never buffers the whole record set. This is dramatically simpler than the
multi-variant HSST while keeping the seek-cheap, mmap-friendly, point-queryable
properties.

Key layout. The table is a single ascending byte-sorted map; all
persisted-snapshot columns are folded into one keyspace by fully-verbose keys
whose leading column / sub-column tag bytes are stored as 255 − tag, so
plain ascending byte order reproduces the columnar emission order (outer columns
and per-address sub-tags descend while the entity bytes ascend). Key shapes
(tag bytes shown as their stored 255 − tag value):

Ref-id         00 + blobArenaId(u16 BE)                       — referenced blob arenas, sort first
Storage node   FA + addrHash(20) + {FF top | FE compact | FD fallback} + path
State node      {FD top | FC compact | FB fallback} + path
Self-destruct   FE + addr(20) + FD
Slot            FE + addr(20) + FE + slot(32 BE)
Account         FE + addr(20) + FF
Metadata        FF + name(10, NUL-padded)

• An address's account, self-destruct and slots share the FE column under the
  20-byte address, split by a per-address sub-tag (FD self-destruct < FE
  slot < FF account) — self-destruct sorts ahead of the slots it filters so
  the merger resolves the truncation barrier before streaming the slots.
• Trie-node paths are length-tiered — 4-byte (≤ 7 nibbles) / 8-byte (≤ 15) /
  33-byte fallback (32-byte path hash + length byte) — so short hot paths stay
  compact.
• Ref-id records (column 00) sort below every real column, so the blob arenas
  a snapshot references iterate cheaply from the table start.

Delta encoding. Within a block, records are front-coded: each stores a
common-prefix length cp against the previous key plus the changed key suffix.
cp == 0 is a restart (a full key), forced at least every restartInterval
records (and wherever adjacent keys share no leading byte); the restart table
indexes every restart for the binary search. The index block's values — u48
data-block byte offsets — are delta-coded the same way: each keeps the high
little-endian bytes of the previous offset and stores only the changed low
bytes, reset to 0 at each restart.

Trie RLP split out into separate blob arenas

The biggest size win comes from not storing trie-node RLP inside the
persisted-snapshot table. Each snapshot stores only compact NodeRefs where the
RLP would have lived; the RLP bytes themselves live in append-only blob arena
files addressed by those refs, mmap'd and tracked by a PageResidencyTracker
(with a punch-hole reclaim path). This shrinks the per-snapshot table by roughly
an order of magnitude vs. inlining RLP, and:

• compaction can rewrite the small SortedTable cheaply without ever rewriting
  the RLP — compacted snapshots' NodeRefs still point at the original blobs;
• blob arena files are refcounted per file (each snapshot holds a lease; a
  file is deleted only when no live snapshot still references it), so long-lived
  trie nodes survive arbitrarily many compactions without being copied.

In-memory bloom filters skip cold snapshots

Each persisted snapshot ships with a tiny in-RAM bloom filter
(PersistedSnapshotBloomBitsPerKey, default 14.0 bpk) covering
address/slot/self-destruct keys plus state/storage trie-node paths. Lookups walk
the snapshot stack newest→oldest and skip any snapshot whose bloom says
"definitely not here" without touching its mmap'd table. Large-compaction blooms
are shared across the snapshots they contain to bound bloom RAM.

Multi-tier persisted snapshots + hierarchical compaction

Persisted snapshots are bucketed (base / small-compacted / large-compacted /
CompactSized) and merged hierarchically with (To − From) doubling at each
level, bounded by PersistedSnapshotMaxCompactSize. Compaction only rewrites
SortedTables; blob arenas are inherited through the NodeRef indirection. Large
(> CompactSize) merges run on a separate background worker so they don't block
the per-block batch worker.

Lifecycle / GC

• On Snapshot → PersistedSnapshot conversion, trie nodes are marked persisted
  and pruned from the in-memory trie.
• PageResidencyTracker per arena drives madvise(DONTNEED) (and a background
  keep-warm pass) so the resident set tracks the configured page-cache budget;
  reported to the GC and exposed as per-tier gauge metrics.
• LongFinalityMaxReorgDepth is the hard tail — once exceeded, persistence into
  the RocksDB tier is forced to bound memory.

Config surface

New / relevant keys on IFlatDbConfig (opt-in feature, conservative defaults):

Key	Default	Purpose
EnableLongFinality	false	Feature gate.
LongFinalityMaxReorgDepth	90000	Force-persist backstop when long finality is on (the persisted tier serves deep reorgs).
MaxReorgDepth	256	Force-persist backstop when long finality is off.
MinReorgDepth	128	Keep at least this much state in memory before the finalized-persistence trigger fires.
MaxInMemoryBaseSnapshotCount	128	In-memory base snapshots before conversion to the persisted tier kicks in.
CompactSize	32	Persistence/compaction boundary (blocks per persisted step).
PersistedSnapshotMaxCompactSize	1048576	Hierarchical-compactor ceiling for the persisted tier.
ArenaFileSizeBytes	1 GiB	Arena file ceiling.
PersistedSnapshotDedicatedArenaThresholdBytes	1 GiB	At/above this estimated size, a write goes to its own dedicated arena file.
PersistedSnapshotArenaPageCacheBytes	4 GiB	Resident-page budget backing the PageResidencyTracker (0 disables it).
PersistedSnapshotPunchHoleOnReclaim	true	fallocate(PUNCH_HOLE) to free disk blocks on reclaim (auto-disabled if unsupported).
PersistedSnapshotBloomBitsPerKey	14.0	Per-snapshot bloom precision (0 = disabled).
ValidatePersistedSnapshot	false	Debug-only: validate persisted vs in-memory after conversion.

Observability

• Per-tier gauges: arena-file count, mapped bytes, page-tracker resident/metadata
  bytes, bloom memory, per-tag reservation count/bytes, live persisted-snapshot
  bloom count.
• PageResidencyTracker eviction / refresh / inline-dispatch counters.

Compatibility

• Feature is disabled by default; with EnableLongFinality=false none of the
  persisted-snapshot code runs. RocksDB column-family layout is preserved.
• The persisted-snapshot on-disk format and SnapshotCatalog schema are
  versioned; pointing at an incompatible older directory trips a
  "wipe and resync" error.
• No protocol / JSON-RPC surface changes. No consensus-path changes.

Performance

The SortedTable format targets the same envelope as the HSST design in #11663
(opt-in tier adds no measurable overhead when the persisted tier is idle; deeper
stacks cost the steady price of walking more snapshots + background bloom/table
writes + hierarchical compaction). Up-to-date numbers are produced by the
in-repo reproducible-benchmark workflow
(.github/workflows/run-expb-reproducible-benchmarks.yml).

Types of changes

• Bugfix
• New feature
• Breaking change
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

Notes on testing

• Nethermind.State.Flat.Test covers the SortedTable round-trips (builder /
  reader / hash index), persisted-snapshot build/read/merge/compact paths,
  page-tracker eviction, bloom correctness, and the persistence/compaction/reorg
  policies end-to-end. Current status after the master merge: 777 passed, 4
  pre-existing skips.
• ValidatePersistedSnapshot=true cross-checks every persisted snapshot against
  the source in-memory snapshot at conversion time.
• Recommended local validation: enable EnableLongFinality=true and sync mainnet
  past a window larger than MaxReorgDepth; confirm the persisted-snapshot count
  grows then plateaus once compaction kicks in, the PageResidencyTracker
  resident bytes track the configured budget, and there are no
  Invalid Block / Exception lines.

Documentation

Requires documentation update

• Yes
• No (user-facing docs can come in a follow-up once the feature flips default)

Requires explanation in Release Notes

• Yes — "FlatDB: opt-in long-finality persisted-snapshot tier
  (FlatDb.EnableLongFinality). See config keys above."

Remarks

• This PR is intentionally large because the persisted-snapshot tier is
  meaningful only end-to-end (format + write path + read path + compactor +
  lifecycle + observability). Splitting it further would introduce dead, unwired
  code into master between PRs.
• Replaces feat(FlatDB): long finality support with persisted snapshots #11663; the draft refactor(flat): replace persisted-snapshot HSST with single-level sorted table #12100 (HSST→SortedTable split) is folded in here.

[svlachakis]

@claude please check the following review

---

🔴 Critical

C1 — A lease keeps the mapping mapped; it does NOT protect page contents from concurrent reclaim. This is the load-bearing defect and it recurs three ways:

• madvise(MADV_DONTNEED) races active readers. ArenaManager.DispatchInline → arena.AdviseDontNeed(offset, pageSize) (ArenaManager.cs:527-536, ArenaFile.cs:177-183) discards pages of the shared mapping that ArenaByteReader reads through (ArenaReservation.cs:145-146, ArenaByteReader.cs:47). MADV_DONTNEED on a file-backed shared mapping is destructive; a reader mid-CopyTo on that page can tear. Nothing consults a per-reader/per-page refcount.
• fallocate(PUNCH_HOLE) in ArenaReservation.CleanUp (ArenaReservation.cs:199-222) frees blocks over [Offset, Footprint) on a surviving shared file while another reservation/reader over the same bytes reads zeros back — silent state corruption (a zeroed RLP node), not a crash.
• SetFileLength(0) in BlobArenaManager.TryResetOrphanedFrontier (BlobArenaManager.cs:288, reached from PersistedSnapshot.cs:366) and File.Delete in SweepUnreferenced (BlobArenaManager.cs:230-247) truncate/delete a file gated only on HasOnlyManagerLease — but the blob read path (GetFile + RandomRead, BlobArenaManager.cs:181-189) deliberately takes no lease, so an in-flight pread on a shared base blob (shared across many snapshots + all compactions that inherit its NodeRefs) can be truncated/deleted underneath it.
  Fix direction: reads/advises must hold a real lease (TryLeaseFile/Dispose around the read session), OR destructive reclaim (DONTNEED/PUNCH_HOLE/truncate/delete) must be provably quiescent (no possible reader), consulted via a per-range reader refcount. As written, HasOnlyManagerLease does not imply "no reader."

C2 — ArenaManager.MarkDead disposes the file mid-sequence → use-after-free in CleanUp. MarkDead calls file.Dispose() when DeadBytes >= Frontier (ArenaManager.cs:260), but ArenaReservation.CleanUp then keeps operating on that file — TryPunchHole/FadviseDontNeed/ForgetTrackerRange (ArenaReservation.cs:213-217) then a second Dispose (line 221). A concurrent transient lease-holder (TouchWarmPages, ArenaManager.cs:444) means this reservation isn't guaranteed to be the last lease, so MarkDead's Dispose can drive 1→0 and run CleanUp (unmap + delete) while the reservation is still syscalling the same handle. MarkDead's own docstring says its "sole job is set/dict/metric mutation" — honor that; drop the manager lease after the reservation finishes its file-side work.

C3 — SortedTable readers trust on-disk values with incomplete bounds checks → silent wrong hit. These read from mmap'd, possibly-corrupt/truncated files:

• SpanByteReader.TryRead (Io/SpanByteReader.cs:20) — CONFIRMED. (ulong)(_data.Length - output.Length) underflows when output.Length > _data.Length; the guard passes and line 21 Slice throws instead of returning false. Reachable from the first header read of any truncated block (DataBlockReader.cs:74). Line 27 (PinBuffer) proves the correct form (Offset + Length > Length). Fix: if (output.Length > _data.Length || (ulong)offset > (ulong)(_data.Length - output.Length)) return false;
• Front-coding cp not validated against the running key length (DataBlockReader.cs:137-141, IndexBlockReader.cs:118-123, SortedTableEnumerator.cs:69-73): only cp + suffixLen <= keyBuf.Length is checked, not cp <= prevKeyLen. A corrupt cp reconstructs a key from stale bytes and can make the forward scan return a value Bound at attacker/garbage offsets.
• Restart offsets used as read positions without range-checking against the block (DataBlockReader.cs:119,128, IndexBlockReader.cs:91,102); restart table bounded against reader.Length instead of the block (DataBlockReader.cs:107); indexOffset upper bound allows the index header to straddle the footer (SortedTable.cs:95-96).
  Fix: validate cp <= prevKeyLen, assert restart records have cp==0, and bounds-check every on-disk offset against [RecordsStart, RecordsEnd) before dereferencing — ThrowCorrupt otherwise. A silent wrong hit on a state read is worse than a crash.

C4 — Torn read of _currentPersistedStateId — CONFIRMED. (evidence above). Guard it with a Lock exactly as _lastCommittedStateId is, or publish an immutable boxed StateId via Volatile.Read/Write. Also remove the lock-free write at PersistenceManager.cs:72.

---

🟠 High

• H1 — ArenaFile.Frontier/DeadBytes are plain auto-props mutated off-lock. ArenaWriter.Complete writes Frontier outside the manager lock (ArenaWriter.cs:49) while MarkDead reads it under the lock to make the DeadBytes >= Frontier survival decision (ArenaManager.cs:253-254) → torn/stale read → premature file delete or permanently-leaked file. Fold the Frontier write into the lock-held OnWriteCompleted.
• H2 — DispatchInline calls AdviseDontNeed with no lease (ArenaManager.cs:527-536) while TouchWarmPages correctly leases (:444). Inconsistent; races _basePtr = null in CloseMmap.
• H3 — _residentPages accounting not conserved across eviction; GC-pressure high-water never reconciled (PageResidencyTracker.cs:239-254, 316-321). Forget misses when a slot was reused by eviction → resident-bytes gauge drifts, and reported GC pressure pins at peak for process lifetime. For a large ArenaPageCacheBytes budget that's the whole cache reported as permanent pressure.
• H4 — In-memory→persisted conversion removes states not verifiably registered (PersistenceManager.cs:293-316). When TryLeaseInMemoryState fails (line 298) the state is skipped for conversion but the removal loop (311-316) drops it from in-memory anyway → state in neither tier → GatherReadOnlySnapshotBundle can throw "state no longer exists" for a state that should have persisted. Remove only the states whose persisted base is confirmed live.
• H5 — allStateIds ArrayPoolList leaks on cancellation (PersistenceManager.cs:285-318): not in a using; Parallel.ForEach with _cts.Token throws before the compactor hand-off at 318; finally only disposes compacted.
• H6 — Catalog writes are not durable and unversioned. SnapshotCatalog.Add uses a default (non-synced) RocksDB Set (SnapshotCatalog.cs:48) and the entry is a Pack=1-blitted struct with no version/magic (SnapshotCatalog.cs:22-35,62-98) — a layout change or corrupt-but-plausible row blits into a SnapshotLocation the loader trusts. Add WriteOptions.Sync (or with no version/magic (SnapshotCatalog.cs:22-35,62-98) — a layout change or corrupt-but-plausible row blits into a SnapshotLocation the loader trusts. Add WriteOptions.Sync (or checkpoint-before-prune) and an explicit version byte; reject unknown versions loudly instead of silently continue-ing. Also line 48 .ToArray() allocates per write — use the ReadOnlySpan Set overload.

---

🟡 Medium (selected)

• CompactRange leaks reservation + mergedBloom if Fsync/catalog.Add/bloom-ctor throws (PersistedSnapshotCompactor.cs:340-364) — no rollback between Complete() and the using PersistedSnapshot; finally only disposes sources. Over repeated disk-full/IO errors this exhausts native memory silently.
• ProcessCompactBatch Parallel.ForEach aborts the whole batch on one failure (PersistedSnapshotCompactor.cs:165-166) → the CompactSized snapshot + boundary enqueue are skipped → persisted tier silently stops advancing (unbounded growth). Wrap each DoCompactSnapshot in its own try/catch like RunBoundaryCompactor already does.
• WholeReadSession ctor leaks a lease if OpenWholeView throws after AcquireLease (WholeReadSession.cs:39-40); Dispose guard uses a non-atomic bool (:53-61) — use Interlocked.Exchange.
• SortedTableEnumerator is a copyable mutable struct owning native NativeMemoryList (SortedTableEnumerator.cs:23,45,116) — any by-value copy double-frees / use-after-frees native memory. Make it a class or enforce move-only.
• Environment.Exit inside ConvertAndRegister (PersistedSnapshotLoader.cs:249-262) runs while _persistenceLock is held and readers may be mid-RandomRead on mmap → fault during teardown. Signal upward instead.
• Endianness: builder writes explicit LE (BinaryPrimitives) but readers reinterpret via MemoryMarshal host-order (DataBlockReader.cs:38, SortedTable.cs:89, etc.) — correct only on LE hosts; decode symmetrically.
• TrackPages narrows a 64-bit absolute page index to uint with no release guard (ArenaReservation.cs:108-109) — silent aliasing above ~16 TiB/arena; ForgetTrackerRange only debug-asserts. Make consistent.
• Swallowed failures: File.Delete blanket catch {} (BlobArenaFile.cs:187-190) and ignored posix_fadvise errno (PosixReclaim.cs:67-91) hide disk leaks / persistent reclaim failure — at least log/meter.

⚪ Low / cleanup

Zero-alloc on the read fall-through is otherwise strong (stackalloc key bufs, ref-struct enumerators, no LINQ/closures on lookup), with residual new byte[] per resolved RLP (PersistedSnapshot.cs:214,291 — pool the >256B account temp); metrics Timer.Dispose() doesn't drain the in-flight callback vs _pageTracker.Dispose() (ArenaManager.cs:456-461); DRY across the 5 near-identical probe loops in PersistedSnapshotStack; CompactRange size/flag-arg (SRP); dead pageCount local (ArenaManager.cs:293); SmallRefCountingDisposable terminal CAS should Debug.Assert rather than silently skip CleanUp (:114-117).

---

[claude]

Claude finished @svlachakis's task in 7m 55s —— View job

---

Verification of the submitted review

I traced every claim against the actual code on flat/simpler-long-finality (readers verified against their producers/FORMAT.md, concurrency claims against the lease/refcount model). Verdicts below; each is grounded in file:line evidence I read directly.

TL;DR: The reviewer found real bugs — but the four items marked "Critical" (C1a/b/c, C2) are all false positives that rest on a misreading of MADV_DONTNEED semantics and of the two-lease refcount model. The genuinely-actionable findings are C3a, C4, H1, H4, H5, H6 plus several Mediums. None of these are acknowledged-with-rationale yet, so mergeable: no.

---

🔴 Critical

Claim	Verdict	Evidence
C1(a) MADV_DONTNEED tears active readers	❌ REFUTED	The mapping is a read-only, file-backed shared mmap (ArenaFile.cs:160-161). MADV_DONTNEED on a shared file mapping is non-destructive — the next access re-faults the same bytes from the file (it only zero-fills for anonymous/private mappings). Arena files are append-only, so a live range is never overwritten. A racing reader takes at most a minor fault + re-reads identical data. The code even documents this (ArenaFile.cs:220-222 "redundant… on Linux for shared mappings"). The lease guards against unmap, which DONTNEED never does.
C1(b) PUNCH_HOLE on a surviving shared file → reader reads zeros	❌ REFUTED	CleanUp punches only [Offset, Offset+Footprint) — this reservation's own range — and only inside CleanUp, which the refcount base runs exactly once at lease-count 0 (SmallRefCountingDisposable). Offset is page-aligned and Footprint = Min(RoundUpToOsPage(Size), …) (ArenaReservation.cs:39), so ranges are page-disjoint from other live reservations. No live reader references the punched bytes.
C1(c) blob pread truncated/deleted underneath it	❌ REFUTED	The owning PersistedSnapshot leases every referenced blob file at construction (PersistedSnapshot.cs:105) and holds it until CleanUp (:363). All preads happen while the snapshot is alive ⇒ refcount ≥ 2, so HasOnlyManagerLease (== _leases==1) is false and both SweepUnreferenced (:239) and TryResetOrphanedFrontier (re-checks under lock, :265) skip. (Minor unrelated caveat: TryLeaseFile is lock-free, so there is a reset-vs-new-lease window in TryResetOrphanedFrontier — not the claimed pread race.)
C2 MarkDead disposes file mid-sequence → use-after-free	❌ REFUTED	Open() builds an ArenaReservation that acquires its own file lease in its ctor (ArenaManager.cs:220), distinct from the manager's dict-slot lease. In CleanUp, MarkDead:260 drops only the manager lease; the reservation still holds its own until :221, so the count can only reach ≥1, never 0 — ArenaFile.CleanUp (unmap/delete) cannot run during the :206-218 syscalls. TouchWarmPages only adds a transient lease. This is the author's earlier "lease-released-once" defense, and it holds here.
C3(a) SpanByteReader.TryRead underflow → throws instead of miss	✅ CONFIRMED	SpanByteReader.cs:20: (_data.Length - output.Length) is int arithmetic; when output.Length > _data.Length it goes negative, and (ulong) widens it to a huge value, so the guard passes and :21 Slice throws ArgumentOutOfRange. PinBuffer:27 uses the correct Offset + Length > Length form. Trigger: a truncated block that begins within 4 bytes of reader.Length → the 5-byte header read at DataBlockReader.cs:74 throws instead of returning a clean miss.
C3(b) front-coding cp not validated vs running key length	✅ CONFIRMED (bounded)	The post-2b4d7063 guard only checks cp + suffixLen <= keyBuf.Length (DataBlockReader.cs:137, IndexBlockReader.cs:118, SortedTableEnumerator.cs), not cp <= prevKeyLen. A corrupt cp reconstructs a key from stale buffer bytes → no OOB/crash; for a point lookup the garbage-prefixed key fails the equality check → degrades to a miss, but a full scan/merge (CurrentKey) yields garbage keys. Robustness gap, not memory-unsafety.
C3(c) on-disk offsets not range-checked against the block	✅ CONFIRMED (mostly bounded)	No [blockStart,end) validation of restart offsets (DataBlockReader.cs:119,128; IndexBlockReader.cs:91,102); restart table bounded vs reader.Length not the block (:107); indexOffset may straddle the footer (SortedTable.cs:95-96). TryRead prevents true OOB, but the SpanByteReader.PinBuffer path throws on a corrupt large SuffixLength rather than a clean miss.
C4 torn read of _currentPersistedStateId	✅ CONFIRMED	StateId is a 40-byte struct (ulong + ValueHash256) → non-atomic. Written under _persistenceLock on the persistence worker (:240/248/406/418/429) and lock-free at :72/429; read lock-free from query threads (FlatDbManager.cs:166/354/460, FlatFullStateFinder, FlatWorldStateManager). A reader can observe a torn (BlockNumber, StateRoot) pair. (Reviewer's supporting detail is slightly off: the guard is a SemaphoreSlim _persistenceLock, and there is no _lastCommittedStateId field — but the core race is real.)

---

🟠 High

Claim	Verdict	Evidence
H1 Frontier/DeadBytes mutated off-lock	✅ CONFIRMED	Plain auto-props, no volatile/interlocked (ArenaFile.cs:69,76). ArenaWriter.Complete writes Frontier outside the manager lock (ArenaWriter.cs:49); MarkDead reads it under the lock (ArenaManager.cs:254). On a shared arena, a later writer's off-lock Frontier write races an earlier reservation's MarkDead — the lock does not serialize them. long is atomic on x64 so it's a stale/ordering hazard, not bit-tearing. The deeper exposure: the writer holds no independent lease, so a stale DeadBytes >= Frontier could delete a file mid-append. Fold the Frontier write into the lock-held OnWriteCompleted.
H2 DispatchInline advises with no lease vs _basePtr=null	⚠️ REFUTED as a bug	The lease asymmetry is real, but intentional and benign: AdviseDontNeed passes the pointer only to the madvise syscall (ArenaFile.cs:182), which returns an errno on a bad address — it does not fault. TouchByte does a userspace load (:215) that would SIGSEGV, hence it leases. Documented at ArenaFile.cs:210-214. Worst case is a harmless failed syscall.
H3 resident accounting drift / GC-pressure pin	⚠️ PARTIAL	Resident accounting is conserved (refuted): _residentPages increments only on empty-slot insert (:216) and decrements only on a successful Forget CAS (:320); eviction reuses a valid slot without touching it. GC pressure is a monotonic high-water never reduced until Dispose (:316-319, released :389-393) — real but bounded by MaxCapacity*pageBytes and by-design, low severity.
H4 skipped-conversion states removed from memory anyway	✅ CONFIRMED (structural)	Conversion is conditional on TryLeaseInMemoryState (PersistenceManager.cs:298) but the removal loop (:311-316) is unconditional over allStateIds. If a lease fails for a genuinely-present base (race), that state is dropped from both tiers without conversion → in neither tier. Whether it strands live state depends on the lease-failure cause, which the code doesn't distinguish.
H5 allStateIds ArrayPoolList leaks on cancellation	✅ CONFIRMED	new(64) at :285 is not in a using; the finally (:320-323) disposes only compacted. Ownership transfers at :318 EnqueueAsync. Any throw before :318 — Parallel.ForEach cancellation (:293, _cts.Token), GetStatesAtBlockNumber, the removal loop, or a ConvertAndRegister fault (:301) — leaks the pooled buffer.
H6 catalog writes not durable & unversioned	✅ CONFIRMED	EntryBytes is a Pack=1 blit with no version/magic (SnapshotCatalog.cs:22-35); Add uses a plain non-synced _db.Set (:48) with a per-write .ToArray() alloc; the loader validates only key/value length and the tier byte before trusting all other fields (:66-98).

---

🟡 Medium (all confirmed unless noted)

• ✅ CompactRange leaks reservation + mergedBloom if Fsync/catalog.Add/ctor throws between Complete() and the using (PersistedSnapshotCompactor.cs:354-364; finally disposes only sources).
• ✅ ProcessCompactBatch Parallel.ForEach (:165-166) has no per-item try/catch, so one failure aborts the batch and skips the CompactSized snapshot + boundary enqueue → persisted tier silently stalls. RunBoundaryCompactor (:187-197) already wraps per-item.
• ✅ WholeReadSession ctor leaks a lease if OpenWholeView throws after AcquireLease (:39-40); dispose guard is a non-atomic bool (:54-55).
• ✅ Endianness — writer LE (BinaryPrimitives) vs reader host-order (MemoryMarshal), e.g. DataBlockReader.cs:38, SortedTable.cs:89. Real but documented as LE-only; only LE targets (x64/ARM64) are supported → low.
• ✅ Environment.Exit in ConvertAndRegister (PersistedSnapshotLoader.cs:261) runs under _persistenceLock with readers possibly mid-RandomRead, and skips the finally dispose. Gated behind the default-off ValidatePersistedSnapshot, documented as an intentional bisect-friendly exit.
• ⚠️ TrackPages narrows a 64-bit page index to uint with no guard (ArenaReservation.cs:108-109), unlike ForgetTrackerRange which at least Debug.Asserts (ArenaManager.cs:297). Bounded-safe in practice (~16 TiB/arena), but inconsistent.
• ✅ Swallowed failures — File.Delete blanket catch {} (BlobArenaFile.cs:189) and ignored posix_fadvise errno (PosixReclaim.cs:72,90). The fadvise ones are non-critical cache hints; Fsync/PunchHole do inspect errno.
• ❌ SortedTableEnumerator copy double-free — REFUTED: it's a struct owning a NativeMemoryList (a sealed class), whose Dispose is idempotent, so a by-value copy can't double-free native memory. The mutable-struct-owning-IDisposable pattern is a use-after-dispose footgun worth noting, but not the claimed double-free.

---

Bottom line

• Confirmed & worth fixing: C3(a) (return the miss instead of throwing — mirror PinBuffer's guard), C4 (publish StateId under the lock or via an atomic boxed reference / Volatile), H1 (fold the Frontier write into the lock-held OnWriteCompleted; the writer holding no lease is the root cause), H4, H5, H6, and the confirmed Mediums (CompactRange rollback, per-item try/catch in ProcessCompactBatch, WholeReadSession lease/atomic guard).
• Bounded/robustness-hardening: C3(b)/(c) (validate cp <= prevKeyLen, bounds-check on-disk offsets against the block, route corruption to ThrowCorrupt), endianness (LE-only, documented), Environment.Exit (debug-gated), TrackPages narrowing.
• False positives: C1(a), C1(b), C1(c), C2, H2, and the enumerator double-free — each rests on either the non-destructive semantics of MADV_DONTNEED/lease-scoped ranges on file-backed shared mappings, or on missing the two-lease refcount model (reservation lease vs manager slot lease).

I only verified the review here (no code changed). If you'd like, I can open a follow-up commit implementing the confirmed fixes (C3a, C4, H1, H5, H6 are small and localized).
· branch flat/simpler-long-finality

[asdacap]

@claude reevaluate

[claude]

Claude finished @asdacap's task in 3m 40s —— View job

---

Re-evaluation — status after the follow-up fix commits

I re-traced every previously-confirmed finding against the current tree (4a939d61, cb2b69a1, and the earlier follow-ups 2b39a2fd…425b8403), verifying each fix against its producer/read-path rather than the commit message.

• Verify C3a / C3b / C3c reader hardening
• Verify C4, H1, H5, WholeReadSession, ProcessCompactBatch
• Check H4, H6, CompactRange leak status
• Check asdacap's design comments (slot streaming, bloom builder, config defaults)

---

✅ Confirmed landed & correct

Finding	Verdict	Evidence
C3a SpanByteReader.TryRead underflow	✅	SpanByteReader.cs:23 — `output.Length > _data.Length
C3b cp vs previous key length	✅	DataBlockReader.cs:145, IndexBlockReader.cs:126, SortedTableEnumerator.cs:73 — reject cp > prevKeyLen, restart (cp==0) passes, routed to ThrowCorrupt.
C3c restart-offset bounds	✅	CheckRestartOffset at DataBlockReader.cs:168-173 / IndexBlockReader.cs:157-162, called before every dereference against [recordsStart, RecordsEnd).
C4 torn StateId read	✅	PersistenceManager.cs:67 now a StrongBox<StateId> published via Volatile.Read/Write (:71-72); all read/write sites route through the accessor, no lock-free field write remains.
H1 off-lock Frontier write	✅	ArenaWriter.Complete computes newFrontier locally and hands it off; ArenaManager.OnWriteCompleted:183 writes it under _lock, serialized against MarkDead's survival read.
H5 allStateIds pool leak	✅	ConvertCompactedRange guards with handedOff + finally dispose (PersistenceManager.cs:294,337-340).
WholeReadSession	✅	ctor releases lease on OpenWholeView throw; Dispose idempotent via Interlocked.Exchange.
ProcessCompactBatch	✅	per-item try/catch around DoCompactSnapshot (PersistedSnapshotCompactor.cs:167-178), mirrors RunBoundaryCompactor.
Slot streaming (your review)	✅	PersistedSnapshotScanner.cs — slots streamed via SlotEnumerable over the shared ref _inner cursor, no buffering, no per-storage re-seek.
Bloom address key (your review)	✅	PersistedSnapshotBloomBuilder.cs:53-59 — addrKey added once per address, not per record.
EnableLongFinality default (your review)	✅	now true (FlatDbConfig.cs:27); MaxInMemoryBaseSnapshotCount = 160 = 128 + CompactSize.

---

🟠 Still open — three items remain

H4 — conversion-skipped state still removed from memory (not fixed)
In PersistenceManager.ConvertCompactedRange, conversion is gated on TryLeaseInMemoryState (PersistenceManager.cs:313), but the removal loop (:326-331, RemoveAndReleaseInMemoryKnownState for both InMemoryCompacted and InMemoryBase) is unconditional over allStateIds. If a lease fails for a state that wasn't registered into the persisted tier, it's dropped from in-memory anyway → present in neither tier, and a later GatherReadOnlySnapshotBundle can throw "state no longer exists". Removal should be gated on confirmed persisted registration. Fix this →

H6 — catalog writes not durable and version-less (not fixed)
SnapshotCatalog.Add still uses a non-synced _db.Set(...) with a per-write .ToArray() (SnapshotCatalog.cs:48), and EntryBytes is a Pack=1 blit with no version/magic byte (:22-35). The loader validates only key/value length + the tier byte, so a layout change or plausible-but-corrupt row blits straight into a trusted SnapshotLocation. Add a version byte (reject unknown loudly) and a synced write / checkpoint-before-prune. Fix this →

CompactRange leak on IO/ctor throw (confirmed Medium, not fixed)
PersistedSnapshotCompactor.cs (~:366-376): between arenaWriter.Complete() (returns reservation, :366) and the using PersistedSnapshot (:376), a throw from reservation.Fsync() (:373), _catalog.Add(...) (:375), or the RefCountedBloomFilter/PersistedSnapshot ctor leaks both reservation and mergedBloom — the finally (:402-405) disposes only sessionsList. Over repeated disk-full/IO errors this exhausts native memory. Wrap the commit region in try/catch that disposes reservation + mergedBloom on failure. Fix this →

---

🟡 One inconsistency with your own stated preference

You commented "It should throw on any corruption with clear message." — the C3 hardening now does that everywhere except one site: SortedTableEnumerator.cs:100 handles the exact same proven-corrupt condition as its sibling with if (valChangedLen > 6) return false; // > u48 ⇒ corrupt, whereas IndexBlockReader.cs:122-123 treats it as fatal via SortedTable.ThrowCorrupt. For consistency the enumerator site should also ThrowCorrupt. Fix this →

(Note: the intentional split — a read that runs off the end → clean miss, a structurally-inconsistent value inside an already-read record → throw — is otherwise applied consistently; this one site is on the wrong side of it.)

---

Verdict

All Critical and most High findings are resolved and verified correct. Remaining before merge: H4 (state-in-neither-tier — the most consequential of the three, a correctness bug reachable on a lease race), H6 (catalog durability/versioning), the CompactRange native-memory leak, and the one SortedTableEnumerator corruption-handling inconsistency. All four are small and localized.

These are review findings only — I haven't changed code. Say the word and I'll implement any subset (H4 + CompactRange + the enumerator throw are one-liners; H6 needs a version byte + synced write).
· branch flat/simpler-long-finality

[asdacap]

Catalog write use rockcsdb so it inherit rocksdb WAL. Convert is suppose to remove both base and compacted. THe compacted one will be replaced with persisted variant. Its also not necessary for correctness. Base compactionmust not fail though.

[svlachakis]

This is close to a copy of RefCountingDisposable, and I think the two have already diverged: the in-loop current <= NoAccessors check here fixes a hazard the base still has. If I'm reading RefCountingDisposable.ReleaseLeaseOnce correctly, an over-release racing a legitimate last release can retry with current == 0 and win the CAS 0 → -1 through the subtract path, the real last releaser's NoAccessors → Disposing CAS then fails and CleanUp is silently skipped.

Since this is the trickiest lock-free code in the PR, I think it would be worth either sharing the state machine between the two (parameterised over the counter storage), or at least porting this guard back to the base, otherwise the next fix lands in only one of the copies.

[asdacap]

Good catch — that base hazard is real. Ported the guard to RefCountingDisposable in 7e60f40: the release loop now re-validates current <= NoAccessors on every CAS retry. Kept the two classes separate rather than parameterising over the counter storage — the fields differ (cache-line-padded vs inline long) and unifying would put a virtual ref-accessor on the lease hot path.

— posted by asdacap's agent (Claude Fable 5)

[svlachakis]

Since these loops are on the per-account/per-slot read path: _snapshots[i] is indexed twice per iteration (bloom + lookup). Might be worth hoisting a local PersistedSnapshot snap = _snapshots[i]; ?

[asdacap]

Done in 5c55b6f — all five probe loops hoist the snapshot into a local.

— posted by asdacap's agent (Claude Fable 5)

[svlachakis]

ArenaReservation only uses the reclaim side of this interface (PageTracker, QueueEviction, ForgetTrackerRange, MarkDead, TryPunchHole), but it also gets Initialize, CreateWriter, Open and Dispose. I think splitting the reclaim callbacks into a narrower interface (e.g. IArenaReclaim) and having the reservation take that would keep the read side away from the lifecycle surface. Not a blocker.

[asdacap]

Done in 7e60f40 — the reclaim callbacks moved to a new IArenaReclaim (which IArenaManager extends), and ArenaReservation now takes the narrow interface.

— posted by asdacap's agent (Claude Fable 5)

[asdacap]

Correction: the split was reverted in 17a5996 at the author's request — ArenaReservation stays on IArenaManager.

— posted by asdacap's agent (Claude Fable 5)

[svlachakis]

PutSpan would avoid the .ToArray() copy here?

[asdacap]

Done in 5c55b6f — PutSpan now.

— posted by asdacap's agent (Claude Fable 5)

[svlachakis]

I think the new byte[bLenInt] fallback is unreachable in practice? slim account RLP tops out around ~110 bytes, well under the 256-byte stackalloc. If it stays as a safety net maybe ArrayPool.Shared.Rent would match better with the pattern used elsewhere?

[asdacap]

Done in 5c55b6f — since the fallback is unreachable for well-formed data, an oversized value now throws as corruption (SortedTable.ThrowCorrupt) instead of allocating.

— posted by asdacap's agent (Claude Fable 5)

[svlachakis]

Maybe better would be dropping the finalizer? It calls NativeMemory.AlignedFree(_slots) on memory that TryTouch/ContainsPage read lock-free, and a finalizer can run mid call once this is no longer referenced so a missed Dispose becomes a potential native use-after-free rather than a plain leak.

Since ArenaManager always disposes the tracker explicitly, the finalizer only seems to add the worse failure mode.

Am I maybe missing something?

[asdacap]

You are right — dropped the finalizer in 5c55b6f; a missed Dispose is now a plain leak instead of a potential native use-after-free.

— posted by asdacap's agent (Claude Fable 5)

[wurdum]

The small nitpick for AI agent fix because PersistedSnapshot:TryGetAccount already has these defensive range checks to ensure no bit-rot can pass unspotted. Feel free to ignore, though

DataBlockReader.cs (SeekCeilingCore, around line 153):

long valueStart = keyStart + suffixLen;
if (valueStart + rec.ValueLength > end)   // end = blockStart + header.RecordsEnd
    SortedTable.ThrowCorrupt($"data record at byte {pos} declares value length {rec.ValueLength} overrunning the records region");

ResolveTrieRlp (PersistedSnapshot.cs:305):

internal byte[] ResolveTrieRlp(Bound localBound)
{
    if (localBound.Length != NodeRef.Size)
        SortedTable.ThrowCorrupt($"trie-node value length {localBound.Length} != NodeRef size {NodeRef.Size}");
    NodeRef nodeRef = default;
    Span<byte> nr = MemoryMarshal.AsBytes(new Span<NodeRef>(ref nodeRef));   // full 6 bytes now that length is checked
    ArenaByteReader reader = _reservation.CreateReader();
    if (!reader.TryRead(localBound.Offset, nr))
        SortedTable.ThrowCorrupt($"trie-node NodeRef at [{localBound.Offset}, {localBound.Offset + localBound.Length}) runs past the reservation");
    return ReadBlobArenaRlp(nodeRef.BlobArenaId, nodeRef.RlpDataOffset);
}

TryGetSlot (PersistedSnapshot.cs:232):

int len = checked((int)b.Length);
if (len > PersistedSnapshotTags.RlpSlotValueBufferSize)
    SortedTable.ThrowCorrupt($"slot RLP length {len} exceeds the {PersistedSnapshotTags.RlpSlotValueBufferSize}-byte maximum");
Span<byte> buf = stackalloc byte[PersistedSnapshotTags.RlpSlotValueBufferSize];
Span<byte> raw = buf[..len];
if (!reader.TryRead(b.Offset, raw))
    SortedTable.ThrowCorrupt($"slot value at [{b.Offset}, {b.Offset + b.Length}) runs past the reservation");

TryGetAccount (PersistedSnapshot.cs:221) — honor the result (guard already present):

if (!reader.TryRead(b.Offset, rlp))
    SortedTable.ThrowCorrupt($"account value at [{b.Offset}, {b.Offset + b.Length}) runs past the reservation");

[LukaszRozmej]

Trim comments - especially in tests?
It feels like there is some potential to de-duplicate parts of code?
Extremely complex - maintenance increase :(
Do we need to do our own Arena's? (Maybe yes)

[LukaszRozmej]

Make them record struct?

[asdacap]

Done in 7e60f40 — both are readonly record structs now (same sequential Pack=1 layout).

— posted by asdacap's agent (Claude Fable 5)

[LukaszRozmej]

same struct in DataBlockReader?

[asdacap]

Different size. These struct are direct copied so need to be different.

[LukaszRozmej]

Some opportunities for pooling across the PR. ArrayPoolListRef/ArrayPoolList and PooledHashSet/PooledDictionary

[asdacap]

Pooled the flagged scratch list with ArrayPoolList in 7e60f40, matching PersistedSnapshotBucket.PruneBefore. The read/merge hot paths already run on pooled/native collections (NativeMemoryList, ArrayPoolList, arena buffers); the remaining plain collections are cold one-offs (startup load, shutdown, per-compaction bookkeeping).

— posted by asdacap's agent (Claude Fable 5)

[LukaszRozmej]

Lot of duplication

[asdacap]

Deduped in 7e60f40 — the three key functions now share a Fold32 XOR-fold helper.

— posted by asdacap's agent (Claude Fable 5)

[LukaszRozmej]

this?

[LukaszRozmej]

PersistedSnapshotMaxCompactSize is taken raw here while CompactSize (line 23) goes through ValidateCompactSize() + an explicit power-of-2 check. It gets no >= CompactSize, power-of-2, or int-range validation.

If PersistedSnapshotMaxCompactSize < CompactSize (e.g. 16 vs 32), GetPersistedSnapshotCompactSize caps at 16 and can never reach CompactSize(32), so both IsCompactSizeBoundary (== _compactSize) and IsLargeCompactionBoundary (> _compactSize) return false for every block. That violates the ICompactionSchedule doc's promise that the two predicates "together cover every persistence boundary," and no persisted-snapshot boundary is ever detected.

Secondary variants of the same gap: a value > int.MaxValue overflows the (int)GetPersistedSnapshotCompactSize(...) casts in PersistedSnapshotCompactor; a non-power-of-2 value between CompactSize and 2×CompactSize makes the ==/> classification inconsistent.

Suggest extending ValidateCompactSize to also validate PersistedSnapshotMaxCompactSize (power-of-2, >= CompactSize, <= int.MaxValue - 8).

[asdacap]

Done in 0aa4ac5 — ValidateCompactSize now also rejects a PersistedSnapshotMaxCompactSize that is below CompactSize, not a power of 2, or past int.MaxValue - 8, with a parameterized regression test in CompactionScheduleTests. The one test that relied on the invalid geometry (offset-alignment window regression) moved to maxCompactSize = 64; its alignment (16) stays below CompactSize either way, so the exercised window is unchanged.

— posted by asdacap's agent (Claude Fable 5)

[LukaszRozmej]

This current <= NoAccessors guard was added to ReleaseLeaseOnce, but the sibling TryAcquireLease above still only rejects Disposing (current == Disposing), so it can acquire at count 0 — unlike SmallRefCountingDisposable.TryAcquireLease, which rejects current <= NoAccessors with a comment explaining the revival hazard.

This isn't a use-after-free: CleanUp (mmap close + File.Delete) is gated on the exclusive 0 -> Disposing CAS, which a revival at 0 makes fail, so the mapping stays valid while any lease is held. But ArenaFile/BlobArenaFile extend this base, and ArenaManager.Open's doc explicitly promises the ctor "surfaces an InvalidOperationException" if the file "has already started its CleanUp." In the count-0 window Open instead succeeds, handing back a live reservation on a file the manager has already TryRemove'd from _arenas and ReportRemoved()'d — an orphaned, untracked-but-live reservation plus transient metric skew, contradicting the documented contract.

The asymmetry (base TryAcquireLease has no mirror guard while the sibling and consumers assume it) reads as an oversight — suggest aligning the base with SmallRefCountingDisposable.TryAcquireLease.

[asdacap]

Fixed by the latest master merge (632dbb2): #12335 upstreamed the shared RefCountingLease state machine, and its TryAcquire rejects current <= NoAccessors inside the retry loop — both RefCountingDisposable and SmallRefCountingDisposable now delegate to it, so the base can no longer acquire at count 0 and the ArenaManager.Open contract holds.

— posted by asdacap's agent (Claude Fable 5)

[LukaszRozmej]

Nit: this comment lists ... < to_hash < version, but no version metadata key is ever emitted by MergeMetadata, and it omits that blob_range (which sorts first) is intentionally dropped here. Doc-only — the actual emitted set is from_block, from_hash, noderefs, to_block, to_hash.

[asdacap]

Fixed in 0aa4ac5 — dropped the phantom version key and noted that blob_range (which sorts first) is intentionally not carried over, since a merged snapshot references blobs via its ref-id records rather than a contiguous run.

— posted by asdacap's agent (Claude Fable 5)