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

Compress y. #87

Merged
merged 17 commits into from Aug 9, 2021
Merged

Compress y. #87

merged 17 commits into from Aug 9, 2021

Conversation

fchirica
Copy link
Contributor

Compress y form from 100 bytes to 33 bytes and adds python bindings for compression/verification. Note that this modifies chiavdf n-wesolowski verification as well, by factoring out the common parts in the verifiers.

@fchirica fchirica requested a review from rostislav June 22, 2021 13:36
@almogdepaz
Copy link

almogdepaz commented Jul 5, 2021

i really think we should sort out the names to match between the repo's and the paper
for example having a method called CheckProofOfTimeNWesolowskiYCompressed where there is no Y in the method itself doesn't make much sense to me

@fchirica fchirica marked this pull request as ready for review July 8, 2021 17:51
@rostislav
Copy link
Contributor

I agree with the above suggestion by Almog that we should keep the naming consistent. The "compressed y" value is actually named B in the paper and elsewhere in the code, so I'd suggest to name the new functions accordingly, e.g. CheckProofOfTimeNWesolowskiWithB.

Secondly, the compress_y_from_n_wesolowski function is performing extra computational work (re-verifying the proof and deriving B) that can be avoided. What we could do instead is to introduce a new prove function named e.g. prove_get_B or modify the existing prove function to return B instead of (or in addition to) y.

Alternatively, if it is desired to have a function to compress y in existing proofs, we may add a Python binding for GetB, which derives B from x and y. Derivation of B is less computationally expensive than verifying the proof.

@fchirica
Copy link
Contributor Author

I'll rename y_compressed into B.

The goal is to turn a proof y, proof, [iters1, B1, proof1], [iters2, B2, proof2], ... into B, proof, [iters1, B1, proof1], [iters2, B2, proof2], ..., so we send B instead of y to save space. The B must refer to the first segment only, not to the whole proof. So in order to create it, we need to "redo" the work verifier does (i.e. get out_y from x, B and proof for all segments but the first, then do x = out_y to prepare for the next segment).

Indeed, we can save this unnecessary check B == GetB(D, x, out_y) ? 0 : -1 in creating B, since we know it's true. But unless I'm missing something, is there any other way to get the first segment x from an existing proof other than duplicating VerifyWesoSegment operations?

Copy link
Contributor

@rostislav rostislav left a comment

Choose a reason for hiding this comment

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

I understand now that for N-weso we have to verify the segments before the last one in order to find x for the last segment. Getting B from 0-weso doesn't require verification work, which is good.
I suggested some minor fixes in inline comments.

src/verifier.h Outdated Show resolved Hide resolved
src/verifier.h Outdated Show resolved Hide resolved
src/verifier.h Outdated

bool CheckProofOfTimeNWesolowski(integer D, const uint8_t* x_s, const uint8_t* proof_blob, int32_t proof_blob_len, uint64_t iterations, uint64 disc_size_bits, int32_t depth) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In this and two other functions disc_size_bits argument is unused and could be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left it for CheckProofOfTimeNWesolowski and verify_n_wesolowski, since those functions are already heavily used both in chiavdf and in chia-blockchain. I removed the field for the 2 new functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually I've tracked and removed it from CheckProofOfTimeNWesolowski, but I'll still keep it in python bindings function (verify_n_wesolowski)

Copy link
Contributor

@rostislav rostislav left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

@almogdepaz almogdepaz left a comment

Choose a reason for hiding this comment

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

tested my new wp code with latest changes from this pr, looks good

@wjblanke wjblanke merged commit 0cd6424 into main Aug 9, 2021
@wjblanke wjblanke deleted the fc.compress_y branch August 9, 2021 17:25
wjblanke added a commit that referenced this pull request Jan 26, 2022
This reverts commit 0cd6424.
@wjblanke wjblanke mentioned this pull request Jan 26, 2022
wjblanke added a commit that referenced this pull request Jan 27, 2022
* Revert "Compress y. (#87)"

This reverts commit 0cd6424.

* Don't revert this.

Co-authored-by: Florin Chirica <fchirica96@gmail.com>
@fchirica fchirica restored the fc.compress_y branch April 5, 2022 11:49
@fchirica fchirica deleted the fc.compress_y branch April 5, 2022 14:38
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.

None yet

4 participants