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 bindings for Banderwagon #477

Merged
merged 36 commits into from
Jan 8, 2025
Merged

Conversation

Richa-iitr
Copy link
Contributor

Added C bindings for verkle-ipa.

@Richa-iitr Richa-iitr marked this pull request as draft October 6, 2024 16:34
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.

Thanks for the contributions.

I think this should be split into 3 steps:

  1. C bindings for banderwagon fr, fp, ec
  2. Creating a proper wrapper for the Ethereumn specific instantiation of IPA. Currently the generic code is forwarded (i.e. constantine/commitments/eth_verkle_ipa.nim) but protocol-specific constants/types should be wrapped and exposed in a proper procedure in the root constantine/eth_verkle_ipa.nim and only there can C bindings be generated from.
  3. Actually do the C bindings.

typedef struct Fr Fr;
typedef struct Banderwagon Banderwagon;
typedef struct EC_TwEdw EC_TwEdw;
typedef struct Fp Fp;
Copy link
Owner

Choose a reason for hiding this comment

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

All the fields and curves related struct should be defined in constantine/include/constantine/curves/banderwagon.h

This would be similar to pallas and vesta as they don't use pairings:

type
bn254_snarks_fr = Fr[BN254_Snarks]
bn254_snarks_fp = Fp[BN254_Snarks]
bn254_snarks_fp2 = Fp2[BN254_Snarks]
bn254_snarks_g1_aff = EC_ShortW_Aff[Fp[BN254_Snarks], G1]
bn254_snarks_g1_jac = EC_ShortW_Jac[Fp[BN254_Snarks], G1]
bn254_snarks_g1_prj = EC_ShortW_Prj[Fp[BN254_Snarks], G1]
bn254_snarks_g2_aff = EC_ShortW_Aff[Fp2[BN254_Snarks], G2]
bn254_snarks_g2_jac = EC_ShortW_Jac[Fp2[BN254_Snarks], G2]
bn254_snarks_g2_prj = EC_ShortW_Prj[Fp2[BN254_Snarks], G2]
collectBindings(cBindings_bn254_snarks):
genBindingsField(big254, bn254_snarks_fr)
genBindingsField(big254, bn254_snarks_fp)
genBindingsFieldSqrt(bn254_snarks_fp)
genBindingsExtField(bn254_snarks_fp2)
genBindingsExtFieldSqrt(bn254_snarks_fp2)
genBindings_EC_ShortW_Affine(bn254_snarks_g1_aff, bn254_snarks_fp)
genBindings_EC_ShortW_NonAffine(bn254_snarks_g1_jac, bn254_snarks_g1_aff, big254, bn254_snarks_fr)
genBindings_EC_ShortW_NonAffine(bn254_snarks_g1_prj, bn254_snarks_g1_aff, big254, bn254_snarks_fr)
genBindings_EC_ShortW_Affine(bn254_snarks_g2_aff, bn254_snarks_fp2)
genBindings_EC_ShortW_NonAffine(bn254_snarks_g2_jac, bn254_snarks_g2_aff, big254, bn254_snarks_fr)
genBindings_EC_ShortW_NonAffine(bn254_snarks_g2_prj, bn254_snarks_g2_aff, big254, bn254_snarks_fr)
genBindings_EC_hash_to_curve(bn254_snarks_g1_aff, svdw, sha256, k = 128)
genBindings_EC_hash_to_curve(bn254_snarks_g1_jac, svdw, sha256, k = 128)
genBindings_EC_hash_to_curve(bn254_snarks_g1_prj, svdw, sha256, k = 128)
genBindings_EC_hash_to_curve(bn254_snarks_g2_aff, svdw, sha256, k = 128)
genBindings_EC_hash_to_curve(bn254_snarks_g2_jac, svdw, sha256, k = 128)
genBindings_EC_hash_to_curve(bn254_snarks_g2_prj, svdw, sha256, k = 128)
collectBindings(cBindings_bn254_snarks_parallel):
genParallelBindings_EC_ShortW_NonAffine(bn254_snarks_g1_jac, bn254_snarks_g1_aff, bn254_snarks_fr)
genParallelBindings_EC_ShortW_NonAffine(bn254_snarks_g1_prj, bn254_snarks_g1_aff, bn254_snarks_fr)

However Banderwagon is an Edwards curve so the template there might need an adaptation.
I don't remember what I add in mind as differences between short-weierstrass and twisted edwards, maybe it was just the affine<->jacobian<->projective coordinate conversion and that could be done as a separate template, see

template genBindings_EC_ShortW_Affine*(EC, Field: untyped) =
when appType == "lib":
{.push noconv, dynlib, exportc, raises: [].} # No exceptions allowed
else:
{.push noconv, exportc, raises: [].} # No exceptions allowed
# --------------------------------------------------------------------------------------
func `ctt _ EC _ is_eq`(P, Q: EC): SecretBool =
P == Q
func `ctt _ EC _ is_neutral`(P: EC): SecretBool =
P.isNeutral()
func `ctt _ EC _ set_neutral`(P: var EC) =
P.setNeutral()
func `ctt _ EC _ ccopy`(P: var EC, Q: EC, ctl: SecretBool) =
P.ccopy(Q, ctl)
func `ctt _ EC _ is_on_curve`(x, y: Field): SecretBool =
isOnCurve(x, y, EC.G)
func `ctt _ EC _ neg`(P: var EC, Q: EC) =
P.neg(Q)
func `ctt _ EC _ neg_in_place`(P: var EC) =
P.neg()
{.pop.}
template genBindings_EC_ShortW_NonAffine*(EC, EcAff, ScalarBig, ScalarField: untyped) =
# TODO: remove the need of explicit ScalarBig and ScalarField
when appType == "lib":
{.push noconv, dynlib, exportc, raises: [].} # No exceptions allowed
else:
{.push noconv, exportc, raises: [].} # No exceptions allowed
# --------------------------------------------------------------------------------------
func `ctt _ EC _ is_eq`(P, Q: EC): SecretBool =
P == Q
func `ctt _ EC _ is_neutral`(P: EC): SecretBool =
P.isNeutral()
func `ctt _ EC _ set_neutral`(P: var EC) =
P.setNeutral()
func `ctt _ EC _ ccopy`(P: var EC, Q: EC, ctl: SecretBool) =
P.ccopy(Q, ctl)
func `ctt _ EC _ neg`(P: var EC, Q: EC) =
P.neg(Q)
func `ctt _ EC _ neg_in_place`(P: var EC) =
P.neg()
func `ctt _ EC _ cneg_in_place`(P: var EC, ctl: SecretBool) =
P.neg()
func `ctt _ EC _ sum`(r: var EC, P, Q: EC) =
r.sum(P, Q)
func `ctt _ EC _ add_in_place`(P: var EC, Q: EC) =
P += Q
func `ctt _ EC _ diff`(r: var EC, P, Q: EC) =
r.diff(P, Q)
func `ctt _ EC _ double`(r: var EC, P: EC) =
r.double(P)
func `ctt _ EC _ double_in_place`(P: var EC) =
P.double()
func `ctt _ EC _ affine`(dst: var EcAff, src: EC) =
dst.affine(src)
func `ctt _ EC _ from_affine`(dst: var EC, src: EcAff) =
dst.fromAffine(src)
func `ctt _ EC _ batch_affine`(dst: ptr UncheckedArray[EcAff], src: ptr UncheckedArray[EC], n: csize_t) =
dst.batchAffine(src, cast[int](n))
func `ctt _ EC _ scalar_mul_big_coef`(
P: var EC, scalar: ScalarBig) =
P.scalarMul(scalar)
func `ctt _ EC _ scalar_mul_fr_coef`(
P: var EC, scalar: ScalarField) =
P.scalarMul(scalar)
func `ctt _ EC _ scalar_mul_big_coef_vartime`(
P: var EC, scalar: ScalarBig) =
P.scalarMul_vartime(scalar)
func `ctt _ EC _ scalar_mul_fr_coef_vartime`(
P: var EC, scalar: ScalarField) =
P.scalarMul_vartime(scalar)
func `ctt _ EC _ multi_scalar_mul_big_coefs_vartime`(
r: var EC,
coefs: ptr UncheckedArray[ScalarBig],
points: ptr UncheckedArray[EcAff],
len: csize_t) =
r.multiScalarMul_vartime(coefs, points, cast[int](len))
func `ctt _ EC _ multi_scalar_mul_fr_coefs_vartime`(
r: var EC,
coefs: ptr UncheckedArray[ScalarField],
points: ptr UncheckedArray[EcAff],
len: csize_t)=
r.multiScalarMul_vartime(coefs, points, cast[int](len))
{.pop.}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mratsim i resolved this and added separate header for banderwagon. Please review and suggest changes. Also how can i verify things are working as expected?

Copy link
Owner

Choose a reason for hiding this comment

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

Good question, I don't have a testsuite for the C API of elliptic curves. It's not ideal but for now we can skip it merge as-is and focus on the C API for the full protocol.

uint32_t u32;
uint64_t u64;
unsigned int u;
} SomeUnsignedInt;
Copy link
Owner

Choose a reason for hiding this comment

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

You cannot emulate generics with union types.

Whatever the platform, the storage reserved will be 64-bit / 8 bytes per entry here.
But if Nim uses 32-bit inputs in an array a, the C code will read both entries with a[0] and will read past the buffer with a[1].

Hence generic procedures cannot be wrapped in C directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, will think of another way here.

@@ -139,7 +143,7 @@ func innerProduct[F](r: var F, a, b: distinct(View[F] or MutableView[F])) =
func ipa_commit*[N: static int, EC, F](
crs: PolynomialEval[N, EC],
r: var EC,
poly: PolynomialEval[N, F]) =
poly: PolynomialEval[N, F]) {.libPrefix: prefix_ipa.} =
Copy link
Owner

Choose a reason for hiding this comment

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

This is not the proper level of exports as this is a generic procedure.

Export should happen at fully specified protocol level (i.e. curve, field, hash functions, N: static int input sizes are known).

In this case, this means having proper instantiated ipa_commit here:

# TODO: proper IPA wrapper for https://github.com/status-im/nim-eth-verkle
#
# For now we reexport
# - eth_verkle_ipa
# - sha256 for transcripts
export eth_verkle_ipa
export hashes

and tagging those with libPrefix.

Copy link
Contributor Author

@Richa-iitr Richa-iitr Oct 20, 2024

Choose a reason for hiding this comment

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

is there any example of a wrapper i can follow to understand?
If i am not wrong something like this

func mapToScalarField*(res: var Fr[Banderwagon], p: EC_TwEdw[Fp[Banderwagon]]): bool {.discardable.} =
## This function takes the x/y value from the above function as Fp element
## and convert that to bytes in Big Endian,
## and then load that to a Fr element
##
## Spec : https://hackmd.io/wliPP_RMT4emsucVuCqfHA?view#MapToFieldElement
var baseField: Fp[Banderwagon]
var baseFieldBytes: array[32, byte]
baseField.mapToBaseField(p) # compute the defined mapping
let check1 = baseFieldBytes.marshalBE(baseField) # Fp -> bytes
let check2 = res.unmarshalBE(baseFieldBytes) # bytes -> Fr
return check1 and check2
can be exported directly?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, anything with generics cannot be exported and need a concretely typed wrapper.

typedef struct {
byte x[32]; // 32-byte x-coordinate
byte y[32]; // 32-byte y-coordinate
} EC_TwEdw_Fp_Banderwagon;
Copy link
Owner

Choose a reason for hiding this comment

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

ditto

typedef struct {
byte x[32]; // 32-byte x-coordinate
byte y[32]; // 32-byte y-coordinate
} EC;
Copy link
Owner

Choose a reason for hiding this comment

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

ditto

typedef struct {
EC* points;
size_t length;
} PolynomialEval_EC;
Copy link
Owner

Choose a reason for hiding this comment

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

the protocol has fixed size polynomials. Nim static int are similar to generics and cannot be represented with C runtime parameters. Instead a no generic/no static procedure should be implemented at the croot of the project in constantine/ethereum_verkle_ipa.nim

typedef struct {
EC* ec_points;
size_t length;
} EcAffArray;
Copy link
Owner

Choose a reason for hiding this comment

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

ditto for all structs up to here

typedef struct EthVerkleTranscript EthVerkleTranscript;

typedef byte EthVerkleIpaProofBytes[544]; // Array of 544 bytes
typedef byte EthVerkleIpaMultiProofBytes[576];
Copy link
Owner

Choose a reason for hiding this comment

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

It's more useful to provide:

  • either ctt_eth_verkle_ipa_proof_sizeof() and ctt_eth_verkle_ipa_multiproof_sizeof() and let the caller do their allocation / destruction
  • or ctt_eth_verkle_ipa_proof_create() / ctt_eth_verkle_ipa_proof_destroy()
    and ctt_eth_verkle_ipa_multiproof_create() / ctt_eth_verkle_ipa_multiproof_destroy()

mratsim
mratsim previously approved these changes Oct 27, 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.

The PR looks good for a merge if we remove the protocols/ethereum_verkle_ipa.h. Its rewrite can be done separately.

@Richa-iitr
Copy link
Contributor Author

The PR looks good for a merge if we remove the protocols/ethereum_verkle_ipa.h. Its rewrite can be done separately.

removed the file, please review

@agnxsh agnxsh marked this pull request as ready for review November 10, 2024 07:51
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.

As mentioned in my previous review (#477 (review)), the PR needs to be separated in elliptic curve API which are well documented and a Verkle API.

For now, let's delete all the Verkle API changes as they cannot be used as-is and I am unsure myself on how to design the API. Furthermore the future of the Verkle Tree upgrade is in flux at the moment.

aff: PolynomialEval[EthVerkleDomain, EC_TwEdw_Aff[Fp[Banderwagon]]]
prj: PolynomialEval[EthVerkleDomain, EC_TwEdw_Prj[Fp[Banderwagon]]]
EthVerkleIpaPolyEvalPoly* {.exportc: prefix_ffi & "poly_eval_poly".} = object
raw: PolynomialEval[256, Fr[Banderwagon]]
Copy link
Owner

Choose a reason for hiding this comment

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

The file should be reverted and not modified.

This approach doubles object size while we only need aff or prj coordinates. The serialized code should just use EthVerkleIpaProofBytes and unsure yet on the C API of the internal proof type, maybe will hide it behind an opaque pointer.

src: EthVerkleIpaProof
): cttEthVerkleIpaStatus {.libPrefix: prefix_ffi.} =

return serialize(dst, src.prj)
Copy link
Owner

Choose a reason for hiding this comment

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

A user will not know what serialization to use between prj and aff, this will lead to errors and so is not the correct API.

// printf("\n");
ctt_eth_verkle_ipa_proof_aff proof;

ctt_eth_verkle_ipa_status stat = ctt_eth_verkle_ipa_deserialize_aff(&proof, &proof_bytes);
Copy link
Owner

Choose a reason for hiding this comment

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

File to delete so the PR is focused on Banderwagon

* at your option. This file may not be copied, modified, or distributed except according to those terms.
*/
#ifndef __CTT_H_ETHEREUM_VERKLE_IPA__
#define __CTT_H_ETHEREUM_VERKLE_IPA__
Copy link
Owner

Choose a reason for hiding this comment

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

File to delete so the PR is focused on Banderwagon / twisted edwards

@Richa-iitr
Copy link
Contributor Author

@mratsim i have reverted the verkle api changes, the pr is ready for review.

mratsim
mratsim previously approved these changes Jan 8, 2025
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.

LGTM, merging after the files have an extra empty line

@mratsim mratsim changed the title C bindings for verkle C bindings for Banderwagon Jan 8, 2025
@mratsim mratsim merged commit d9db7ab into mratsim:master Jan 8, 2025
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