feat: impl aggr sig spec #446

GrapeBaBa · 2025-12-22T04:52:25Z

Copilot

Pull request overview

This PR implements the aggregated signature specification, addressing issue #432. The changes refactor the attestation system from individual validator attestations to aggregated attestations with bitlists, aligning with the consensus specification pattern where multiple validators can attest to the same data efficiently.

Key changes include:

Transitioning from Attestations (list of individual attestations) to AggregatedAttestations (list of aggregated attestations with bitlists)
Restructuring BlockSignatures from a flat list to a nested structure with attestation signature groups and a separate proposer signature
Adding aggregation logic to combine attestations with identical data roots

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
pkgs/types/src/attestation.zig	Adds aggregation bit manipulation utilities and renames `attestation_bits` to `aggregation_bits`
pkgs/types/src/block.zig	Refactors block signatures structure and adds `aggregateSignedAttestations` function
pkgs/types/src/state.zig	Updates attestation processing to handle aggregated attestations and adds duplicate detection
pkgs/types/src/utils.zig	Adds `DuplicateAttestationData` error type
pkgs/types/src/lib.zig	Exports new aggregation-related functions and types
pkgs/state-transition/src/transition.zig	Updates signature verification to handle aggregated attestations
pkgs/state-transition/src/mock.zig	Adapts mock chain generation to use aggregated attestations
pkgs/node/src/chain.zig	Updates block processing to aggregate attestations and handle signature groups
pkgs/node/src/validator_client.zig	Adjusts proposer signature attachment to new structure
pkgs/spectest/src/runner/*.zig	Updates test fixtures to parse aggregated attestations with bitlists
pkgs/database/src/*.zig	Adapts test helpers and verification logic for new signature structure
pkgs/key-manager/src/lib.zig	Changes signing to hash `AttestationData` instead of full `Attestation`
build.zig.zon	Updates ssz dependency to version with required bug fix

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pkgs/types/src/state.zig

Copilot · 2025-12-22T04:54:25Z

pkgs/types/src/block.zig

+                if (groups.pop()) |removed| {
+                    var cleanup = removed;
+                    cleanup.bits.deinit();
+                    cleanup.signatures.deinit();
+                }


The error handling pattern uses an if statement with groups.pop() which returns an optional. However, since we just appended to groups on line 355, the pop should never return null. The cleanup logic would be clearer as var cleanup = groups.pop().?; with the assertion that it must exist, or restructure to use the errdefer pattern consistently.

Suggested change

if (groups.pop()) |removed| {

var cleanup = removed;

cleanup.bits.deinit();

cleanup.signatures.deinit();

}

var cleanup = groups.pop().?;

cleanup.bits.deinit();

cleanup.signatures.deinit();

Copilot · 2025-12-22T04:54:26Z

pkgs/types/src/attestation.zig

+        try obj.put("validator_id", json.Value{ .integer = @as(i64, @intCast(self.message.validator_id)) });
+        try obj.put("message", try self.message.data.toJson(allocator));


The JSON structure for SignedAttestation has changed from {\"message\": <Attestation>} to {\"validator_id\": <id>, \"message\": <AttestationData>}. This is a breaking change to the API format that flattens the attestation structure. If this JSON is consumed by external systems or stored persistently, this could cause compatibility issues.

Suggested change

try obj.put("validator_id", json.Value{ .integer = @as(i64, @intCast(self.message.validator_id)) });

try obj.put("message", try self.message.data.toJson(allocator));

try obj.put("message", try self.message.toJson(allocator));

Copilot · 2025-12-22T04:54:26Z

pkgs/node/src/chain.zig

-                },
+                .attestations = aggregation.attestations,
            },
        };


The errdefer block.deinit() on line 207 will double-free resources. If an error occurs after this point, block.deinit() will be called, and then the caller will likely also try to clean up the block since it was assigned aggregation.attestations on line 204. The aggregation.attestations ownership is transferred to block, so the errdefer aggregation.deinit() on line 192 could cause issues if both errdefers trigger.

Suggested change

};

};

// Ownership of `aggregation.attestations` has been transferred to `block`,

// so clear it from `aggregation` to avoid double-free in `aggregation.deinit()`.

aggregation.attestations = std.mem.zeroes(@TypeOf(aggregation.attestations));

Copilot · 2025-12-22T04:54:26Z

pkgs/types/src/block.zig

+        for (self.attestations.constSlice()) |att_value| {
+            var att = att_value;


Creating a mutable copy var att = att_value and calling att.deinit() on the copy does not actually clean up the original att_value in the slice. The deinit() should be called on a mutable reference to the original, or the attestations list should store pointers. This pattern will leak memory from the aggregation_bits.

Suggested change

for (self.attestations.constSlice()) |att_value| {

var att = att_value;

for (self.attestations.slice()) |*att| {

Copilot · 2025-12-22T04:54:26Z

pkgs/types/src/block.zig

+        for (self.attestation_signatures.constSlice()) |group_value| {
+            var group = group_value;


Same issue as with attestations deinit: calling deinit() on a copy var group = group_value does not clean up the original value in the slice. This will leak memory from the signature lists.

Suggested change

for (self.attestation_signatures.constSlice()) |group_value| {

var group = group_value;

// Deinitialize each group in-place to ensure their resources are freed.

for (self.attestation_signatures.slice()) |*group| {

Copilot · 2025-12-22T04:54:27Z

pkgs/state-transition/src/transition.zig

        state,
-        &signed_block.message.proposer_attestation,
-        &signatures[signatures.len - 1],
+        @intCast(proposer_attestation.validator_id),


The @intCast is redundant here since validator_id is already of type ValidatorIndex which should be compatible with usize. On line 108, the function parameter is directly assigned without casting. This inconsistency suggests either the cast is unnecessary or the parameter type should be ValidatorIndex.

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

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

This reverts commit f8d59070b9dc31f7f07cdb74c8f1e29c442683f1. Signed-off-by: Chen Kai <[email protected]>

Copilot

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-03T14:02:44Z

pkgs/types/src/state.zig

                is_target_already_justified or
-                !has_correct_source_root or
-                !has_correct_target_root or
+                !has_known_root or


The variable name has_known_root is misleading. The logic checks if either source OR target root is correct, but the variable name suggests that at least one root is known/correct. However, the negation !has_known_root in the validation check implies we're skipping when NO roots match. Consider renaming to has_matching_root or has_valid_root for clarity.

Copilot · 2026-01-03T14:02:44Z

pkgs/types/src/block.zig

+                if (groups.pop()) |removed| {
+                    var cleanup = removed;
+                    cleanup.bits.deinit();
+                    cleanup.signatures.deinit();
+                }


The error handling in this block is duplicating cleanup logic that already exists in the errdefer at line 338-341. When put fails, the errdefer will automatically handle cleanup of all groups including the one just appended. The explicit pop() and cleanup here is redundant and could lead to double-free if the errdefer also runs.

Suggested change

if (groups.pop()) |removed| {

var cleanup = removed;

cleanup.bits.deinit();

cleanup.signatures.deinit();

}

Copilot · 2026-01-03T14:02:45Z

pkgs/types/src/attestation.zig

+    }
 };

 pub const AggregatedAttestation = struct {


Field renamed from attestation_bits to aggregation_bits without a corresponding comment in the diff. While aggregation_bits is clearer and follows the spec naming, this is a breaking change to the public API.

Suggested change

pub const AggregatedAttestation = struct {

pub const AggregatedAttestation = struct {

// NOTE: Field renamed from `attestation_bits` to `aggregation_bits` to match the spec.

// This is a breaking change to the public API and is kept for clarity with spec naming.

build.zig.zon

pkgs/types/src/state.zig

pkgs/types/src/block.zig

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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

Copilot

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-07T16:09:54Z

pkgs/types/src/state.zig

            };

-            target_justifications[validator_id] = 1;
+            for (validator_indices.items) |validator_index| {


Missing check for duplicate validator indices before processing. If the same validator appears multiple times in the aggregation bits, their justification will be counted multiple times, potentially leading to incorrect justification counts.

Copilot · 2026-01-07T16:09:55Z

pkgs/types/src/block.zig

+            const new_group = try AggregationGroup.init(allocator, signed_attestation);
+            try groups.append(new_group);
+            const inserted_index = groups.items.len - 1;
+            root_indices.put(root, inserted_index) catch |err| {


If root_indices.put fails, the cleanup code only removes the last item from groups, but the hashmap may still contain a stale entry for this root pointing to a now-invalid index. This could cause issues if the same root is encountered again.

pkgs/node/src/chain.zig

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

anshalshukla · 2026-01-09T09:35:44Z

I see some discrepancies and have some suggestions, please wait for my review

pkgs/types/src/attestation.zig

anshalshukla · 2026-01-09T08:55:07Z

pkgs/types/src/attestation.zig

+    try std.testing.expect(encoded.items.len > 0);

-    // Convert to hex and compare with expected value
-    // Expected value is "0" * 6496 (6496 hex characters = 3248 bytes)
-    const expected_hex_len = 6496;
-    const expected_value = try std.testing.allocator.alloc(u8, expected_hex_len);
-    defer std.testing.allocator.free(expected_value);
-    @memset(expected_value, '0');
-
-    const encoded_hex = try std.fmt.allocPrint(std.testing.allocator, "{s}", .{std.fmt.fmtSliceHexLower(encoded.items)});
-    defer std.testing.allocator.free(encoded_hex);
-    try std.testing.expectEqualStrings(expected_value, encoded_hex);
-
-    // Decode
    var decoded: SignedAttestation = undefined;


nit: I think we should maintain the hardcoded byte number as it was proxy to verify that the signature length is right.

anshalshukla · 2026-01-09T08:58:11Z

pkgs/types/src/block.zig

+        for (self.attestations.slice()) |*att| {
+            att.deinit();
+        }


I think self.attestations.deinit() should handle this logic as well.

Okay, they are sszlist we cannot have a deep deinit there, you can skip this

anshalshukla · 2026-01-09T10:11:40Z

pkgs/node/src/chain.zig

+        errdefer {
+            for (attestation_signatures.slice()) |*sig_group| {
+                sig_group.deinit();
+            }


can attestation_signatures.deinit() do a deep deinit?

not required

Signed-off-by: grapebaba <[email protected]>

Copilot

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-09T13:46:14Z

pkgs/types/src/block.zig

+            const new_group = try AggregationGroup.init(allocator, signed_attestation);
+            try groups.append(new_group);
+            const inserted_index = groups.items.len - 1;
+            root_indices.put(root, inserted_index) catch |err| return err;


The error handling pattern here is redundant. The catch |err| return err is equivalent to just using try. Use try root_indices.put(root, inserted_index); instead for cleaner code.

Suggested change

root_indices.put(root, inserted_index) catch |err| return err;

try root_indices.put(root, inserted_index);

Copilot · 2026-01-09T13:46:14Z

pkgs/node/src/chain.zig

+        var aggregation_opt: ?types.AggregatedAttestationsResult = try types.aggregateSignedAttestations(self.allocator, signed_attestations);
+        errdefer if (aggregation_opt) |*aggregation| aggregation.deinit();
+
+        const aggregation = &aggregation_opt.?;
+


The optional wrapping pattern for cleanup tracking is overly complex. Consider using a simpler approach where you call aggregation.deinit() directly in the errdefer and set ownership transfer more explicitly without the optional wrapper.

Suggested change

var aggregation_opt: ?types.AggregatedAttestationsResult = try types.aggregateSignedAttestations(self.allocator, signed_attestations);

errdefer if (aggregation_opt) |*aggregation| aggregation.deinit();

const aggregation = &aggregation_opt.?;

var aggregation = try types.aggregateSignedAttestations(self.allocator, signed_attestations);

errdefer aggregation.deinit();

if error happen after aggregation ownership transferred, we shouldn't call deinit.

Signed-off-by: grapebaba <[email protected]>

GrapeBaBa requested review from anshalshukla, Copilot and g11tech December 22, 2025 04:52

Copilot AI reviewed Dec 22, 2025

View reviewed changes

feat: impl aggr sig spec

3064be8

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

GrapeBaBa force-pushed the new_spec_fresh branch from 0ab84cb to 3064be8 Compare December 28, 2025 13:31

Merge branch 'main' into new_spec_fresh

f31da52

Copilot AI review requested due to automatic review settings January 1, 2026 11:40

Copilot AI reviewed Jan 1, 2026

View reviewed changes

GrapeBaBa added 2 commits January 3, 2026 21:05

Merge branch 'main' into new_spec_fresh

86a63fd

Revert "chore(leanSpec): bump submodule to 90d6111"

466a443

This reverts commit f8d59070b9dc31f7f07cdb74c8f1e29c442683f1. Signed-off-by: Chen Kai <[email protected]>

Copilot AI review requested due to automatic review settings January 3, 2026 14:01

Copilot AI reviewed Jan 3, 2026

View reviewed changes

Merge branch 'main' into new_spec_fresh

fab65a5

g11tech reviewed Jan 5, 2026

View reviewed changes

build.zig.zon Show resolved Hide resolved

g11tech reviewed Jan 5, 2026

View reviewed changes

pkgs/types/src/state.zig Outdated Show resolved Hide resolved

g11tech reviewed Jan 7, 2026

View reviewed changes

pkgs/types/src/block.zig Show resolved Hide resolved

g11tech reviewed Jan 7, 2026

View reviewed changes

pkgs/types/src/block.zig Show resolved Hide resolved

Merge branch 'main' into new_spec_fresh

672af48

Copilot AI review requested due to automatic review settings January 7, 2026 14:17

Copilot AI reviewed Jan 7, 2026

View reviewed changes

GrapeBaBa added 2 commits January 7, 2026 23:47

fix: fix review comment

0c93d9c

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

fix: fix review comments

e509ea4

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

GrapeBaBa requested review from Copilot and g11tech January 7, 2026 16:08

Copilot AI reviewed Jan 7, 2026

View reviewed changes

g11tech reviewed Jan 8, 2026

View reviewed changes

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

fix: fix review comments

8a30009

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

GrapeBaBa requested a review from g11tech January 9, 2026 07:30

anshalshukla requested changes Jan 9, 2026

View reviewed changes

fix: fix review comments

f32d504

Signed-off-by: grapebaba <[email protected]>

Copilot AI review requested due to automatic review settings January 9, 2026 13:44

Copilot AI reviewed Jan 9, 2026

View reviewed changes

fix: fix comments

ae1e4d3

Signed-off-by: grapebaba <[email protected]>

		try obj.put("validator_id", json.Value{ .integer = @as(i64, @intCast(self.message.validator_id)) });
		try obj.put("message", try self.message.data.toJson(allocator));

-        };
+        };
+        // Ownership of `aggregation.attestations` has been transferred to `block`,
+        // so clear it from `aggregation` to avoid double-free in `aggregation.deinit()`.
+        aggregation.attestations = std.mem.zeroes(@TypeOf(aggregation.attestations));

		for (self.attestations.constSlice()) \|att_value\| {
		var att = att_value;

	for (self.attestations.constSlice()) \|att_value\| {
	var att = att_value;
	for (self.attestations.slice()) \|*att\| {

		for (self.attestation_signatures.constSlice()) \|group_value\| {
		var group = group_value;

	root_indices.put(root, inserted_index) catch \|err\| return err;
	try root_indices.put(root, inserted_index);

feat: impl aggr sig spec #446

Are you sure you want to change the base?

feat: impl aggr sig spec #446

Uh oh!

Conversation

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

Uh oh!

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 Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anshalshukla commented Jan 9, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anshalshukla Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

anshalshukla Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

anshalshukla Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

GrapeBaBa commented Dec 22, 2025 •

edited

Loading