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

Rust API for BLS signatures and EVM precompiles #387

Merged
merged 8 commits into from
Jun 23, 2024
Merged

Conversation

Vindaar
Copy link
Collaborator

@Vindaar Vindaar commented May 30, 2024

And here we are finally for the first part of the Rust API, namely BLS signatures.

This is still a draft, because there's again a few questions about how this should ideally be wrapped. Currently the code simply uses type aliases similar to the Go API. This is "fine", but leads to the user having to write code like:

        let mut pub_key = MaybeUninit::<EthBlsPubKey>::uninit();
        let status = deserialize_pubkey_compressed(pub_key.as_mut_ptr(), inp.as_ref());
        let pub_key = unsafe { pub_key.assume_init() };

which imo is rather ugly.

2 alternatives come to mind:

  • at least for these types simply have deserialize_*_compressed (and friends) return the deserialized type directly. That way we can hide the unsafe code in the fake constructor.
  • wrap the C types in a Rust struct and abstract it away that way. This approach will just mean we have to do more work on API implementation side, but at least we get shorter function names (because we can then "overload" names like deserialize) and we shouldn't suffer from copies, because we can of course just pass the pointers of the underlying object to C.

Another point:
Funnily (or ironically), while we can nicely turn the nested messages in batch_verify etc. into a bunch of CttSpans, we still have to make a copy of the data due to Rusts extreme stance on pointer mutability. Given that the messages argument is just a reference (wouldn't make sense to force it to be &mut), we are not allowed to turn them into a CttSpan (or ctt_span to be exact), because it is defined as having a byte* field and not a const byte* field. I'm a bit perplexed that we cannot even cast an immutable to a mutable pointer inside of an unsafe block. At least that's my understanding so far.
I'm not sure if we want to restrict us so far as to have the ctt_span have a const byte* field, because there should be valid reasons to use the type for mutation as well? We could of course have a ctt_span_const, but that's also a bit ridiculous.

Anyway, while the tests are not pretty due to so much error checking and MaybeUninit work, at least they all pass.

@Vindaar Vindaar changed the title Rust API for BLS signatures Rust API for BLS signatures and EVM precompiles May 30, 2024
@Vindaar
Copy link
Collaborator Author

Vindaar commented May 30, 2024

Just finished porting over the EVM precompiles as well. Decided to just do it in the same PR now.

@Vindaar Vindaar force-pushed the goEvmApi branch 2 times, most recently from c0c8f0d to 61d0599 Compare June 12, 2024 10:42
Base automatically changed from goEvmApi to master June 12, 2024 11:26
@Vindaar Vindaar marked this pull request as ready for review June 13, 2024 20:23
Done by running:

```sh
sh scripts/gen_rust_bindings.sh
```
from Constantine's root dir
NOTE: I'm not sure right now whether `lib.rs` is *also* generated by
`bindgen` or not? I would guess not? Because in that case this change
wouldn't be a good idea obviously (overwritten on next wrapper update)
@mratsim mratsim merged commit af7fa73 into master Jun 23, 2024
12 checks passed
@mratsim mratsim deleted the rustBlsSigApi branch June 23, 2024 14: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