Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/consensus_types/phase0.zig
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub const Checkpoint = ssz.FixedContainerType(struct {
root: p.Root,
});

pub const Validator = ssz.FixedContainerType(struct {
pub const Validator = ssz.StructContainerType(struct {
pubkey: p.BLSPubkey,
withdrawal_credentials: p.Root,
effective_balance: p.Gwei,
Expand Down
113 changes: 104 additions & 9 deletions src/persistent_merkle_tree/Node.zig
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ pub const Error = error{
///
/// `[1, next_free]`
///
/// If the high bit is not set, the next two bits determine the `node_type`
/// The following 29 bits are used for the `ref_count`.
/// If the high bit is not set, the next 3 bits determine the `node_type`
/// The following 28 bits are used for the `ref_count`.
///
/// `[0, node_type, ref_count]`
pub const State = enum(u32) {
Expand All @@ -45,14 +45,16 @@ pub const State = enum(u32) {

pub const max_next_free = 0x7FFFFFFF;

// four types of nodes
const node_type = 0x60000000;
// five types of nodes, use 3 bits
const node_type = 0x70000000;
pub const zero: State = @enumFromInt(0x00000000);
pub const leaf: State = @enumFromInt(0x20000000);
pub const branch_lazy: State = @enumFromInt(0x40000000);
pub const branch_computed: State = @enumFromInt(0x60000000);
pub const leaf: State = @enumFromInt(0x10000000);
pub const branch_lazy: State = @enumFromInt(0x20000000);
pub const branch_computed: State = @enumFromInt(0x30000000);
pub const branch_struct_lazy: State = @enumFromInt(0x40000000);
pub const branch_struct_computed: State = @enumFromInt(0x50000000);

pub const max_ref_count = 0x1FFFFFFF;
pub const max_ref_count = 0x0FFFFFFF;

pub inline fn isFree(node: State) bool {
return @intFromEnum(node) & @intFromEnum(free) != 0;
Expand Down Expand Up @@ -86,10 +88,26 @@ pub const State = enum(u32) {
return @intFromEnum(node) & node_type == @intFromEnum(branch_computed);
}

pub inline fn isBranchStruct(node: State) bool {
return @intFromEnum(node) & @intFromEnum(branch_struct_lazy) != 0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. 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)

}

pub inline fn isBranchStructLazy(node: State) bool {
Comment thread
twoeths marked this conversation as resolved.
return @intFromEnum(node) & node_type == @intFromEnum(branch_struct_lazy);
}

pub inline fn isBranchStructComputed(node: State) bool {
return @intFromEnum(node) & node_type == @intFromEnum(branch_struct_computed);
}

pub inline fn setBranchComputed(node: *State) void {
node.* = @enumFromInt(@intFromEnum(node.*) | @intFromEnum(branch_computed));
}

pub inline fn setBranchStructComputed(node: *State) void {
node.* = @enumFromInt(@intFromEnum(node.*) | @intFromEnum(branch_struct_computed));
}

pub inline fn initRefCount(node: State) State {
return node;
}
Expand Down Expand Up @@ -123,6 +141,12 @@ pub const Pool = struct {
nodes: std.MultiArrayList(Node).Slice,
next_free_node: Id,

pub const BranchStructRef = struct {
ptr: *anyopaque,
get_root: *const fn (ptr: *const anyopaque, out: *[32]u8) void,
deinit: *const fn (ptr: *anyopaque, allocator: Allocator) void,
};

pub const free_bit: u32 = 0x80000000;
pub const max_ref_count: u32 = 0x7FFFFFFF;

Expand Down Expand Up @@ -246,6 +270,44 @@ pub const Pool = struct {
return node_id;
}

/// 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 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. The style guide mandates asserting all function arguments to detect programmer errors early. This function is missing a null check for the ptr argument. (link)

const cloned = try T.init(self.allocator, ptr);
errdefer @constCast(cloned).deinit(self.allocator);

const branch_struct_ref = try self.allocator.create(BranchStructRef);
errdefer self.allocator.destroy(branch_struct_ref);

branch_struct_ref.* = .{
.ptr = @ptrCast(@constCast(cloned)),
.get_root = struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fn call(ptr_erased: *const anyopaque, out: *[32]u8) void {
const typed_ptr: *const T = @ptrCast(@alignCast(ptr_erased));
typed_ptr.getRoot(out);
}
}.call,
.deinit = struct {
fn call(ptr_erased: *anyopaque, allocator: Allocator) void {
const typed_ptr: *T = @ptrCast(@alignCast(ptr_erased));
typed_ptr.deinit(allocator);
}
}.call,
};

const node_id = try self.create();
const ptr_usize = @intFromPtr(branch_struct_ref);
const right_ptr_value: u32 = @intCast(ptr_usize & 0xFFFFFFFF);
self.nodes.items(.right)[@intFromEnum(node_id)] = @enumFromInt(right_ptr_value);
if (comptime @sizeOf(usize) == 8) {
const left_ptr_value: u32 = @intCast(ptr_usize >> 32);
self.nodes.items(.left)[@intFromEnum(node_id)] = @enumFromInt(left_ptr_value);
} else {
self.nodes.items(.left)[@intFromEnum(node_id)] = @enumFromInt(0);
}
self.nodes.items(.state)[@intFromEnum(node_id)] = State.branch_struct_lazy.initRefCount();
return node_id;
}

/// Allocates nodes into the pool.
///
/// All nodes are allocated with refcount=0.
Expand Down Expand Up @@ -333,6 +395,26 @@ pub const Pool = struct {
_ = try states[@intFromEnum(node_id)].incRefCount();
}

pub fn getStructPtr(self: *Pool, node_id: Id, comptime T: type) Error!*const T {
const state = self.nodes.items(.state)[@intFromEnum(node_id)];
if (!state.isBranchStruct()) {
return Error.InvalidNode;
}
const struct_ref = self.getBranchStructRefUnsafe(node_id);
const ptr: *const T = @ptrCast(@alignCast(struct_ref.ptr));
return ptr;
}

pub fn getBranchStructRefUnsafe(self: *Pool, node_id: Id) *BranchStructRef {
const left_ptr_value: u32 = @intFromEnum(self.nodes.items(.left)[@intFromEnum(node_id)]);
const right_ptr_value: u32 = @intFromEnum(self.nodes.items(.right)[@intFromEnum(node_id)]);
const ptr_int: usize = if (comptime @sizeOf(usize) == 8)
@as(u64, left_ptr_value) << 32 | @as(u64, right_ptr_value)
else
right_ptr_value;
return @ptrFromInt(ptr_int);
}

pub fn unref(self: *Pool, node_id: Id) void {
const states = self.nodes.items(.state);
const lefts = self.nodes.items(.left);
Expand Down Expand Up @@ -381,6 +463,13 @@ pub const Pool = struct {
} else {
current = null;
}

if (states[@intFromEnum(id)].isBranchStruct()) {
const struct_ref = self.getBranchStructRefUnsafe(id);
struct_ref.deinit(struct_ref.ptr, self.allocator);
self.allocator.destroy(struct_ref);
}

// Return the node to the free list
states[@intFromEnum(id)] = State.initNextFree(self.next_free_node);
self.next_free_node = id;
Expand All @@ -396,7 +485,7 @@ pub const Id = enum(u32) {

/// Returns true if navigation to the child node is not possible
pub inline fn noChild(node_id: Id, state: State) bool {
return state.isLeaf() or @intFromEnum(node_id) == 0;
return state.isLeaf() or state.isBranchStruct() or @intFromEnum(node_id) == 0;
}

/// Returns the root hash of the tree, computing any lazy branches as needed.
Expand All @@ -410,6 +499,12 @@ pub const Id = enum(u32) {
hashOne(hash, left, right);
state.setBranchComputed();
}

if (state.isBranchStructLazy()) {
const struct_ref = pool.getBranchStructRefUnsafe(node_id);
struct_ref.get_root(struct_ref.ptr, hash);
state.setBranchStructComputed();
}
return hash;
}

Expand Down
2 changes: 2 additions & 0 deletions src/ssz/root.zig
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ pub const VariableVectorType = types.VariableVectorType;

pub const FixedContainerType = types.FixedContainerType;
pub const VariableContainerType = types.VariableContainerType;
pub const StructContainerType = types.StructContainerType;

pub const getPathGindex = types.getPathGindex;

Expand All @@ -42,6 +43,7 @@ pub const HasherData = hasher.HasherData;

const tree_view = @import("tree_view/root.zig");
pub const ContainerTreeView = tree_view.ContainerTreeView;
pub const StructContainerTreeView = tree_view.StructContainerTreeView;
pub const ArrayBasicTreeView = tree_view.ArrayBasicTreeView;
pub const ArrayCompositeTreeView = tree_view.ArrayCompositeTreeView;
pub const ListBasicTreeView = tree_view.ListBasicTreeView;
Expand Down
Loading
Loading