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

Manta-Network-Milestone-1 #96

Merged
merged 14 commits into from
Mar 24, 2021
Merged

Manta-Network-Milestone-1 #96

merged 14 commits into from
Mar 24, 2021

Conversation

stechu
Copy link
Contributor

@stechu stechu commented Feb 8, 2021

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 done by the same account, which is responsible for the pull request of the accepted application.
  • In the case of acceptance, the payment will be transferred to the initial BTC payment address.
  • The delivery is according to the milestone delivery guidelines.

@mmagician
Copy link
Contributor

@stechu Thanks for the delivery. As part of the requirement for an Open Grants Program, all code must be open-sourced and a docker image is not enough. If you prefer to wait before making your backend repos public, feel free to close this PR for now. Thanks for the understanding.

@stechu
Copy link
Contributor Author

stechu commented Feb 9, 2021

@mmagician we hear you. I updated the open sourced pallet implementation.

@stechu
Copy link
Contributor Author

stechu commented Feb 22, 2021

@mmagician any updates?

@mmagician
Copy link
Contributor

@stechu Sorry for the delay. One of our team members is already looking at your milestone, but please allow a couple of extra days due to a large number of applications received recently.

@stechu
Copy link
Contributor Author

stechu commented Feb 22, 2021

@mmagician No worry. Happy to answer questions about the code here.

@alxs alxs self-assigned this Feb 24, 2021
@alxs
Copy link
Contributor

alxs commented Feb 26, 2021

Hi @stechu and thank you for the delivery. Also thanks for the additional front end deliverable, it's appreciated.

I have a few initial comments:

  • As is, the project is currently Unlicensed and not Apache 2.0. Is this deliberate?
  • We would like to ask you to please create your own README or appropriate documentation and refrain from reusing the one from the Substrate node template. Ideally, you would create a separate repository for your pallet (eventually publishing it on crates.io) and include that in your node. Then you only need to create documentation for the pallet, which is the actual deliverable.
  • In your application you specify that you will provide benchmarks on Manta DAP transaction latency and throughput, however in your delivery you only provide benchmarking for SNARK verification. Could you please add this? Also, what hardware did you run the benches on?
  • Could you provide instructions on how to generate the values necessary to mint/transfer and what they represent? I have no way of testing these functions without the required parameters.
  • I would suggest that you coordinate with your research team to find a way to a) hide the value in private coins and b) split/merge private coins ASAP, as the former constitutes a considerable privacy concern and the latter is basic expected functionality. It may be a good idea to implement this before the DAX.
  • Think you could provide some more unit tests? We generally expect deliveries to achieve >70% code coverage.

@alxs
Copy link
Contributor

alxs commented Feb 26, 2021

Also just wanted to add that manta_bench failed to get past key generation within an hour on my machine.

@stechu
Copy link
Contributor Author

stechu commented Feb 26, 2021

@alxs Thanks for your reviews. We will address all the issues in the coming changes. Briefly:

  1. encrypted transfer is already working in our development repo, will port it to the open sourced one
  2. having a separate repo only for the pallet makes a lot of sense. Will do that.
  3. Yeah, it is well known that the key generation of zkSNARK is expensive. We will remove that from benchmark test.
  4. It will be Apache licenced.

@stechu
Copy link
Contributor Author

stechu commented Mar 10, 2021

@mmagician @alxs we updated the milestone delivery with almost all the requested changes implemented.

  1. Reorganize the code to 3 repos with a stand alone manta-dap-pallet implementation
  2. Add an Apache license (we will switch to GNU GPL soon actually)
  3. Add a forfeit function
  4. Make the transferred amount encrypted
  5. Benchmark using frame-bench
  6. Better unit test with >90% test coverage

The only thing that we didn't implement yet is the split/merge. You will still get privacy by using a tornado cash styled fix nomination transfer. We plan to implement split/merge before the DAX implementation for sure.

@alxs
Copy link
Contributor

alxs commented Mar 10, 2021

Many thanks for the updates. Looks really good at first sight. I'll look into it in the coming days, next week at the latest.

@mmagician
Copy link
Contributor

I'm putting this temporarily on hold due to parallel participation in the hackathon. The hackathon organisers will contact you regarding further steps.

@stechu
Copy link
Contributor Author

stechu commented Mar 22, 2021

@mmagician mind removing the on-hold since the hackathon is completed?

@mmagician mmagician removed the on hold label Mar 22, 2021
@alxs
Copy link
Contributor

alxs commented Mar 23, 2021

Hi @stechu. Thanks for all the added effort you've put into this. I have a few follow-up questions:

  • I understand it doesn't make sense to include ZKP generation in the benchmarks as this is unrelated to on-chain latency. However, since it's still relevant to the usability of the application, could you provide an estimate on the generation time and an easy way to verify it for a test transaction?
  • Could you indicate what the flags you provide to run the tests do and why these are needed?
  • This had previously caught my attention, what does the list field in the TransferCircuit represent? The state of the ledger should most certainly not be included in the transfer circuit, but perhaps one of these is not what it seems.
  • Finally, could you provide a Docker file like you initially did, as per your application? Feel free to wait until we've agreed on everything else so you can just create one for the final version.

@zhenfeizhang
Copy link
Contributor

Hi @alxs

I understand it doesn't make sense to include ZKP generation in the benchmarks as this is unrelated to on-chain latency. However, since it's still relevant to the usability of the application, could you provide an estimate on the generation time and an easy way to verify it for a test transaction?

Please see Manta-Network/pallet-manta-pay@b049b78
With our benchmark, current proof generation is around 5 second. You can verify this via cargo bench

Could you indicate what the flags you provide to run the tests do and why these are needed?

For pallet-manta-dap, you can simply use cargo test --release.
For manda-node, we used make run with the default flags from the template, i.e., cargo run --release -p manta-node -- --dev --tmp. Here --dev is to specify that the realy chain is a dev, and --tmp is to discard the log files, etc.
We do not need additional flags.

This had previously caught my attention, what does the list field in the TransferCircuit represent? The state of the ledger should most certainly not be included in the transfer circuit, but perhaps one of these is not what it seems.

This is the list of all the commitments that have ever appeared on chain. It is not the state of the ledger, although its size may actually be larger than the ledger state.

This is related to the privacy. In the proof, we prove that we know a commitment from the list, and obviously, the larger the list, the less information is leaked. There are researches on reducing this list via sliding windows. We currently do not support this method.

Finally, could you provide a Docker file like you initially did, as per your application? Feel free to wait until we've agreed on everything else so you can just create one for the final version.

Sounds good! Be happy to.

Please let us know if you have any additional questions.

Zhenfei

@alxs
Copy link
Contributor

alxs commented Mar 24, 2021

Maybe I didn't make myself clear. The 5 second benchmark is with the pregenerated values, correct? I'm interested in what happens when you also need to generate these values. I think the execution time of some of the tests is closer to what I'm asking about.

I'm referring to these flags in the pallet-manta-dap README:

export CARGO_INCREMENTAL=0
export RUSTFLAGS="-Zprofile -Ccodegen-units=1 -Copt-level=0 -Clink-dead-code -Coverflow-checks=off -Zpanic_abort_tests -Cpanic=abort"
export RUSTDOCFLAGS="-Cpanic=abort"

Regarding the list field, I understand that, but shouldn't you be storing the list of commitments in a Merkle tree and then only including a Merkle path + root in the circuit instead of the plain list?

@zhenfeizhang
Copy link
Contributor

zhenfeizhang commented Mar 24, 2021

Maybe I didn't make myself clear. The 5 second benchmark is with the pregenerated values, correct? I'm interested in what happens when you also need to generate these values. I think the execution time of some of the tests is closer to what I'm asking about.

Let me clarify.

  • The pre-generated values are the common reference string which is setup once (through some MPC protocol) and can be reused by all users. This process is long (a few minutes), but is in some sense orthogonal to the performance of our protocol -- the standard way to setup CRS is through certain ceremony which may take, say, a few months of time.
  • The proving function does not take any additional pre-generated values other than this CRS. The time to generate a proof, given the CRS and the required fields of the sender's and receiver's tokens as inputs, is around 5 seconds.

I'm referring to these flags in the pallet-manta-dap README:

those flags are required by gcrov: https://github.com/mozilla/grcov which produces the test coverage report.

for normal test/use of our code, we do not need those flags

Regarding the list field, I understand that, but shouldn't you be storing the list of commitments in a Merkle tree and then only including a Merkle path + root in the circuit instead of the plain list?

Yeah, right. This will work.
Only path is used in the r1cs generation. See
https://github.com/Manta-Network/pallet-manta-dap/blob/b049b782ca65d2052101d2b7b93ff8eaa7417810/src/transfer.rs#L238
We may either allow the caller to generate the path and then pass it to the circuit,
or pass the whole merkle tree to the circuit and generate the path within the circuit function (but outside of the r1cs generation).
We chose the send method.
The r1cs is not taking the whole list as input, in either case.

(edited since I miss understood your last question)

@alxs
Copy link
Contributor

alxs commented Mar 24, 2021

Sounds good! Your last edit clears it, that's what I suspected.

So my last question would be, does that mean the CRS is generated as part of the tests and why don't you use the precomputed one?

Pending the addition of the Docker file, I'll accept the milestone.

@zhenfeizhang
Copy link
Contributor

So my last question would be, does that mean the CRS is generated as part of the tests and why don't you use the precomputed one?

it depends.
In some tests, for instance,
https://github.com/Manta-Network/pallet-manta-dap/blob/86665a28710605e42cbbd5d418d891d89de2fea3/src/test/mod.rs#L669,
the CRS is generated on the fly.
For other tests,
https://github.com/Manta-Network/pallet-manta-dap/blob/86665a28710605e42cbbd5d418d891d89de2fea3/src/test/mod.rs#L411
it loads pre-generated file.
We want to have some test coverage on CRS generation, while still have a somewhat nice testing experience :-)

Working on the docker now. Thanks!

@zhenfeizhang
Copy link
Contributor

zhenfeizhang commented Mar 24, 2021

Working on the docker now. Thanks!

Done. Following are the instructions:

  1. download and run the manta node:
    1. docker pull mantalab/manta-node:w3f-milestone-1
    2. docker container run -p 9944:9944 mantalab/manta-node:w3f-milestone-1
  2. setup the front end
    1. git clone https://github.com/Manta-Network/manta-front-end
    2. cd manta-front-end
    3. yarn install
    4. yarn start

@alxs
Copy link
Contributor

alxs commented Mar 24, 2021

Thanks, everything is working now. I'm happy to tell you that the milestone is a pass, you can find the evaluation notes here. I'll forward your invoice internally.

I'm aware that you already announced the grant a bit ahead of time, but if you'd like to collaborate on an official announcement, feel free to reach out to grantsPR@web3.foundation. We tweet out info on grant recipients on the second Monday of each month.

Also just a reminder that you may add the grants badge to your repos now, but that it shouldn't be used as a general endorsement e.g. on your website.

@alxs alxs merged commit 9fddecf into w3f:master Mar 24, 2021
@RouvenP
Copy link

RouvenP commented Mar 30, 2021

hi @zhenfeizhang we just sent a test transaction. Could you confirm if received?

@kennymuli
Copy link

Hi @RouvenP I just confirmed with Shumo that we received it.

@RouvenP
Copy link

RouvenP commented Apr 1, 2021

@kennymuli thanks for confirming. we transferred the remainder.

failfmi pushed a commit to LimeChain/Grant-Milestone-Delivery that referenced this pull request Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants