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

protocol-determined vdaf representation #419

Merged
merged 5 commits into from
Aug 24, 2023

Conversation

jbr
Copy link
Contributor

@jbr jbr commented Aug 18, 2023

closes #410

src/entity/task/vdaf.rs Fixed Show fixed Hide fixed
src/entity/task/vdaf.rs Fixed Show fixed Hide fixed
@jbr jbr force-pushed the protocol-contingent-vdaf-representation branch 3 times, most recently from 31a7b3d to d06df76 Compare August 22, 2023 19:08
@jbr jbr marked this pull request as ready for review August 22, 2023 19:24
@jbr jbr requested a review from a team as a code owner August 22, 2023 19:24
@jbr
Copy link
Contributor Author

jbr commented Aug 22, 2023

blocked on divviup/janus#1743 no longer blocked, but will send mismatched histogram representations in dap-05 until that pr is merged

@jbr jbr force-pushed the protocol-contingent-vdaf-representation branch from 98964bc to 1ef84c1 Compare August 22, 2023 21:45
Copy link
Contributor

@inahga inahga left a comment

Choose a reason for hiding this comment

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

It otherwise looks good, I like the representation_for_protocol pattern.

Copy link
Contributor

@tgeoghegan tgeoghegan left a comment

Choose a reason for hiding this comment

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

Couple high level questions:

  • This sets protocol = DAP-04 on existing paired aggregators, which makes sense. Do we require that all further aggregators someone pairs have to include their supported protocol in the capabilities API?
  • Why store the protocol version in the tasks table? If each aggregator supports exactly one protocol version (which I think is the right thing to do), then a task's protocol version is implied by the protocol version of the aggregators it uses.

@jbr
Copy link
Contributor Author

jbr commented Aug 24, 2023

This sets protocol = DAP-04 on existing paired aggregators, which makes sense. Do we require that all further aggregators someone pairs have to include their supported protocol in the capabilities API?

Yes. If they do not, they will be treated as DAP-04 for posterity. This is a mandatory field in the config api now.

Edit: Wait, maybe I misunderstood. If they are in fact DAP-04, they don't need to advertise a protocol and it will work fine. There's a serde(default) on Protocol and the default is Dap04.

Why store the protocol version in the tasks table? If each aggregator supports exactly one protocol version (which I think is the right thing to do), then a task's protocol version is implied by the protocol version of the aggregators it uses.

I'm open to dropping it. I know aggregators cannot currently upgrade or support multiple DAP versions, but I was defensively programming against a future scenario in which it isn't always possible to determine from the aggregator pair. This is also why there's a variable named resolved_protocol — with the idea that there may be some further logic around protocol selection given a pair of aggregators. I think if I were reviewing this, I'd say that's currently unnecessary and we can add it and backfill it when/if we get there.

Edit: Removed the task protocol column

@jbr jbr force-pushed the protocol-contingent-vdaf-representation branch 2 times, most recently from e0628b2 to fcb7fb3 Compare August 24, 2023 01:22
Copy link
Contributor

@tgeoghegan tgeoghegan left a comment

Choose a reason for hiding this comment

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

This all works for me:

  • existing DAP-04 aggregators will be treated as supporting DAP-04, but any new aggregators that support DAP-05 will include the protocol version advertisement so this will all work
  • inferring task protocol version from aggregator version also makes sense.

src/entity/task/vdaf.rs Outdated Show resolved Hide resolved
migration/src/lib.rs Outdated Show resolved Hide resolved
cli/src/tasks.rs Outdated Show resolved Hide resolved
@jbr jbr force-pushed the protocol-contingent-vdaf-representation branch from fcb7fb3 to 22d230d Compare August 24, 2023 19:50
jbr and others added 3 commits August 24, 2023 12:57
this is because there's always an implicit top bucket greater than the highest bound
@jbr jbr requested a review from divergentdave August 24, 2023 20:38
@jbr jbr merged commit 8fc8d6f into main Aug 24, 2023
@jbr jbr deleted the protocol-contingent-vdaf-representation branch August 24, 2023 21:02
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.

Deal with DAP-05 / VDAF-06 / Janus 0.6
4 participants