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.

Sign up for GitHub

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

Jump to bottom

Implement Draft04 #259

Merged
merged 1 commit into from
Apr 7, 2023
Merged

Implement Draft04 #259

merged 1 commit into from
Apr 7, 2023

Conversation

bhalleycf
Copy link
Contributor

No description provided.

cjpatton
cjpatton reviewed Apr 5, 2023
View reviewed changes
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.

Comments up to daphne/src/messages/mod.rs

daphne/src/constants.rs Show resolved Hide resolved
daphne/src/constants.rs Show resolved Hide resolved
daphne/src/lib.rs Show resolved Hide resolved
daphne/src/lib.rs Outdated Show resolved Hide resolved
daphne/src/lib.rs Show resolved Hide resolved
daphne/src/lib.rs Outdated Show resolved Hide resolved
daphne/src/lib.rs Show resolved Hide resolved
daphne/src/lib.rs Outdated Show resolved Hide resolved
daphne/src/lib.rs Outdated Show resolved Hide resolved
daphne/src/messages/mod.rs Outdated Show resolved Hide resolved
cjpatton
cjpatton reviewed Apr 5, 2023
View reviewed changes
daphne/src/messages/mod.rs Outdated Show resolved Hide resolved
daphne/src/messages/mod.rs Outdated Show resolved Hide resolved
daphne/src/messages/mod.rs Outdated Show resolved Hide resolved
@cjpatton cjpatton self-requested a review April 6, 2023 16:23
@cjpatton
Copy link
Contributor

cjpatton commented Apr 6, 2023

Rebased.

cjpatton
cjpatton reviewed Apr 6, 2023
View reviewed changes
daphne/src/lib.rs Outdated Show resolved Hide resolved
daphne/src/messages/mod.rs Outdated Show resolved Hide resolved
daphne/src/messages/mod.rs Outdated Show resolved Hide resolved
daphne/src/roles.rs Show resolved Hide resolved
daphne/src/roles.rs Outdated Show resolved Hide resolved
daphne/src/testing.rs Outdated Show resolved Hide resolved
daphne/src/vdaf/mod.rs Show resolved Hide resolved
daphne_worker/src/config.rs Show resolved Hide resolved
daphne_worker/src/config.rs Show resolved Hide resolved
daphne_worker/src/dap.rs Outdated Show resolved Hide resolved
@cjpatton cjpatton self-requested a review April 7, 2023 16:23
@cjpatton cjpatton mentioned this pull request Apr 7, 2023
Closed
cjpatton
cjpatton reviewed Apr 7, 2023
View reviewed changes
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.

Everything looks good to me now modulo some changes in the HTTP API.

daphne_worker/src/dap.rs Outdated Show resolved Hide resolved
daphne_worker/src/dap.rs Show resolved Hide resolved
daphne_worker/src/dap.rs Show resolved Hide resolved
daphne_worker/src/durable/leader_col_job_queue.rs Show resolved Hide resolved
daphne_worker/src/durable/leader_col_job_queue.rs Outdated Show resolved Hide resolved
daphne_worker/src/lib.rs Show resolved Hide resolved
daphne_worker_test/Cargo.toml Outdated Show resolved Hide resolved
daphne_worker_test/tests/e2e.rs Outdated Show resolved Hide resolved
daphne_worker_test/tests/test_runner.rs Outdated Show resolved Hide resolved
daphne_worker_test/tests/test_runner.rs Show resolved Hide resolved
@cjpatton
Copy link
Contributor

cjpatton commented Apr 7, 2023

One more thing I noticed: There is an XXX in daphne/src/roles_test.rs -- the fact that the test passes doesn't seem to align with our expectations. Will investigate in a bit.

@cjpatton
Copy link
Contributor

cjpatton commented Apr 7, 2023

Squashed.

@bhalleycf @cjpatton
Replace draft-ietf-ppm-dap-03 with draft-ietf-ppm-dap-04
1edfa69 
Add support for draft04, drop support for draft03, and continue support
for draft02. No wire-breaking changes are expected, but deployments will
impacted by the following breaking changes:

* In draft02, we now use a 16-byte value for the collection job ID
  rather than 32. This matches the type specified in draft04. This does
  not break interop, since the structure of the collect URI is not
  specified in draft02; however, compatibility with older versions of
  LeaderCollectionJobQueue.

* LeaderCollectionJobQueue: The schema for lookup keys for collect jobs
  to durable storage has been changed.

* PrgAea128 is deprecated as of prio version 0.12.0; we now use PrgSha3
  for internal purposes. As a result, collection job IDs computed in
  draft02 will have changed, as well as the mapping of reports to
  ReportsPending and ReportsProcessed shards has changed.

* The environment variable DAP_COLLECT_ID_KEY was renamed to
  DAP_COLLECTION_JOB_ID_KEY.

* The interfaces for ReportsPending and LeaderCollectionJobQueue have
  changed.

Co-authored-by: Christopher Patton <cpatton@cloudflare.com>
@bhalleycf bhalleycf merged commit b150547 into main Apr 7, 2023
@bhalleycf bhalleycf deleted the draft04 branch April 7, 2023 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjpatton cjpatton cjpatton approved these changes

@armfazh armfazh Awaiting requested review from armfazh armfazh is a code owner

@chris-wood chris-wood Awaiting requested review from chris-wood

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bhalleycf @cjpatton