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

Should arkworks-rs have hooks? #470

Open
weikengchen opened this issue Sep 5, 2022 · 16 comments
Open

Should arkworks-rs have hooks? #470

weikengchen opened this issue Sep 5, 2022 · 16 comments

Comments

@weikengchen
Copy link
Member

weikengchen commented Sep 5, 2022

A random thought related to ingonyama-zk/blaze#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.
rust-lang/rust#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
Copy link
Contributor

burdges commented Sep 5, 2022

@weikengchen
Copy link
Member Author

I was thinking whether or not one can redirect msm_bigint to another function not this one:

/// Optimized implementation of multi-scalar multiplication.

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

@weikengchen
Copy link
Member Author

weikengchen commented Sep 5, 2022

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

@burdges
Copy link
Contributor

burdges commented Sep 5, 2022

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
Copy link
Contributor

burdges commented Sep 5, 2022

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
Copy link
Member Author

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
Copy link
Member

Pratyush commented Sep 6, 2022

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
Copy link
Member Author

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
Copy link
Member

Pratyush commented Sep 6, 2022

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

@weikengchen
Copy link
Member Author

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

Basically we need to box everything it needs?

@burdges
Copy link
Contributor

burdges commented Sep 7, 2022

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

@weikengchen
Copy link
Member Author

weikengchen commented Sep 7, 2022

@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
Copy link
Contributor

burdges commented Sep 7, 2022

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
Copy link
Contributor

(also Weikeng here)

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

rust-lang/cargo#5653

@Pratyush
Copy link
Member

Pratyush commented Sep 6, 2023

Does #528 solve this @weikengchen ?

@weikengchen
Copy link
Member Author

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

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

No branches or pull requests

4 participants