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

Shared ZK implementation #1013

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Conversation

hannahdaviscrypto
Copy link
Contributor

@hannahdaviscrypto hannahdaviscrypto commented Apr 29, 2024

Partially addresses #947.

A module for computing fully linear proofs on data that is already secret shared, using the Prio3 method to derive joint randomness from the input shares via an XOF.

@hannahdaviscrypto hannahdaviscrypto requested a review from a team as a code owner April 29, 2024 15:56
@hannahdaviscrypto
Copy link
Contributor Author

@cjpatton Would love if you could take a look at this, thank you!

src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

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

Kicking this off with a round of Rust-related comments. I'll get to the crypto bits once the code starts to smooth out. Looking good so far!

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/mastic.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@cjpatton cjpatton changed the title Shared ZK implementation (draft) Shared ZK implementation May 8, 2024
Copy link
Contributor

@divergentdave divergentdave left a comment

Choose a reason for hiding this comment

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

I noticed that you rebased or cherry-picked main onto this feature branch. This doesn't work well because each new change from main increases the diff between the feature branch and the last common ancestor commit, which means more chances for conflicts. Ideally, the diff should only have the new src/szk.rs file, and two lines in src/lib.rs declaring the new module.

There are a few ways to get out of this loop. Merging main into this feature branch would give you a new last common ancestor commit, thus simplifying the diff, though leaving the branch with a complicated history. Rebasing in the other direction, i.e. rebasing the feature branch onto the latest commit from main, would provide the cleanest end result history, though it may take some care. The commits that already appear on main would need to be dropped: rebasing can often do this automatically, but if not this can be manually controlled in an interactive rebase. There may be conflicts to resolve along the way as well, particularly for fixup commits touching existing parts of the library. I think you could alternatively squash the whole feature branch into one commit by first doing a soft reset, cleaning up the changes in the index by unstaging changes from cherry-picked main commits, and then creating a new commit from the index's contents.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

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

Looking really good. I have one substantive question (inline); all others I think are easy to resolve. I would expect just one more round of review from me.

src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@divergentdave divergentdave left a comment

Choose a reason for hiding this comment

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

Looking good! My one major comment is on the API of Szk::decide(), as there's currently a hole between query() and decide() that has to be bridged with some manual unsharding and Option reshuffling.

src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
@cjpatton cjpatton self-requested a review May 25, 2024 13:15
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
src/szk.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

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

👏 LGTM. Before merging:

  1. Let's make sure @divergentdave is happy
  2. We need to squash this into a single commit.

Copy link
Contributor

@divergentdave divergentdave left a comment

Choose a reason for hiding this comment

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

Looking good, I have a few small comments.

src/flp/szk.rs Outdated Show resolved Hide resolved
src/flp/szk.rs Outdated Show resolved Hide resolved
src/flp/szk.rs Outdated Show resolved Hide resolved
Szk wraps an FLP proof system and uses an Xof to derive joint random
coins using a method inspired by Prio3. However, unlike Prio3, Szk
is designed to prove validity of an input that is already secret-shared
between two parties. This is necessary for Mastic, where the input
will be shared via a VIDPF instead of linearly.
@hannahdaviscrypto
Copy link
Contributor Author

hannahdaviscrypto commented Jun 12, 2024

Squashed and ready to merge, pending CI checks! @cjpatton

@divergentdave divergentdave merged commit 790ba4d into divviup:main Jun 12, 2024
6 checks passed
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