test: add negative BLS verify and FAV cases by gorusys · Pull Request #304 · ChainSafe/lodestar-z

gorusys · 2026-04-02T16:59:53Z

No description provided.

gemini-code-assist · 2026-04-02T17:00:02Z

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 improves the robustness of the BLS signature utility by adding necessary negative test cases to ensure the verification logic correctly handles invalid inputs. Additionally, it clarifies the expected behavior of the fastAggregateVerify function through updated documentation.

Highlights

Documentation Update: Added a documentation comment to the fastAggregateVerify function clarifying that only the first 32 bytes of the message are used.
Enhanced Test Coverage: Implemented new negative test cases for BLS signature verification, including scenarios for incorrect messages and mismatched public keys, as well as a negative test for fastAggregateVerify.

🧠 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request improves the BLS utility by adding documentation to the fastAggregateVerify function and introducing unit tests for signature verification failures involving incorrect messages and public keys. The review feedback identifies style guide violations regarding doc comment formatting and line length limits. Additionally, it recommends adding explicit assertions for function arguments to comply with safety standards and assertion density requirements.

gemini-code-assist · 2026-04-02T17:01:50Z

    try sig.verify(sig_groupcheck, msg, DST, null, pk, pk_validate);
 }

+/// `msg` must be at least 32 bytes; only the first 32 are passed to fast aggregate verification.


The doc comment should start with a capital letter to comply with the style guide (Rule 304). Additionally, the line length is very close to the 100-column limit (Rule 400); wrapping it improves readability and ensures adherence to typographic constraints.

/// The `msg` must be at least 32 bytes; only the first 32 are passed to /// fast aggregate verification.

References

Comments are sentences, with a space after the slash, with a capital letter and a full stop. ^(link)

Hard limit all line lengths, without exception, to at most 100 columns. ^(link)

gemini-code-assist · 2026-04-02T17:01:50Z

    try sig.verify(sig_groupcheck, msg, DST, null, pk, pk_validate);
 }

+/// `msg` must be at least 32 bytes; only the first 32 are passed to fast aggregate verification.


While the doc comment correctly identifies the requirement for msg length, the style guide (Rule 51) requires that all function arguments be asserted. Since the implementation performs a slice msg[0..32], an explicit std.debug.assert(msg.len >= 32); should be added to the function body to prevent out-of-bounds panics and provide better documentation of the invariant. This also helps meet the requirement of a minimum of two assertions per function.

References

Assert all function arguments and return values, pre/postconditions and invariants. A function must not operate blindly on data it has not checked. The assertion density of the code must average a minimum of two assertions per function. ^(link)

gorusys · 2026-04-02T17:13:36Z

@gemini-code-assist
Review again.

gemini-code-assist

Code Review

This pull request enhances the BLS utility by adding documentation and a message length assertion to the fastAggregateVerify function, while also introducing new unit tests for signature and aggregate verification failure cases. The review feedback highlights several violations of the project's strict style guide: functions require a higher assertion density (minimum of two per function), variable names must not be abbreviated (e.g., ikm should be initial_keying_material), qualifiers must be placed at the end of variable names, and assertions should be stated positively to avoid negations. Additionally, some code lines in the new tests need to be wrapped to comply with the 100-column limit.

gemini-code-assist · 2026-04-02T17:15:30Z

+/// The `msg` must be at least 32 bytes; only the first 32 are passed to
+/// fast aggregate verification.
 pub fn fastAggregateVerify(msg: []const u8, pks: []const PublicKey, sig: *const Signature, in_pk_validate: ?bool, in_sigs_group_check: ?bool) !bool {
+    std.debug.assert(msg.len >= 32);


The style guide requires that all function arguments be asserted to ensure the function does not operate blindly on data it has not checked. Furthermore, the assertion density should average at least two assertions per function. Adding an assertion for pks.len helps satisfy these requirements.

std.debug.assert(msg.len >= 32); std.debug.assert(pks.len > 0);

References

Assert all function arguments and return values, pre/postconditions and invariants. The assertion density of the code must average a minimum of two assertions per function. ^(link)

gemini-code-assist · 2026-04-02T17:15:30Z

+    const ikm: [32]u8 = [_]u8{
+        0x93, 0xad, 0x7e, 0x65, 0xde, 0xad, 0x05, 0x2a, 0x08, 0x3a,
+        0x91, 0x0c, 0x8b, 0x72, 0x85, 0x91, 0x46, 0x4c, 0xca, 0x56,
+        0x60, 0x5b, 0xb0, 0x56, 0xed, 0xfe, 0x2b, 0x60, 0xa6, 0x3c,
+        0x48, 0x99,
+    };
+    const sk = try SecretKey.keyGen(ikm[0..], null);
+    var msg = [_]u8{1} ** 32;
+    const sig = sign(sk, &msg);
+    const pk = sk.toPublicKey();
+    msg[0] ^= 1;
+    try std.testing.expectError(bls.BlstError.VerifyFail, verify(&msg, &pk, &sig, null, null));


The style guide prohibits the use of abbreviations for variable names (except for specific cases like sort indices). Please expand ikm, sk, msg, sig, and pk to their full descriptive names. Additionally, the call to expectError should be wrapped to stay within the 100-column limit.

const initial_keying_material: [32]u8 = [_]u8{ 0x93, 0xad, 0x7e, 0x65, 0xde, 0xad, 0x05, 0x2a, 0x08, 0x3a, 0x91, 0x0c, 0x8b, 0x72, 0x85, 0x91, 0x46, 0x4c, 0xca, 0x56, 0x60, 0x5b, 0xb0, 0x56, 0xed, 0xfe, 0x2b, 0x60, 0xa6, 0x3c, 0x48, 0x99, }; const secret_key = try SecretKey.keyGen(initial_keying_material[0..], null); var message = [_]u8{1} ** 32; const signature = sign(secret_key, &message); const public_key = secret_key.toPublicKey(); message[0] ^= 1; try std.testing.expectError( bls.BlstError.VerifyFail, verify(&message, &public_key, &signature, null, null), );

References

Do not abbreviate variable names, unless the variable is a primitive integer type used as an argument to a sort function or matrix calculation. ^(link)

gemini-code-assist · 2026-04-02T17:15:30Z

+    const ikm_a: [32]u8 = [_]u8{
+        0x93, 0xad, 0x7e, 0x65, 0xde, 0xad, 0x05, 0x2a, 0x08, 0x3a,
+        0x91, 0x0c, 0x8b, 0x72, 0x85, 0x91, 0x46, 0x4c, 0xca, 0x56,
+        0x60, 0x5b, 0xb0, 0x56, 0xed, 0xfe, 0x2b, 0x60, 0xa6, 0x3c,
+        0x48, 0x99,
+    };
+    const ikm_b: [32]u8 = [_]u8{
+        0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa,
+        0xbb, 0xcc, 0xdd, 0xee, 0xff, 0x00, 0x01, 0x02, 0x03, 0x04,
+        0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e,
+        0x0f, 0x10,
+    };
+    const sk_a = try SecretKey.keyGen(ikm_a[0..], null);
+    const sk_b = try SecretKey.keyGen(ikm_b[0..], null);
+    const msg = [_]u8{1} ** 32;
+    const sig = sign(sk_a, &msg);
+    const pk_b = sk_b.toPublicKey();
+    try std.testing.expectError(bls.BlstError.VerifyFail, verify(&msg, &pk_b, &sig, null, null));


Expand abbreviations ikm_a, ikm_b, sk_a, sk_b, msg, sig, and pk_b to adhere to the style guide's rule against abbreviations. Qualifiers like 'a' and 'b' should be placed last. Also, ensure the expectError call is properly wrapped.

const initial_keying_material_a: [32]u8 = [_]u8{ 0x93, 0xad, 0x7e, 0x65, 0xde, 0xad, 0x05, 0x2a, 0x08, 0x3a, 0x91, 0x0c, 0x8b, 0x72, 0x85, 0x91, 0x46, 0x4c, 0xca, 0x56, 0x60, 0x5b, 0xb0, 0x56, 0xed, 0xfe, 0x2b, 0x60, 0xa6, 0x3c, 0x48, 0x99, }; const initial_keying_material_b: [32]u8 = [_]u8{ 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, }; const secret_key_a = try SecretKey.keyGen(initial_keying_material_a[0..], null); const secret_key_b = try SecretKey.keyGen(initial_keying_material_b[0..], null); const message = [_]u8{1} ** 32; const signature = sign(secret_key_a, &message); const public_key_b = secret_key_b.toPublicKey(); try std.testing.expectError( bls.BlstError.VerifyFail, verify(&message, &public_key_b, &signature, null, null), );

References

Do not abbreviate variable names. Qualifiers like 'a' and 'b' should be placed last. ^(link)

gemini-code-assist · 2026-04-02T17:15:30Z

+    const ikm: [32]u8 = [_]u8{
+        0x93, 0xad, 0x7e, 0x65, 0xde, 0xad, 0x05, 0x2a, 0x08, 0x3a,
+        0x91, 0x0c, 0x8b, 0x72, 0x85, 0x91, 0x46, 0x4c, 0xca, 0x56,
+        0x60, 0x5b, 0xb0, 0x56, 0xed, 0xfe, 0x2b, 0x60, 0xa6, 0x3c,
+        0x48, 0x99,
+    };
+    const sk = try SecretKey.keyGen(ikm[0..], null);
+    var msg = [_]u8{1} ** 32;
+    const sig = sign(sk, &msg);
+    const pk = sk.toPublicKey();
+    msg[31] ^= 0xff;
+    var pks = [_]PublicKey{pk};
+    const result = try fastAggregateVerify(&msg, pks[0..], &sig, null, null);
+    try std.testing.expect(!result);


Expand abbreviations ikm, sk, msg, sig, pk, and pks. Additionally, avoid negations in assertions by using expectEqual(false, ...) to state the expected outcome positively.

const initial_keying_material: [32]u8 = [_]u8{ 0x93, 0xad, 0x7e, 0x65, 0xde, 0xad, 0x05, 0x2a, 0x08, 0x3a, 0x91, 0x0c, 0x8b, 0x72, 0x85, 0x91, 0x46, 0x4c, 0xca, 0x56, 0x60, 0x5b, 0xb0, 0x56, 0xed, 0xfe, 0x2b, 0x60, 0xa6, 0x3c, 0x48, 0x99, }; const secret_key = try SecretKey.keyGen(initial_keying_material[0..], null); var message = [_]u8{1} ** 32; const signature = sign(secret_key, &message); const public_key = secret_key.toPublicKey(); message[31] ^= 0xff; var public_keys = [_]PublicKey{public_key}; const result = try fastAggregateVerify(&message, &public_keys, &signature, null, null); try std.testing.expectEqual(false, result);

References

Do not abbreviate variable names. State invariants positively and avoid negations. ^(link)

@gorusys nit but this is a good suggestion IMO

GrapeBaBa · 2026-04-03T03:47:29Z

    return sig.fastAggregateVerify(sigs_groupcheck, &pairing_buf, msg[0..32], DST, pks, pks_validate) catch return false;
 }

-// TODO: unit tests


Perhaps adding more positive tests for other functions if dropped this comment

gorusys · 2026-04-08T01:02:01Z

@gemini-code-assist
I've updated the variable names and used expectEqual.
Review again.

gemini-code-assist

Code Review

This pull request refactors the BLS utility functions by improving parameter naming and significantly expanding the unit test suite to cover various edge cases, including variable-length messages and subgroup checks. The review feedback focuses on strict adherence to the repository's style guide, specifically recommending an increase in assertion density to at least two per function, the removal of abbreviations like 'pk' and 'sig' from variable names, and the adoption of the options struct pattern for functions with multiple boolean parameters. Additionally, suggestions were made to simplify return types and remove redundant slicing operations in the test code.

gemini-code-assist · 2026-04-08T01:04:31Z

+pub fn sign(secret_key: SecretKey, message: []const u8) Signature {
+    return secret_key.sign(message, DST, null);
 }


The repository style guide requires asserting all function arguments and maintaining an average of at least two assertions per function to ensure safety and document invariants (lines 51-55). Consider adding assertions for the message length and the DST constant to meet this requirement.

pub fn sign(secret_key: SecretKey, message: []const u8) Signature { std.debug.assert(message.len > 0); std.debug.assert(DST.len > 0); return secret_key.sign(message, DST, null); }

References

Assert all function arguments and return values, pre/postconditions and invariants. The assertion density of the code must average a minimum of two assertions per function. ^(link)

gemini-code-assist · 2026-04-08T01:04:32Z

+pub fn verify(message: []const u8, public_key: *const PublicKey, signature: *const Signature, in_pk_validate: ?bool, in_sig_groupcheck: ?bool) bls.BlstError!void {
    const sig_groupcheck = in_sig_groupcheck orelse false;
    const pk_validate = in_pk_validate orelse false;
-    try sig.verify(sig_groupcheck, msg, DST, null, pk, pk_validate);
+    try signature.verify(sig_groupcheck, message, DST, null, public_key, pk_validate);
 }


Following the repository style guide:

Assertions: Added assertions for message.len and DST.len to comply with the requirement of at least two assertions per function (lines 51-55).

Naming: Renamed in_pk_validate and in_sig_groupcheck to avoid abbreviations (pk, sig) as per line 221.

Options Pattern: Since this function takes multiple optional boolean flags that can be easily mixed up, it is a strong candidate for the options: struct pattern recommended in the guide (line 287).

pub fn verify( message: []const u8, public_key: *const PublicKey, signature: *const Signature, public_key_validate: ?bool, signature_group_check: ?bool, ) bls.BlstError!void { std.debug.assert(message.len > 0); std.debug.assert(DST.len > 0); const sig_groupcheck = signature_group_check orelse false; const pk_validate = public_key_validate orelse false; try signature.verify(sig_groupcheck, message, DST, null, public_key, pk_validate); }

References

Assert all function arguments and return values, pre/postconditions and invariants. The assertion density of the code must average a minimum of two assertions per function. ^(link)

Do not abbreviate variable names. Add units or qualifiers to variable names, and put the units or qualifiers last. ^(link)

Zig has named arguments through the options: struct pattern. Use it when arguments can be mixed up. ^(link)

gemini-code-assist · 2026-04-08T01:04:32Z

+pub fn fastAggregateVerify(message: []const u8, public_keys: []const PublicKey, signature: *const Signature, in_pk_validate: ?bool, in_sigs_group_check: ?bool) !bool {
+    std.debug.assert(message.len >= 32);
+
    var pairing_buf: [bls.Pairing.sizeOf()]u8 align(bls.Pairing.buf_align) = undefined;

    const sigs_groupcheck = in_sigs_group_check orelse false;
-    const pks_validate = in_pk_validate orelse false;
-    return sig.fastAggregateVerify(sigs_groupcheck, &pairing_buf, msg[0..32], DST, pks, pks_validate) catch return false;
+    const public_keys_validate = in_pk_validate orelse false;
+    return signature.fastAggregateVerify(sigs_groupcheck, &pairing_buf, message[0..32], DST, public_keys, public_keys_validate) catch return false;
 }


Following the repository style guide:

Assertions: Added an assertion for public_keys.len to ensure the function does not operate on empty data and to meet the assertion density requirement (lines 51-55).

Naming: Renamed in_pk_validate and in_sigs_group_check to avoid abbreviations.

Return Type: Since the function catches all errors and returns false (line 34), it effectively never returns an error. Consider changing the return type to bool for simplicity, as simpler return types are preferred (line 365).

pub fn fastAggregateVerify( message: []const u8, public_keys: []const PublicKey, signature: *const Signature, public_key_validate: ?bool, signatures_group_check: ?bool, ) !bool { std.debug.assert(message.len >= 32); std.debug.assert(public_keys.len > 0); var pairing_buf: [bls.Pairing.sizeOf()]u8 align(bls.Pairing.buf_align) = undefined; const sigs_groupcheck = signatures_group_check orelse false; const pks_validate = public_key_validate orelse false; return signature.fastAggregateVerify(sigs_groupcheck, &pairing_buf, message[0..32], DST, public_keys, pks_validate) catch return false; }

References

Assert all function arguments and return values, pre/postconditions and invariants. The assertion density of the code must average a minimum of two assertions per function. ^(link)

Use simpler function signatures and return types to reduce dimensionality at the call site... void trumps bool, bool trumps u64, u64 trumps ?u64, and ?u64 trumps !u64. ^(link)

gemini-code-assist · 2026-04-08T01:04:32Z

+
+    var public_keys = [_]PublicKey{public_key};
+    var public_keys_slice: []const PublicKey = public_keys[0..1];
+    const fast_aggregate_verified = try fastAggregateVerify(&message, public_keys_slice[0..], &signature, null, null);


The slicing public_keys_slice[0..] is redundant because public_keys_slice is already a slice of the same type and length. Removing it improves clarity and adheres to the organization's "zero technical debt" policy.

const fast_aggregate_verified = try fastAggregateVerify(&message, public_keys_slice, &signature, null, null);

References

TigerBeetle has a “zero technical debt” policy. We do it right the first time. ^(link)

GrapeBaBa · 2026-04-08T04:24:04Z


 /// Verify a signature against a message and public key.
 ///
 /// If `pk_validate` is `true`, the public key will be infinity and group checked.


nit: comment arg name not same with function arg name

@spiral-ladder is in_ pk_validate better or pk_validate?

test: add negative BLS verify and FAV cases

74d8ceb

gorusys requested a review from a team as a code owner April 2, 2026 16:59

gemini-code-assist Bot reviewed Apr 2, 2026

View reviewed changes

chore: wrap fastAggregateVerify msg comment & assert len >= 32

44407ca

gemini-code-assist Bot reviewed Apr 2, 2026

View reviewed changes

GrapeBaBa reviewed Apr 3, 2026

View reviewed changes

test: add positive BLS utils coverage

a8550ca

gorusys requested a review from GrapeBaBa April 6, 2026 06:11

GrapeBaBa previously approved these changes Apr 7, 2026

View reviewed changes

test(bls): clarify variable names and use expectEqual

94e805d

gorusys dismissed GrapeBaBa’s stale review via 94e805d April 8, 2026 01:00

gemini-code-assist Bot reviewed Apr 8, 2026

View reviewed changes

chore(bls): drop redundant slice in bls sanity test

5e1eb80

gorusys requested a review from GrapeBaBa April 8, 2026 04:20

GrapeBaBa reviewed Apr 8, 2026

View reviewed changes

GrapeBaBa previously approved these changes Apr 8, 2026

View reviewed changes

chore(bls): align function parameter names with doc comments

594cfa7

gorusys dismissed GrapeBaBa’s stale review via 594cfa7 April 8, 2026 12:00

GrapeBaBa previously approved these changes Apr 8, 2026

View reviewed changes

refactor: deduplicate test key material in bls.zig tests

9cafa2d

gorusys dismissed GrapeBaBa’s stale review via 9cafa2d April 13, 2026 18:04

gorusys requested a review from GrapeBaBa April 14, 2026 05:31

Conversation

gorusys commented Apr 2, 2026

Uh oh!

gemini-code-assist Bot commented Apr 2, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

gorusys commented Apr 2, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

GrapeBaBa Apr 6, 2026 • 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 Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

gorusys commented Apr 8, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

GrapeBaBa Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

GrapeBaBa Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

GrapeBaBa Apr 6, 2026 •

edited

Loading