Repository navigation
perf(bls): add cache-aware signature verifier #562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
0f53b80
eb09dce
9ec52ec
748bf8c
d8013f6
f76824f
ab1bf3d
7f86f6d
1b3f9ad
b9e339b
f3985dd
345ba3c
7cc2b0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,254 @@ | ||
| const std = @import("std"); | ||
| const zapi = @import("zapi:zapi"); | ||
| const js = zapi.js; | ||
| const napi = zapi.napi; | ||
| const bls = @import("bls"); | ||
| const preset = @import("preset").preset; | ||
| const blst_bindings = @import("./blst.zig"); | ||
| const blst_verifier = @import("./blst_verifier.zig"); | ||
| const pubkeys = @import("./pubkeys.zig"); | ||
|
|
||
| const NativePublicKey = bls.PublicKey; | ||
| const NativeSignature = bls.Signature; | ||
| const BatchVerifyItem = bls.BatchVerifyItem; | ||
| const DST = bls.DST; | ||
|
|
||
| const max_verify_sets = 256; | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| const max_same_message_sets = bls.MAX_AGGREGATE_PER_JOB; | ||
| const max_indices_per_set = preset.MAX_VALIDATORS_PER_COMMITTEE * preset.MAX_COMMITTEES_PER_SLOT; | ||
|
|
||
| const SetType = enum(u32) { | ||
| indexed = 0, | ||
| aggregate = 1, | ||
| single = 2, | ||
| }; | ||
|
|
||
| fn uint8Slice(value: napi.Value) ![]u8 { | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| return blst_verifier.uint8Slice(value); | ||
| } | ||
|
|
||
| fn uint32Slice(value: napi.Value) ![]u32 { | ||
| if (!(try value.isTypedarray())) return error.TypeMismatch; | ||
| const info = try value.getTypedarrayInfo(); | ||
| if (info.array_type != .uint32) return error.TypeMismatch; | ||
|
|
||
| const byte_ptr: [*]u8 = info.data.ptr; | ||
| const typed_ptr: [*]u32 = @ptrCast(@alignCast(byte_ptr)); | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| return typed_ptr[0..info.length]; | ||
| } | ||
|
|
||
| fn uint32(value: napi.Value) !u32 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. may bump the zapi to use PR
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes that would be nice. will leave for a separate PR since next release of zapi will be breaking |
||
| const number = try value.getValueDouble(); | ||
| const max_u32: f64 = @floatFromInt(std.math.maxInt(u32)); | ||
| if (!std.math.isFinite(number) or number < 0 or number > max_u32 or @floor(number) != number) { | ||
| return error.InvalidUint32; | ||
| } | ||
| return @intFromFloat(number); | ||
| } | ||
|
|
||
| fn parseMessage(set: napi.Value) ![32]u8 { | ||
| const message = try uint8Slice(try set.getNamedProperty("message")); | ||
| if (message.len != 32) return error.InvalidMessageLength; | ||
| return message[0..32].*; | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| } | ||
|
|
||
| fn parseSignature(set: napi.Value) !?NativeSignature { | ||
| const bytes = try uint8Slice(try set.getNamedProperty("signature")); | ||
| return blst_verifier.parseSignature(bytes); | ||
| } | ||
|
|
||
| fn parsePublicKey(set: napi.Value) !?NativePublicKey { | ||
| const bytes = try uint8Slice(try set.getNamedProperty("pubkey")); | ||
| if (bytes.len != NativePublicKey.COMPRESS_SIZE and | ||
| bytes.len != NativePublicKey.SERIALIZE_SIZE) | ||
| { | ||
| return null; | ||
| } | ||
| var public_key = NativePublicKey.deserialize(bytes) catch return null; | ||
| public_key.validate() catch return null; | ||
| return public_key; | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| } | ||
|
|
||
| fn resolvePublicKey(set: napi.Value, set_type: SetType) !?NativePublicKey { | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| const io = js.io(); | ||
| return switch (set_type) { | ||
| .indexed => blk: { | ||
| if (!pubkeys.state.initialized) return error.PubkeyIndexNotInitialized; | ||
| const index = try uint32(try set.getNamedProperty("index")); | ||
| break :blk pubkeys.state.cache.getPubkey(io, index) orelse | ||
| return error.PubkeyIndexNotFound; | ||
| }, | ||
| .aggregate => blk: { | ||
| if (!pubkeys.state.initialized) return error.PubkeyIndexNotInitialized; | ||
| const indices = try uint32Slice(try set.getNamedProperty("indices")); | ||
| if (indices.len == 0) return error.EmptyIndices; | ||
| if (indices.len > max_indices_per_set) return error.TooManyIndices; | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
|
|
||
| break :blk pubkeys.state.cache.aggregateU32(io, indices) catch |err| switch (err) { | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| error.InvalidIndex => return error.PubkeyIndexNotFound, | ||
| error.InvalidLength => return error.EmptyIndices, | ||
| }; | ||
| }, | ||
| .single => try parsePublicKey(set), | ||
| }; | ||
| } | ||
|
|
||
| /// Verify indexed, aggregate, and raw-pubkey signature sets synchronously. | ||
| /// | ||
| /// Invalid cryptographic inputs return false. Cache misses and malformed | ||
| /// interface inputs throw so callers do not classify operational failures as | ||
| /// invalid signatures. | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| pub fn verifySignatureSets(sets: js.Array) !js.Boolean { | ||
| const count = try sets.length(); | ||
| if (count == 0) return js.Boolean.from(false); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. semantics nit: would passing an empty set count as cryptographic failure? I guess in some ways it can be interpreted as such (passing in sets of length 0 = nothing to verify = not verified?) |
||
| if (count > max_verify_sets) return error.TooManySets; | ||
|
|
||
| var messages: [max_verify_sets][32]u8 = undefined; | ||
| var public_keys: [max_verify_sets]NativePublicKey = undefined; | ||
| var signatures: [max_verify_sets]NativeSignature = undefined; | ||
| var items: [max_verify_sets]BatchVerifyItem = undefined; | ||
|
|
||
| for (0..count) |i| { | ||
| const set = (try sets.get(@intCast(i))).toValue(); | ||
| const set_type: SetType = switch (try uint32(try set.getNamedProperty("type"))) { | ||
| @intFromEnum(SetType.indexed) => .indexed, | ||
| @intFromEnum(SetType.aggregate) => .aggregate, | ||
| @intFromEnum(SetType.single) => .single, | ||
| else => return error.InvalidSetType, | ||
| }; | ||
|
|
||
| messages[i] = try parseMessage(set); | ||
|
|
||
| public_keys[i] = (try resolvePublicKey(set, set_type)) orelse | ||
| return js.Boolean.from(false); | ||
|
|
||
| signatures[i] = (try parseSignature(set)) orelse | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| return js.Boolean.from(false); | ||
|
|
||
| items[i] = .{ | ||
| .message = messages[i], | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| .public_key = &public_keys[i], | ||
| .signature = &signatures[i], | ||
| .randomness = undefined, | ||
| }; | ||
| } | ||
|
|
||
| if (count == 1) { | ||
| signatures[0].verify( | ||
| false, | ||
| &messages[0], | ||
| DST, | ||
| null, | ||
| &public_keys[0], | ||
| false, | ||
| ) catch return js.Boolean.from(false); | ||
| return js.Boolean.from(true); | ||
| } | ||
|
|
||
| const pool = blst_bindings.state.thread_pool orelse | ||
| return error.ThreadPoolNotInitialized; | ||
| const result = try blst_verifier.verifySignatureSets( | ||
| js.io(), | ||
| pool, | ||
| items[0..count], | ||
| false, | ||
| false, | ||
| true, | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| ); | ||
|
|
||
| return js.Boolean.from(result); | ||
| } | ||
|
|
||
| /// Randomly aggregate and verify indexed signatures over the same message. | ||
| /// Returns one result per input, falling back to individual checks only when | ||
| /// the aggregate check fails. | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| pub fn verifySignatureSetsSameMessage(sets: js.Array, message: js.Uint8Array) !js.Array { | ||
| const count = try sets.length(); | ||
| if (count > max_same_message_sets) return error.TooManySets; | ||
|
|
||
| const results = js.Array.createWithLength(count); | ||
| if (count == 0) return results; | ||
|
|
||
| const message_slice = try message.toSlice(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The refactor making message as a pointer seems violate the NAPI v8 memory rule, because there are some js function calls may triggered GC before get the value later. And store the pointer in the item seems also not safety.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed. The shared message slice was retained across per-set property reads that can execute JavaScript. I added a regression where a signature getter mutates the source message, watched it fail as
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you check all such kind of code including the existing code? I suspected there are some similar issues.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I audited the BLS NAPI typed-array lifetimes and found three additional affected paths: the mixed-set verifier,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think issue is GC, by itself, here. Its a synchronous function with a handle scope around the Uint8Array. The handle scope ensures that the Uint8Array doesn't get GC'd. Any synchronous function has the same guarantees. But there is a real (a contrived, but real nonetheless) re-entrency risk, where an overridden getter of a later object access can modify the Uint8Array, or detach the underlying ArrayBuffer. Imo these kinds of getters are deliberately contrived and malicious, and obviously not what Lodestar does or what these bindings need to handle (This is all trusted input, lodestar just constructs vanilla objects). Happy to change it but its worth noting that this is a very low-risk correctness hardening, not a real production bug.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I noticed most are
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes—the distinction is liveness versus stability. The handle scope keeps the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, seems correct, but the zapi comment for typed array seems a little bit inaccurate, so that we don't need #564. |
||
| if (message_slice.len != 32) return error.InvalidMessageLength; | ||
| const exact_message = message_slice[0..32].*; | ||
|
|
||
| if (!pubkeys.state.initialized) return error.PubkeyIndexNotInitialized; | ||
|
|
||
| var public_keys: [max_same_message_sets]NativePublicKey = undefined; | ||
| var public_key_refs: [max_same_message_sets]*const NativePublicKey = undefined; | ||
| var signatures: [max_same_message_sets]NativeSignature = undefined; | ||
| var signature_refs: [max_same_message_sets]*const NativeSignature = undefined; | ||
| var signature_valid: [max_same_message_sets]bool = undefined; | ||
| var can_aggregate = true; | ||
|
|
||
| const io = js.io(); | ||
| for (0..count) |i| { | ||
| const set = (try sets.get(@intCast(i))).toValue(); | ||
| const index = try uint32(try set.getNamedProperty("index")); | ||
| public_keys[i] = pubkeys.state.cache.getPubkey(io, index) orelse | ||
| return error.PubkeyIndexNotFound; | ||
| public_key_refs[i] = &public_keys[i]; | ||
|
|
||
| if (try parseSignature(set)) |signature| { | ||
| signatures[i] = signature; | ||
| signature_refs[i] = &signatures[i]; | ||
| signature_valid[i] = true; | ||
| } else { | ||
| signature_valid[i] = false; | ||
| can_aggregate = false; | ||
| } | ||
| } | ||
|
|
||
| if (can_aggregate) { | ||
| const pool = blst_bindings.state.thread_pool orelse | ||
| return error.ThreadPoolNotInitialized; | ||
| if (try blst_verifier.verifySameMessage( | ||
| io, | ||
| pool, | ||
| public_key_refs[0..count], | ||
| signature_refs[0..count], | ||
| &exact_message, | ||
| )) { | ||
| for (0..count) |i| try results.set(@intCast(i), js.Boolean.from(true)); | ||
| return results; | ||
| } | ||
| } | ||
|
|
||
| for (0..count) |i| { | ||
| const is_valid = signature_valid[i] and blk: { | ||
| signatures[i].verify( | ||
| false, | ||
| &exact_message, | ||
| DST, | ||
| null, | ||
| &public_keys[i], | ||
| false, | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| ) catch break :blk false; | ||
| break :blk true; | ||
| }; | ||
| try results.set(@intCast(i), js.Boolean.from(is_valid)); | ||
|
wemeetagain marked this conversation as resolved.
Resolved
|
||
| } | ||
|
|
||
| return results; | ||
| } | ||
|
|
||
| pub fn indexedSetType() js.Number { | ||
| return js.Number.from(@intFromEnum(SetType.indexed)); | ||
| } | ||
|
|
||
| pub fn aggregateSetType() js.Number { | ||
| return js.Number.from(@intFromEnum(SetType.aggregate)); | ||
| } | ||
|
|
||
| pub fn singleSetType() js.Number { | ||
| return js.Number.from(@intFromEnum(SetType.single)); | ||
| } | ||
|
|
||
| pub fn maxBatchSize() js.Number { | ||
| return js.Number.from(max_verify_sets); | ||
| } | ||
|
|
||
| pub fn maxSameMessageBatchSize() js.Number { | ||
| return js.Number.from(max_same_message_sets); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.