feat: change topic str follow the lean spec#227
Conversation
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the gossip topic system to follow the lean consensus specification by introducing a structured topic format and improving type safety across the networking layer. The changes replace the simple enum-based topic system with a comprehensive struct-based approach that includes encoding, network name, and proper memory management.
Key changes:
- Replaced
GossipTopicenum with a structuredGossipTopicstruct containing kind, encoding, and network fields - Updated all networking interfaces to use the new topic system with proper encoding/decoding
- Added explicit network name parameter to network initialization and improved memory management
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/src/libp2p_bridge.rs | Updated FFI interface to accept topics string parameter and parse it for gossip subscription |
| pkgs/types/src/lib.zig | Added deinit method to ChainSpec for proper memory cleanup |
| pkgs/node/src/node.zig | Updated gossip subscription to use TopicKind instead of GossipTopic |
| pkgs/node/src/chain.zig | Added chain config deinitialization in BeamChain cleanup |
| pkgs/network/src/mock.zig | Updated mock implementation to use TopicKind in subscribe function |
| pkgs/network/src/interface.zig | Major refactor introducing GossipTopic struct, TopicKind enum, and encoding logic |
| pkgs/network/src/ethlibp2p.zig | Updated to work with new topic system and added network name parameter |
| pkgs/configs/src/lib.zig | Added deinit method to ChainConfig for proper cleanup |
| pkgs/cli/src/node.zig | Updated network initialization to pass network name and handle memory ownership |
| pkgs/cli/src/main.zig | Updated network setup with network name parameter and ownership transfer comments |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let topics = CStr::from_ptr(topics_str) | ||
| .to_string_lossy() | ||
| .split(",") | ||
| .map(|s| s.trim().to_string()) | ||
| .filter(|s| !s.is_empty()) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
The topics_str parameter should be validated for null pointer before dereferencing. Add a null check before calling CStr::from_ptr(topics_str) to prevent potential segmentation fault.
| let topics = CStr::from_ptr(topics_str) | |
| .to_string_lossy() | |
| .split(",") | |
| .map(|s| s.trim().to_string()) | |
| .filter(|s| !s.is_empty()) | |
| .collect::<Vec<_>>(); | |
| let topics = if topics_str.is_null() { | |
| Vec::new() | |
| } else { | |
| CStr::from_ptr(topics_str) | |
| .to_string_lossy() | |
| .split(",") | |
| .map(|s| s.trim().to_string()) | |
| .filter(|s| !s.is_empty()) | |
| .collect::<Vec<_>>() | |
| }; |
| const Self = @This(); | ||
| pub fn init(allocator: Allocator, loop: *xev.Loop, networkId: u32, logger: zeam_utils.ModuleLogger) !Self { | ||
| const timer = try xev.Timer.init(); | ||
| errdefer timer.deinit(); |
There was a problem hiding this comment.
The errdefer for timer.deinit() is misplaced. It should be after the timer initialization succeeds but before any other operations that could fail. Move this line immediately after const timer = try xev.Timer.init();
pkgs/network/src/interface.zig
Outdated
| errdefer onGossipHandlers.deinit(); | ||
| for (std.enums.values(TopicKind)) |topic| { | ||
| errdefer { | ||
| var it = onGossipHandlers.iterator(); | ||
| while (it.next()) |entry| { | ||
| entry.value_ptr.deinit(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The errdefer cleanup logic is placed inside the for loop, which means it will be executed for each topic iteration. This should be moved outside the loop to only execute once if any iteration fails.
| errdefer onGossipHandlers.deinit(); | |
| for (std.enums.values(TopicKind)) |topic| { | |
| errdefer { | |
| var it = onGossipHandlers.iterator(); | |
| while (it.next()) |entry| { | |
| entry.value_ptr.deinit(); | |
| } | |
| } | |
| errdefer { | |
| var it = onGossipHandlers.iterator(); | |
| while (it.next()) |entry| { | |
| entry.value_ptr.deinit(); | |
| } | |
| onGossipHandlers.deinit(); | |
| } | |
| for (std.enums.values(TopicKind)) |topic| { |
Signed-off-by: Chen Kai <281165273grape@gmail.com>
pkgs/network/src/ethlibp2p.zig
Outdated
| } | ||
|
|
||
| pub fn subscribe(ptr: *anyopaque, topics: []interface.GossipTopic, handler: interface.OnGossipCbHandler) anyerror!void { | ||
| pub fn subscribe(ptr: *anyopaque, topics: []interface.TopicKind, handler: interface.OnGossipCbHandler) anyerror!void { |
There was a problem hiding this comment.
I would let it it remain GossipTopic and the new topic string LeanNetworkTopic
pkgs/network/src/interface.zig
Outdated
| for (std.enums.values(GossipTopic)) |variant| { | ||
| if (std.mem.eql(u8, topic, @ptrCast(@tagName(variant)))) { | ||
| pub fn decode(encoded: []const u8) !TopicKind { | ||
| for (std.enums.values(TopicKind)) |variant| { |
There was a problem hiding this comment.
add comment
// TODO the network topic string to topic lookup can be optimized by a map lookup
unless you want to address it now
Co-authored-by: g11tech <develop@g11tech.io>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
This pull request introduces significant improvements and refactoring to the gossip topic handling and network initialization logic in the networking and CLI layers. The core changes revolve around making gossip topics more structured and type-safe, passing network names explicitly, and improving memory management and deinitialization for dynamically allocated resources.
Key changes:
Gossip Topic Refactor and Type Safety
GossipTopicenum with a newGossipTopicstruct, which now includeskind,encoding, andnetworkfields, making topics more explicit and type-safe. Added encoding/decoding logic for topics and a newGossipEncodingenum. (pkgs/network/src/interface.zig) [1] [2]pkgs/network/src/interface.zig,pkgs/network/src/ethlibp2p.zig) [1] [2] [3] [4] [5] [6]Network Initialization and Memory Management
EthLibp2p.init) to require anetwork_nameparameter, which is now passed and managed explicitly from CLI and node code. The network name is used for topic construction and is properly freed indeinit. (pkgs/cli/src/main.zig,pkgs/cli/src/node.zig,pkgs/network/src/ethlibp2p.zig) [1] [2] [3] [4]deinitmethods where necessary. (pkgs/network/src/ethlibp2p.zig,pkgs/network/src/interface.zig,pkgs/configs/src/lib.zig) [1] [2] [3]CLI and Node Changes
pkgs/cli/src/main.zig,pkgs/cli/src/node.zig,pkgs/configs/src/lib.zig) [1] [2] [3]FFI and Rust Bridge Updates
pkgs/network/src/ethlibp2p.zig)These changes collectively make the gossip protocol more robust and extensible, improve the clarity and safety of network configuration, and ensure proper memory management throughout the networking stack.
Fix #195