-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
C API for BLS signature aggregate_verify
& batch_verify
(incl parallel)
#381
Conversation
examples-c/README.md
Outdated
|
||
```sh | ||
gcc ethereum_bls_signatures.c -o ethereum_bls_signatures -I../include -L../lib -lconstantine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gcc ethereum_bls_signatures.c -o ethereum_bls_signatures -I../include -L../lib -lconstantine | |
clang ethereum_bls_signatures.c -o ethereum_bls_signatures -I../include -L../lib -lconstantine |
I'd rather use Clang everywhere as example.
GCC performance is just bad #357
examples-c/README.md
Outdated
|
||
```sh | ||
LD_LIBRARY_PATH=../lib ./ethereum_bls_signutares |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LD_LIBRARY_PATH=../lib ./ethereum_bls_signutares | |
LD_LIBRARY_PATH=../lib ./ethereum_bls_signatures |
// type View*[byte] = object # with T = byte | ||
// data: ptr UncheckedArray[byte] # 8 bytes | ||
// len*: int # 8 bytes (Nim `int` is a 64bit int type) | ||
typedef struct { byte* data; size_t len; } ByteView; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typedef struct { byte* data; size_t len; } ByteView; | |
typedef struct { byte* data; size_t len; } ctt_span; |
C++20 and C# have settled to call this abstraction a span, see https://www.cppstories.com/2023/span-cpp20/, and original proposal: https://open-std.org/JTC1/SC22/WG21/docs/papers/2016/p0122r1.pdf
I think we should follow suit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we 'embrace' it like this, I guess we could consider moving it to core/datatypes.h
as well?
edit: Also, we don't want to specify that this is just a span for a byte type? Given that in Nim / C++ it would be a generic / template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to my. Yes we can call it ctt_span_bytes
, but I think it's fine for now, it's only used for arrays of messages. For the others we just have pointer+length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A not on the randomness source, otherwise LGTM!
examples-c/ethereum_bls_signatures.c
Outdated
{ message, 32 } | ||
}; | ||
const ctt_eth_bls_signature sigs[3] = { sig, sig, sig }; | ||
byte srb[32] = {0}; // just a bunch of zeros as random "secure" bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bunch of 7, 8 or anything is better, because internally we multiply the randomness with the inputs and multiplying with 0 gives 0, and pairing of a zero input always gives 1.
To prevent that we re-hash here
constantine/constantine/signatures/bls_signatures.nim
Lines 404 to 411 in 2a2bf9c
while true: # Ensure we don't multiply by 0 for blinding | |
H.hash(ctx.secureBlinding, ctx.secureBlinding) | |
var accum = byte 0 | |
for i in 0 ..< 8: | |
accum = accum or ctx.secureBlinding[i] | |
if accum != byte 0: | |
break |
But I think it is a good occasion to show sysrand
We had a leftover generic and the `messages` were passed not as an openArray as required
The previous code recursed into itself instead of the main `update` above
Adds the C API for BLS signature
aggregate_verify
andbatch_verify
functions. The parallel version using a threadpool is also exposed.Note: For the batched API we currently provide a
ByteView
typedef on the C side (happy to choose a different name, probably better snake_case and lower case, maybectt_byte_view
?). We could do an anonymous struct in the function signatures, but at least I'm not aware of a way to get around 'incompatible pointer' warnings with GCC / Clang in that case.It includes 2 minor fixes for the
batch_verify(_parallel)
Nim C targeted procs. They were accidentally generic and missing atoOpenArray
for themessages
. In addition it fixes a minor bug in theupdate
proc for theView[byte]
overload, which lead to an infinite recursion.