Skip to content

Starks Network Milestone 1#44

Merged
Noc2 merged 3 commits intow3f:masterfrom
gbctech:master
Dec 4, 2020
Merged

Starks Network Milestone 1#44
Noc2 merged 3 commits intow3f:masterfrom
gbctech:master

Conversation

@xz-cn
Copy link
Copy Markdown
Contributor

@xz-cn xz-cn commented Oct 15, 2020

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.

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

@Noc2
Copy link
Copy Markdown
Contributor

Noc2 commented Oct 15, 2020

Thanks for delivering the milestone, we will look into it as soon as possible.

@Noc2
Copy link
Copy Markdown
Contributor

Noc2 commented Oct 27, 2020

Hi. Sorry for the late reply. I started to look into your project and have a few questions/comments:

  • The distaff vm license link doesn’t work here https://github.com/gbctech/starks-node/blob/master/frame/distaff-vm/LICENSE
  • In general it seems you only copy pasted the distaff vm code. It would be nice to point this out in the distaff vm readme and link to the original repo.
  • How did you integrate the following”We will reorganize the Distaff VM project, split its functions into sub-modules and create frame pallet(s) following the conventions of the Substrate framework. We will perform configuration/optimization work to make the VM module easier to use for typical use cases of the Starks Network.” From a first look it doesn’t seem like you reorganized the project or did any optimization.
  • The tests you linked in your delivery are only the once that were already part of the original distaff vm. Did you integrate any additional tests for the new code that you integrated?
  • Is anything actually accessible on-chain at this point (e.g. runtime_interface, off-chain worker)?

@xz-cn
Copy link
Copy Markdown
Contributor Author

xz-cn commented Nov 2, 2020

Hello! Thank you very much for your comments on our milestone on Github. Regarding your questions:

  1. We will fix the License links.

  2. We left the Distaff VM readme file as it was to retain the original information. And sure we can update it to point to the original repo.

  3. There are a few steps we had in mind to fully integrate the Distaff VM into the Starks Node.

(1) Convert the Distaff VM from a binary project to a library project and make it as a native module of the Substrate node. Then make the verification function accessible via the rpc/rpc-api interface in the client directory.
(2) Change of the Polkadot.js app to enable the web side access to the proof verification function.
(3) Demonstrate of the concept of the node and front-end working together using docker containers.
(4) Split the proof generation/verification codes into two separate Rust packages and keep only the verification part on chain.
(5) The splitting will make the Distaff VM in the node simpler than it is right now. We can then proceed with further optimization/rewrite of it without using the Rust standard library.
(6) Convert the Distaff VM (verification part) into a wasm compatible pallet instead of a native module using Substrate pallet macros.

Right now the submitted milestone include steps (1), (2) and (3). We were working on step (4) by the time we made the milestone submission (as we understood we must submit something within one month). We were progressing a bit late than we expected, partly because of the Chinese national holiday (almost 10 days off for everyone).

In addition, step (5) may involve the rewrite of related cryptographic libraries using no_std Rust. It will probably involve some research into these libraries and it is much more difficult than we expected. We are almost sure we cannot cover it within the timeframe/resource available in this open grant and we may need some external help for this.

So indeed, I think we should have divided what we wanted to do/deliver with this item in more details in our open grant proposal. And for this open grant milestone, we will finish step (4) to keep only the verifier part of the VM in Substrate.

  1. We did write some test to test the rpc part we added in the client/rpc/src/distaff_vm. But we ran into some problems. Thing is if we use 'cargo test', it will take a very long time to test the entire repo and the result for the part of the rpc test will be buried in the output logs. If we use 'cargo test -p sc-rpc’ to only test the sc-rpc part (without adding our test yet), the test will fail with some errors. We were a bit confused about the test approach used in Substrate and we really welcome any hints from you.

  2. The Distaff VM verification function is available on-chain right now and it is fully ready to be accessed by 1) rpc via front-end (demoed in our docker container) and 2) a wasm pallet (the off-chain worker to be designed) via runtime_interface. The off-chain worker we are going to design will include the runtime_interface code to access the VM. But that is another milestone we will cover later.

Hopefully this clarifies some of your questions.

@Noc2
Copy link
Copy Markdown
Contributor

Noc2 commented Nov 5, 2020

Thanks for your detailed response. As I wrote in my email, we don’t enforce the timeline in this case and give you sufficient time to integrate these changes. Could you ping me once you integrated it and also create an amendment/PR to update the original contract, reflecting those changes: https://github.com/w3f/Open-Grants-Program/blob/master/applications/starks_network.md Regarding 5, just for clarification, so currently there is nothing actually stored on-chain, correct?

@xz-cn
Copy link
Copy Markdown
Contributor Author

xz-cn commented Nov 5, 2020

Thanks for your detailed response. As I wrote in my email, we don’t enforce the timeline in this case and give you sufficient time to integrate these changes. Could you ping me once you integrated it and also create an amendment/PR to update the original contract, reflecting those changes: https://github.com/w3f/Open-Grants-Program/blob/master/applications/starks_network.md Regarding 5, just for clarification, so currently there is nothing actually stored on-chain, correct?

Thank you for your response. We will make the integrations needed and submit the changes later on. Concerning 5, yes, at this moment nothing will be stored on-chain yet.

@xz-cn
Copy link
Copy Markdown
Contributor Author

xz-cn commented Dec 1, 2020

@Noc2 Hello! We have updated the milestone delivery. Please check it out.

Regarding previous questions:

  1. The repo now uses the Apache 2.0 license.
  2. As mentioned above, we have finished (1)-(4) of step 3 to integrate the Distaff VM into the Substrate Node. The Distaff VM has been trimmed to keep only the proof verification function—the proof generation part has been removed. Now it has been split into two directories. The VM pallet resides in the frame directory and the starks verifier function and related crypto libraries are put into the primitives directory. A link to the original Distaff VM repo has been given in the readme file.
  3. Tests of the two parts above are carried out separately as specified in the delivery file.

We have also modified the open grant proposal to reflect the actual work we have done at this stage. The PR to change the proposal is here.

Please feel free to commenting in case of any questions. Thank you!

@Noc2
Copy link
Copy Markdown
Contributor

Noc2 commented Dec 2, 2020

@xz-cn Thanks for the update. Now that the amendment is merged, I took another look at your delivery, but it seems the distaff vm pallet isn’t integrated into the runtime (e.g. it neither shows up under chainstate nor under extrinsics in polkadot.js). Also the inline documentation is still based on the template here. Could you improve this?

@xz-cn
Copy link
Copy Markdown
Contributor Author

xz-cn commented Dec 2, 2020

@xz-cn Thanks for the update. Now that the amendment is merged, I took another look at your delivery, but it seems the distaff vm pallet isn’t integrated into the runtime (e.g. it neither shows up under chainstate nor under extrinsics in polkadot.js). Also the inline documentation is still based on the template here. Could you improve this?

@Noc2 Thank you for your comments. Yes we can further integrate the pallet into the runtime.

@xz-cn
Copy link
Copy Markdown
Contributor Author

xz-cn commented Dec 4, 2020

@Noc2 We have integrated the vm pallet into the node runtime and now it is visible under extrinsics in polkadot.js UI. The inline documentation have also been improved. Please have a look. Thank you!

@Noc2
Copy link
Copy Markdown
Contributor

Noc2 commented Dec 4, 2020

Thanks for fixing everything. I’m happy to confirm that this milestone is a pass! I made some notes regarding the evaluation here and I will forward your invoice internally.

@Noc2 Noc2 merged commit b27e739 into w3f:master Dec 4, 2020
@xz-cn
Copy link
Copy Markdown
Contributor Author

xz-cn commented Dec 4, 2020

@Noc2 Thank you very much for your review and support!

failfmi pushed a commit to LimeChain/Grant-Milestone-Delivery that referenced this pull request Sep 26, 2022
Substrate startkit GUI Grant application revision
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.

2 participants