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

Bump the version of VDAF used to v13 #734

Merged
merged 1 commit into from
Dec 18, 2024
Merged

Bump the version of VDAF used to v13 #734

merged 1 commit into from
Dec 18, 2024

Conversation

jhoyla
Copy link
Contributor

@jhoyla jhoyla commented Dec 9, 2024

No description provided.

Copy link
Contributor

@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.

This actually isn't too bad. There are some places where macros might help with de-duplication. When @mendess has a look, he might have some ideas.

Once you make the following changes, please mark as ready for review:

  • Please rebase the PR.
  • I commented on a few modules where I think you can probably use the latest version of prio rather than the old one. In fact, I'm fairly certain the only place you can't is pine. Please use the latest version wherever possible.
  • Naming convention: When there are two versions of a type, we generally follow the convention that the latest version gets the "normal" name and the old version gets a distinguished name. E.g., thing and thing_draft09 rather than thing_latest and thing. This makes it easier to eventually remove the old version.

crates/daphne/src/vdaf/prio3_latest.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/prio2.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/mod.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/mod.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
crates/daphne/src/hpke.rs Outdated Show resolved Hide resolved
crates/daphne/src/lib.rs Show resolved Hide resolved
crates/daphne/src/messages/mod.rs Outdated Show resolved Hide resolved
crates/daphne/src/messages/taskprov.rs Outdated Show resolved Hide resolved
crates/daphne/src/roles/aggregator.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@jhoyla jhoyla force-pushed the jhoyla/bump-vdaf branch 2 times, most recently from 032f242 to 98b9f63 Compare December 15, 2024 18:02
@jhoyla jhoyla marked this pull request as ready for review December 15, 2024 18:02
@cjpatton cjpatton self-requested a review December 16, 2024 15:32
Copy link
Contributor

@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 pretty good overall!

Cargo.toml Outdated Show resolved Hide resolved
crates/daphne-worker-test/Cargo.toml Outdated Show resolved Hide resolved
crates/daphne/src/lib.rs Outdated Show resolved Hide resolved
crates/daphne/src/protocol/mod.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/prio3_latest.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/prio3_latest.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/prio3_latest.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/prio3_latest.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/prio3_latest.rs Outdated Show resolved Hide resolved
crates/daphne/src/protocol/mod.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/mod.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/mod.rs Show resolved Hide resolved
crates/daphne/src/vdaf/mod.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/mod.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/prio3.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/prio3.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/prio3.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Just some nits

crates/daphne/src/lib.rs Outdated Show resolved Hide resolved
crates/daphne/src/protocol/aggregator.rs Outdated Show resolved Hide resolved
crates/daphne/src/protocol/mod.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/mastic.rs Outdated Show resolved Hide resolved
crates/daphne/src/vdaf/mastic.rs Outdated Show resolved Hide resolved
@jhoyla jhoyla requested a review from cjpatton December 17, 2024 18:17
Copy link
Contributor

@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.

One last minor comment. Note that I've tacked on a couple of commits using macros to de-duplicate some code.

Before merging:

  • Looks like you'll need to rebase.
  • Please squash all commits into one.
  • Make sure @mendess approves. This is a big enough change that we should have a couple of eyes on it.

crates/daphne/src/protocol/aggregator.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mendess mendess left a comment

Choose a reason for hiding this comment

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

It's gonna be a pain to work with this now, but such is life. LGTM

@jhoyla jhoyla merged commit 4f5cbf2 into main Dec 18, 2024
3 checks passed
@jhoyla jhoyla deleted the jhoyla/bump-vdaf branch December 18, 2024 14:30
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