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

Implement Verifiable Incremental Distributed Point Function (VIDPF). #954

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented Feb 22, 2024

Partially addresses #947.

Implements Verifiable Incremental Distributed Point Function (VIDPF).

The VIDPF construction is specified in [draft-mouris-cfrg-mastic] and builds on techniques from [MST23] and [CP22] to lift an IDPF to a VIDPF.

@armfazh armfazh requested a review from a team as a code owner February 22, 2024 18:52
@cjpatton cjpatton self-requested a review February 22, 2024 18:56
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.

Some inital comments, focused mainly on rust conventions.

src/vidpf.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.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.

Really nice work so far. I've gotten through gen(); next up is convert(), hash_one(), and then evaluation.

src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.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.

Still working my way through eval(), but I wanted to give you some comments to catch up on in the meantime. Looking good!

  1. Be careful about what we expose in the public API.
  2. I'd like to find more meaningful names than "hash_one" and "hash_two". What about something like "node_proof" and "node_proof_adjustment" respectively? This is more or less what we came up in the draft.
  3. Documentation: Be consistent about notation for the struct name in the code comment. All of the following are used: Name, [Name], and Name. Note that the are all rendered differently in cargo doc.
  4. Resolve the conflicts on this branch.

src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated
}

fn prg(&self, seed: &Seed) -> Sequence {
let dst = b"100";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bump: If you don't want to work on this in this PR, let me know and I"ll find some place to track the TODO.

src/vidpf.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 very good! It looks to me like you've properly translated the paper. Just one minor thing to fix in prg() to match the (eventual) draft.

The only other high level thing is that we want a default implementation of VidpfWeight for Vec<F> instead of [F; N].

src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated
}

fn node_proof(input: &VidpfInput, level: usize, seed: &Seed) -> Result<Proof, VidpfError> {
let mut binder = input.prefix(level).to_bytes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this part of the binder is variable-length, which differs from the current draft-mouris-cfrg-mastic implementation. It sounds like we may want to wait on further draft changes before we try fully aligning with it, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Also, it's likely than there are some other discrepancies between this implementation and the Python impl.

src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/idpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
@armfazh armfazh force-pushed the impl_vidpf branch 2 times, most recently from c72abbe to 93006fb Compare March 7, 2024 22:48
@armfazh
Copy link
Contributor Author

armfazh commented Mar 7, 2024

I've addressed your comments, and added more tests for coverage.

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.

Looks good to me. All of my remaining comments are debatable gripes about the API. Take these as suggestions, and let @divergentdave make the final call.

  1. For consistency with the rest of this crate, each struct should begin with Vidpf. (See inline comments.)
  2. Each docuco mment for a struct begins with a refereence to that struct. This gets rendered into a hyperlink, which looks strange in cargo doc:
    image

src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
src/vidpf.rs Outdated Show resolved Hide resolved
The VIDPF construction is specified in [[draft-mouris-cfrg-mastic]] and builds
on techniques from [[MST23]] and [[CP22]] to lift an IDPF to a VIDPF.

[CP22]: https://eprint.iacr.org/2021/580
[MST23]: https://eprint.iacr.org/2023/080
[draft-mouris-cfrg-mastic]: https://datatracker.ietf.org/doc/draft-mouris-cfrg-mastic/02/
@cjpatton
Copy link
Collaborator

@armfazh go ahead and merge at will.

@divergentdave divergentdave merged commit dea2192 into divviup:main Mar 11, 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.

3 participants