Skip to content
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

Merged
merged 13 commits into from
May 18, 2024

Conversation

Vindaar
Copy link
Collaborator

@Vindaar Vindaar commented May 9, 2024

Adds the C API for BLS signature aggregate_verify and batch_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, maybe ctt_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 a toOpenArray for the messages. In addition it fixes a minor bug in the update proc for the View[byte] overload, which lead to an infinite recursion.

@Vindaar Vindaar mentioned this pull request May 9, 2024

```sh
gcc ethereum_bls_signatures.c -o ethereum_bls_signatures -I../include -L../lib -lconstantine
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
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


```sh
LD_LIBRARY_PATH=../lib ./ethereum_bls_signutares
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
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;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Collaborator Author

@Vindaar Vindaar May 11, 2024

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.

Copy link
Owner

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.

mratsim
mratsim previously approved these changes May 16, 2024
Copy link
Owner

@mratsim mratsim left a 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!

{ 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.
Copy link
Owner

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

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

@mratsim mratsim merged commit 070e6eb into master May 18, 2024
12 checks passed
@mratsim mratsim deleted the blsSigCAPI branch May 18, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants