Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions bindings/napi/blst.zig
Original file line number Diff line number Diff line change
Expand Up @@ -468,13 +468,12 @@ pub fn Signature_aggregate(env: napi.Env, cb: napi.CallbackInfo(2)) !napi.Value
const sigs_len = try sigs_array.getArrayLength();
if (sigs_len == 0) return error.EmptySignatureArray;

const sigs = try allocator.alloc(Signature, sigs_len);
const sigs = try allocator.alloc(*const Signature, sigs_len);
defer allocator.free(sigs);

for (0..sigs_len) |i| {
const sig_value = try sigs_array.getElement(@intCast(i));
const sig = try env.unwrap(Signature, sig_value);
sigs[i] = sig.*;
sigs[i] = try env.unwrap(Signature, sig_value);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

MEDIUM: According to the Lodestar State Transition Zig Style Guide, it's better to explicitly pass options to library functions instead of relying on defaults. While napi.newInstance might have sensible defaults, explicitly passing options improves readability and prevents potential bugs if the library changes its defaults in the future. See Repository Style Guide, lines 170-173.

        sigs[i] = try env.unwrap(Signature, sig_value, .{}); // Explicitly pass options, even if empty

References
  1. Explicitly pass options to library functions at the call site, instead of relying on the defaults. This improves readability and avoids latent bugs if the library changes its defaults. (link)


const agg_sig = AggregateSignature.aggregate(sigs, sigs_groupcheck) catch return error.AggregationFailed;
Expand Down Expand Up @@ -557,8 +556,9 @@ pub fn blst_aggregateVerify(

const msgs = try allocator.alloc([32]u8, msgs_len);
defer allocator.free(msgs);
const pk_ptrs = try allocator.alloc(*PublicKey, pks_len);
defer allocator.free(pk_ptrs);

const pks = try allocator.alloc(*PublicKey, pks_len);
defer allocator.free(pks);

for (0..msgs_len) |i| {
const msg_value = try msgs_array.getElement(@intCast(i));
Expand All @@ -567,11 +567,11 @@ pub fn blst_aggregateVerify(
@memcpy(&msgs[i], msg_info.data[0..32]);

const pk_value = try pks_array.getElement(@intCast(i));
pk_ptrs[i] = try env.unwrap(PublicKey, pk_value);
pks[i] = try env.unwrap(PublicKey, pk_value);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

MEDIUM: Following the style guide, explicitly pass options to env.unwrap to ensure that the code is robust against future changes in the library's default behavior. See Repository Style Guide, lines 170-173.

        pks[i] = try env.unwrap(PublicKey, pk_value, .{}); // Explicitly pass options

References
  1. Explicitly pass options to library functions at the call site, instead of relying on the defaults. This improves readability and avoids latent bugs if the library changes its defaults. (link)

}

const pool = thread_pool orelse @panic("ThreadPool not initialized; call initThreadPool first");
const result = pool.aggregateVerify(sig, sig_groupcheck, msgs, DST, pk_ptrs, pks_validate) catch {
const result = pool.aggregateVerify(sig, sig_groupcheck, msgs, DST, pks, pks_validate) catch {
return try env.getBoolean(false);
};

Expand Down Expand Up @@ -603,13 +603,12 @@ pub fn blst_fastAggregateVerify(env: napi.Env, cb: napi.CallbackInfo(4)) !napi.V
return try env.getBoolean(false);
}

const pks = try allocator.alloc(PublicKey, pks_len);
const pks = try allocator.alloc(*const PublicKey, pks_len);
defer allocator.free(pks);

for (0..pks_len) |i| {
const pk_value = try pks_array.getElement(@intCast(i));
const pk = try env.unwrap(PublicKey, pk_value);
pks[i] = pk.*;
pks[i] = try env.unwrap(PublicKey, pk_value);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

MEDIUM: Per the style guide, explicitly pass options to env.unwrap to make the code more readable and prevent potential issues if the library's defaults change. See Repository Style Guide, lines 170-173.

        pks[i] = try env.unwrap(PublicKey, pk_value, .{}); // Explicitly pass options

References
  1. Explicitly pass options to library functions at the call site, instead of relying on the defaults. This improves readability and avoids latent bugs if the library changes its defaults. (link)

}

var pairing_buf: [Pairing.sizeOf()]u8 align(@alignOf(Pairing)) = undefined;
Expand Down Expand Up @@ -717,13 +716,12 @@ pub fn blst_aggregateSignatures(env: napi.Env, cb: napi.CallbackInfo(2)) !napi.V

if (sigs_len == 0) return error.EmptySignatureArray;

const sigs = try allocator.alloc(Signature, sigs_len);
const sigs = try allocator.alloc(*const Signature, sigs_len);
defer allocator.free(sigs);

for (0..sigs_len) |i| {
const sig_value = try sigs_array.getElement(@intCast(i));
const sig = try env.unwrap(Signature, sig_value);
sigs[i] = sig.*;
sigs[i] = try env.unwrap(Signature, sig_value);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

MEDIUM: For consistency and to adhere to the style guide, explicitly pass options to env.unwrap. See Repository Style Guide, lines 170-173.

        sigs[i] = try env.unwrap(Signature, sig_value, .{}); // Explicitly pass options

References
  1. Explicitly pass options to library functions at the call site, instead of relying on the defaults. This improves readability and avoids latent bugs if the library changes its defaults. (link)

}

const agg_sig = AggregateSignature.aggregate(sigs, sigs_groupcheck) catch return error.AggregationFailed;
Expand Down Expand Up @@ -754,13 +752,12 @@ pub fn blst_aggregatePublicKeys(env: napi.Env, cb: napi.CallbackInfo(2)) !napi.V
return error.EmptyPublicKeyArray;
}

const pks = try allocator.alloc(PublicKey, pks_len);
const pks = try allocator.alloc(*const PublicKey, pks_len);
defer allocator.free(pks);

for (0..pks_len) |i| {
const pk_value = try pks_array.getElement(@intCast(i));
const pk = try env.unwrap(PublicKey, pk_value);
pks[i] = pk.*;
pks[i] = try env.unwrap(PublicKey, pk_value);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

MEDIUM: To improve code clarity and prevent potential issues, explicitly pass options to env.unwrap. See Repository Style Guide, lines 170-173.

        pks[i] = try env.unwrap(PublicKey, pk_value, .{}); // Explicitly pass options

References
  1. Explicitly pass options to library functions at the call site, instead of relying on the defaults. This improves readability and avoids latent bugs if the library changes its defaults. (link)

}

const agg_pk = AggregatePublicKey.aggregate(pks, pks_validate) catch return error.AggregationFailed;
Expand Down Expand Up @@ -789,16 +786,19 @@ pub fn blst_aggregateSerializedPublicKeys(env: napi.Env, cb: napi.CallbackInfo(2

const pks = try allocator.alloc(PublicKey, pks_len);
defer allocator.free(pks);
const pk_ptrs = try allocator.alloc(*const PublicKey, pks_len);
defer allocator.free(pk_ptrs);

for (0..pks_len) |i| {
const pk_bytes_value = try pks_array.getElement(@intCast(i));
const bytes_info = try pk_bytes_value.getTypedarrayInfo();

pks[i] = PublicKey.deserialize(bytes_info.data) catch
return error.DeserializationFailed;
pk_ptrs[i] = &pks[i];
}

const agg_pk = AggregatePublicKey.aggregate(pks, pks_validate) catch return error.AggregationFailed;
const agg_pk = AggregatePublicKey.aggregate(pk_ptrs, pks_validate) catch return error.AggregationFailed;
const result_pk = agg_pk.toPublicKey();

const pk_value = try newPublicKeyInstance(env);
Expand Down
9 changes: 7 additions & 2 deletions src/bls/AggregatePublicKey.zig
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub fn toPublicKey(self: *const Self) PublicKey {
/// If pks_validate is true, validates each public key before aggregation.
///
/// Returns an error if the slice is empty or if any public key validation fails.
pub fn aggregate(pks: []const PublicKey, pks_validate: bool) BlstError!Self {
pub fn aggregate(pks: []const *const PublicKey, pks_validate: bool) BlstError!Self {
if (pks.len == 0) return BlstError.AggrTypeMismatch;
if (pks_validate) for (pks) |pk| try pk.validate();

Expand Down Expand Up @@ -144,7 +144,12 @@ test aggregate {
sigs[i] = sig;
}

_ = try aggregate(pks[0..], true);
var pk_ptrs: [num_sigs]*const PublicKey = undefined;
for (0..num_sigs) |i| {
pk_ptrs[i] = &pks[i];
}

_ = try aggregate(&pk_ptrs, true);
}

const std = @import("std");
Expand Down
2 changes: 1 addition & 1 deletion src/bls/AggregateSignature.zig
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub fn toSignature(self: *const Self) Signature {
///
/// Validates each signature before aggregation if `sigs_groupcheck` is true.
/// Errors if the `sigs` slice is empty or if any signature validation fails.
pub fn aggregate(sigs: []const Signature, sigs_groupcheck: bool) BlstError!Self {
pub fn aggregate(sigs: []const *const Signature, sigs_groupcheck: bool) BlstError!Self {
if (sigs.len == 0) return BlstError.AggrTypeMismatch;
if (sigs_groupcheck) for (sigs) |sig| try sig.validate(false);

Expand Down
24 changes: 15 additions & 9 deletions src/bls/Signature.zig
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pub fn aggregateVerify(
buffer: *align(Pairing.buf_align) [Pairing.sizeOf()]u8,
msgs: []const [32]u8,
dst: []const u8,
pks: []const PublicKey,
pks: []const *const PublicKey,
pks_validate: bool,
) BlstError!bool {
const n_elems = pks.len;
Expand All @@ -73,7 +73,7 @@ pub fn aggregateVerify(
}
var pairing = Pairing.init(buffer, true, dst);
try pairing.aggregate(
&pks[0],
pks[0],
pks_validate,
Comment on lines +76 to 77
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

CRITICAL: The code is dereferencing a pointer without ensuring that the pks slice is not empty. Accessing pks[0] when pks is empty will cause a crash. Add a check to ensure pks.len > 0 before accessing pks[0].

    if (n_elems == 0 or msgs.len != n_elems) {
        return BlstError.VerifyFail;
    }
    if (pks.len == 0) {
        return BlstError.VerifyFail; // Add this check
    }
    var pairing = Pairing.init(buffer, true, dst);
    try pairing.aggregate(
        pks[0],
        pks_validate,
        self,
        sig_groupcheck,
        &msgs[0],
        null,
    );
References
  1. Ensure that slices are not empty before accessing their elements to prevent out-of-bounds access.

self,
sig_groupcheck,
Expand All @@ -83,7 +83,7 @@ pub fn aggregateVerify(

for (1..n_elems) |i| {
try pairing.aggregate(
&pks[i],
pks[i],
pks_validate,
null,
sig_groupcheck,
Expand All @@ -108,7 +108,7 @@ pub fn fastAggregateVerify(
buffer: *align(Pairing.buf_align) [Pairing.sizeOf()]u8,
msg: *const [32]u8,
dst: []const u8,
pks: []const PublicKey,
pks: []const *const PublicKey,
pks_validate: bool,
) BlstError!bool {
const agg_pk = try AggregatePublicKey.aggregate(pks, pks_validate);
Expand All @@ -119,7 +119,7 @@ pub fn fastAggregateVerify(
buffer,
@ptrCast(msg),
dst,
&[_]PublicKey{pk},
&[_]*const PublicKey{&pk},
false,
);
}
Expand All @@ -135,13 +135,12 @@ pub fn fastAggregateVerifyPreAggregated(
dst: []const u8,
pk: *const PublicKey,
) BlstError!bool {
const pks: [*]const PublicKey = @ptrCast(pk);
return try self.aggregateVerify(
sig_groupcheck,
buffer,
@ptrCast(msg),
dst,
pks[0..1],
&[_]*const PublicKey{pk},
false,
);
}
Expand Down Expand Up @@ -281,8 +280,15 @@ test aggregateVerify {
sigs[i] = sig;
}

const agg_sig = try AggregateSignature.aggregate(&sigs, false);
var sig_ptrs: [num_sigs]*const @This() = undefined;
var pk_ptrs: [num_sigs]*const PublicKey = undefined;
for (0..num_sigs) |i| {
sig_ptrs[i] = &sigs[i];
pk_ptrs[i] = &pks[i];
}

const agg_sig = try AggregateSignature.aggregate(&sig_ptrs, false);
const sig = @This().fromAggregate(&agg_sig);

try std.testing.expect(try sig.aggregateVerify(false, &buffer, &msgs, dst, &pks, false));
try std.testing.expect(try sig.aggregateVerify(false, &buffer, &msgs, dst, &pk_ptrs, false));
}
6 changes: 5 additions & 1 deletion src/bls/ThreadPool.zig
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,11 @@ test "aggregateVerify multi-threaded" {
pk_ptrs[i] = &pks[i];
}

const agg_sig = AggregateSignature.aggregate(&sigs, false) catch return error.AggregationFailed;
var sig_ptrs: [num_sigs]*const Signature = undefined;
for (0..num_sigs) |i| {
sig_ptrs[i] = &sigs[i];
}
const agg_sig = AggregateSignature.aggregate(&sig_ptrs, false) catch return error.AggregationFailed;
const final_sig = agg_sig.toSignature();

try std.testing.expect(try pool.aggregateVerify(
Expand Down
10 changes: 5 additions & 5 deletions src/state_transition/block/process_sync_committee.zig
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ pub fn processSyncAggregate(
const root_signed = try getBlockRootAtSlot(fork, state, previous_slot);
const domain = try config.getDomain(epoch_cache.epoch, c.DOMAIN_SYNC_COMMITTEE, previous_slot);

const pubkeys = try allocator.alloc(bls.PublicKey, participant_indices.items.len);
const pubkeys = try allocator.alloc(*const bls.PublicKey, participant_indices.items.len);
defer allocator.free(pubkeys);
for (0..participant_indices.items.len) |i| {
pubkeys[i] = epoch_cache.index_to_pubkey.items[participant_indices.items[i]];
pubkeys[i] = &epoch_cache.index_to_pubkey.items[participant_indices.items[i]];
}

var signing_root: Root = undefined;
Expand Down Expand Up @@ -158,9 +158,9 @@ pub fn getSyncCommitteeSignatureSet(

const domain = try config.getDomain(epoch_cache.epoch, c.DOMAIN_SYNC_COMMITTEE, previous_slot);

const pubkeys = try allocator.alloc(bls.PublicKey, participant_indices_.len);
const pubkeys = try allocator.alloc(*const bls.PublicKey, participant_indices_.len);
for (0..participant_indices_.len) |i| {
pubkeys[i] = epoch_cache.index_to_pubkey.items[participant_indices_[i]];
pubkeys[i] = &epoch_cache.index_to_pubkey.items[participant_indices_[i]];
}
var signing_root: Root = undefined;
try computeSigningRoot(types.primitive.Root, &root_signed, domain, &signing_root);
Expand Down Expand Up @@ -199,7 +199,7 @@ test "process sync aggregate - sanity" {
const sig0 = try test_utils.interopSign(committee_indices[0], &signing_root);
// validator 1 signs
const sig1 = try test_utils.interopSign(committee_indices[1], &signing_root);
const agg_sig = try bls.AggregateSignature.aggregate(&.{ sig0, sig1 }, true);
const agg_sig = try bls.AggregateSignature.aggregate(&.{ &sig0, &sig1 }, true);

var sync_aggregate: types.electra.SyncAggregate.Type = types.electra.SyncAggregate.default_value;
sync_aggregate.sync_committee_signature = agg_sig.toSignature().compress();
Expand Down
4 changes: 2 additions & 2 deletions src/state_transition/signature_sets/indexed_attestation.zig
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ pub fn getAttestationWithIndicesSignatureSet(
signature: BLSSignature,
attesting_indices: []u64,
) !AggregatedSignatureSet {
const pubkeys = try allocator.alloc(PublicKey, attesting_indices.len);
const pubkeys = try allocator.alloc(*const PublicKey, attesting_indices.len);
errdefer allocator.free(pubkeys);
for (0..attesting_indices.len) |i| {
pubkeys[i] = epoch_cache.index_to_pubkey.items[@intCast(attesting_indices[i])];
pubkeys[i] = &epoch_cache.index_to_pubkey.items[@intCast(attesting_indices[i])];
}

var signing_root: Root = undefined;
Expand Down
4 changes: 3 additions & 1 deletion src/state_transition/test_utils/generate_state.zig
Original file line number Diff line number Diff line change
Expand Up @@ -139,18 +139,20 @@ pub fn generateElectraState(allocator: Allocator, pool: *Node.Pool, chain_config

var next_sync_committee_pubkeys: [preset.SYNC_COMMITTEE_SIZE]BLSPubkey = undefined;
var next_sync_committee_pubkeys_slices: [preset.SYNC_COMMITTEE_SIZE]bls.PublicKey = undefined;
var next_sync_committee_pubkey_ptrs: [preset.SYNC_COMMITTEE_SIZE]*const bls.PublicKey = undefined;
var validators = try beacon_state.validators();
for (next_sync_committee_indices, 0..next_sync_committee_indices.len) |index, i| {
var validator = try validators.get(@intCast(index));
var pubkey_view = try validator.get("pubkey");
_ = try pubkey_view.getAllInto(next_sync_committee_pubkeys[i][0..]);
next_sync_committee_pubkeys_slices[i] = try bls.PublicKey.uncompress(&next_sync_committee_pubkeys[i]);
next_sync_committee_pubkey_ptrs[i] = &next_sync_committee_pubkeys_slices[i];
}

var current_sync_committee = try beacon_state.currentSyncCommittee();
var next_sync_committee = try beacon_state.nextSyncCommittee();
// Rotate syncCommittee in state
const aggregate_pubkey = (try bls.AggregatePublicKey.aggregate(&next_sync_committee_pubkeys_slices, false)).toPublicKey().compress();
const aggregate_pubkey = (try bls.AggregatePublicKey.aggregate(&next_sync_committee_pubkey_ptrs, false)).toPublicKey().compress();
try next_sync_committee.setValue("pubkeys", &next_sync_committee_pubkeys);
try next_sync_committee.setValue("aggregate_pubkey", &aggregate_pubkey);

Expand Down
7 changes: 3 additions & 4 deletions src/state_transition/utils/bls.zig
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub fn verify(msg: []const u8, pk: *const PublicKey, sig: *const Signature, in_p
try sig.verify(sig_groupcheck, msg, DST, null, pk, pk_validate);
}

pub fn fastAggregateVerify(msg: []const u8, pks: []const PublicKey, sig: *const Signature, in_pk_validate: ?bool, in_sigs_group_check: ?bool) !bool {
pub fn fastAggregateVerify(msg: []const u8, pks: []const *const PublicKey, sig: *const Signature, in_pk_validate: ?bool, in_sigs_group_check: ?bool) !bool {
var pairing_buf: [bls.Pairing.sizeOf()]u8 align(@alignOf(bls.Pairing)) = undefined;

const sigs_groupcheck = in_sigs_group_check orelse false;
Expand All @@ -44,8 +44,7 @@ test "bls - sanity" {
const pk = sk.toPublicKey();
try verify(&msg, &pk, &sig, null, null);

var pks = [_]PublicKey{pk};
var pks_slice: []const PublicKey = pks[0..1];
const result = try fastAggregateVerify(&msg, pks_slice[0..], &sig, null, null);
var pk_ptrs = [_]*const PublicKey{&pk};
const result = try fastAggregateVerify(&msg, &pk_ptrs, &sig, null, null);
try std.testing.expect(result);
}
5 changes: 2 additions & 3 deletions src/state_transition/utils/signature_sets.zig
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ pub const SingleSignatureSet = struct {
};

pub const AggregatedSignatureSet = struct {
// fastAggregateVerify also requires []*const PublicKey
pubkeys: []const PublicKey,
pubkeys: []const *const PublicKey,
signing_root: Root,
signature: BLSSignature,
};
Expand Down Expand Up @@ -47,7 +46,7 @@ pub fn createSingleSignatureSetFromComponents(pubkey: *const PublicKey, signing_
};
}

pub fn createAggregateSignatureSetFromComponents(pubkeys: []const PublicKey, signing_root: Root, signature: BLSSignature) AggregatedSignatureSet {
pub fn createAggregateSignatureSetFromComponents(pubkeys: []const *const PublicKey, signing_root: Root, signature: BLSSignature) AggregatedSignatureSet {
return .{
.pubkeys = pubkeys,
.signing_root = signing_root,
Expand Down
4 changes: 3 additions & 1 deletion src/state_transition/utils/sync_committee.zig
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,17 @@ pub fn getNextSyncCommittee(
// Using the index2pubkey cache is slower because it needs the serialized pubkey.
const pubkeys = &out.sync_committee.pubkeys;
var pubkeys_uncompressed: [preset.SYNC_COMMITTEE_SIZE]bls.PublicKey = undefined;
var pubkey_ptrs: [preset.SYNC_COMMITTEE_SIZE]*const bls.PublicKey = undefined;
for (indices, 0..indices.len) |index, i| {
var validator_view = try validators_view.get(index);
var validator: types.phase0.Validator.Type = undefined;
try validator_view.toValue(allocator, &validator);
pubkeys[i] = validator.pubkey;
pubkeys_uncompressed[i] = try bls.PublicKey.uncompress(&pubkeys[i]);
pubkey_ptrs[i] = &pubkeys_uncompressed[i];
}

const aggregated_pk = try AggregatePublicKey.aggregate(&pubkeys_uncompressed, false);
const aggregated_pk = try AggregatePublicKey.aggregate(&pubkey_ptrs, false);
out.sync_committee.aggregate_pubkey = aggregated_pk.toPublicKey().compress();
}

Expand Down
Loading
Loading