feat: refactor #49

wemeetagain · 2025-07-09T00:48:13Z

This is an opinionated refactoring, an effort to refactor the zig codebase into 1. a zig library (for consumption in zig) and 2. a bun-centric c-abi wrapper (for consumption in js).
build.zig and bun bindings are also tweaked.

leaving as draft because this is still not feature complete with current main branch.

TODO

tests
multithreaded verify functionality

EDIT (bing):

dcca904 merges #50 into this PR, which is another opinionated refactor of this refactor that only implements the min_pk variant of the blst library.

Here are some notes on what the PR changed:

Supports only min_pk variant. This means that we do not support min_sig, and we do not need to have SigVariant types, or the confusing createSigVariant from before, which can be confusing to follow when reading the code.
Removed memory_pool.zig and thread_pool.zig implementation. Had some initial discussions in private with @wemeetagain to decouple the memory related implementations from the library, but after some refactor work it feels like a memory/thread pool purpose-built for this library seems necessary?
Removed a lot of redundant C ABI functions that aren't called on the bun side. This is mostly from looking at unused code + the used code within production lodestar.

README.md

wemeetagain · 2025-09-25T14:12:55Z

README.md

+Install and generate bun bindings:
+


Is the plan in this PR to keep the bun bindings and move to lodestar-bun in a separate PR?

(I'm not planning on giving a deep review of the bun code until a PR into lodestar-bun)

I kept this here for ease of development, just to make sure the bun side still passes tests+benchmarks without the hassle. I will move the bun bindings from this PR to a new PR in lodestar-bun.

wemeetagain · 2025-09-25T14:18:04Z

src/AggregatePublicKey.zig

@@ -0,0 +1,134 @@
+//! AggregatePublicKey definition for BLS signature scheme based on BLS12-381.


is there a reason AggregatePublicKey.zig and AggregateSignature.zig are top-level structs vs public_key.zig, signature.zig etc? They shouldn't need to be extern structs imo.

good point, I just kept them that way picking up from your PR. Let's change it

wemeetagain · 2025-09-25T14:25:16Z

src/pairing.zig

-            return PairingError.DstTooSmall;
-        }
+    pub fn init(buffer: *[Self.sizeOf()]u8, hash_or_encode: bool, dst: []const u8) Self {
+        std.debug.assert(buffer.len == Self.sizeOf());


is it possible for an array to have a length that's different than its defined length?

Nope, I think we can remove this assert

wemeetagain · 2025-09-25T14:27:03Z

src/pairing.zig

+    /// Initializes a pairing context with the provided `buffer` and other parameters.
+    ///
+    /// Note: Rust always use a heap allocation here, but adding an allocator as param for Zig is too complex
    /// instead of that we provide a buffer that's big enough for the struct to operate on so that:
    /// - it does not have allocator in its api
    /// - can use stack allocation at consumer side
    /// - can reuse memory if it makes sense at consumer side


Might add that the Pairing takes ownership of the buffer

src/min_pk.zig

src/signature.zig

wemeetagain · 2025-09-25T15:03:30Z

src/public_key.zig

+    pub fn fromAggregate(agg_pk: *const min_pk.AggPublicKey) Self {
+        var pk_aff = min_pk.PublicKey{};
+        c.blst_p1_to_affine(&pk_aff.point, &agg_pk.point);
+        return pk_aff;
+    }
+
+    /// Convert a regular `PublicKey` to a `AggPublicKey`.
+    pub fn toAggregate(self: *const Self) min_pk.AggPublicKey {
+        var agg_pk = min_pk.AggPublicKey{};
+        c.blst_p1_from_affine(&agg_pk.point, &self.point);


an example of why the min_pk types are a bit confusing.

Hmm, what do you mean here?

I don't think agg_pk.point exists in either fn. Confusing bc its confused min_pk.AggPublicKey with AggregatePublicKey. If we just use c.blst_pX we won't have this issue

* add blst/ to .gitignore * fix test * feat: remove sig_groupcheck * fix(testSets): SECRET_KEY_LENGTH -> SECRET_KEY_SIZE * fix(index): remove missing verify.js * rebirth * chore: a bit more restructuring on zig * more updates * feat: implement verifyMultipleAggregateSignatures * remove redundant log * chore: simplify signature and pk size consts * update aggregateWithRandomness with writing pks and sigs * simple test for AggregateSignature * revert some changes * fix(sig): fix fromBytes * update bindings * fix(verifyMultipleAggSigs): fix writers for verifyMultipleAggregateSignatures * uncomment tests * missing *Signature * chore(signature): remove unused sigValidate * fix(signature.ts): do fromBytes first then validate * fix test * fix randomness * fix: uncomment aggregateVerify test * chore: simplify naming for signature length constants * remove unused code * add writeSignatures * more test fixes * test(pk): add test for aggregatePublicKeys * define and use MAX_AGGREGATE_PER_JOB * use pointers to PublicKey in aggregatePublicKey * fix length for aggregate signatures * remove redundant print * AggregateSignature: fix rands input for aggregateWithRandomness * fix scratch for aggsig and aggpk * remove comment * uncomment more checks * chore: remove redundant SignatureSet * fix rands input to aggregateWithRandomness * fix stuff related to rands * fixes * remove redundant c abi code; mostly unused stuff * update README * chore(error): remove redundant error definitions * chore(aggPk): cleanup AggregatePublicKey.zig * small correciton * more minor changes to aggPk * chore(aggSig): doc comment and tidy up * add dst as a const * fix link for DST * doc comments for pairing.zig * finish doc comments * fix comment * remove comment * remove unnecessary import

Moved declarations to more appropriate places; for example for sizes of `PublicKey` (serialize and compressed), moved their declarations to `PublicKey`.

github-actions · 2025-09-27T11:48:24Z

Performance Report

✔️ no performance regression detected

Full benchmark results

Benchmark suite	Current: `385918d`	Previous: null	Ratio
SecretKey.fromKeygen	2.4120 us/op
SecretKey serialization	666.00 ns/op
SecretKey deserialization	570.00 ns/op
SecretKey.toPublicKey	154.11 us/op
SecretKey.sign	487.39 us/op
PublicKey serialization	581.00 ns/op
PublicKey deserialize	18.595 us/op
PublicKey deserialize and validate - 1 keys	82.759 us/op
PublicKey deserialize and validate - 100 keys	8.1827 ms/op
PublicKey deserialize and validate - 10000 keys	813.65 ms/op
Signature serialization	944.00 ns/op
Signature deserialize	36.852 us/op
Signatures deserialize and validate - 1 sets	102.90 us/op
Signatures deserialize and validate - 100 sets	10.230 ms/op
Signatures deserialize and validate - 10000 sets	1.0225 s/op
aggregatePublicKeys - 1 sets	675.00 ns/op
aggregatePublicKeys - 8 sets	8.8760 us/op
aggregatePublicKeys - 32 sets	26.763 us/op
aggregatePublicKeys - 128 sets	97.215 us/op
aggregatePublicKeys - 256 sets	198.83 us/op
aggregateSignatures - 1 sets	852.00 ns/op
aggregateSignatures - 8 sets	16.281 us/op
aggregateSignatures - 32 sets	56.403 us/op
aggregateSignatures - 128 sets	217.63 us/op
aggregateSignatures - 256 sets	435.41 us/op
aggregateWithRandomness - 1 sets	270.33 us/op
aggregateWithRandomness - 16 sets	2.7149 ms/op
aggregateWithRandomness - 128 sets	17.114 ms/op
aggregateVerify - 1 sets	1.6529 ms/op
aggregateVerify - 8 sets	5.1001 ms/op
aggregateVerify - 32 sets	17.364 ms/op
aggregateVerify - 128 sets	66.481 ms/op
verifyMultipleAggregateSignatures - 1 sets	1.8816 ms/op
verifyMultipleAggregateSignatures - 8 sets	6.4150 ms/op
verifyMultipleAggregateSignatures - 32 sets	22.414 ms/op
verifyMultipleAggregateSignatures - 128 sets	86.540 ms/op
Same message - 1 sets	1.9109 ms/op
Same message - 8 sets	2.6648 ms/op
Same message - 32 sets	5.1730 ms/op
Same message - 128 sets	15.208 ms/op
Same message - 256 sets	28.590 ms/op

by benchmarkbot/action

spiral-ladder · 2025-09-27T17:45:34Z

I've converted all the extern structs to top-level native zig structs, and I think I addressed some of the issues as well:

I still think it might be easy to mix up p1 vs p2 vs p1_affine, so I still defined a Point type in each of the relevant struct, for example PublicKey.Point is of type c.blst_p1_affine.
I nuked min_pk.zig - I think this way there's less confusion, since I noticed there were double definitions for things with the same name before, eg. a min_pk.PublicKey != PublicKey
Removed all the unnecessary C pointers (which should only be used in the ABI)
Changed check to errorFromInt (more obvious what it does, and we have a intFromError anyway)

spiral-ladder · 2025-09-29T15:49:23Z

Seems like there's a flaky test in aggregating public keys, not quite sure what's the issue just yet. Had to re-run the macOS job 3 times for CI to pass. Creating an issue for this.

wemeetagain · 2025-09-29T16:07:35Z

src/AggregatePublicKey.zig

+    if (pks_validate) for (pks) |pk| try pk.validate();
+
+    var scalars_refs: [MAX_AGGREGATE_PER_JOB]*const u8 = undefined;
+    var pk_points: [MAX_AGGREGATE_PER_JOB]*const PublicKey.Point = undefined;


this is the only use of PublicKey.Point (and Signature.Point is not used).
Consider not copying pointer here, using the same cast as in AggregateSignature.aggregateWithRandomness

Also consider removing Point entirely. (As an alternative, consider adding a doc comment to PublicKey, Signature, etc that says something to the effect of eg: "PublicKey is a p1_affine" or "PublicKey is a point in G1 in affine form." which can mitigate the possible confusion)

spiral-ladder · 2025-09-30T11:58:03Z

bun/test/__fixtures__/index.ts

-	["Uint32Array", new Uint32Array()],
-	["Map", new Map()],
-	["Set", new Set()],
+	//	["boolean", true],


i'm thinking to even remove these tests because i'm not sure if testing for invalid inputs make sense at all

wemeetagain

great work, looks really good

wemeetagain · 2025-09-30T17:56:30Z

src/root.zig

 const testing = std.testing;

+/// Expose c types for lodestar-bun bindings
+pub const c = @cImport({


can we reexport the upstream blst artifact in build.zig? would that be equivalent?

can be resolved later

wemeetagain

lgtm

spiral-ladder force-pushed the cayman/refactor branch from 568c6d1 to a53ee89 Compare September 4, 2025 02:44

wemeetagain marked this pull request as ready for review September 25, 2025 14:06

wemeetagain commented Sep 25, 2025

View reviewed changes

spiral-ladder mentioned this pull request Sep 26, 2025

feat(build): use blst packaged for zig using zig build system #51

Merged

wemeetagain and others added 3 commits September 27, 2025 11:17

wip

a223812

pairing: remove unnecessary assert

6c3db51

spiral-ladder force-pushed the cayman/refactor branch from dcca904 to 6c3db51 Compare September 27, 2025 03:22

spiral-ladder added 4 commits September 27, 2025 12:44

remove redundant portable

b0d51da

pairing: explain that pairing owns memory

3458ad3

remove all usage of c pointers in native zig code

3cb5c4e

remove min_pk.zig and move declarations

704c411

Moved declarations to more appropriate places; for example for sizes of `PublicKey` (serialize and compressed), moved their declarations to `PublicKey`.

spiral-ladder force-pushed the cayman/refactor branch from 13cbd59 to 704c411 Compare September 27, 2025 11:20

fix bun lints

f783845

spiral-ladder added 9 commits September 27, 2025 19:48

pairing: refactor pairing as top-level struct

9b5a4e0

secret_key: refactor secret_key as top-level struct

0fbfccf

pk: make PublicKey top level struct

bfeed94

move fast verification to own file

5fa5c90

signature: make Signature top-level struct

2b0752d

fix PublicKey.point

fe96b7b

move aggregateVerify test to Signature.zig

ad21ee4

move more tests around

0ce15b8

fix abi

bfbae85

spiral-ladder force-pushed the cayman/refactor branch from 6369ec9 to bfbae85 Compare September 27, 2025 17:32

spiral-ladder added 3 commits September 28, 2025 01:34

more descriptive errorFromInt

a4c7453

chore: more descriptive namings

ca85a6b

fix and expose fast verif

aa5bd95

remove blst from gitignore now that we use zig package

c5c0384

bun lint --fix

7099c29

spiral-ladder mentioned this pull request Sep 29, 2025

move blst bun bindings from blst-z ChainSafe/lodestar-bun#14

Open

spiral-ladder mentioned this pull request Sep 29, 2025

flaky public key aggregation test #52

Open

wemeetagain commented Sep 29, 2025

View reviewed changes

spiral-ladder added 5 commits September 30, 2025 01:57

undo extern struct changes

fcc141c

bun lint fix'

19e0b22

minor AggregateSignature fixes

6c0bceb

do not copy pks

f2a8d1c

bun lint

d20d683

spiral-ladder force-pushed the cayman/refactor branch from 706a139 to d20d683 Compare September 29, 2025 18:01

use raw C types

7bb299f

spiral-ladder force-pushed the cayman/refactor branch from 27dd49f to 7bb299f Compare September 30, 2025 03:08

spiral-ladder added 5 commits September 30, 2025 16:26

refactor entire eth_c_abi.zig to use raw C types as params

62380af

bun lint --fix

1506925

fix signature.zig

58cc49e

bun lint --fix

6ec2cdd

Update README

7b8c78c

spiral-ladder force-pushed the cayman/refactor branch from 14ea1e4 to 7b8c78c Compare September 30, 2025 11:52

spiral-ladder reviewed Sep 30, 2025

View reviewed changes

spiral-ladder added 2 commits September 30, 2025 20:02

fix aggregateVerify test count

20e7b73

bun lint --fix

2bb5395

wemeetagain commented Sep 30, 2025

View reviewed changes

expose c

ea0ae94

spiral-ladder mentioned this pull request Sep 30, 2025

feat: add blst bindings ChainSafe/lodestar-bun#18

Open

wemeetagain commented Sep 30, 2025

View reviewed changes

wemeetagain commented Oct 1, 2025

View reviewed changes

wemeetagain merged commit 88fea8c into main Oct 1, 2025
11 of 14 checks passed

wemeetagain deleted the cayman/refactor branch October 1, 2025 19:59

twoeths mentioned this pull request Oct 2, 2025

Consume the new blst-z refactor ChainSafe/state-transition-z#54

Closed

		@@ -0,0 +1,134 @@
		//! AggregatePublicKey definition for BLS signature scheme based on BLS12-381.

feat: refactor #49

feat: refactor #49

Uh oh!

Conversation

wemeetagain commented Jul 9, 2025 • edited by spiral-ladder Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

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!

github-actions bot commented Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance Report

Uh oh!

spiral-ladder commented Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spiral-ladder commented Sep 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wemeetagain left a comment

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!

wemeetagain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wemeetagain commented Jul 9, 2025 •

edited by spiral-ladder

Loading

github-actions bot commented Sep 27, 2025 •

edited

Loading

spiral-ladder commented Sep 27, 2025 •

edited

Loading