arkworks-rs / algebra

Libraries for finite field, elliptic curve, and polynomial arithmetic
https://arkworks.rs
Apache License 2.0
636 stars 248 forks source link

Should arkworks-rs have hooks? #470

Open weikengchen opened 2 years ago

weikengchen commented 2 years ago

A random thought related to https://github.com/ingonyama-zk/cloud-ZK/issues/1

Should we consider one day that we will have some hooks for this library? Note that we should not hook every method/function---we can focus on a method/function that has a legit reason to be constantly hooked. One that comes to my mind is hardware acceleration for MSM and/or NTT.

Note that hooks are becoming more and more common in Rust. We have panic hooks. It seems that OOM hooks are stabilizing soon.
https://github.com/rust-lang/rust/issues/51245

If we implement the hook, there would be an AtomicPtr around the method that is hookable, and the corresponding struct/trait will have implementations that allow runtime hooking by modifying this pointer.

burdges commented 2 years ago

Aren't the new traits sufficient for this? https://github.com/arkworks-rs/algebra/blob/master/ec/src/scalar_mul/variable_base/mod.rs#L12

weikengchen commented 2 years ago

I was thinking whether or not one can redirect msm_bigint to another function not this one: https://github.com/arkworks-rs/algebra/blob/fc7dc3e2a226eade8cda02f6eb2305bdd90b1da8/ec/src/scalar_mul/variable_base/mod.rs#L43

without forking the proof system repo, such as Groth16: https://github.com/arkworks-rs/groth16/blob/sync-algebra/src/prover.rs#L92

weikengchen commented 2 years ago

In other words, I am thinking about a "patch-crates.io" for the default implementation in VariableBaseMSM.

burdges commented 2 years ago

We want to do basically this in polkadot to have fast elliptic curve host calls from WASM. We'll build wrapper types around the arkworks curve types, which then implement the host calls. A priori, I'd think hardware acceleration should similarly use wrapper types that add hooks.

There exist some overhead in that users must add wrapper traits for their protocol constants like base points, but maybe it'd make more sense to simplify doing constants somehow? We might for example recommend that people provide constants using polymorphic functions instead of wrapper traits, although not sure if this really makes sense.

You could also just hide hooks behind a feature flag I guess, maybe that's simplest..

burdges commented 2 years ago

As an aside, rand avoided hooks in part for performance but also for security so that malicious dependencies could not corrupt the randomness. I guess these hooks are less of a vulnerability than bad randomness, since malicious dependencies would still need to exfiltrate any secret key material, but bad randomness is an attack all by itself. If this is a concern then wrappers doing the hooks fixes those concerns.

weikengchen commented 2 years ago

I am thinking about an opposite flag. That is, if the most upper layer application specifies "no_hooks", then hooks are disabled. This is to make sure that the last layer can decide whether hooks are allowed on the Cargo.toml level rather than the runtime level.

There can also be a system of "Seal of the Prophets", i.e., a hook can mark the hook as finalized and prevent new hooks from generated (or, serving as a gatekeeper and accepting only certain hooks).

Pratyush commented 2 years ago

I'm in favour of this, but how would we support this in a generic way? i.e., we want to have a global "MSM Handler" that supports any curve, (or at least can panic if the curve is unsupported).

Note that this feature can be added in a backwards-compatible way; we just have to change the default implementations of msm_bigint

weikengchen commented 2 years ago

I am thinking about an application-driven approach---let me start seeing what APIs are needed for that hardware acceleration implementation from @omershlo and then see what we need to add.

I think we can have that globally as, based on the panic and OOM hooks, Rust actually is able to do that.

Pratyush commented 2 years ago

The issue is that the panic and OOM hooks are neither (a) generic, nor (b) taking references to any external state. See here: https://github.com/rust-lang/rust/issues/51245#issuecomment-397387973

weikengchen commented 2 years ago

"Only closures that do not capture anything can be cast to function pointers"

Basically we need to box everything it needs?

burdges commented 2 years ago

You'd want non-generic per curve hooks, no? Also Miller loops?

weikengchen commented 2 years ago

@burdges We are thinking about an approach that seems to be the same one you are looking for...

That is, instead of placing a hook in algebra; instead, users fork a crate ark-bls12-377SPECIAL in which, compared with ark-bls12-377, the Projective and Pairing traits are implemented in a very different way.

This may also be transparent to the rest of the code, because most of the application-level code, including proof systems, are generic over curves.

burdges commented 2 years ago

Cool @mmagician might've looked at wrapper type set ups by now, not sure.

As I said above, the only annoyance is that users need a bit more genericity in their own code, really not a bad thing. And may help produce gadgets more worth up streaming.

findora-crypto commented 2 years ago

(also Weikeng here)

Here is a magic trick: ark-bls12-381 = { package = "w3f-bls12-381", version = "0.1" }

https://github.com/rust-lang/cargo/issues/5653

Pratyush commented 1 year ago

Does #528 solve this @weikengchen ?

weikengchen commented 1 year ago

I think it solves mostly. But do you want to keep this issue around as the discussion would be expansive?