fix: make start node test friendly and fix several memory leak by GrapeBaBa · Pull Request #198 · blockblaz/zeam

GrapeBaBa · 2025-09-17T04:44:54Z

This PR makes starting node more testing friendly. Such as #194 it need access BeamNode to check finalize, but right now can't do it with startNode function in the node.zig. At the same time the refactor seems make Clock, BeamNode lifecycle more easy. Also try to fix some potential memory leak issue.

Signed-off-by: Chen Kai <[email protected]>

Copilot

Pull Request Overview

This PR refactors the node architecture to make it more testing-friendly by introducing a proper Node struct that encapsulates all components and provides explicit lifecycle management. The changes also address memory leaks by adding deinit methods throughout the codebase.

Transforms the standalone startNode function into a Node struct with clear initialization and cleanup methods
Adds comprehensive deinit methods to prevent memory leaks across multiple components
Improves ownership management for allocated resources like network addresses and callback wrappers

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
pkgs/types/src/lib.zig	Adds deinit method to BeamState for cleaning up allocated slices
pkgs/node/src/node.zig	Adds deinit method to BeamNode for proper chain cleanup
pkgs/node/src/clock.zig	Adds deinit method to Clock for timer and callback cleanup
pkgs/node/src/chain.zig	Adds deinit method to BeamChain for states HashMap cleanup
pkgs/network/src/ethlibp2p.zig	Adds deinit method to EthLibp2p for address cleanup
pkgs/cli/src/node.zig	Major refactor introducing Node struct and moving ownership management
pkgs/cli/src/metrics_server.riz	Adds cleanup for metrics server allocation
pkgs/cli/src/main.zig	Updates to use new Node struct API

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

pkgs/types/src/lib.zig

Copilot · 2025-09-17T05:18:44Z

pkgs/cli/src/node.zig

+
+        var node_multiaddrs = try self.enr.multiaddrP2PQUIC(allocator);
+        defer node_multiaddrs.deinit(allocator);
+        // move the ownership to the `EthLibp2p`, will be freed in its deinit


[nitpick] These comments about ownership transfer are helpful but could be more specific about what happens if the EthLibp2p initialization fails after ownership transfer. Consider documenting the cleanup responsibility in case of initialization failure.

Suggested change

// move the ownership to the `EthLibp2p`, will be freed in its deinit

// Ownership of `listen_addresses` is transferred to `EthLibp2p` and will be freed in its deinit.

// If EthLibp2p initialization fails after this point, the caller is responsible for freeing

// `listen_addresses` to avoid a memory leak.

Copilot · 2025-09-17T05:18:45Z

pkgs/cli/src/node.zig

+            }
+        }
+
+        // move the ownership to the `EthLibp2p`, will be freed in its deinit


[nitpick] These comments about ownership transfer are helpful but could be more specific about what happens if the EthLibp2p initialization fails after ownership transfer. Consider documenting the cleanup responsibility in case of initialization failure.

Suggested change

// move the ownership to the `EthLibp2p`, will be freed in its deinit

// Ownership of `connect_peers` is transferred to `EthLibp2p` and will be freed in its deinit.

// If `EthLibp2p` initialization fails after this point, the caller is responsible for freeing `connect_peers`.

Copilot · 2025-09-17T05:18:45Z

pkgs/cli/src/node.zig

+
+        self.clock = try Clock.init(allocator, chain_config.genesis.genesis_time, &self.loop);
+
+        const beam_node = try BeamNode.init(allocator, .{


[nitpick] The variable beam_node is only used to assign to self.beam_node. Consider directly assigning the result of BeamNode.init to self.beam_node to eliminate the intermediate variable.

Copilot · 2025-09-17T05:18:45Z

pkgs/cli/src/node.zig

+            .logger = options.logger,
+        });
+
+        self.beam_node = beam_node;


[nitpick] The variable beam_node is only used to assign to self.beam_node. Consider directly assigning the result of BeamNode.init to self.beam_node to eliminate the intermediate variable.

Copilot · 2025-09-17T05:18:45Z

pkgs/cli/src/metrics_server.zig

    port: u16,

    fn run(self: *SimpleMetricsServer) !void {
+        // `startMetricsServer` creates this, so we need to free it here


[nitpick] The comment references startMetricsServer but this function name is not visible in the current diff context. Consider making the comment more explicit about where this allocation occurs or use a more generic description.

Suggested change

// `startMetricsServer` creates this, so we need to free it here

// This struct was heap-allocated and must be freed here to avoid memory leaks

Co-authored-by: Copilot <[email protected]>

g11tech · 2025-09-17T08:06:29Z

pkgs/types/src/lib.zig

+        // historical_block_hashes and justified_slots are slices so need to be freed
+        // justifications_roots and justifications_validators not freed for now as they are not allocated
+        allocator.free(self.historical_block_hashes);
+        allocator.free(self.justified_slots);


we need a ssz.deinit which frees all the slices/allocated stuff recursively, so its better to add that and use that

These seems not managed by ssz, ssz returns the ownership to caller.

And I remembered the types ser/dser using ssz lib can't have allocator field, since the ssz lib will reflect all the fields in the types.

i am not asking it to be managed by ssz, I am asking to add a recursive freeing fn, similar to how serialize

like : sszFree(T: type, object: T)

and free if any slices, lists there recurisvely because the shape of the objects will change so its better implemented as a generic ssz free/denit fn

It may need more consider as not all the slices allocated such as justifications_roots and justifications_validators. It is better to create a specific issue for this.

agreed this is a bigger issue and needs to be addressed in ssz as well where slices also need to come from toOwnedSlice, and then a sszFree needs to recursively free slices. @noopur23 create an issue on blockblaz/ssz.zig repo for this

actually this slice freeing will become irrelevant because we are working on a PR to replace it by ssz list,

@noopur23 add another task in zeam to add "deinit" functions to all ssz types which will use the sszFree fn to be created in ssz lib as above

Signed-off-by: Chen Kai <[email protected]>

g11tech · 2025-09-17T18:58:09Z

pkgs/cli/src/node.zig

+
+    const Self = @This();
+
+    pub fn init(self: *Self, allocator: std.mem.Allocator, options: *const NodeOptions) !void {


this is a weird form of init where one passes the reference to a self which is unitialized

need @gballet 's opinion on this

This is a general and better approach in Zig. https://github.com/tigerbeetle/tigerbeetle/blob/main/docs/TIGER_STYLE.md#cache-invalidation

ok, then we need to move a whole lot of inits to this form

think it also depends on how large the struct is as the link mentions, if the struct isn't large in size then this form is not necessary.

right, most of structs in node pkgs are large

pkgs/cli/src/node.zig

pkgs/cli/src/main.zig

g11tech · 2025-09-18T06:25:41Z

pkgs/cli/src/node.zig

+
+        const ascii_art =
+            \\
+            \\  ███████╗███████╗ █████╗ ███╗   ███╗


cool art ❤️

we can probably also ascii convert our logo and add it suitably, @noopur23 create an issue for this

pkgs/cli/src/node.zig

Signed-off-by: Chen Kai <[email protected]>

g11tech

lgtm

…blaz#198) * fix: make start node test friendly and fix several memory leak Signed-off-by: Chen Kai <[email protected]> * Update pkgs/types/src/lib.zig Co-authored-by: Copilot <[email protected]> * fix: fix more memory leak when error Signed-off-by: Chen Kai <[email protected]> * fix: fix one more error meory leak Signed-off-by: Chen Kai <[email protected]> * Update pkgs/cli/src/node.zig * Update pkgs/cli/src/main.zig * fix: flattening order (blockblaz#208) * update sig size to 4000 bytes as per spec (blockblaz#210) * fix: abstract away network args into a function Signed-off-by: Chen Kai <[email protected]> --------- Signed-off-by: Chen Kai <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: g11tech <[email protected]> Co-authored-by: Anshal Shukla <[email protected]> Co-authored-by: g11tech <[email protected]>

* fix: make start node test friendly and fix several memory leak (#198) * fix: make start node test friendly and fix several memory leak Signed-off-by: Chen Kai <[email protected]> * Update pkgs/types/src/lib.zig Co-authored-by: Copilot <[email protected]> * fix: fix more memory leak when error Signed-off-by: Chen Kai <[email protected]> * fix: fix one more error meory leak Signed-off-by: Chen Kai <[email protected]> * Update pkgs/cli/src/node.zig * Update pkgs/cli/src/main.zig * fix: flattening order (#208) * update sig size to 4000 bytes as per spec (#210) * fix: abstract away network args into a function Signed-off-by: Chen Kai <[email protected]> --------- Signed-off-by: Chen Kai <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: g11tech <[email protected]> Co-authored-by: Anshal Shukla <[email protected]> Co-authored-by: g11tech <[email protected]> * refactor: abstract help fn for justifications Fix #182 * fix: fix review comments Signed-off-by: Chen Kai <[email protected]> * fix: resolve merge issue Signed-off-by: Chen Kai <[email protected]> * fix: fix merge error Signed-off-by: Chen Kai <[email protected]> * fix: make justifications deinit more explict Signed-off-by: Chen Kai <[email protected]> * free justifications values * spacing * fix log spacing --------- Signed-off-by: Chen Kai <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: g11tech <[email protected]> Co-authored-by: Anshal Shukla <[email protected]> Co-authored-by: g11tech <[email protected]>

fix: make start node test friendly and fix several memory leak

f7d372e

Signed-off-by: Chen Kai <[email protected]>

GrapeBaBa requested review from bomanaps, Copilot, g11tech and gballet and removed request for bomanaps, Copilot and g11tech September 17, 2025 04:44

Copilot AI reviewed Sep 17, 2025

View reviewed changes

Update pkgs/types/src/lib.zig

409ec57

Co-authored-by: Copilot <[email protected]>

bomanaps mentioned this pull request Sep 17, 2025

Investigate and add tests for potential memory leaks #199

Open

g11tech reviewed Sep 17, 2025

View reviewed changes

GrapeBaBa added 2 commits September 17, 2025 16:11

fix: fix more memory leak when error

c25a707

Signed-off-by: Chen Kai <[email protected]>

fix: fix one more error meory leak

a9d7d25

Signed-off-by: Chen Kai <[email protected]>

g11tech reviewed Sep 17, 2025

View reviewed changes

pkgs/cli/src/node.zig Outdated Show resolved Hide resolved

g11tech reviewed Sep 18, 2025

View reviewed changes

pkgs/cli/src/main.zig Outdated Show resolved Hide resolved

g11tech reviewed Sep 18, 2025

View reviewed changes

pkgs/cli/src/node.zig Show resolved Hide resolved

g11tech added 2 commits September 18, 2025 12:44

Update pkgs/cli/src/node.zig

cb5e3fa

Update pkgs/cli/src/main.zig

9e64a28

This was referenced Sep 18, 2025

free slices recursively using sszFree blockblaz/ssz.zig#38

Open

add "deinit" functions to all ssz types which will use the sszFree fn #206

Open

ascii convert Zeam logo and add it suitably #207

Closed

anshalshukla and others added 3 commits September 18, 2025 23:25

fix: flattening order (blockblaz#208)

86278e2

update sig size to 4000 bytes as per spec (blockblaz#210)

b637c20

fix: abstract away network args into a function

300faf2

Signed-off-by: Chen Kai <[email protected]>

GrapeBaBa requested a review from g11tech September 18, 2025 15:35

g11tech approved these changes Sep 18, 2025

View reviewed changes

g11tech merged commit fb0905b into blockblaz:main Sep 18, 2025
6 checks passed

-        // move the ownership to the `EthLibp2p`, will be freed in its deinit
+        // Ownership of `listen_addresses` is transferred to `EthLibp2p` and will be freed in its deinit.
+        // If EthLibp2p initialization fails after this point, the caller is responsible for freeing
+        // `listen_addresses` to avoid a memory leak.

	// move the ownership to the `EthLibp2p`, will be freed in its deinit
	// Ownership of `connect_peers` is transferred to `EthLibp2p` and will be freed in its deinit.
	// If `EthLibp2p` initialization fails after this point, the caller is responsible for freeing `connect_peers`.


		self.clock = try Clock.init(allocator, chain_config.genesis.genesis_time, &self.loop);

		const beam_node = try BeamNode.init(allocator, .{

	// `startMetricsServer` creates this, so we need to free it here
	// This struct was heap-allocated and must be freed here to avoid memory leaks


		const Self = @This();

		pub fn init(self: Self, allocator: std.mem.Allocator, options: const NodeOptions) !void {

Conversation

GrapeBaBa commented Sep 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GrapeBaBa Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g11tech Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GrapeBaBa Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

g11tech left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

GrapeBaBa Sep 17, 2025 •

edited

Loading

g11tech Sep 17, 2025 •

edited

Loading

GrapeBaBa Sep 18, 2025 •

edited

Loading