Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical performance bottleneck during epoch transitions by revamping the internal representation of Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant performance optimization by changing how Validator objects are stored in the SSZ merkle tree. Instead of a tree of fields, they are now stored as a pointer to a struct, enabling O(1) access. This is achieved by adding a new branch_struct node type to the Node.Pool, along with a new StructContainerType and StructContainerTreeView. The implementation is clever, using type erasure and comptime features of Zig. My review focuses on improving the safety and clarity of the new code, in line with the repository's style guide. I've identified a few areas where assertions can be added and where pointer casting can be made safer and more explicit.
Note: Security Review did not run due to the size of the PR.
| pub fn fromValue(pool: *Node.Pool, value: *const Type) !Node.Id { | ||
| const wrapped_ptr: *const WrappedT = @ptrCast(value); | ||
| return try pool.createBranchStruct(WrappedT, wrapped_ptr); | ||
| } |
There was a problem hiding this comment.
The current implementation relies on a potentially unsafe pointer cast from *const Type to *const WrappedT. While it may work due to the memory layout of WrappedT, it's not explicit and could be fragile. This is inconsistent with the safer approach taken in deserializeFromBytes. Creating a temporary WrappedT on the stack and passing its pointer is safer and clearer, aligning with the style guide's emphasis on clarity and simplicity (rule 32).
pub fn fromValue(pool: *Node.Pool, value: *const Type) !Node.Id {
const wrapped = WrappedT{ .value = value.* };
return try pool.createBranchStruct(WrappedT, &wrapped);
}
References
- The style guide recommends using simple and explicit code. The current pointer cast is subtle and relies on memory layout, which is not explicit. The suggested change makes the code safer and easier to understand. (link)
| } | ||
|
|
||
| pub inline fn isBranchStruct(node: State) bool { | ||
| return @intFromEnum(node) & @intFromEnum(branch_struct_lazy) != 0; |
There was a problem hiding this comment.
Using the explicit helper functions isBranchStructLazy() and isBranchStructComputed() makes the intent clearer and aligns better with the style guide's preference for simplicity and explicitness over subtle bitwise operations (rule 32).
return node.isBranchStructLazy() or node.isBranchStructComputed();
References
- The style guide recommends using simple and explicit control flow for clarity. The suggested change replaces a subtle bitwise operation with a more readable and explicit boolean expression using existing helper functions. (link)
| } | ||
|
|
||
| /// The pool allocates and owns a clone of `ptr`; the caller retains ownership of its data. | ||
| pub fn createBranchStruct(self: *Pool, comptime T: type, ptr: *const T) Error!Id { |
There was a problem hiding this comment.
The style guide (rule 51) requires asserting all function arguments. The ptr argument is not checked for null. Please add std.debug.assert(ptr != null); at the beginning of this function to prevent potential null pointer dereferences.
References
- The style guide mandates asserting all function arguments to detect programmer errors early. This function is missing a null check for the
ptrargument. (link)
|
closing in favor of #247 |
twoeths
force-pushed
the
te/container_node_struct_2
branch
from
e9c9929 to
6f0c12b
Compare
lodekeeper-z
left a comment
lodekeeper-z
left a comment
There was a problem hiding this comment.
Review: feat: model phase0 Validator as struct
Important PR — this is one of two competing approaches (alongside #247) for the critical BeaconStateView workstream. Both target the same problem: O(log n) per-field access on Validator tree views is too slow for process_epoch with ~1M validators. I reviewed #247 in detail; this PR deserves equal scrutiny.
Core Design
The approach stores the entire Validator struct in a heap-allocated BranchStructRef pointed to from the pool's left/right slots (pointer split across two u32 fields). A type-erased vtable (get_root, deinit) enables the pool to hash and clean up the struct without knowing its concrete type. The StructContainerTreeView then uses pool.getStructPtr() for O(1) field access instead of tree traversal.
What's good
-
O(1) access without view instantiation. Any code that navigates to a validator node can call
pool.getStructPtr(node_id, T)directly — no need to create aStructContainerTreeView. Forprocess_epochiterating ~1M validators, this eliminates the view allocation + tree-readback overhead per validator. This is the key advantage over #247, which requires instantiating a view for every access. -
Optional(T)utility is clean. Comptime-generating a struct with all fields?Tfor dirty tracking is elegant and type-safe. -
Incremental
commit()only rebuilds the node when fields were actually changed, and the changed-field detection viaOptionalis lightweight. -
CI is mostly green — core build, tests, spec tests all pass. Only bindings fail (expected, needs coordinated update).
Issues
🔴 1. isBranchStruct has a subtle bit-pattern bug
pub inline fn isBranchStruct(node: State) bool {
return @intFromEnum(node) & @intFromEnum(branch_struct_lazy) != 0;
}branch_struct_lazy = 0x40000000 (bit pattern 0100...). This check tests whether bit 30 is set. But branch_struct_computed = 0x50000000 (bit pattern 0101...) also has bit 30 set, so isBranchStruct correctly catches both. However, any future node type with bit 30 set (type codes 4-7) would also match. More concerning: if someone adds type 0x60000000 or 0x70000000 later, isBranchStruct would return true for non-struct-branch types.
The existing isBranch has the same pattern (& branch_lazy != 0), and it "works" because branch_lazy = 0x20000000 and branch_computed = 0x30000000 both have bit 29 set. But with 3-bit type codes, the bitwise shortcut is fragile.
Safer: @intFromEnum(node) & node_type >= @intFromEnum(branch_struct_lazy) or explicitly check both: isBranchStructLazy() or isBranchStructComputed().
🔴 2. max_ref_count halved — is 268M enough?
The 3-bit type code reduces max_ref_count from 0x1FFFFFFF (536M) to 0x0FFFFFFF (268M). For mainnet with ~1M validators × ~8 nodes per validator tree = ~8M nodes, max refcount would need to accommodate deep sharing (e.g., one root referenced by many views). 268M is likely still fine, but this should be documented as an intentional tradeoff. If node pools grow for checkpoint states (multiple states sharing subtrees), refcounts could increase.
🔴 3. Pointer encoding in left/right is fragile across architectures
Storing a pointer split across two u32 slots works on 64-bit, but:
- On 32-bit, only
rightis used andleftis zeroed. This breaksgetLeftChild()/getRightChild()for any code that traverses abranch_structnode without checkingnoChildfirst. - The
@intFromPtr/@ptrFromIntround-trip assumes the allocator returns pointers in the lower 48 bits (standard for x86-64 user space). If Zig ever uses a custom allocator mapping high memory (or tagged pointers), this silently breaks. getBranchStructRefUnsafeis called fromunref's cleanup path — if the pointer gets corrupted (e.g., partial write), thedeinitcall will crash non-deterministically.
Consider: store BranchStructRef pointers in a separate ArrayList indexed by node_id, keeping the pool's SoA layout clean. The memory overhead is one pointer per branch-struct node (vs zero with pointer encoding), but the safety and maintainability win is significant.
🟡 4. fromValue does an unsafe @ptrCast from *const Type to *const WrappedT
pub fn fromValue(pool: *Node.Pool, value: *const Type) !Node.Id {
const wrapped_ptr: *const WrappedT = @ptrCast(value);
return try pool.createBranchStruct(WrappedT, wrapped_ptr);
}WrappedT wraps Type as its first (and only data) field. The @ptrCast relies on WrappedT having the same address as its first field. This is guaranteed by Zig for extern struct but not for auto layout (the compiler may reorder or pad fields). Since WrappedT uses auto layout and has function pointers via vtable, this cast is technically UB if the compiler inserts padding before .value.
Fix: explicitly construct a WrappedT from the value instead of casting:
const wrapped = WrappedT{ .value = value.* };
return try pool.createBranchStruct(WrappedT, &wrapped);🟡 5. StructContainerTreeView.commit() replaces the root — but callers may hold stale references
When commit() creates a new branch_struct node and updates self.root, any other code holding the old root ID still points to the pre-mutation tree. This is fine for COW semantics (both the old and new trees are valid), but the self.original_value pointer now points into the new node's allocation while the old node may still be alive via other references. Since original_value is a borrow from the pool, this is correct — but subtle. A doc comment explaining the ownership transfer would help.
🟡 6. No test for pool.unref cleanup of branch_struct nodes
The test creates a view, uses it, and defers deinit. But there's no test verifying that when the last reference to a branch_struct node is dropped, the BranchStructRef.deinit is called and the allocation freed. A test with pool.unref() driving refcount to zero + checking for leaks (tight pool) would catch regressions in the cleanup path.
Devil's advocate: complexity budget
This PR adds a new node type to the pool — the most fundamental data structure in the SSZ tree. Every function that switches on node types (unref, getRoot, noChild, setNodesAtDepth, rebind) must now handle branch_struct correctly. Missing a case = silent corruption.
Compare #247: zero pool changes, view-level only, every existing pool operation works unchanged. The tradeoff is performance (view instantiation cost per access), which is real — but the blast radius of a pool bug is much larger than a view bug.
My recommendation: If this lands, add exhaustive switch statements on State.node_type (instead of bitwise checks) wherever node-type-dependent behavior exists. This way, adding a new node type forces handling at every callsite — the compiler catches it.
vs #247
| #232 (this PR) | #247 | |
|---|---|---|
| Pool changes | New node type + pointer encoding | None |
| Access pattern | O(1) via getStructPtr directly |
O(1) via view (must instantiate first) |
| Blast radius | Pool-level (affects all tree ops) | View-level only |
process_epoch perf |
Better (no view alloc per validator) | Worse (view alloc + tree readback per access) |
| Composability | Pool knows about struct storage | Pool stays clean |
Both approaches are correct and well-tested. The ideal path may be landing #247 first (simpler, safer), then layering #232's pool optimization underneath once benchmarks confirm the view-instantiation cost matters. But if #232's perf advantage is significant for process_epoch, it could land directly with the safety improvements noted above.
Overall: architecturally sound, the O(1) access advantage is real, but needs the safety fixes (especially the @ptrCast and bit-pattern issues) before merging. 👍
|
|
||
| branch_struct_ref.* = .{ | ||
| .ptr = @ptrCast(@constCast(cloned)), | ||
| .get_root = struct { |
There was a problem hiding this comment.
🟡 Pointer split across left/right — consider a side table.
Encoding a heap pointer into two u32 node fields works but couples node storage with pointer representation. Every pool operation touching left/right now needs to know that some nodes store pointers, not child IDs.
Alternative: a HashMap(Id, *BranchStructRef) or compact ArrayList indexed by node ID. Memory cost is one pointer per struct-branch node (negligible vs the struct itself), but pool operations stay clean — left/right always mean child node IDs.
Not blocking since the current approach works correctly on 64-bit, but worth considering for maintainability.
There was a problem hiding this comment.
Encoding a heap pointer into two u32 node fields works but couples node storage with pointer representation
I agree if we go from the current/old design point of view. With the new approach of this PR, it changes semantic of node storage: it could be either left/right nodes or pointer to some data
Every pool operation touching left/right now needs to know that some nodes store pointers, not child IDs.
good call on it. The only getLeft()/getRight() calls that affect is proof, which is addressed in 91b030a
for ssz, we call getRight() to get length of a list, and that's encoded as a regular branch node, not branch_struct node
| } | ||
|
|
||
| pub fn hashTreeRootInto(self: *Self, out: *[32]u8) !void { | ||
| try self.commit(); |
There was a problem hiding this comment.
🟡 commit() ownership transfer is subtle.
After commit, self.original_value points into the new node's allocation (via pool.getStructPtr(new_root, T)), the old root is unref'd. If other code holds a reference to the old root, they still see the pre-mutation struct via their own getStructPtr — correct COW.
But self.original_value is a raw pointer borrowed from the pool — if the pool compacts or the node is freed by another path, this dangling. Worth a doc comment explaining that original_value is valid only while self.root's refcount > 0.
There was a problem hiding this comment.
I simplified the TreeView implementation following https://github.com/ChainSafe/lodestar-z/pull/247/changes#diff-daf52008231bbeea53f1ef1e1668986abf072f4c7d57c0d8d26c0895545cc8e6R430
original_value is not tracked anymore
|
@lodekeeper-z thanks for the review and detailed comments.
Fixed.
not a red flag. While this branch reduces
See inline replies below.
This implementation stores both the high and low bits of the pointer integer and reconstructs the full
I don’t think “partial write” is a realistic failure mode here. The pointer is written once during node creation and decoded later during cleanup. If memory is corrupted by some unrelated bug then dereferencing it would of course be unsafe, but that is not specific to this encoding.
I don’t think that tradeoff is clearly better here. Using a side table adds extra memory, an additional indirection, and worse locality for a very large pool. Reusing the existing
Fixed.
Fixed as part of simplifying the TreeView implementation in #247.
The unit test was enhanced to verify that the |
|
Verified the fixes: #1 isBranchStruct (49f2caf): Correct — explicit equality checks instead of bit masking. No more false positives from overlapping patterns. #4 fromValue (640b420): Correct — stack copy instead of type-punning. Clean. #3 pointer encoding: Fair points. I was thinking about it from the perspective of adding a new node type to an existing SoA pool, but you're right that the pool's SoA fields are already coupled to node representation — Remaining: deferring to #247 for #5 (commit stale refs) and the broader TreeView simplification. Will re-review there. |
|
@lodekeeper-z I'm not even sure if we should merge #247 first. Initially I was in favor of that PR because it's way simpler but it did not work (for the performance benefit) so I have to revisit this PR I think we should disregard #247 and focus on this PR, you may request to bring some good simplification from there (I already brought the TreeView from there) even with this PR, it changes fundamental design of
I simplified the TreeView in this PR |
|
Makes sense. If #247's simpler approach doesn't deliver the performance win, no point merging it first just to undo it later. Happy to keep reviewing this PR as you shape it up. Let me know when you want another pass — or if there are specific parts of #247's simplifications you want me to evaluate for porting over. |
@lodekeeper-z please review again. Feel free to create another PR based on this branch for any optimization you find in #247 (or some new findings during your review) |
lodekeeper-z
left a comment
lodekeeper-z
left a comment
There was a problem hiding this comment.
Re-reviewed after fixes. Overall the design is solid — branch_struct reduces validator node count from ~17 to 1, which is a big win for memory and traversal. Findings:
✅ Verified fixes:
isBranchStructbit-pattern (49f2caf) — correct. Explicit type comparison instead of bit tricks, necessary with the 3-bit encoding.fromValuestack copy (640b420) — correct. No longer casts stack pointer to const reference.
✅ Looks good:
unref()path:isBranch()returns false for branch_struct, so we don't traverse packed pointer bits as child IDs. Struct cleanup happens correctly before returning to free list.- Proof materialization:
createSingleProoflimits to 1 branch_struct hop (NestedBranchStruct error).createCompactMultiProofusestemporary_rootsarray for lifetime management. Both clean up materialized trees on all paths. noChild()correctly blocks navigation into branch_struct left/right.setBranchStructComputed:0x40000000 | 0x50000000 = 0x50000000✅ (lazy→computed transition preserves refcount bits)- Hashing: delegates to
FixedContainerType.hashTreeRootviaWrappedT.getRoot— same hash whether struct or tree-backed. - Serialization: delegates entirely to
FixedContainerType— no divergence possible. - Test coverage: struct proof test verifies witness equivalence between struct and tree paths.
🟡 Minor observations (non-blocking):
-
commit()recreates entire branch_struct on every commit — even if only 1 of 8 fields changed. Thechangedbitset tracks which fields were modified butcommit()callsST.tree.fromValue(pool, &self.value)which clones the whole struct. This is fine for now since Validator is small (121 bytes), but worth noting for future struct types. -
getFieldRootleaks a node —ChildST.tree.fromValue(pool, &field_value)creates a new tree node but never unrefs it. The returned root pointer is into the pool so it stays valid, but the node's refcount starts at 0 and will be reclaimed on next alloc. Currently only used in tests, so not a real leak, but could surprise future callers. -
WrappedT.initclones viaFixedCT.clone— for Validator this is a memcpy of 121 bytes per create. Efficient enough, just noting the allocation pattern: eachcreateBranchStructdoes 2 allocations (WrappedT + BranchStructRef).
No blocking issues found. Approve.
2 participants
Motivation
Epoch transition accesses and mutates
Validatorstructs for every validator on every epoch. The existingFixedContainerTyperepresents each validator as a merkle tree of individual leaf nodes, making field access andhashTreeRootexpensive due to per-field tree traversal. This PR introduces a specialized container type that stores validators as plain structs in the pool, significantly reducing epoch transition overhead.Description
New
branch_structnode kind inNode.Pool— the pool now supports nodes that hold a type-erasedBranchStructRef(a vtable withget_rootanddeinit). The pointer is stored by splitting ausizeacross the existingleftandrightu32 fields (high 32 bits inleft, low 32 bits inright). Node state encoding is extended from 2-bit to 3-bit to accommodatebranch_struct_lazyandbranch_struct_computedstates.StructContainerType— a new SSZ container variant wrappingFixedContainerTypethat usesbranch_structnodes instead of per-field leaf trees. Exposes aWrappedTinterface satisfying the pool's vtable requirements.StructContainerTreeView— tree view backed directly by a struct pointer in the pool. Mutations are buffered in anOptional(T)(all fields wrapped in?T) and flushed lazily oncommit().Validatormigrated toStructContainerType, making validator field access and hash root computation O(1) instead of O(fields).Benchmark:
saved 700ms - 800ms on my Macbook
this branch
main