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

Constantine backend #251

Merged
merged 17 commits into from
Jan 15, 2024
Merged

Conversation

lynxcs
Copy link
Contributor

@lynxcs lynxcs commented Dec 31, 2023

Constantine backend wasn't fully integrated / has some caveats:

  • C-KZG-4844 C bindings use constantine ECC lib, but our implementations of eip4844 functions, this is because required constantine context can only be loaded from file path, whereas bindings require 2 loading methods - from g1/g2 bytes & from FILE* (recovering file path might be possible on linux but would be quite hacky)
  • Added 'mixed' kzg settings variant, which can either be our generic kzg settings which use our implementation of eip4844 functions, or constantine implementations, HOWEVER, the 'interface' which we expose for those functions from rust side has already done conversions from bytes to types, but the constantine version (kinda correctly) requires the byte representation, so the constantine version has a (pretty significant) additional conversion cost.
  • Integrating 'fully' won't be possible, because kzg context internals are hidden, whereas our KzgSettings trait requires access to g1/g2 secrets & a few other things

To be fair to constantine ECC backend in benchmarks, a total of 3 versions were created:

  • regular/generic eip4844 benchmark - constantine ECC lib, but our eip4844 impls (can be used to compare 'pure' ECC operation performance between backends)
  • 'mixed' eip4844 benchmark - constantine ECC lib, constantine eip4844 funcs, but our interface, which has conversion penalty
  • 'fully' constantine benchmark - constantine ECC lib, constantine eip4844 funcs, no byte conversion done beforehand. This is the best representation of actual constantine performance.

On top of that, feature flag constantine_msm was added to replace our g1_linear_combination MSM func with the one provided by constantine

Methods which partly use blst & weren't/can't (currently) be replaced to use constantine:

  • pairings verify - requires fp12 & pairing support, though only really used in tests, so can be skipped
  • CtFr from_bytes_unchecked() Big255 deserialize without validation
  • CtFr rand() - requires Big255/u64[4] -> Fr conversion, though only used in tests, so can be skipped
  • CtFr from_bytes() - requires Big255 -> Fr conversion
  • CtFr from_bytes_unchecked() - seems like no equivalent for deserializing to scalar without validity checks, but only used in tests, so can be skipped
  • CtFr from_u64_arr() - requires Big255/u64[4] -> Fr conversion
  • CtFr to_bytes() - requires Fr -> Big255 conversion
  • CtFr to_u64_arr() - requires Fr -> Big255/u64[4] conversion
  • CtFr to_scalar() -> requires Fr- > Big255 conversion
  • CtG1 mul() -> requires G1 * Fr(or Big255/Scalar) operation
  • CtG2 mul() -> requires G2 * Fr(or Big255/Scalar) operation
  • Batched G1 -> Affine conversion (falling back to singular variant of constantine)

For now we use a fork of constantine rust bindings where the fields were made public (+ Default derived, though it's possible to avoid doing this if really required by using MaybeUninit), because it was required by our implementation

Also having some linker errors with c-kzg-4844 <-> rust lib <-> constantine lib bindings
Issue was because constantine doesn't compile with position independent code, which is required when linking static <-> shared library. Rust does this by default, so should consider changing make_rust_lib in constantine to also compile with -fPIC, or alternatively add option in build.rs of constantine-sys to enable this feature PR for enabling -fPIC in constantine was merged

@lynxcs lynxcs marked this pull request as draft December 31, 2023 12:08
@lynxcs lynxcs force-pushed the feat/constantine branch 4 times, most recently from 82e29f6 to 4c5210e Compare December 31, 2023 13:17
@sauliusgrigaitis
Copy link
Member

Great work! Could you check https://github.com/mratsim/constantine/tree/constantine-public-sys that @mratsim just created. We should ask constantine-public-sys to expose everything that we need and blst already exposes. Maybe even constantine-sys could expose it.

@sauliusgrigaitis
Copy link
Member

@lynxcs is there anything else we need from Constantine to ship this PR?

@lynxcs
Copy link
Contributor Author

lynxcs commented Jan 10, 2024

@sauliusgrigaitis Technically can be merged already, however (as noted in description), we are still missing constantine equivalents for Binary<->Big255<->Fr type conversions as well as for G1/G2 mul with Fr.

AFAIK @Armantidas already asked for these functions to be exposed (and I think G1*Mul should have been added but was removed by mistake? Due to there is some issues with G2 code gen on constantine side).

So TL;DR I think it can be merged since it works and is 99% finished. BLST parts can be removed in small PRs as constantine versions become available.

@lynxcs lynxcs marked this pull request as ready for review January 10, 2024 13:04
@sauliusgrigaitis
Copy link
Member

@mratsim any chance to look at this?

@mratsim
Copy link

mratsim commented Jan 11, 2024

All added in mratsim/constantine#338, including G2 scalar mul, found a workaround.

@sauliusgrigaitis
Copy link
Member

Thanks @mratsim! @lynxcs @Armantidas could you update and confirm that we don't need to use blst anymore in constantine integration?

@lynxcs
Copy link
Contributor Author

lynxcs commented Jan 11, 2024

There's always going to be dependency on blst due to how it's used in eip_4844. But main parts have been changed.

EDIT: Also still used in pairings verify & CtFr from_bytes_unchecked because there's no Bytes->Big255 conversion which doesn't validate data

@sauliusgrigaitis
Copy link
Member

@lynxcs thanks. @mratsim let us know if these two items below are intentionally skipped or there is a reason:

pairings verify - requires fp12 & pairing support, though only really used in tests, so can be skipped
CtFr from_bytes_unchecked() Big255 deserialize without validation

@mratsim
Copy link

mratsim commented Jan 11, 2024

pairings verify - requires fp12 & pairing support

I assume it's a function similar to: https://github.com/mratsim/constantine/blob/2aac21d/research/kzg/polynomials.nim#L39-L72

I don't provide Fp12 at the moment because serialization is different between arkworks, geth, blst. See: supranational/blst#101.

What may be possible is providing:

I have limited bandwidth at the moment though.

CtFr from_bytes_unchecked because there's no Bytes->Big255 conversion which doesn't validate data

The data check has almost no cost.

i.e. the check is false if the byte-array is bigger than what the Bigint can physically store so for a big255 if the byte array takes more than 32 bytes. If the length you specify is less than 32, conversion to a big255 will not fail.

@lynxcs
Copy link
Contributor Author

lynxcs commented Jan 11, 2024

@mratsim we need the non-checked version not for performance reasons, but for our eip4844 implementation. There's just slight difference in how blst handles it.

In blst case it copies over the whole buffer, then there's a separate function for validating.

As I understand it in constantine, if it's not a valid scalar it won't be written (so just ignoring return won't work)

Would it be possible to add ctt_bls12_381_deserialize_scalar_unchecked? No pressure if not though, it's not a major issue/blocker for us.

Actually, just used ctt_big255_unmarshalBE instead & it worked for our use case 😄

EDIT: yeah regarding pairing, https://github.com/mratsim/constantine/blob/2aac21d/research/kzg/polynomials.nim#L39-L72 is exactly what we need

@mratsim
Copy link

mratsim commented Jan 15, 2024

For the pairing addition, don't wait on me if it's just for tests, I don't have the bandwidth to do it at the moment.

@sauliusgrigaitis
Copy link
Member

No probs, we will merge as it is now. Benchmarks:

cd constantine 
cargo bench --bench eip_4844 # non parallel
cargo bench --bench eip_4844 --features parallel # parallel
cargo bench --bench eip_4844 --features parallel --features constantine_msm # parallel with `constantine`s msm
cargo bench --bench eip_4844_mixed --features parallel --features constantine_msm # parallel with `constantine`s msm
cargo bench --bench eip_4844_constantine --features parallel # constantine kzg implementation
cargo bench --bench eip_4844_constantine_no_conv --features parallel # constantine kzg implementation without types conversion

Seems there is a significant penalty converting types, and this needs to be further investigated.

@sauliusgrigaitis sauliusgrigaitis merged commit 5e48edd into grandinetech:integration Jan 15, 2024
18 of 24 checks passed
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.

4 participants