refactor: align TreeView implementations to ssz-ts #139
Conversation
Summary of ChangesHello @twoeths, 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 refactors the 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. 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 is a significant refactoring of the ContainerTreeView implementation to align it with the ssz-ts version, decoupling it from BaseTreeView. The changes are extensive and introduce a more self-contained and feature-rich ContainerTreeView.
I've found a couple of critical issues related to memory safety and correctness in the new implementation that need to be addressed. One is a bug in the commit function that could lead to reading uninitialized memory, and the other is an unsafe shallow copy in the set function for composite types, which could cause memory corruption. I've also included a medium-severity suggestion regarding adherence to the project's style guide for struct initialization, which could improve performance and reduce stack usage.
src/ssz/tree_view/container.zig
Outdated
| .pool = self.pool, | ||
| .child_data = .{null} ** ST.chunk_count, | ||
| .original_nodes = .{null} ** ST.chunk_count, | ||
| .root = self.root, |
There was a problem hiding this comment.
if we copy self.root without ref, will we run into double unrefs if the original self.root was unrefed?
There was a problem hiding this comment.
good catch, resolved it via calling init() instead 27d8022
There was a problem hiding this comment.
Pull request overview
This PR refactors the TreeView implementations to better align with the TypeScript ssz-ts library, removing the tightly coupled BaseTreeView abstraction and giving each TreeView type more freedom in its implementation.
Key Changes:
- Removed BaseTreeView and TreeViewData abstractions in favor of isolated TreeView implementations
- Changed TreeView.init() to return pointers (*Self) instead of values (Self) for better memory management
- Implemented ContainerTreeView using tuples to store child data/views instead of hash maps
- Added utility modules for shared functionality (clone_opts.zig, child_nodes.zig, assert.zig)
- Refactored list and array views to track length internally and only update tree on commit()
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/int/ssz/tree_view/list_composite.zig | Updated to use pointer-based TreeView API, changed getRoot() calls, and fixed deref patterns |
| test/int/ssz/tree_view/list_basic.zig | Updated getAll() to not require allocator parameter, changed to pointer-based API |
| test/int/ssz/tree_view/container.zig | Updated Field() to return pointer types for composite fields, adjusted getRoot() usage |
| test/int/ssz/tree_view/bit_vector.zig | Changed from base_view.data to direct data field access |
| test/int/ssz/tree_view/bit_list.zig | Changed from base_view.data to direct data field access |
| test/int/ssz/tree_view/array_composite.zig | Updated to pointer-based API and removed unnecessary deinit calls for borrowed references |
| test/int/ssz/tree_view/array_basic.zig | Updated getAll() signature and pointer-based API usage |
| src/ssz/tree_view/utils/clone_opts.zig | New utility file defining CloneOpts struct for clone operations |
| src/ssz/tree_view/utils/child_nodes.zig | New utility file with shared child node management functions |
| src/ssz/tree_view/utils/assert.zig | New utility file for compile-time TreeView type assertions |
| src/ssz/tree_view/root.zig | Removed BaseTreeView and TreeViewData exports |
| src/ssz/tree_view/list_composite.zig | Refactored to store chunks directly and track length internally |
| src/ssz/tree_view/list_basic.zig | Refactored to store chunks directly and track length internally |
| src/ssz/tree_view/container.zig | Complete rewrite using tuple-based child storage with test added |
| src/ssz/tree_view/chunks.zig | Refactored BasicPackedChunks and CompositeChunks to be self-contained |
| src/ssz/tree_view/bit_vector.zig | Refactored to store BitArray data directly |
| src/ssz/tree_view/bit_list.zig | Refactored to store BitArray data directly |
| src/ssz/tree_view/bit_array.zig | Refactored BitArray to be self-contained with own fields |
| src/ssz/tree_view/base.zig | Deleted file - BaseTreeView and TreeViewData removed |
| src/ssz/tree_view/array_composite.zig | Refactored to store chunks directly |
| src/ssz/tree_view/array_basic.zig | Refactored to store chunks directly |
| src/ssz/root.zig | Removed BaseTreeView and TreeViewData exports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
spiral-ladder
left a comment
spiral-ladder
left a comment
There was a problem hiding this comment.
Partial review with @wemeetagain on a recorded call
src/ssz/tree_view/list_basic.zig
Outdated
| fn getLength(self: *Self) !usize { | ||
| return self.chunks.getLength(); | ||
| } |
There was a problem hiding this comment.
| fn getLength(self: *Self) !usize { | |
| return self.chunks.getLength(); | |
| } |
Consider removing this entirely since we only call this during init(). Prefer self.chunks.getLength() directly.
src/ssz/tree_view/list_basic.zig
Outdated
| if (self._len > ST.limit) { | ||
| return error.LengthOverLimit; | ||
| } |
There was a problem hiding this comment.
| if (self._len > ST.limit) { | |
| return error.LengthOverLimit; | |
| } | |
| if (self._len >= ST.limit) { | |
| return error.LengthOverLimit; | |
| } |
Seems like a redundant check since every update to self._len should include this check by correctness (which we do already check when we update it above) + this should be inclusive of ST.limit. If we want to keep this, we can turn this into an assertion.
| if (existing) |child_value| { | ||
| return child_value; | ||
| } else { | ||
| const node = try self.root.getNodeAtDepth(self.pool, ST.chunk_depth, field_index); |
There was a problem hiding this comment.
| const node = try self.root.getNodeAtDepth(self.pool, ST.chunk_depth, field_index); | |
| const node = try self.root.getNodeAtDepth(self.pool, ST.chunk_depth, field_index); | |
| errdefer self.pool.unref(node); |
missing errdefer here + other similar areas
There was a problem hiding this comment.
I don't see why we have to errdefer unref() here, the get() api simply returns a borrowed reference to the child the getNodeAtDepth() does not create a new ref at all
| try child_view.commit(); | ||
| const child_changed = if (self.original_nodes[i]) |orig_node| blk: { | ||
| break :blk orig_node != child_view.getRoot(); | ||
| } else true; |
There was a problem hiding this comment.
| try child_view.commit(); | |
| const child_changed = if (self.original_nodes[i]) |orig_node| blk: { | |
| break :blk orig_node != child_view.getRoot(); | |
| } else true; | |
| const child_changed = try child_view.commit(); |
Could we perhaps return a bool here instead of using original_nodes to track if the child changed?
There was a problem hiding this comment.
sounds good, it involves changes across all TreeViews and could avoid having to store child_nodes
will track as a separate issue
| self.child_data[i] = null; | ||
| } | ||
| } | ||
| inline for (0..ST.chunk_count) |i| { | ||
| // these nodes are unref by root | ||
| self.original_nodes[i] = null; |
There was a problem hiding this comment.
| self.child_data[i] = null; | |
| } | |
| } | |
| inline for (0..ST.chunk_count) |i| { | |
| // these nodes are unref by root | |
| self.original_nodes[i] = null; | |
| } | |
| } | |
| inline for (0..ST.chunk_count) |i| { | |
| // these nodes are unref by root |
Do we need to set these to null if we're only calling this at deinit? Plus, could we perhaps just inline this function since deinit() is the only place we use this?
There was a problem hiding this comment.
Do we need to set these to null if we're only calling this at deinit?
yes, deinit is for cleaning everything, plus we don't want to track a dangling pointers there where child TreeViews are also deinited
Plus, could we perhaps just inline this function since deinit() is the only place we use this?
that's part of the step when you look at different TreeView structs so prefer to leave it there
I make them private to make it easier to reason about through see 28c0ca6
There was a problem hiding this comment.
perhaps rename this to vector_basic.zig? Same comment with array_composite.zig
There was a problem hiding this comment.
not part of this work, happy to do it in a separate PR
Motivation
we tightly couple all TreeView implementations with BaseTreeView which make it difficult to enhance it
lengthor cached readonly array in child TreeViews, see feat: implgetAll/push/sliceTo/sliceFromAPI for list tree view #100 (comment){.base_view=...}which contains potential bugs in terms of double free or dangling pointer. Instead we could/should store reference of the child TreeView itself and just return itwe are not able to get a child TreeView, modify it and let parent commit() like in the current lodestar. We can resolve that by storing chid TreeViews as references instead
code isolation: implementation for a TreeView should be isolated to its own struct only, in the end I get it really close to our typescript implementation
This PR implements
ContainerTreeViewusing tuple (instead of Map) which gives us more freedom for other TreeViews: given acomptime indexwe can get the type ofchild_data[index]Description
isolate each TreeView implementation to its own struct
commit()commit()getChildNode()setChildNode()getLength()setLength()BaseTreeViewpart of #78