search

ZK-Garage / plonk

A pure Rust PLONK implementation using arkworks as a backend.
https://discord.gg/XWJdhVf37F
Mozilla Public License 2.0
295 stars 76 forks source link

Remove `pub` visibility from embedded to outer curve scalar conversions #28

Open CPerezz opened 3 years ago

CPerezz commented 3 years ago

These conversions like the one performed in: https://github.com/ZK-Garage/plonk/blob/master/plonk-core/src/util.rs#L93 pub fn from_embedded_curve_scalar<F, P>( embedded_scalar: <P as ModelParameters>::ScalarField, ) -> F https://github.com/ZK-Garage/plonk/blob/master/plonk-core/src/util.rs#L122 pub(crate) fn to_embedded_curve_scalar<F, P>(pfc_scalar: F) -> P::ScalarField

Are never safe unless we have 100% clear that our StandardComposer API previously enforced some constraints(not circuit constraints) that guarantee that we can safely make this conversions.

We should remove the pub visibility and make sure that the invariants are never broken for them.

bhgomes commented 2 years ago

We should use a custom variable type for embedded scalars which can only be constructed by doing the relevant checks. Since the curves are known at compile-time, there should be a marker trait or associated constant that can tell us if the embedded scalar fits inside the constraint scalar and performs no checks (basically just type check at compile-time suffices). Then, when performing operations like scalar_mul you request that the user submit a value of this custom variable instead of a native value which they need to ensure themselves that they checked correctly.

For the case where the constraint scalar field is smaller than the embedded scalar field, you use a similar technology to create embedded scalars but it would involve more than one constraint scalar, this is much more complicated.

CPerezz commented 2 years ago

I like the idea. I would wait until @joebebel 's PR gets merged to then impl it on the cleanest way possible.

markulf commented 2 years ago

What is the PR referred to here, and is this still open?

I am confused by the if modulus.num_bits() >= scalar_repr.num_bits() { and the same checks once being performed with a <<F as PrimeField>::BigInt and once with a <<P::ScalarField as PrimeField>::BigInt? are these really different types, or are we just defensive here in case they might end up being different types?

If this is not just a temporary solution, I also would like to add tests to check that these functions indeed work correctly.

bhgomes commented 2 years ago

These BigInt can be different types, yes. We can't guarantee they are the same.