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

Allow to overwrite the default implementation of msm #528

Merged
merged 24 commits into from
Dec 14, 2022

Conversation

achimcc
Copy link

@achimcc achimcc commented Nov 30, 2022

Description

Multi scalar multiplication is usually an expensive operation on different platforms. It is slow in WebAssembly runtimes and can usually get speed up by special hardware. This makes it interesting to define a custom msm function in the model which overwrites the default implementation. This custom function could compute the msm by calling into host functions from a WebAssembly runtime or by delegating it to customized hardware.

This PR:

  • reimplements msm from the VariableBaseMSM trait in the SWCurveConfig trait, refers there to msm_unchecked from the VariableBaseMSM trait. This function ois called model_msm
  • refers to model_msm implementation provided by the SWCurveConfig trait in the VariableBaseMSM implementation for Projective<P>
  • now the model_msm bigint function can get overwritten by curves which are implementing the SWCurveConfig trait.

Similar motivation and technique then the already merged PR #534 . As discussed in issue #537.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
    - [ ] Wrote unit tests non-breaking change, all existing tests are still passing.
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@achimcc achimcc requested review from a team as code owners November 30, 2022 08:48
@achimcc achimcc requested review from Pratyush, mmagician and weikengchen and removed request for a team November 30, 2022 08:48
@achimcc achimcc marked this pull request as draft November 30, 2022 12:38
@achimcc
Copy link
Author

achimcc commented Nov 30, 2022

It would be great if we could pass a custom bigint_msm function to the model which overwrites the default implementation!

It looks like this is not the way to go, since it is producing some recursive compile time loop problem?

@burdges
Copy link
Contributor

burdges commented Nov 30, 2022

I suppose mul_projective and mul_affine kinda establish this pattern already? I think default type parameters could maybe help, which always risks breakage if people have the opportunity. Anyways..

Are there methods besides mul_projective, mul_affine, bigint_msm, and the pairing calls that'll require replacement? FFTs? It's also plausible we're best off being able to maintain forks more easily by going outside the trait system.

@Pratyush
Copy link
Member

Another idea would be to have a global "registry" that contains function pointers for MSMs and FFTs. Something like the following, perhaps:

pub struct Registry {
    msm: /* type signature for MSM method */
    fft: /* type signature for FFT methods */
}

lazy_static! { REGISTRY: Registry = /* init */ }; // or use OnceCell

// In MSM impl
fn msm(...) {
    REGISTRY.msm(...);
}

We will provide default impls for REGISTRY, but binaries that use arkworks can override this method. Perhaps we can take inspiration from tracing's subscriber infrastructure to allow fine-grained overrides for specific curves only.

@Pratyush
Copy link
Member

The foregoing design is basically the idea in #470

@Pratyush
Copy link
Member

Pratyush commented Dec 1, 2022

Actually, it would be nice to have a design that's a hybrid between tracing and rayon. I.e. we can support both scoped usage:

ark_ec::with_msm(msm_fn1, || { /* scoped code that uses `msm_fn1` */});
ark_ec::with_msm(msm_fn2, || { /* scoped code that uses `msm_fn2` */});

and global replacement

ark_ec::set_global_msm(msm_fn1);
/* all subsequent code uses `msm_fn1` */

@burdges
Copy link
Contributor

burdges commented Dec 1, 2022

A registry incurs some global cost, unless some default feature disables it, but if your talking about ASICs or FPGA then maybe you'd want it for some curves but not others.

Another part of the story would be what/how gadget code depends upon the models? ala your comments in arkworks-rs/curves#76 Assuming "not really" then..

We could override mul_projective and mul_affine easily by cloning curve repos already, no?
https://github.com/arkworks-rs/algebra/blob/master/ec/src/models/short_weierstrass/mod.rs#L80-L106

I kinda wish there was some co-git tool that helped maintain branch-like repository differences across repositories, given we're seemingly talking about such a small delta: https://github.com/arkworks-rs/algebra/blob/master/ec/src/models/short_weierstrass/group.rs#L637

Anyways..

We could do this specialization manually if we really like the specialization solution here, no?

pub trait VariableBaseMSM: ScalarMul {
    ...

    /// Optimized implementation of multi-scalar multiplication.
    fn msm_bigint(
        bases: &[Self::MulBase],
        bigints: &[<Self::ScalarField as PrimeField>::BigInt],
    ) -> Self {
        unhooked_msm_bigint(based,bigints)
    }

    /// Raw optimized implementation of multi-scalar multiplication, only for usage by curve models.
    fn unhooked_msm_bigint(
        bases: &[Self::MulBase],
        bigints: &[<Self::ScalarField as PrimeField>::BigInt],
    ) -> Self {
        ... current msm_bigint body ...
    }

    ...
}
pub trait SWCurveConfig: super::CurveConfig {
    ...

    fn model_msm_bigint(
        bases: &[Affine<Self>],
        bigints: &[<Self::ScalarField as PrimeField>::BigInt],
    ) -> Projective<Self> {
        Projective<Self>::unhooked_msm_bitint(bases,bigints)
    }

    ...
}
impl<P: SWCurveConfig> VariableBaseMSM for Projective<P> {
    fn msm_bigint(
        bases: &[Self::MulBase],
        bigints: &[<Self::ScalarField as PrimeField>::BigInt],
    ) -> Self {
        P::model_msm_bigint(bases, bigints)
    }
}

@Pratyush
Copy link
Member

Pratyush commented Dec 1, 2022

A registry incurs some global cost, unless some default feature disables it, but if your talking about ASICs or FPGA then maybe you'd want it for some curves but not others.

In practice I suspect the overhead of the registry will be negligible compared to the cost of the MSM.

We could do this specialization manually if we really like this solution, no? ...

Yes, the "hook"/"registry" idea is compatible with this PR, but I think this PR would be redundant if we implemented the former. A further advantage of the registry idea is that only the default (current) MSM impl needs to live in ark-ec; the other MSM impls can live in their own crates.

Achim Schneider added 2 commits December 1, 2022 08:26
@burdges
Copy link
Contributor

burdges commented Dec 1, 2022

In practice I suspect the overhead of the registry will be negligible compared to the cost of the MSM.

Yes of course, but what I mean is: What would need hooks?

It's clear Miller looks and final exps are costly enough to justify hooks this way, but they can be patched without hooks without much code duplication. I suppose mul_projective, mul_affine, point preperation, batch inversion, and serialization would not get hooks?

the other MSM impls can live in their own crates.

Yes, this sounds more relevant, and I guess answers my question above. Are the trade offs in MSMs mostly just size?

Rust has no polymorphic static muts so hooks might need to be declared in a macro used in curve crates.

@achimcc achimcc marked this pull request as ready for review December 5, 2022 10:50
@achimcc
Copy link
Author

achimcc commented Dec 5, 2022

I adopted my implementation to what @burdges suggested. This way tests are passing, the recursive compile time loop is gone. If we want to implement this solution, the PR would be ready for review. It would definitely drastically cut down the amount of code that we have to fork for our implementation.

ec/src/scalar_mul/variable_base/mod.rs Outdated Show resolved Hide resolved
ec/src/models/short_weierstrass/mod.rs Outdated Show resolved Hide resolved
@achimcc
Copy link
Author

achimcc commented Dec 12, 2022

I changed the design a bit, now we are re-implementing the msm function body in the default implementation of the model_msm function in the SWCurveConfig trait. This also avoids any recursive compile time bugs and removes the superficial hook function.

@achimcc achimcc changed the title Pass msm bigint Allow to overwrite the default implementation of msm Dec 12, 2022
@Pratyush Pratyush merged commit 6e3e804 into arkworks-rs:master Dec 14, 2022
@achimcc achimcc deleted the pass-msm-bigint branch December 14, 2022 21:49
andrewmilson added a commit to andrewmilson/algebra that referenced this pull request Jan 1, 2023
* upstream/master: (29 commits)
  Fix some clippy lints (arkworks-rs#570)
  Correct tag name & complete command suggestion (arkworks-rs#569)
  Open a "release-PR" against a `releases` branch (arkworks-rs#566)
  Allow to overwrite default impl of `msm` in TwistedEdwards form (arkworks-rs#567)
  Remove poly-benches. (arkworks-rs#558)
  DO NOT MERGE YET. Release 0.4 (arkworks-rs#512)
  otherwise downstream users that have not migrated will not see warning (arkworks-rs#563)
  use `into_bigint()` in `Debug` for `Fp<P, N>` (arkworks-rs#562)
  Add `frobenius_map_in_place` (arkworks-rs#557)
  Fix test_sw_properties for some cofactor groups (arkworks-rs#555)
  Move h2c tests to test-templates (arkworks-rs#554)
  impl `CanonicalSerialize/Deserialize` for `BigUint` (arkworks-rs#551)
  Fix MontFp issue in fields with 64 * k bits (arkworks-rs#550)
  Fix tests for Modulus plus one div four (arkworks-rs#552)
  fix (arkworks-rs#547)
  Rename all `*Parameters` to `*Config` (arkworks-rs#545)
  Fix doc-comment on `SWUMap` and CamelCase `(CO)DOMAIN`
  Small cleanups in hash-to-curve (arkworks-rs#544)
  Allow to overwrite the default implementation of `msm` (arkworks-rs#528)
  Move `multi_miller_loop` and `final_exponentiation` into `BW6Config` (arkworks-rs#542)
  ...
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.

4 participants