fix(pmt,ssz): harden chunked-leaf and zero-copy tree-view memory safety (#400)

## Motivation

A review of the chunked-leaf packing and zero-copy tree-view paths
surfaced a handful of memory-safety issues.

## Description

- **Composite `set`/`push`/`setValue` ownership.** Make them
caller-retains-on-failure (the std/Ghostty model): `chunks.set` no
longer deinits the passed view on its own reservation OOM, and
`setValue`/`pushValue` carry an `errdefer` over the view they build.
Fixes a double-free in `load_state` (`applyModifiedValidators` /
`appendNewValidators`), where the caller's `errdefer` and `set`'s
self-free both ran on the `ensureUnusedCapacity` OOM path.
- **ChunkedLeaf root recompute.** `getRoot`'s `.chunked_leaf` arm uses a
reused Pool scratch field + `computeRoot` instead of
`computeRootAllocating`, removing the only `@panic("OOM")` in `src/`
(aborted the Node.js host on OOM) and the per-recompute malloc/free on
the hashTreeRoot path. A Pool field rather than a stack buffer because
`getRoot` recurses to tree depth (~47 on a mainnet validators path), and
chunked_leaf is a recursion leaf so one shared scratch is always safe.
- **`sumTargetUnslashedBalanceIncrements`.** Assert `participations.len
== validators.len`; the zero-copy validator pointer slice turns a
cross-list length mismatch into a garbage-pointer dereference.
- **`ContainerTreeView.deserialize`.** Add the `errdefer
pool.unref(root)` its two siblings already carry, so an `init` OOM no
longer strands the deserialized subtree.
- **Delete dead `fillToLength` / `fillToDepth`.** Pool-corrupting on
first use, zero callers, superseded by `fillWithContents`.
- **`ChunkedLeaf.computeRoot` trailing-zero assert.** Assert chunks past
`len` are zero — a violated invariant would silently hash stale data
into a wrong (consensus-divergent) root.
- **`getChunkedLeafPtr` exclusive-ownership assert.** Assert `refCount()
== 0` before handing out a mutable blob pointer; in-place mutation of a
shared node corrupts every tree referencing it.
- **List `setLength` → `growTo`, grow-only.** New positions read as zero
(the data subtree is already the virtual zero subtree), so growing is
O(1) and correct by construction; a bare length *cut* only rewrites the
length mix-in, leaving stale chunk data in the merkleized root — a
silent wrong hashTreeRoot. Now asserted (`new_length >= _len`) and
documented: shrinking must go through `sliceTo`. All production callers
grow (upgrade-to-altair); the one shrink user (the loadState trim test
generator) now truncates a value-level state, keeping the test fixture
independent of `sliceTo`, which loadState itself uses to trim.
- **`ContainerTreeView.getFieldRoot` per-call pool-node leak.** On a
dirty basic field it built a temporary node from the cached value and
never unref'd it — one orphaned pool slot per call, invisible to leak
detectors (`Pool.deinit` frees every in-use slot on teardown). Mirrors
the fix its `StructContainerTreeView` sibling already carries: copy the
hash into a per-field backing store, unref the node, return a pointer
into the store. Pinned by a `getNodesInUse`-baseline test (10 calls
leaked 10 slots before; baseline-stable after).
- **Cloning a dirty tree view — two latent bugs.** A transfer-clone
deliberately *drops* uncommitted writes (the rc-0 staged nodes are
exclusively owned and can't be shared in the refcount model). The
composite path handles this correctly; the basic-list path had two gaps.
(1) **Leak:** `TreeViewState.clone` dropped the staged `children_nodes`
entries *without* `unref`, orphaning a pool slot (and any chunked_leaf
blob) per dropped write — invisible to leak detectors because
`Pool.deinit` frees every in-use slot on teardown; now caught by a
`getNodesInUse` baseline. (2) **`_len` skew:** the clone kept the
uncommitted `_len`, so a dropped push left length N+1 over an N-element
tree → wrong root on commit; the clone now reflects the committed
length. Both latent (callers commit before cloning).
- **`StructContainerTreeView.clone` semantics.** It committed the source
first, so uncommitted writes survived into both views and `clone()`
mutated the source's root — the opposite of every other view's drop
semantics. It now clones the committed state and drops uncommitted
writes (from the source too on transfer).
- **`ProofFixture` dangling Pool (sync-committee witness tests).** The
fixture returned its `Pool` by value after handing `&pool` to the views,
leaving them pointing at a dead stack frame; the tests passed only by
stack-layout luck. The fixture now initializes in place.
- **Allocator-lane routing.** Two transient buffers (the chunked-leaf
serialize Id scratch, the compact-multiproof arena) allocated from the
page-allocator lane reserved for the pool's node columns; they now use
the general allocator lane.

Author: GrapeBaBa

src/persistent_merkle_tree/ChunkedLeaf.zig

@@ -4,7 +4,6 @@
 //! array + length) referenced by one `.chunked_leaf` Node. Self-contained,
 //! ref-counted via the Pool's Node ref count, copy-on-write on mutation.
 const std = @import("std");
-const Allocator = std.mem.Allocator;
 const hashing = @import("hashing");
 const hash = hashing.hash;
 
@@ -32,13 +31,14 @@ len: u16,
 /// padding. Each reduction is one batched `hash()` call so hashtree's
 /// SIMD lanes stay saturated.
 ///
-/// `scratch` is a caller-supplied K/2-element buffer. `computeRootAllocating`
-/// wraps this with a per-call `allocator.alignedAlloc` + free.
+/// `scratch` is a caller-supplied K/2-element buffer, so root computation is allocation-free
+/// and infallible.
 ///
 /// First round reads `chunks` directly into `scratch` (avoids the
 /// in-place mutation that `hashing.merkleize` would require on `*const
 /// chunks`). Later rounds halve in-place on `scratch`.
 pub fn computeRoot(self: *const ChunkedLeaf, scratch: *align(64) [K / 2][32]u8, out: *[32]u8) void {
+    for (self.chunks[self.len..]) |*chunk| std.debug.assert(std.mem.allEqual(u8, chunk, 0));
     hash(scratch[0..], self.chunks[0..]) catch unreachable;
 
     var width: usize = K / 2;
@@ -49,14 +49,6 @@ pub fn computeRoot(self: *const ChunkedLeaf, scratch: *align(64) [K / 2][32]u8,
     out.* = scratch[0];
 }
 
-/// `computeRoot` wrapper that owns the scratch via `allocator`.
-pub fn computeRootAllocating(self: *const ChunkedLeaf, allocator: Allocator, out: *[32]u8) void {
-    const scratch_slice = allocator.alignedAlloc([32]u8, .@"64", K / 2) catch @panic("OOM");
-    defer allocator.free(scratch_slice);
-    const scratch_arr: *align(64) [K / 2][32]u8 = @ptrCast(scratch_slice.ptr);
-    self.computeRoot(scratch_arr, out);
-}
-
 const Node = @import("Node.zig");
 
 test "computeRoot for all-zero chunked_leaf equals getZeroHash(k_log2)" {

src/persistent_merkle_tree/Node.zig

@@ -261,6 +261,9 @@ pub const Pool = struct {
     allocator: Allocator,
     nodes: std.MultiArrayList(Node).Slice,
     next_free_node: Id,
+    // Reused scratch for chunked_leaf root recompute: single-threaded, and chunked_leaf is a leaf
+    // of getRoot's recursion, so at most one computeRoot uses it at a time.
+    chunked_leaf_scratch: [ChunkedLeaf.K / 2][32]u8 align(64),
 
     pub const InitOptions = struct {
         page_allocator: Allocator = std.heap.page_allocator,
@@ -278,6 +281,7 @@ pub const Pool = struct {
             .allocator = opts.allocator,
             .nodes = undefined,
             .next_free_node = @enumFromInt(max_depth),
+            .chunked_leaf_scratch = undefined,
         };
 
         var list = std.MultiArrayList(Node).empty;
@@ -776,7 +780,7 @@ pub const Id = enum(u32) {
                 }
                 const storage = chunkedLeafPtr(pool.nodes.items(.payload), idx);
                 var hash: [32]u8 = undefined;
-                storage.computeRootAllocating(pool.allocator, &hash);
+                storage.computeRoot(&pool.chunked_leaf_scratch, &hash);
                 roots[idx] = hash;
                 return &roots[idx];
             },
@@ -822,6 +826,7 @@ pub const Id = enum(u32) {
     pub fn getChunkedLeafPtr(node_id: Id, pool: *Pool) Error!*ChunkedLeaf {
         const idx = @intFromEnum(node_id);
         if (pool.nodes.items(.state)[idx].kind() != .chunked_leaf) return Error.InvalidNode;
+        std.debug.assert(pool.nodes.items(.state)[idx].refCount() == 0);
         return chunkedLeafPtr(pool.nodes.items(.payload), idx);
     }
 
@@ -1530,92 +1535,6 @@ pub const Id = enum(u32) {
     }
 };
 
-/// Fill a view to the specified depth, returning the new root node id.
-pub fn fillToDepth(pool: *Pool, bottom: Id, depth: Depth) Error!Id {
-    var d = depth;
-    var node = bottom;
-    while (d > 0) : (d -= 1) {
-        node = try pool.createBranch(node, node);
-    }
-
-    return node;
-}
-
-/// Fill a view to the specified length and depth, returning the new root node id.
-pub fn fillToLength(pool: *Pool, leaf: Id, depth: Depth, length: usize) Error!Id {
-    const max_length = @as(Gindex.Uint, 1) << depth;
-    if (length > max_length) {
-        return Error.InvalidLength;
-    }
-
-    // fill a full view to the specified depth
-    var node_id = try fillToDepth(pool, leaf, depth);
-
-    // if the requested length is the same as the max length, return the node
-    if (length == max_length) {
-        return node_id;
-    }
-
-    // otherwise, traverse down to the specified length
-    const gindex: Gindex = @enumFromInt(max_length | length);
-    const path_len = gindex.pathLen();
-    var path = gindex.toPath();
-
-    var parents_buf: [max_depth]Id = undefined;
-    var lefts_buf: [max_depth]Id = undefined;
-    var rights_buf: [max_depth]Id = undefined;
-
-    const path_parents = parents_buf[0..path_len];
-    const path_lefts = lefts_buf[0..path_len];
-    const path_rights = rights_buf[0..path_len];
-
-    const states = pool.nodes.items(.state);
-    const payloads = pool.nodes.items(.payload);
-
-    for (0..path_len - 1) |i| {
-        const idx = @intFromEnum(node_id);
-        const k = states[idx].kind();
-        if (noChildKind(node_id, k)) {
-            return Error.InvalidNode;
-        }
-        const c = childrenOf(node_id, k, payloads);
-        if (path.left()) {
-            path_lefts[i] = path_parents[i + 1];
-            path_rights[i] = c.right;
-            node_id = c.left;
-        } else {
-            path_lefts[i] = c.left;
-            path_rights[i] = path_parents[i + 1];
-            node_id = c.right;
-        }
-        path.next();
-    }
-
-    // and rebind with zero(0)
-    {
-        const idx = @intFromEnum(node_id);
-        const k = states[idx].kind();
-        if (noChildKind(node_id, k)) return Error.InvalidNode;
-        const c = childrenOf(node_id, k, payloads);
-        if (path.left()) {
-            path_lefts[path_len - 1] = @enumFromInt(0);
-            path_rights[path_len - 1] = c.right;
-        } else {
-            path_lefts[path_len - 1] = c.left;
-            path_rights[path_len - 1] = @enumFromInt(0);
-        }
-    }
-
-    // and rebind with zero(0)
-    try pool.rebind(
-        path_parents,
-        path_lefts,
-        path_rights,
-    );
-
-    return path_parents[0];
-}
-
 /// Fill a view with the specified contents, returning the new root node id.
 ///
 /// Note: contents is mutated.

src/persistent_merkle_tree/node_test.zig

@@ -5,6 +5,7 @@ const Depth = @import("hashing").Depth;
 
 const Node = @import("Node.zig");
 const Gindex = @import("gindex.zig").Gindex;
+const ChunkedLeaf = @import("ChunkedLeaf.zig");
 
 // Allocate until the pool is full, so the next request has to grow (and fail). Returns the filler.
 fn drainPoolToFull(pool: *Node.Pool, out: *std.ArrayList(Node.Id)) !void {
@@ -125,6 +126,22 @@ test "Node.State predicates" {
     _ = free_state.nextFree();
 }
 
+test "chunked_leaf getRoot recomputes without touching the pool allocator" {
+    var counter = std.testing.FailingAllocator.init(std.testing.allocator, .{ .fail_index = std.math.maxInt(usize) });
+    var pool = try Node.Pool.init(.{ .page_allocator = std.testing.allocator, .allocator = counter.allocator(), .pool_size = 16 });
+    defer pool.deinit();
+
+    var chunks: [ChunkedLeaf.K][32]u8 align(64) = undefined;
+    for (&chunks, 0..) |*c, i| c.* = [_]u8{@intCast(i & 0xff)} ** 32;
+
+    const node = try pool.createChunkedLeaf(&chunks, ChunkedLeaf.K);
+    defer pool.unref(node);
+
+    const allocs_before = counter.alloc_index;
+    _ = node.getRoot(&pool); // root starts lazy → this recomputes
+    try std.testing.expectEqual(allocs_before, counter.alloc_index);
+}
+
 test "Pool" {
     const allocator = std.testing.allocator;
     var pool = try Node.Pool.init(.{ .page_allocator = allocator, .allocator = allocator, .pool_size = 10 });

src/persistent_merkle_tree/proof.zig

@@ -564,7 +564,7 @@ pub fn createNodeFromCompactMultiProof(
     leaves: [][32]u8,
     descriptor: []const u8,
 ) (Node.Error || Error)!Node.Id {
-    var arena = std.heap.ArenaAllocator.init(pool.page_allocator);
+    var arena = std.heap.ArenaAllocator.init(pool.allocator);
     defer arena.deinit();
     const temp_allocator = arena.allocator();
 

src/ssz/tree_view/array_basic.zig

@@ -174,6 +174,46 @@ test "TreeView vector element roundtrip" {
     try std.testing.expectEqualSlices(u64, &expected, &roundtrip);
 }
 
+test "TreeView vector chunked_leaf roundtrip across the chunked_leaf boundary" {
+    const allocator = std.testing.allocator;
+    var pool = try Node.Pool.init(.{ .page_allocator = allocator, .allocator = allocator, .pool_size = 4096 });
+    defer pool.deinit();
+
+    const Uint64 = UintType(64);
+    const VectorType = FixedVectorType(Uint64, 276, .{ .chunked_leaf = true });
+
+    var original: VectorType.Type = undefined;
+    for (&original, 0..) |*e, i| e.* = i;
+
+    const root_node = try VectorType.tree.fromValue(&pool, &original);
+    var view = try VectorType.TreeView.init(allocator, &pool, root_node);
+    defer view.deinit();
+
+    try std.testing.expectEqual(@as(u64, 0), try view.get(0));
+    try std.testing.expectEqual(@as(u64, 255), try view.get(255));
+    try std.testing.expectEqual(@as(u64, 256), try view.get(256));
+    try std.testing.expectEqual(@as(u64, 275), try view.get(275));
+
+    try view.set(255, 999);
+    try view.set(256, 1000);
+    try view.commit();
+
+    var expected = original;
+    expected[255] = 999;
+    expected[256] = 1000;
+
+    var expected_root: [32]u8 = undefined;
+    try VectorType.hashTreeRoot(&expected, &expected_root);
+
+    var actual_root: [32]u8 = undefined;
+    try view.hashTreeRootInto(&actual_root);
+    try std.testing.expectEqualSlices(u8, &expected_root, &actual_root);
+
+    var roundtrip: VectorType.Type = undefined;
+    try VectorType.tree.toValue(view.getRoot(), &pool, &roundtrip);
+    try std.testing.expectEqualSlices(u64, &expected, &roundtrip);
+}
+
 test "TreeView vector getAll fills provided buffer" {
     const allocator = std.testing.allocator;
     var pool = try Node.Pool.init(.{ .page_allocator = allocator, .allocator = allocator, .pool_size = 256 });

src/ssz/tree_view/chunks.zig

@@ -442,20 +442,16 @@ pub fn CompositeChunks(
             return child_ptr;
         }
 
-        /// Takes ownership of `value` (and deinits it if a reservation fails). Deinits whatever
-        /// child was cached for `index`, so any earlier get()/getReadonly() of it is now invalid.
-        /// Pass a view you own — never a get()/getReadonly() pointer for this same index, or a
-        /// failed set would deinit a view the cache still holds (double-free).
+        /// Takes ownership of `value` only on success; on any error `value` is left untouched for
+        /// the caller to free. Deinits whatever child was cached for `index`, so any earlier
+        /// get()/getReadonly() of it is now invalid. Pass a view you own — never a
+        /// get()/getReadonly() pointer for this same index, or the displaced-old deinit would free a
+        /// view the cache still holds (double-free).
         pub fn set(self: *Self, index: usize, value: ElementPtr) !void {
             const gindex = Gindex.fromDepth(chunk_depth, index);
-            // Reserve before storing so neither store can fail. A failure mid-store would drop
-            // `value` (we own it now, the caller won't free it) or leave `changed` and
-            // `children_data` out of sync.
-            {
-                errdefer value.deinit();
-                try self.state.changed.ensureUnusedCapacity(self.state.allocator, 1);
-                try self.children_data.ensureUnusedCapacity(self.state.allocator, 1);
-            }
+            // Reserve first so the commit below cannot fail.
+            try self.state.changed.ensureUnusedCapacity(self.state.allocator, 1);
+            try self.children_data.ensureUnusedCapacity(self.state.allocator, 1);
             self.state.changed.putAssumeCapacity(gindex, {});
             const opt_old_data = self.children_data.fetchPutAssumeCapacity(gindex, value);
             if (opt_old_data) |old_data_value| {
@@ -505,12 +501,11 @@ pub fn CompositeChunks(
         /// Set a child from an SSZ value type.
         pub fn setValue(self: *Self, index: usize, value: *const Value) !void {
             const root = try ST.Element.tree.fromValue(self.state.pool, value);
-            // Free `root` only if init fails. Once init succeeds, `set` owns `child_view` on every
-            // path, so we must not deinit it here; that would double-free if set later fails.
             const child_view = Element.init(self.state.allocator, self.state.pool, root) catch |err| {
                 self.state.pool.unref(root);
                 return err;
             };
+            errdefer child_view.deinit();
             try self.set(index, child_view);
         }