refactor(flat): replace persisted-snapshot HSST with single-level sorted table

[asdacap]

Part of splitting the large flat/long-finality work into reviewable pieces. This PR isolates the persisted-snapshot on-disk format: it removes the nested columnar HSST and replaces it with a single, deliberately-unoptimized, binary-search sorted table. Trie-node RLP still lives in blob arenas as NodeRefs; only the metadata format changes.

Changes

• New format (PersistedSnapshots/Sorted/): SortedTable / SortedTableBuilder / SortedTableReader / SortedTableEnumerator + FORMAT.md. Interleaved [ks u16][key][vs u16][value] records, a u32 offset index, and a footer. The builder buffers all keys and sorts the offsets at Build; the reader binary searches (O(log N), no caching).
• Verbose keys (PersistedSnapshotKey): fully-materialized <column><addr><subcolumn><slot> keys with the column/subcolumn tag bytes stored as 255 − tag, so a plain ascending byte sort reproduces the HSST reverse-tag emission order. The builder/compacter ordering and comparers are kept unchanged so a future proper-HSST serializer can reuse them.
• Rewired PersistedSnapshotBuilder / PersistedSnapshotReader / PersistedSnapshotMerger / PersistedSnapshotScanner / PersistedSnapshot over the sorted table. Dropped AddressBoundCache and the address-BTree-root caching. The merger preserves newest-wins plus per-address self-destruct truncation.
• Removed all HSST format code (Hsst/{BTree,PackedArray,DenseByteIndex,TwoByteSlot}, the dispatchers, the N-way merge plumbing). Kept the seam interfaces (IHsstByteReader/Bound, IByteBufferWriter, IHsstReaderSource, SpanByteReader, PooledByteBufferWriter), the Storage/ arena layer, bloom, and orchestration.
• Catalog version bumped 1 → 2: existing long-finality persisted-snapshot directories are unreadable by the new reader and fail fast with a "wipe and resync" message.
• Deleted the HSST format unit tests; added SortedTableTests.

Types of changes

What types of changes does your code introduce?

• Breaking change (a change that causes existing functionality not to work as expected)
• Refactoring

Breaking only for the experimental EnableLongFinality on-disk format (off by default): the persisted_snapshot directory must be wiped.

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

Notes on testing

Full Nethermind.slnx builds. Nethermind.State.Flat.Test is green — 729 existing tests (4 pre-existing skips) plus 4 new SortedTableTests — including the compactor tests that cover newest-wins and per-address self-destruct truncation.

Documentation

Requires documentation update

• No

Format documented in-repo at PersistedSnapshots/Sorted/FORMAT.md.

Requires explanation in Release Notes

• No

Experimental feature, off by default.

Remarks

Intermediate step: the format is intentionally unoptimized (a full record per slot, buffer-all-keys at build). Proper HSST returns in a later PR, reusing the unchanged builder/compacter ordering.

🤖 Generated with Claude Code

[claude]

Claude finished @asdacap's task in 10m 4s —— View job

---

PR Review — refactor(flat): replace persisted-snapshot HSST with single-level sorted table

• Gather context and understand the PR
• Review core sorted table format implementation
• Review persisted snapshot builder/reader/merger/scanner
• Review storage layer (arena, blob arena, catalog)
• Review key encoding and ordering correctness
• Review resource management and dispose patterns
• Review tests
• Post inline comments and final summary

---

Summary

The PR cleanly replaces the nested HSST columnar format with a simpler single-level sorted table backed by a binary-search reader, and the overall design is sound. The new SortedTable wire format is well-specified and the FORMAT.md is a good addition. The arena/blob-arena storage layer is sophisticated but well-structured. However, four High issues need to be resolved before merge:

---

Critical: 0 | High: 4 | Medium: 5 | Low: 4

---

High

H1 — SmallRefCountingDisposable: resurrection race window (line 41)

Between the CAS that drives _leases from 1 → 0 and the second CAS that drives 0 → Disposing, a concurrent TryAcquireLease sees _leases == 0 (NoAccessors), passes the == Disposing guard, and successfully CASes 0 → 1. The caller holds a live lease on an object that has logically hit zero count. Fix: also treat NoAccessors as a rejection in TryAcquireLease:

if (current is Disposing or NoAccessors) return false;

H2 — PersistedSnapshotMerger: TryRead return ignored; 0x00 default treated as Destructed (line 246)

SelfDestructDestructedMarkerByte = 0x00 and flag is initialised to 0. If TryRead returns false (corrupt value bound, seek failure), flag stays 0 = DestructedMarkerByte, so barrier = i is set. A re-created contract's NewMarker (0x01) entry failing to read silently triggers the self-destruct barrier, dropping that address's slots in the compacted snapshot.

H3 — PersistedSnapshotMerger: merged self-destruct tag is wrong for re-created contracts (line 250)

When a compaction range spans a destruction (source A, Destructed) followed by a re-creation (source B > A, New), the loop correctly sets barrier = A and keeps source-B's slots, but the emitted tag is Destructed (because barrier >= 0) rather than New. Stack consumers then see "contract destroyed" and refuse to serve the re-created contract's state. The emitted tag should be taken from the newest matching source, not from any historical destruction.

H4 — PersistedSnapshotBuilder: TryGet* return ignored, null-forgiving dereference (line 274)

snapshot.TryGetStateNode(path, out TrieNode? node) and snapshot.TryGetStorageNode(...) return values are discarded, then node!.FullRlp is dereferenced. Keys are pre-extracted with IsPersisted = true, but a concurrent prune or eviction between extraction and the write pass can null the node, producing an unguarded NullReferenceException that silently aborts the snapshot build.

---

Medium

M1 — ArrayPool rental not in finally (line 261)
rlpBuffer is rented at line 202 and returned at line 261. Any exception inside the per-address loop leaks the rental. Wrap in try/finally.

M2 — SortedTable.TryReadFooter: signed overflow in n * OffsetSize (line 62)
For a crafted/corrupt footer with n near long.MaxValue / 4, n * OffsetSize silently overflows to a negative value; the > table.Length guard then passes, corrupting offsetRegionStart. Pre-validate n <= (table.Length - FooterSize) / OffsetSize before multiplying.

M3 — MergeEntries: stackalloc int[n] with runtime-variable n (line 105)
Unbounded stackalloc risks StackOverflowException for large compaction batches. Add an upper-bound guard or use a fixed stack size with heap fallback.

M4 — Leb128: signed/unsigned mismatch (file)
Method signatures accept long but encoding is unsigned LEB128. Write(-1) encodes 10 bytes; Read returns long.MaxValue. Either change to ulong or document the non-negative constraint explicitly.

M5 — PersistedSnapshotCompactor.EnsureStarted not thread-safe
The _compactPersistedTask ??= ... and _boundaryCompactorTasks is null checks are unguarded. Two concurrent callers could spawn duplicate worker sets and race to consume the bounded channel. Should be protected by the existing _lock or Lazy<T>.

---

Low

L1 — ArenaReservation.CleanUp: wasted FadviseDontNeed when file is already dead (line 192)
FadviseDontNeed fires even when fileSurvives == false (file is about to be deleted). Gate it on fileSurvives.

L2 — SortedTableBuilder.Build: raw pointer captured before Sort (line 70)
keyBase is snapshotted from _keyBuf before _entries.Sort. Safe today because _keyBuf is never written during the sort, but there is no comment asserting this invariant. Add one to prevent a subtle use-after-realloc in future.

L3 — SortedTableEnumerator: footer failure is silent
When TryReadFooter returns false, the enumerator behaves as empty with no way for callers to distinguish "empty table" from "corrupt table". An IsValid property or a thrown exception on first MoveNext would make misuse visible.

L4 — SortedTableEnumerator: key buffer grows to exact fit
if (keyLength > _keyBuf.Length) _keyBuf = new byte[keyLength] — allocates on every growth. Use Math.Max(_keyBuf.Length * 2, keyLength) to amortize.

---

Design note

The 255 − tag encoding used for column/subcolumn bytes in PersistedSnapshotKey is correct and produces the desired sort order. The inline comment at SortedTable that explains the sort → HSST compatibility story is clear and helpful for future reviewers.

[claude]

High — TryAcquireLease can succeed on an object mid-disposal

Between the CAS that atomically moves _leases from Single (1) → 0 and the second CAS that moves 0 → Disposing (-1), a concurrent TryAcquireLease sees _leases == 0, which is NoAccessors — not Disposing — so the guard at line 41 passes and the CAS on line 49 (0 → 1) succeeds. The caller gets a live lease on an object that should be dead.

The object is cleaned up eventually (when the re-acquirer releases), but in between it passes TryAcquireLease-based guards that callers rely on to detect a doomed file/arena.

Fix: also reject NoAccessors in the initial check:

Suggested change

	if (current == Disposing)
	if (current is Disposing or NoAccessors)

This makes TryAcquireLease return false as soon as the count hits zero, closing the resurrection window. The inner-loop retry already handles the Disposing case, so this only adds the 0 guard to the fast-path.

[claude]

High — TryRead return value ignored; failure silently masquerades as Destructed

SelfDestructDestructedMarkerByte = 0x00 and flag is initialised to 0. If TryRead returns false (e.g. a corrupted value bound), flag remains 0, which equals DestructedMarkerByte, so barrier = i is set. A corrupt/missing self-destruct entry for a re-created contract (which would correctly hold NewMarker = 0x01) is silently treated as a destruction, causing the merger to drop post-creation slots.

if (!r.TryRead(enums[i].CurrentValue.Offset, new Span<byte>(ref flag))) continue; // skip unreadable entries
if (flag == PersistedSnapshotTags.SelfDestructDestructedMarkerByte) barrier = i;

[claude]

High — Merged self-destruct tag is wrong when a newer source re-creates the contract

The loop sets barrier to the newest source that wrote DestructedMarker, but the emitted tag uses barrier >= 0 to decide whether to write Destructed. If sources cover blocks where a contract was destroyed (source A) and then re-created (source B, index > A), the loop leaves barrier = A (the New byte 0x01 != 0x00 does not update barrier), and the emitted tag is Destructed even though the newest version is New.

Consumers of the compacted snapshot (e.g. PersistedSnapshotStack.TryGetSelfDestruct) will report the contract as destroyed and refuse to serve its storage slots, causing incorrect "contract doesn't exist" responses for a live re-created contract.

Fix: the emitted tag should be taken from the newest matching source rather than from the presence of any historical destruction:

// newest source wins for the written tag; barrier still controls slot filtering
byte newestFlag = 0;
r = views[matching[^1]].CreateReader();
r.TryRead(enums[matching[^1]].CurrentValue.Offset, new Span<byte>(ref newestFlag));
table.Add(key, newestFlag == PersistedSnapshotTags.SelfDestructDestructedMarkerByte
    ? PersistedSnapshotTags.SelfDestructDestructedMarker
    : PersistedSnapshotTags.SelfDestructNewMarker);

[claude]

High — TryGetStateNode return value ignored; node! dereference will NRE if node was evicted

Keys are extracted from snapshot.StateNodes in the parallel lambda (line 87–95), where IsPersisted = true is set to signal that these nodes should not be evicted before the write pass. However if the IsPersisted flag race or a concurrent prune between the extraction and this write pass removes the node, TryGetStateNode returns false and node is null. The ! null-forgiving operator then throws NullReferenceException, crashing the persisted-snapshot build silently. Same pattern at line 300–301 for WriteStorageNodes.

At minimum, guard and throw a descriptive exception:

if (!snapshot.TryGetStateNode(path, out TrieNode? node) || node is null)
    throw new InvalidOperationException($"State node at {path} was evicted between extraction and persist.");

[claude]

Medium — ArrayPool rental not in a finally block; leaks on any exception in the per-address loop

rlpBuffer is rented at line 202 and returned here. Any exception thrown inside the for loop (e.g. from table.Add, Rlp.Encode, or bloom operations) skips this Return, leaking the rental until the next GC:

byte[] rlpBuffer = ArrayPool<byte>.Shared.Rent(256);
try
{
    // ... for loop ...
}
finally
{
    ArrayPool<byte>.Shared.Return(rlpBuffer);
}

[claude]

Medium — Signed overflow in n * OffsetSize before the bounds guard for adversarial inputs

n is read directly from the on-disk footer as an i64. If a crafted or corrupted file provides n near long.MaxValue / 4, the multiplication n * OffsetSize silently overflows to a negative value. The guard on line 63 would then pass (negative + FooterSize < table.Length), and offsetRegionStart would be computed from the wrong (negative) offset length. Pre-validate n before multiplying:

// Guard before multiplying to prevent signed overflow on adversarial input
if (n > (table.Length - FooterSize) / OffsetSize) return false;
long offsetRegionLength = n * OffsetSize;

[claude]

Medium — stackalloc int[n] with runtime-variable, unbounded n

n = views.Length is the number of snapshot inputs. In practice this is small (typically 2–16), but there is no upper-bound guard. A large compaction batch (or a fuzzer passing many inputs) would cause StackOverflowException. A safe guard or heap fallback for large N:

Span<int> matching = n <= 64
    ? stackalloc int[64]
    : new int[n];
matching = matching[..n];

[claude]

Medium — Signed/unsigned mismatch: method signatures say long but encoding is unsigned LEB128

Write casts value to ulong before encoding, so negative long values are encoded as their two's-complement bit pattern (10 bytes of 0xFF/0x01). Read performs unsigned decode with no sign-extension, so Read(Write(-1)) returns long.MaxValue (not -1). Any caller passing a negative long will silently get a corrupt round-trip.

Either change both signatures to ulong, or add signed LEB128 support (sign-extend the final group's high bit). At a minimum, document the constraint in the XML summary:

/// <summary>Reads an unsigned LEB128-encoded value. Only non-negative <see cref="long"/> values
/// are supported; the encoding is the two's-complement unsigned representation.</summary>

[claude]

Low — Wasted FadviseDontNeed syscall when the arena file is already dead

When fileSurvives == false, MarkDead has released the manager's lease on ArenaFile. The ArenaReservation still holds its own lease (so the handle remains open), but the file is about to be File.Deleted on the last-lease release at line 197. The fadvise call here is therefore wasted — the kernel will drop those pages when the file is unlinked anyway. Guard it on fileSurvives:

Suggested change

	if (!punched)
	if (!punched && fileSurvives)
	_arenaFile.FadviseDontNeed(Offset, footprint);

[claude]

Low — Raw pointer captured before Sort; safe today but latent footgun

keyBase is a byte* into _keyBuf's backing store, snapshotted before _entries.Sort(...). NativeMemoryList<byte> never grows during Sort (sorting is done on _entries, not _keyBuf), so the pointer is stable. However, this two-step pattern — capture pointer, then mutate the sibling list — is fragile: a future maintainer could trigger a _keyBuf.Add inside the comparator (e.g. for diagnostics) and create a dangling pointer. A comment asserting the invariant would prevent this:

// _keyBuf is not touched during sort — only _entries is reordered.
// keyBase remains valid for the lifetime of the sort.
byte* keyBase = (byte*)Unsafe.AsPointer(ref MemoryMarshal.GetReference(_keyBuf.AsSpan()));