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

feat: phase2-cli merge command #1242

Merged
merged 1 commit into from
Aug 5, 2020
Merged

Conversation

DrPeterVanNostrand
Copy link
Contributor

@DrPeterVanNostrand DrPeterVanNostrand commented Aug 5, 2020

The phase2-cli merge command merges large and small param files.

The final phase2 participant generates small-raw params, which are converted into non-raw using phase2 convert. The final small-nonraw params are then merged into the large initial params to produce the final MPCParameters file for a circuit.

@DrPeterVanNostrand DrPeterVanNostrand changed the title feature: phase2-cli merge command feat: phase2-cli merge command Aug 5, 2020
porcuquine
porcuquine previously approved these changes Aug 5, 2020
Copy link
Collaborator

@porcuquine porcuquine left a comment

Choose a reason for hiding this comment

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

Looks good.

filecoin-proofs/src/bin/phase2.rs Show resolved Hide resolved
Copy link
Contributor

@vmx vmx left a comment

Choose a reason for hiding this comment

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

Your PR description really explains nicely what it does, it would be great to have that text also in the commit message for future reference. BTW: if you put that into your commit message, it also automatically shows up as the description of the PR (in case you don't have much experience with re-writing Git commits, I'm happy to help/do it).

Things could be simplified a bit, though I wasn't sure whether this additional parsing was intentional or not. It was easier to do it, than explaining it in comments, hence I created a new PR: #1244

filecoin-proofs/src/bin/phase2.rs Outdated Show resolved Hide resolved
filecoin-proofs/src/bin/phase2.rs Show resolved Hide resolved
@DrPeterVanNostrand
Copy link
Contributor Author

@vmx Thanks, I added the PR description to the commit message.

PR: #1244

Yup, you're right. There is dead code.

I have one more phase2-cli pr for a command phase2 parse <path>, which just parses a params file into FileInfo and prints it. Which I've been using to verify that small and large params have the same values for delta_g1/g2, h/l_len, h/l_first, h/l_last

@vmx
Copy link
Contributor

vmx commented Aug 5, 2020

@DrPeterVanNostrand This PR needs a rebase. I guess the rest of it is then ready to go (i'd merge this one first, so that you don't have to rebase yet another time, i'll rebase my "split-keys" PR after this on is merged).

porcuquine
porcuquine previously approved these changes Aug 5, 2020
The phase2-cli 'merge' command merges large and small param files.

The final phase2 participant generates small-raw params, which are
converted into small-nonraw using 'phase2 convert'. Those small-nonraw
params are then merged into the large initial params to produce the
final 'MPCParameters' file for a circuit.
@DrPeterVanNostrand DrPeterVanNostrand merged commit abe7355 into master Aug 5, 2020
@vmx vmx deleted the phase2-merge-command branch August 5, 2020 18:41
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.

4 participants