Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

arkworks integration #13031

Merged
merged 432 commits into from
Jun 6, 2023
Merged

Conversation

achimcc
Copy link
Contributor

@achimcc achimcc commented Dec 29, 2022

  • What does it do?
    We provide host function calls for the elliptic curve arithmetic used by Arkworks. We need this for an efficient computation of those arithmetics in the WASM runtime. We provide host calls for bls12_381, bls12_377, ed_on_bls12_381, ed_on_bls12_77, bw6_761.
    Those arithmetics, which are provided by the host calls, are:
    • multi miller loop and final exponentiation for bls12_377, bls12_381 and bw6_761. They are required to compute a bilinear pairing between two elliptic curve points, e.g. for groth16 verification.
    • multi scalar multiplications for all curves
    • affine multiplications for all curves (projective multiplications are computed by converting them to affine first and then calling into the affine multiplication host calls).

The host functions receive their arguments serialized to Vec<u8> by ark-scale and return the result as serialized Result<Vec<u8>, ()>, which then needs to be deserialized by ark-scale.

We add the host function calls to the sp_crypto_ec_utils crate. By now they are an experimental feature which one can enable with the feature flag ec-utils-experimental. We finally decided not to hide the host calls behind a feature flag, since they are in an independent crate and not part of sp-io for now, so that they will not be added to Polkadot/Kusama automatically anyway.

We define the curves in https://github.com/paritytech/ark-substrate), and they receive the host functions by dependency injection. The reason for using dependency injection here, is that we intend to use ark-substrate in Substrate with Ring VRFs/Sassafrass in the future, which are part of the consensus mechanism and therefore they are implemented in the Substrate core repo as well. Hence, since we can't have circular dependencies, ark-substrate can't depend directly on Substrate. We have an additional library substrate-curves, which equips the curves from ark-substrate with the host function calls added here, to provide an end user implementation of curves analogous to ark-curves.

The following dependency diagram visualizes this (the dashed arrow represents dependency injection):
dependency-diagram
Dependency Diagram

An overview and comparison of benchmark results can is here.

  • Is there something left for follow-up PRs?
    Adding more curves and replacing more operations by host function calls could be further possible optimisations.

  • Labels: You labeled the PR appropriately if you have permissions to do so:

    • A* for PR status (one required)
    • B* for changelog (one required)
    • C* for release notes (exactly one required)
    • D* for various implications/requirements
    • Github project assignment
  • Related Issues: You mentioned a related issue if this PR is related to it, e.g. Fixes #228 or Related #1337.

  • 2 Reviewers: You asked at least two reviewers to review. If you aren't sure, start with GH suggestions.

  • Style Guide: Your PR adheres to the style guide

    • In particular, mind the maximal line length of 100 (120 in exceptional circumstances).
    • There is no commented code checked in unless necessary.
    • Any panickers in the runtime have a proof or were removed.
  • Runtime Version: You bumped the runtime version if there are breaking changes in the runtime.

  • Docs: You updated any rustdocs which may need to change.

  • Polkadot Companion: Has the PR altered the external API or interfaces used by Polkadot?

    • If so, do you have the corresponding Polkadot PR ready?
    • Optionally: Do you have a corresponding Cumulus PR?

@davxy
Copy link
Member

davxy commented Jan 7, 2023

(Maybe) dumb questions:

  1. why the upstream version doesn't satisfy our use case? What patches were applied to the upstream? If I understood correctly some computational intesive tasks are offloaded to host functions? What are these tasks/functions?
  2. why we need to integrate all this stuff in Substrate? IMO (in general) is not the best to embed an entire lib in an (already giant) codebase
  3. where all this cryptographic thing required? I mean, I see that some host functions were implemented to use these primitives. But then as far as I can see there is no usage of these functions. What is the end goal?

As a side note, we don't directly implement any cryptographic primitive in Substrate, we use external crates.

@achimcc
Copy link
Contributor Author

achimcc commented Jan 8, 2023

(Maybe) dumb questions:

  1. why the upstream version doesn't satisfy our use case? What patches were applied to the upstream? If I understood correctly some computational intesive tasks are offloaded to host functions? What are these tasks/functions?
  2. why we need to integrate all this stuff in Substrate? IMO (in general) is not the best to embed an entire lib in an (already giant) codebase
  3. where all this cryptographic thing required? I mean, I see that some host functions were implemented to use these primitives. But then as far as I can see there is no usage of these functions. What is the end goal?

As a side note, we don't directly implement any cryptographic primitive in Substrate, we use external crates.

  1. The operations on arithmetic curves are way to slow in WebAssembly. We have to replace the costly operations by host function calls. The host function tasks are the arithmetic operations on elliptic curves which are slow in WebAssembly. Those are: computation of biliner pairings (ate pairing), multi scalar multiplication, affine and projective multiplications.
  2. I'll implement it this way now. As explained above it is a lot more work since the curves have to call into host functions. If we want to make the host functions available to them, we have to pass them by dependency injection and that requires a lot of modifications on the arkworks code.
  3. Those are the curves used by arkworks.rs which make all the tools from this library available to every Substrate pallet. Direct examples are Zero Knowledge Proof verification, especially verification of plonk or groth16 proofs. I created a simple example of a groth16 proof verification in a Substrate pallet which uses these curves (which is based on my PR branch of Substrate) here: https://github.com/achimcc/substrate-groth16. Other applications include RingVRF's which are interesting in their own, but will also become a basic building block of Sassafrass consensus. @burdges might comment more on use-cases.

@davxy
Copy link
Member

davxy commented Jan 8, 2023

@achimcc I'm very interested since I'm working on sassafras implementation :-)

  1. The "prove" component will always run in native client code in order to produce ring membership proofs (to be sent onchain along with tickets).
  2. The "verify" is indeed the critical component. Since it may be very computational intensive (and maybe also no no-std... but I don't know this for sure) and run by the runtime when checking for submitted tickets before saving them onchain.

But can't we offer a host function (i.e. verify) doing the overall verification job? I mean, why should we have to run part of the verification code in wasm at all?
I think that a verify host function would be a good interface in order to maintain independence from the actual library. Is there some detail I'm missing that prevents to do it?

@achimcc
Copy link
Contributor Author

achimcc commented Jan 8, 2023

@davxy We also want to use the arkworks.rs stuff in other context. @burdges said, a complete native code implementation of a verify function is not what we want, but I've to admit that I don't know the reasoning behind it.

@davxy
Copy link
Member

davxy commented Jan 9, 2023

@burdges why we don't want a full native verify?

@bkchr
Copy link
Member

bkchr commented Jan 9, 2023

@burdges why we don't want a full native verify?

AFAIK the idea is to use artworks as a building library to compose different kind of cryptography algorithms. Adding one verify for each cryptographic algorithm isn't such a great idea in the longterm.

@achimcc achimcc force-pushed the achimcc/arkworks-integration branch 9 times, most recently from 06f4214 to 280e690 Compare January 12, 2023 09:17
@achimcc
Copy link
Contributor Author

achimcc commented Jan 13, 2023

This all builds and all tests pass! If no one complains, I will just wait for the official arkworks-rs 0.4.0 release and will turn it into Ready for review then!

@burdges
Copy link

burdges commented Jan 13, 2023

We could've a thinner substrate change @bkchr but then have an external repo that implements the runtime code for the curve models and curves themselves. I didn't ask for this initially because I figured it'd complicate doing tests, but achim said that's not really true. I agree it's a good idea now that I think about it:

It's true, we need it to be easy to translate PRs from arkworks' to our forked code. It's easy-ish to do this for curves, which live in the flat repo https://github.com/arkworks-rs/curves/ so we could've a similar flat sub-ark-curves repo perhaps.

It's trickier to do curve models as they're nested into the ark-ec crate https://github.com/arkworks-rs/algebra/tree/master/ec/src/models so I figured this winds up being roughly git diff and git apply, but afaik the curve models could live in the same sub-ark-curves repo.

We'll upgrade only intermittently I guess, so really I think sub-ark-curves being separate from substrate itself actually benefits us in that parachains upgrade sub-ark-curves and all their crpytography dependencies independently form substrate itself. We could even have testing infrastructure that uses all supported versions of sub-ark-curves.


Arkworks has need generous in permitting our changes into their traits, and we've tried to keep our changes in line with what gpu and fpga projects require, but arkworks success requires being kinda a "standard library". We also want zcash's traits to work via these same host calls, but they'll never permit even the slightest code smell into their traits, so..

We should think carefully about any changes to the arkworks trait hierarchy. In fact, we almost got G2Prepared serialization merged, which risks incompatiblity with some optimizations we like.

@achimcc
Copy link
Contributor Author

achimcc commented Jan 13, 2023

We could've a thinner substrate change @bkchr but then have an external repo that implements the runtime code for the curve models and curves themselves. I didn't ask for this initially because I figured it'd complicate doing tests, but achim said that's not really true. I agree it's a good idea now that I think about it:

It's true, we need it to be easy to translate PRs from arkworks' to our forked code. It's easy-ish to do this for curves, which live in the flat repo https://github.com/arkworks-rs/curves/ so we could've a similar flat sub-ark-curves repo perhaps.

It's trickier to do curve models as they're nested into the ark-ec crate https://github.com/arkworks-rs/algebra/tree/master/ec/src/models so I figured this winds up being roughly git diff and git apply, but afaik the curve models could live in the same sub-ark-curves repo.

We'll upgrade only intermittently I guess, so really I think sub-ark-curves being separate from substrate itself actually benefits us in that parachains upgrade sub-ark-curves and all their crpytography dependencies independently form substrate itself. We could even have testing infrastructure that uses all supported versions of sub-ark-curves.

We have a forked curve repo already which contains the forked arkworks curves: https://github.com/achimcc/ark-substrate

I'm using it for this PR!

@achimcc achimcc closed this Jan 13, 2023
@achimcc achimcc reopened this Jan 13, 2023
@burdges
Copy link

burdges commented Jan 13, 2023

Ahh, you've even got the models there, cool.

@achimcc
Copy link
Contributor Author

achimcc commented Jan 13, 2023

We have a funny testing duality: the curves repo is running the tests against the host functions in Substrate and Substrate is running the tests against the curve repo. In both cases we use the curves with host function implementations.

@bkchr
Copy link
Member

bkchr commented Jan 15, 2023

Is there any need to have the host functions in substrate? From Substrate itself there is no need that the host functions are being part of this repo. Host functions can be defined anywhere.

@achimcc
Copy link
Contributor Author

achimcc commented Jan 16, 2023

AFAIK Sassafrass consensus will use RingVRF's and with this also arkworks and the host functions. I was assuming that the consensus layer needs to be part of the Substrate core repo?

@bkchr
Copy link
Member

bkchr commented Jan 16, 2023

Good point! If this is the case, we can add it here :)

A general node, the artworks stuff should go into some special "namespace/trait".

@achimcc
Copy link
Contributor Author

achimcc commented Jun 5, 2023

One suggestion from my side: we decided not to Scale encode/decode the host function arguments, I think it makes sense to add some more doc comments instead, about the format of the received/returned arguments and also about the used (de-)serialization tool (ark-scale) @davxy ?

I added the types that each host function receives/returns in encoded Vec<u8> to their corresponding doc comments. If @burdges approves the PR, we can merge :)

Copy link

@burdges burdges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy. :)

I'll also remark that Achim explored the differences between zcash & arkworks internal projective representations. It's be cheap if we ever need to internally adopt their representation.

@achimcc
Copy link
Contributor Author

achimcc commented Jun 6, 2023

It seems like @burdges approval doesn't count for the CI, since he is not from Parity. @arkpar , @bkchr , can one of you help me out here with a second approval?

@achimcc
Copy link
Contributor Author

achimcc commented Jun 6, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 13ed450 into master Jun 6, 2023
@paritytech-processbot paritytech-processbot bot deleted the achimcc/arkworks-integration branch June 6, 2023 10:23
@tomaka
Copy link
Contributor

tomaka commented Jun 6, 2023

Just to make sure: this is completely unstable, correct? And we don't want any relay chain or parachain to actually use them, right?

@achimcc
Copy link
Contributor Author

achimcc commented Jun 6, 2023

Just to make sure: this is completely unstable, correct? And we don't want any relay chain or parachain to actually use them, right?

Yes, correct! We had a discussion about hiding it behind a feature flag up thread: #13031 (comment)

We finally decided not to do this, and to move the host functions into their own crate, instead of providing them as part of sp-io. This way, they will not get deployed on Polkadot/Kusama automatically.

Finally, they are an internal API for ark-substrate and we have a warning there, that we not recommend using this for a live deployment on Polkadot/Kusama at the moment.

@bkchr
Copy link
Member

bkchr commented Jun 6, 2023

But we will probably before see this on some test network, like Rococo or Westend, but nothing was decided so far on this!

@burdges
Copy link

burdges commented Jun 6, 2023

I've posted design notes in https://hackmd.io/@rgbPIkIdTwSICPuAq67Jbw/rkipBanIn

agryaznov pushed a commit that referenced this pull request Jun 9, 2023
* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix compression

* fix compression

* fix compression

* fix compression

* fix compression

* fix std leak

* fix std leak

* fix std leak

* merge master

* merge master

* cargo update

* cargo update

* cargo update

* cargo update

* cargo update

* use serialize_result

* cargo update

* cargo update

* cargo update

* cargo update

* reduce boilerplate code

* remove host function muls

* reduce boilerplate code

* remove patches

* uuse correct ark-substrate branch

* reduce boilerplate code

* cleanup

* cleanup

* proper error handling

* derive serialize for error

* proper error handling

* proper error handling

* proper error handling

* derive Debug for PairingError

* sp-arkworks path

* cargo update

* adopt tests to error handling

* fix tests

* cargo update

* remove results

* deserialize as G2Affine

* cargo update

* add codex index to PairingError

* replace Vec<Vec<u8>>

* replace Vec<Vec<u8>>

* use into_iter for chunks

* use chunks for scalars

* fix ersialized_size

* use into

* collect as vec

* collect as vec

* no collect Vec

* use into_iter

* import AffineRepr

* fix typo

* cargo update

* new serialization

* fix typo

* unwrap results

* unwrap results

* use correct deserialization

* fix bugs, cleanup

* correct len

* vec without capacity

* Revert "vec without capacity"

This reverts commit 2b1cd00.

* Revert "correct len"

This reverts commit b85de86.

* Revert "fix bugs, cleanup"

This reverts commit eef4c77.

* Revert "use correct deserialization"

This reverts commit 9eacba9.

* Revert "unwrap results"

This reverts commit b0df1e1.

* Revert "unwrap results"

This reverts commit de3cfbd.

* Revert "fix typo"

This reverts commit c12045d.

* Revert "new serialization"

This reverts commit e56a088.

* Revert "cargo update"

This reverts commit 15898da.

* Revert "fix typo"

This reverts commit c89e963.

* Revert "import AffineRepr"

This reverts commit 5a103ac.

* Revert "use into_iter"

This reverts commit 2e31d91.

* Revert "no collect Vec"

This reverts commit db18dca.

* Revert "collect as vec"

This reverts commit dd3f809.

* Revert "collect as vec"

This reverts commit 9167d59.

* Revert "use into"

This reverts commit 344cfff.

* Revert "fix ersialized_size"

This reverts commit c6a7609.

* Revert "use chunks for scalars"

This reverts commit 67987ae.

* Revert "use into_iter for chunks"

This reverts commit 1ddd6b8.

* Revert "replace Vec<Vec<u8>>"

This reverts commit 4d3b13c.

* cargo update

* cargo update

* Revert "replace Vec<Vec<u8>>"

This reverts commit 4389714.

* cargo update

* add error

* add error

* add error

* fix typo

* fix imports

* import coded

* import codec

* import PairingError

* fix patches

* sp-arkworks

* sp-arkworks

* use random values for multiplications

* cargo update

* fix imports

* fix imports

* add host functions

* re-add mul impls

* cargo update

* cargo update

* cargo update

* cargo update

* cargo update

* cargo update

* cargo update

* PairingError -> ()

* remove PairingError

* cargo update

* cargo update

* cargo update

* reduce boilerplate code

* cargo update

* update comments

* cargo update

* optimize code quality

* use ark_scale (#13954)

* use ark_scale

* fix tests

* fix tests

* cleanup & comments

* use correct PR branch

* hazmat

* ed curves, use ArkScaleProjective

* Achimcc/arkworks integration remove affine hostcalls (#13971)

* remove affine host-calls

* remove affine host-call impls, also in tests

* cargo update

* ark-substrate: use main branch

* cargo update

* Achimcc/arkworks integration bandersnatch (#13977)

* use bandersnatch

* bandersnatch

* add abndersnatch sw msm

* use correct PR branch

* cargo update

* cargo update

* fix tests

* cleanup

* cleanup

* fix tests

* refactor tests

* cargo update

* cargo update

* cargo update

* refactor tests

* cleanup & update tests

* upgrade arkworks/algebra

* cargo update

* adopt tests

* versioning ark-substrate

* cargo update

* remove patched deps

* bump ark-scale

* use crates-io deps

* fix doc comments

* Cargo.toml, linebreaks at end

* reorgainze tests

* sp-arkworks -> sp-crypto-ec-utils

* move host functions to crypto-ec-utils

* fmt

* remove sp-ec-crypto-utils from io

* remove unwrap from te msm

* remove elliptic_curves references in test

* elliptic_curves references in test

* update doc comments

* remove warn missing docs

* fmt

* cargo update

* update doc comments

* cargo update

* cargo update, bump arkworks, codec versions

* bump runtime version in sp-crypto-ec-utils

* remove feature flag ec-utils-experimental

* crypto-ec-utils -> crypto/ec-utils

* tests/ -> test-data/

* update doc comments for signatures

* update comments

* update doc comments for signatures

* fix doc comments

* fix doc comments

* fix doc comments

* fix doc comments

* fix doc comments

* cleanup

* fix doc comments

* cargo update

* fix doc comments

* cargo update
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix

* fix compression

* fix compression

* fix compression

* fix compression

* fix compression

* fix std leak

* fix std leak

* fix std leak

* merge master

* merge master

* cargo update

* cargo update

* cargo update

* cargo update

* cargo update

* use serialize_result

* cargo update

* cargo update

* cargo update

* cargo update

* reduce boilerplate code

* remove host function muls

* reduce boilerplate code

* remove patches

* uuse correct ark-substrate branch

* reduce boilerplate code

* cleanup

* cleanup

* proper error handling

* derive serialize for error

* proper error handling

* proper error handling

* proper error handling

* derive Debug for PairingError

* sp-arkworks path

* cargo update

* adopt tests to error handling

* fix tests

* cargo update

* remove results

* deserialize as G2Affine

* cargo update

* add codex index to PairingError

* replace Vec<Vec<u8>>

* replace Vec<Vec<u8>>

* use into_iter for chunks

* use chunks for scalars

* fix ersialized_size

* use into

* collect as vec

* collect as vec

* no collect Vec

* use into_iter

* import AffineRepr

* fix typo

* cargo update

* new serialization

* fix typo

* unwrap results

* unwrap results

* use correct deserialization

* fix bugs, cleanup

* correct len

* vec without capacity

* Revert "vec without capacity"

This reverts commit 2b1cd00.

* Revert "correct len"

This reverts commit b85de86.

* Revert "fix bugs, cleanup"

This reverts commit eef4c77.

* Revert "use correct deserialization"

This reverts commit 9eacba9.

* Revert "unwrap results"

This reverts commit b0df1e1.

* Revert "unwrap results"

This reverts commit de3cfbd.

* Revert "fix typo"

This reverts commit c12045d.

* Revert "new serialization"

This reverts commit e56a088.

* Revert "cargo update"

This reverts commit 15898da.

* Revert "fix typo"

This reverts commit c89e963.

* Revert "import AffineRepr"

This reverts commit 5a103ac.

* Revert "use into_iter"

This reverts commit 2e31d91.

* Revert "no collect Vec"

This reverts commit db18dca.

* Revert "collect as vec"

This reverts commit dd3f809.

* Revert "collect as vec"

This reverts commit 9167d59.

* Revert "use into"

This reverts commit 344cfff.

* Revert "fix ersialized_size"

This reverts commit c6a7609.

* Revert "use chunks for scalars"

This reverts commit 67987ae.

* Revert "use into_iter for chunks"

This reverts commit 1ddd6b8.

* Revert "replace Vec<Vec<u8>>"

This reverts commit 4d3b13c.

* cargo update

* cargo update

* Revert "replace Vec<Vec<u8>>"

This reverts commit 4389714.

* cargo update

* add error

* add error

* add error

* fix typo

* fix imports

* import coded

* import codec

* import PairingError

* fix patches

* sp-arkworks

* sp-arkworks

* use random values for multiplications

* cargo update

* fix imports

* fix imports

* add host functions

* re-add mul impls

* cargo update

* cargo update

* cargo update

* cargo update

* cargo update

* cargo update

* cargo update

* PairingError -> ()

* remove PairingError

* cargo update

* cargo update

* cargo update

* reduce boilerplate code

* cargo update

* update comments

* cargo update

* optimize code quality

* use ark_scale (paritytech#13954)

* use ark_scale

* fix tests

* fix tests

* cleanup & comments

* use correct PR branch

* hazmat

* ed curves, use ArkScaleProjective

* Achimcc/arkworks integration remove affine hostcalls (paritytech#13971)

* remove affine host-calls

* remove affine host-call impls, also in tests

* cargo update

* ark-substrate: use main branch

* cargo update

* Achimcc/arkworks integration bandersnatch (paritytech#13977)

* use bandersnatch

* bandersnatch

* add abndersnatch sw msm

* use correct PR branch

* cargo update

* cargo update

* fix tests

* cleanup

* cleanup

* fix tests

* refactor tests

* cargo update

* cargo update

* cargo update

* refactor tests

* cleanup & update tests

* upgrade arkworks/algebra

* cargo update

* adopt tests

* versioning ark-substrate

* cargo update

* remove patched deps

* bump ark-scale

* use crates-io deps

* fix doc comments

* Cargo.toml, linebreaks at end

* reorgainze tests

* sp-arkworks -> sp-crypto-ec-utils

* move host functions to crypto-ec-utils

* fmt

* remove sp-ec-crypto-utils from io

* remove unwrap from te msm

* remove elliptic_curves references in test

* elliptic_curves references in test

* update doc comments

* remove warn missing docs

* fmt

* cargo update

* update doc comments

* cargo update

* cargo update, bump arkworks, codec versions

* bump runtime version in sp-crypto-ec-utils

* remove feature flag ec-utils-experimental

* crypto-ec-utils -> crypto/ec-utils

* tests/ -> test-data/

* update doc comments for signatures

* update comments

* update doc comments for signatures

* fix doc comments

* fix doc comments

* fix doc comments

* fix doc comments

* fix doc comments

* cleanup

* fix doc comments

* cargo update

* fix doc comments

* cargo update
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited E3-host_functions PR adds new host functions which requires a node release before a runtime upgrade. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.