plonky2 icon indicating copy to clipboard operation
plonky2 copied to clipboard

improve portability

Open Sladuca opened this issue 3 years ago • 8 comments

There are a lot of dependencies that are either 1) only used in tests/examples (e.g. rand), or 2) do not compile in rust-based smart contract runtimes (e.g. env_logger, log, std::time, rayon). If we intend this library to be used outside a standard environment, we need to be able to build the library without these dependencies.

To address this, I suggest the following:

  1. feature-gate log / TimingTree behind a feature that's enabled by default, and use conditional compilation to remove timed!, with_context!, TimingTree, and ContextTree if disabled
  2. Wrap rayon's par_iter with a macro cfg_iter that conditionally switches between iter() and par_iter like arkworks does here
  3. add a test_utils feature to deal with stuff like the gate_testing.rs in the interrim until cargo supports this functionality

Sladuca avatar Jul 15 '22 15:07 Sladuca

Happy to open a PR for this (I'm already working on it because I need it)

Sladuca avatar Jul 15 '22 15:07 Sladuca

I've got a branch I'm using for this at https://github.com/proxima-one/plonky2/tree/feature-gate-stuff

Sladuca avatar Jul 15 '22 18:07 Sladuca

For logging, shouldn't log itself be portable since it no-ops by default? I see why env_logger could be an issue though; would moving it to dev-dependencies address the concern?

For rand, I think a feature gate sounds good, like num has.

For Rayon, I think the feature would ideally be in Rayon itself, see rayon-rs/rayon#861. If we don't want to wait for that we can add a shim temporarily, but I don't really like the arkworks approach since it changes all the call sites to use macros. I'd prefer either

  • conditionally importing a rayon module with shims for ParallelIterator etc, or
  • having a maybe_rayon module, containing a MaybeParallelIterator with a maybe_par_iter(), etc

A feature gate for timed! and with_context! sounds fine, as long as it doesn't change the API; seems like the changes can be internal to those macros.

dlubarov avatar Jul 15 '22 18:07 dlubarov

I personally need a shim for rayon now, but I can keep that part in our branch for now if others don't need it too urgently.

As for timed! and with_context!, the main thing is that all of the callsites change because the timing parameter taken by many functions won't exist if there's no TimingTree (no TimingTree because no std::time). So you still end up having conditional compilation to change the invocation each time timed! or with_context! is called, along with any time any function that takes a timing parameter is called even if you're making the change within the macro. I'm not sure if there's a cleaner way to do it.

Log might actually be fine after looking at the source - I was running into some compilation issues with it but it might have actually been something else.

Sladuca avatar Jul 15 '22 18:07 Sladuca

Do you have an idea for how we could handle ZK configurations if rand is disabled? It looks like there's still a rand_vec call in your branch (see oracle.rs). Since ZK is a runtime setting while rand would be a compile time setting, I guess we would need to just add an assert!(!zero_knowledge) and panic at runtime.

dlubarov avatar Jul 15 '22 18:07 dlubarov

Oh, that's a good point. I guess you'd need to have that assertion. Then it can be a separate feature that some runtimes can enable. For instance in a browser, where there's no rayon but rand might be available.

Sladuca avatar Jul 15 '22 18:07 Sladuca

For Rayon, I think the feature would ideally be in Rayon itself, see rayon-rs/rayon#861. If we don't want to wait for that we can add a shim temporarily, but I don't really like the arkworks approach since it changes all the call sites to use macros. I'd prefer either

  • conditionally importing a rayon module with shims for ParallelIterator etc, or
  • having a maybe_rayon module, containing a MaybeParallelIterator with a maybe_par_iter(), etc

Of these, I think I prefer the second option, as it's more explicit that's what's happening and it's less code to write. In either case, the iter() method of stuff like strings, slices, and vecs aren't part of an Iter trait, so we'll have to implement the trait for each type we want to usemaybe_par_iter() on. We'll also need to repeat the process for maybe_into_par_iter(), maybe_par_iter_mut(), maybe_par_chunks(), and maybe_par_chunks_mut().

It ends up being a lot more code than the macro approach, which can be replaced later relatively easily once https://github.com/rayon-rs/rayon/issues/861 happens. You just have to go through and replace all the call sites, which doesn't take that long.

Sladuca avatar Jul 21 '22 17:07 Sladuca

We can also just yoink similar instances of it like this: https://github.com/shssoichiro/oxipng/blob/master/src/rayon.rs. I'll look over the options and open a PR for whatever seems cleaner.

Sladuca avatar Jul 21 '22 17:07 Sladuca

@dlubarov @Sladuca -- Can we close this?

pgebheim avatar Nov 20 '23 18:11 pgebheim

I think the original inspiration for this (Plonky2 verification on Solana) went away, so it's at least not a priority; I think we can close for now and maybe reopen if the need arises

dlubarov avatar Nov 21 '23 02:11 dlubarov