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

Serialization improvements #358

Merged
merged 23 commits into from
Apr 14, 2021
Merged

Serialization improvements #358

merged 23 commits into from
Apr 14, 2021

Conversation

dtebbs
Copy link
Contributor

@dtebbs dtebbs commented Mar 5, 2021

  • standard way to export variable assignments
  • some remaining interface inconsistencies and composability fixes
  • modified binary format with maximum compatibility with other implemenations. serializers for all relevant objects.

@dtebbs dtebbs force-pushed the binary-serialization branch 7 times, most recently from 656f479 to ef74dae Compare March 31, 2021 16:04
@AntoineRondelet AntoineRondelet marked this pull request as ready for review March 31, 2021 17:11
@AntoineRondelet
Copy link
Contributor

@dtebbs - I see that you modified the submodule here. Was that necessary for the CI to pass?
I'm asking in the context of #361 for which the CI fails with:

npm WARN The package scrypt is included as both a dev and production dependency.

npm ERR! code 1
npm ERR! Command failed: git submodule update -q --init --recursive
npm ERR! warning: templates not found in /tmp/pacote-git-template-tmp/git-clone-1caab2ca
npm ERR! fatal: remote error: upload-pack: not our ref 1710792e1b6da5905dadda19167475f458f2b2ba
npm ERR! Fetched in submodule path 'depends/libff', but it did not contain 1710792e1b6da5905dadda19167475f458f2b2ba. Direct fetching of that commit failed.
npm ERR! 

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/2021-03-31T16_54_31_814Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! zeth-contracts@1.0.0 install: `ln -s ../../depends/ganache-cli node_modules/ganache-cli && cd node_modules/ganache-cli && npm install`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the zeth-contracts@1.0.0 install script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/2021-03-31T16_54_33_344Z-debug.log

So I'm curious to know if you had the same here.

Also, I saw you requested a review here, so I changed the status of the PR from Draft to Ready to review. Please feel free to revert if my review was asked by mistake. Finally, I can see some unchecked boxes in your PR description, can I let you check them all if this PR is ready to go?

@dtebbs
Copy link
Contributor Author

dtebbs commented Apr 1, 2021

@dtebbs - I see that you modified the submodule here. Was that necessary for the CI to pass?
I'm asking in the context of #361 for which the CI fails with:

So I'm curious to know if you had the same here.

Yes, that looks like the same issue. While fixing the libff.js reference to a purged commit I hit other version related issues (stale version numbers in some package-lock.json files etc.). I'll put just the submodule update in a new PR so we can merge it independently. That should fix your PR too.

Also, I saw you requested a review here, so I changed the status of the PR from Draft to Ready to review. Please feel free to revert if my review was asked by mistake. Finally, I can see some unchecked boxes in your PR description, can I let you check them all if this PR is ready to go?

Yes, no problem. Thanks.

@AntoineRondelet
Copy link
Contributor

AntoineRondelet commented Apr 1, 2021

Yes, that looks like the same issue. While fixing the libff.js reference to a purged commit I hit other version related issues (stale version numbers in some package-lock.json files etc.). I'll put just the submodule update in a new PR so we can merge it independently. That should fix your PR too.

Great, thanks for confirming and moving the submodule update in another PR @dtebbs . Do you plan to rebase this one on top of #362?

[EDIT] Merged #362 since this PR is already rebased on top of it

verifier/verifier.cpp Outdated Show resolved Hide resolved
libzeth/core/field_element_utils.hpp Outdated Show resolved Hide resolved
libzeth/core/group_element_utils.hpp Outdated Show resolved Hide resolved
libzeth/core/group_element_utils.tcc Show resolved Hide resolved
libzeth/serialization/stream_utils.hpp Show resolved Hide resolved
libzeth/snarks/groth16/groth16_snark.hpp Show resolved Hide resolved
libzeth/snarks/groth16/groth16_snark.tcc Show resolved Hide resolved
@dtebbs
Copy link
Contributor Author

dtebbs commented Apr 12, 2021

Thanks for the comments @AntoineRondelet . Hopefully all addressed now. Note I added one small commit to end of this PR (to write out full assignments, required to generate test data for external provers), so you may want to review that in isolation. Thanks.

Copy link
Contributor

@AntoineRondelet AntoineRondelet left a comment

Choose a reason for hiding this comment

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

LGTM overall - thanks @dtebbs
The CI failed though, so I just restarted the jobs - let's see if that passes.

@dtebbs
Copy link
Contributor Author

dtebbs commented Apr 14, 2021

LGTM overall - thanks @dtebbs
The CI failed though, so I just restarted the jobs - let's see if that passes.

Ah, looks like I broke something in the final commit. I'll check.

@dtebbs
Copy link
Contributor Author

dtebbs commented Apr 14, 2021

LGTM overall - thanks @dtebbs
The CI failed though, so I just restarted the jobs - let's see if that passes.

Ah, looks like I broke something in the final commit. I'll check.

@AntoineRondelet Should be fixed now. Sorry about that.

@AntoineRondelet
Copy link
Contributor

Great, thanks @dtebbs - LGTM

@AntoineRondelet AntoineRondelet merged commit 676c386 into develop Apr 14, 2021
@AntoineRondelet AntoineRondelet deleted the binary-serialization branch April 29, 2021 16:26
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