zk-plonk delivery milestone 1

[ashWhiteHat]

Milestone Delivery Checklist

• The milestone-delivery-template.md has been copied and updated.
• The invoice form 📝 has been filled out for this milestone.
• This pull request is being made by the same account as the accepted application.
• In case of acceptance, the payment will be transferred to the BTC/ETH payment address provided in the application.
• The delivery is according to the Guidelines for Milestone Deliverables.

Link to the application pull request: w3f/Grants-Program#454

[semuelle]

Thanks for the delivery, @noctrlz. We will look into it as soon as possible.

[mmagician]

Hi again @noctrlz! I've started looking into your delivery and so far it looks mostly good.
It's exciting to see PLONK being integrated into substrate and I appreciate the effort you've taken to make sure all libraries used under the hood are no-std compatible and that any types needed are SCALE serialised. There are, however, some issues that I would like to ask you to address before continuing:

dependencies

1. naming a fork of dusk-plonk parity-plonk - why? You are not part of parity, nor is it a fork approved by parity (AFAIK).
2. same for parity-subtle
3. same for parity_rand_xorshift in rngs repo. You even re-export XorShiftRng as ParityRng?
4. jubjub renamed to plonk-jubjub - very confusing!
5. same for bls12_381

I don't understand why renaming jubjub to plonk-jubjub etc. is unnecessary. Rather, why not keep the forks under original names, and in your Cargo.toml specify the dependency as git = ..., instead of pointing to the default crates.io? Otherwise, when I first saw plonk-jubjub, I didn't even suspect that it is not the original version.
Where relevant, you can also try to merge upstream. I know that it probably doesn't apply in most cases, since I've seen you adding very substrate specific support, but maybe I missed some where it would work.

Examples

• I tried to add your pallet to a fresh copy of substrate node template, you can find my fork here.
  Then I ran it in dev mode and tried to submit some Tx from polkadot-js/apps, but it fails with the following error:

createType(Call):: Call: failed decoding zkTest.trustedSetup:: Struct: failed on args: :: Struct: failed on rng: Type:: DoNotConstruct: Cannot construct unknown type ParityRng

• types.json in your example is empty

Please let me know once you've address the above issues & comments, so we can proceed further, and thanks for all the great work so far!

[ashWhiteHat]

Hi @mmagician
Thank you for the review!
I have addressed dependencies and there is a question about example.

Dependencies

I changed the dependencies name as following.

parity-plonk -> fullcodec-plonk
parity-subtle -> fullcodec-subtle
parity_rand_xorshift -> fullcodec_rand_xorshift
plonk-jubjub -> fullcodec-jubjub
bls12_381 -> fullcodec-bls12_381
ParityRng -> FullcodecRng

Previous crates were yanked.

You even re-export XorShiftRng as ParityRng?

The reason why I re-export the XorShiftRng is that the XorShiftRng is well known crate and if they use original XorShiftRng to import themself, this crate wouldn't work because this crate is using customized XorShiftRng.
I think it's confusing so I renamed in order not developer to import original XorShiftRng.

I don't understand why renaming jubjub to plonk-jubjub etc. is unnecessary. Rather, why not keep the forks under original names, and in your Cargo.toml specify the dependency as git = ..., instead of pointing to the default crates.io?

The crates.io doesn't allow to publish the crate with git = ..., rev = ... dependencies so I needed to publish crate with renaming.
I think this is because if that dependencies repo was deleted, that crate wouldn't work anymore...

Examples

types.json in your example is empty

I removed the types.json.

Question

Then I ran it in dev mode and tried to submit some Tx from polkadot-js/apps, but it fails with the following error:

How did you test with polkadot-js/apps?
Did you implement it on your locally?

Thank you for the polite review!
I am going to address the rest issue as soon as possible 😎

[mmagician]

But do you really need to publish to crates.io?
Why not just use the GitHub dependencies in your code? As I had already written, this would allow you to keep the original names.
Especially for such forks, I don't see why having the crate in crates.io would bring any benefit - your forks are likely not going to become the de-facto standard and replace the original.

Regarding local testing: I followed your tutorial to add the pallet to a substrate-node-template, see my previous comment. You should be able to clone my repo and test locally too.

I'm not sure deleting types.json is the best solution - there are some custom types and users should be able to use them from whatever client they choose (e.g. the Polkadot-js API will need types definition).

[ashWhiteHat]

Thank you for the fast reply!

But do you really need to publish to crates.io?
Why not just use the GitHub dependencies in your code? As I had already written, this would allow you to keep the original names.

In the case that plonk-pallet has Github dependencies, this pallet can't be published.
I mean every dependencies can't be described as git and rev.
Do you mean that developers use this pallet to fork this repo and import from locally?

When we publish the crate with Github dependencies, the following error happens.

➜  plonk git:(master) cargo publish --allow-dirty
    Updating crates.io index
error: all dependencies must have a version specified when publishing.
dependency `fullcodec-jubjub` does not specify a version
Note: The published dependency will use the version from crates.io,
the `git` specification will be removed from the dependency declaration.

Regarding local testing: I followed your tutorial to add the pallet to a substrate-node-template, see my previous comment. You should be able to clone my repo and test locally too.

I think in my tutorial, there is no description about polkadotjs/apps.
I think substrate-node-template doesn't have polkadotjs/apps script too.
Could you indicate which scripts you used?

I'm not sure deleting types.json is the best solution - there are some custom types and users should be able to use them from whatever client they choose (e.g. the Polkadot-js API will need types definition).

Yeah, I think that type definition should be in pallet not in example.

[mmagician]

So I'm saying - don't publish it :) just keep it on github, and developers can use it as described here: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-git-repositories

Do you mean that developers use this pallet to fork this repo and import from locally?

No, of course not. But in their own Cargo.toml, they can just specify it as:

[dependencies]
plonk-pallet = { git = "https://github.com/AstarNetwork/plonk" }

Absolutely nothing wrong with that! As a reference, you can see how some of Polkadot's dependencies are specified: https://github.com/paritytech/polkadot/blob/master/runtime/parachains/Cargo.toml#L21

I think in my tutorial, there is no description about polkadotjs/apps.

No, there isn't. It's just what I used for testing a minimal chain with your pallet, which is not a unit test. I don't have any script, I just tried to run my chain locally and interact with its pallets via polkadot-js/apps. Otherwise, I can't really test it properly, since, as I said, the tests you provide are only unit tests and cannot be used to substitute a full integration test.
It would be great if you could provide a script that tests all the functionality against a locally-running substrate node, such as the one I have shared earlier.

Btw, here are some minor improvements to the tutorial:
AstarNetwork/plonk#3

[ashWhiteHat]

No, of course not. But in their own Cargo.toml, they can just specify it as:

Aha that makes sense!
Okay I am going to define dependencies as git.

It would be great if you could provide a script that tests all the functionality against a locally-running substrate node, such as the one I have shared earlier.

I think this grant project purpose is to implement plonk pallet which can be referred from other pallet as api.
I already implemented plonk pallet which is compatible with substrate and described the tutorial how to refer it from other pallet.
I think this is out of scope.

Btw, here are some minor improvements to the tutorial:

hahaha cool!
I merged.
Thank you for your contribution!

[mmagician]

Yeah, fair. The deliveries clearly state just unit tests.
Still, I would insist on keeping a types.json around. In the end, it's not going to be just chain developers importing your pallet, but also dapp developers who just need to call your API - and for this they need type definitions.

Yeah, I think that type definition should be in pallet not in example.

Sure.

[ashWhiteHat]

I fixed the dependencies according to your review!

Still, I would insist on keeping a types.json around. In the end, it's not going to be just chain developers importing your pallet, but also dapp developers who just need to call your API - and for this they need type definitions.

I found the issue about types.json on stackoverflow.
Above says that types.json won't be necessary after the following PR are merged.
paritytech/substrate#8615
And also polkadotjs has PR about it and both were merged.
paritytech/polkadot#3336
The types.json problem is fixed by referring the latest version of polkadotjs.

I would appreciate it if you could confirm the chages!
Thank you!

[mmagician]

Hmm yeah I've seen this PR some time ago. However, I'm using the latest version of polkadot-js and still seeing this.
It should be very easy to recreate the error on your side - just build the chain I've created and try to submit a "trusted setup" transaction through your pallet via polkadot-js/apps.

Thanks for updating the dependencies, will check on it soon.

[ashWhiteHat]

just build the chain I've created and try to submit a "trusted setup" transaction through your pallet via polkadot-js/apps

I would like to know how you submitted transaction.
It's okay just rough one.

[mmagician]

[mmagician]

@noctrlz any updates? Were you able to reproduce the error?

[ashWhiteHat]

@mmagician
I was successful for reproduce but I couldn't fix the error so I asked polkadot-js now.
polkadot-js/apps#6729

Sorry for late...

[mmagician]

@noctrlz I'll put this milestone on hold for now, as it seems the issue hasn't been resolved yet.

[ashWhiteHat]

Hi @mmagician
I added types.json and checked it worked when it was imported on polkadotjs app.
https://github.com/AstarNetwork/plonk/blob/master/types.json
Please confirm!

[ashWhiteHat]

Hi @mmagician
Is everything going well?

[mmagician]

@noctrlz apologies for the delay. So far I haven't managed to review your changes. I will do my best next week and if not, ask another colleague to help out. Thanks for the patience!

[ashWhiteHat]

@mmagician
All right 👍
Please feel free to ping me if you have a question.

[alxs]

@noctrlz I'll take a look at this.

In your application, you describe this sequence of steps to build a circuit. Could you provide documentation covering all the steps? I haven't followed the tutorial you provide, but it seems to be quite a different process from that. Also, where can I review the circuit template library you mention?

Besides, could you make sure your project complies with the dusk-network plonk license? If you're using files from it, you must preserve copyright and license notices. You also need to add a license to your repository.

[alxs]

@noctrlz friendly reminder.

[ashWhiteHat]

Hi @alxs !
Sorry for late.

In your application, you describe w3f/Grants-Program#454 (comment) to build a circuit. Could you provide documentation covering all the steps?

As we told here, this grant scope was to implement pallet, and how to deploy and interact with node is described as substrate documentation.
With the tutorial I created, The developers can understand how to import this library and use plonk functionality.

Also, where can I review the circuit template library you mention?

The custom circuit is following.
https://github.com/AstarNetwork/plonk/blob/master/examples/src/tests.rs#L78
The api is the same with dusk-plonk.

Besides, could you make sure your project complies with the dusk-network plonk license? If you're using files from it, you must preserve copyright and license notices. You also need to add a license to your repository.

Exactly!
I added LICENSE and copyright credit.
Please confirm!

[alxs]

The application also lists the following as in scope for this milestone, and I can't find either:

• a basic tutorial that explains how a developer builds a circuit and a user prove their computation through the circuit.
• a set of plonk-based zkSNARK libraries that allow a developer to build their own circuit and a user to prove their computation validity.

[ashWhiteHat]

Thank you for the review! @alxs

a basic tutorial that explains how a developer builds a circuit

This is described in following section.
TestCircuit is the example of build circuit.
https://astarnetwork.github.io/plonk/#4import-the-coupling-pallet-to-testruntime-and-define-your-circuit

a user prove their computation through the circuit.

This is described in following section.
Through circuit.prove(&pp, &pk, b"Test").unwrap(), the user prove the computation.
https://astarnetwork.github.io/plonk/#5test-whether-the-functions-work-correctly
In above section, the user create proof through circuit with their witness.

a set of plonk-based zkSNARK libraries that allow a developer to build their own circuit and a user to prove their computation validity.

This question seems the same question as above.
If you want, I can explain overall using zoom or something.
Please confirm!

[alxs]

Yeah ok, I fundamentally misunderstood what was being delivered here. The delivery looks good. I was confused by the wording at places, such as this being called a "circuit template library" (though what it provides are rather components with which to build a circuit) and the multiple mentions of this allowing a developer to build their own circuit, while that's really the PLONK library's feat and you're "just" integrating it.

Nevertheless, this is irrelevant to the implementation and I was able to successfully test a simple trusted setup. I'm happy to tell you that the milestone is a pass. You can find my evaluation notes here. I'll notify the operations team to pay out your invoice.

[alxs]

@noctrlz we have no invoice submission from you. Could you fill in the form please?

[ashWhiteHat]

Hi @alxs
Thank you for the fast review!

I was confused by the wording at places

Yeah the zk has a lot of terminologies so kind of a hard.
We are going to fill in the form soon!

[ashWhiteHat]

Hi @alxs
I filled in the form!
Please confirm.
Thank you.

[meldien]

Hi @noctrlz
Many thanks for the invoice.
I noted the address is unfortunately missing. Could you please update the invoice with following address information:
Web 3.0 Technologies Foundations
Baarerstrasse 14
6300 Zug

Many thanks.

[ashWhiteHat]

Hi @meldien
I'm sorry for your inconvenience.
I filled it again.
Please confirm.
Thank you.

[meldien]

Hi @noctrlz
I confirm the invoice.
Many thanks.

[RouvenP]

hi @noctrlz we transferred the payment today. thanks!

[ashWhiteHat]

Hi @RouvenP
Thank you!

[ashWhiteHat]

@RouvenP @meldien
I have received!