Square up with new VDAF ping-pong functions #494
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The corresponding VDAF changes: cfrg/draft-irtf-cfrg-vdaf#281 Ideally we'll wait for a VDAF including that PR to be published before we take this change to DAP. |
cjpatton
reviewed
Aug 18, 2023
There was a problem hiding this comment.
I propose we continue to handle the outputs of Vdaf as opaque bytes strings, including the length prefix. It's somewhat wasteful in terms of number of bytes on the wire, but it's useful to keep a clean separation between messages defined here and messages defined in VDAF.
draft-ietf-ppm-dap.md
Outdated
| @@ -1230,7 +1227,7 @@ struct { | |||
|
|
|||
| struct { | |||
| ReportShare report_share; | |||
| opaque payload<0..2^32-1>; | |||
| Message ping_pong_message; | |||
There was a problem hiding this comment.
Hmm, we need to pick a more distinctive name than Message. Another option is to continue to handle this as an opaque byte string, including the length prefix. In fact, I think I'd prefer no to reference messages defined in another document.
There was a problem hiding this comment.
If we do that, then we should change the function definitions in VDAF so that instead of accepting and returning objects of type Message, they take opaque byte buffers. So instead of:
def ping_pong_continued(
Vdaf,
is_leader: bool,
agg_param: bytes,
state: State,
inbound: Message,
) -> (State, Optional[Message])
We would have
def ping_pong_continued(
Vdaf,
is_leader: bool,
agg_param: bytes,
state: State,
inbound: bytes,
) -> (State, Optional[bytes])
Which is less expressive, but keeps anything to do with Message inside of VDAF. I don't mind doing that because I think the layering concern you bring up is valid. I'm curious what others think.
There was a problem hiding this comment.
Sounds like a reasonable solution to me.
There was a problem hiding this comment.
OK, I made this change, with a corresponding change on my VDAF PR (commit 2066205 cfrg/draft-irtf-cfrg-vdaf#281) to change the ping-pong interfaces.
tgeoghegan
pushed a commit
to divviup/janus
that referenced
this pull request
Aug 27, 2023
This change implements the DAP-05 ping-pong topology in which aggregators take turns preprocessing prepare shares into prepare messages. While this topology first appeared in DAP-05, this implementation follows the changes in [1], which should appear in DAP-06. This change depends on the implementation of the VDAF ping-pong topology added to crate `prio` in [2], which in turn conforms to the specification added after VDAF-06 and further tweaked in [3] (we expect this to be published soon as VDAF-07). This commit makes some changes to what intermediate values are stored by aggregators. In the case where an aggregator is continuing, it will have computed a prepare state, a prepare message for the current round and a prepare share for the next round. The existing implementation would store all three objects in the database, significantly increasing the per-report storage requirements. In particular, this makes things worse for the Helper, which previously never needed to store a prepare share because the Leader always took responsibility for combining prepare shares. To mitigate this, we instead have aggregators store a `prio::ping_pong::topology::Transition`, which will contain a prepare state and a prepare message (both of which are generally much smaller than prepare shares), from which the next prepare state and importantly prepare share can be recomputed. The main benefit of this change is to reduce how many round trips between aggregators are needed to prepare reports. Quite a few tests used Prio3 but depended on having the leader or helper in the `Waiting` state after running aggregation initialization. Accordingly, those tests are changed to run Poplar1, which now takes 2 rounds. [1]: ietf-wg-ppm/draft-ietf-ppm-dap#494 [2]: divviup/libprio-rs#683 [3]: cfrg/draft-irtf-cfrg-vdaf#281 Part of #1669
cjpatton
approved these changes
Aug 29, 2023
divergentdave
approved these changes
Aug 29, 2023
Recent VDAF changes replaced the ping-pong family of functions from VDAF-06. We now use the new ones which will appear in VDAF-07. Additionally, we use the form `Vdaf.function_name()` instead of `VDAF.function_name()` to harmonize with the VDAF document.
tgeoghegan
force-pushed
the
timg/update-ping-pong
branch
from
7202bfb to
da262cc
Compare
tgeoghegan
pushed a commit
to divviup/janus
that referenced
this pull request
Aug 31, 2023
This change implements the DAP-05 ping-pong topology in which aggregators take turns preprocessing prepare shares into prepare messages. While this topology first appeared in DAP-05, this implementation follows the changes in [1], which should appear in DAP-06. This change depends on the implementation of the VDAF ping-pong topology added to crate `prio` in [2], which in turn conforms to the specification added after VDAF-06 and further tweaked in [3] (we expect this to be published soon as VDAF-07). This commit makes some changes to what intermediate values are stored by aggregators. In the case where an aggregator is continuing, it will have computed a prepare state, a prepare message for the current round and a prepare share for the next round. The existing implementation would store all three objects in the database, significantly increasing the per-report storage requirements. In particular, this makes things worse for the Helper, which previously never needed to store a prepare share because the Leader always took responsibility for combining prepare shares. To mitigate this, we instead have aggregators store a `prio::ping_pong::topology::Transition`, which will contain a prepare state and a prepare message (both of which are generally much smaller than prepare shares), from which the next prepare state and importantly prepare share can be recomputed. The main benefit of this change is to reduce how many round trips between aggregators are needed to prepare reports. Quite a few tests used Prio3 but depended on having the leader or helper in the `Waiting` state after running aggregation initialization. Accordingly, those tests are changed to run Poplar1, which now takes 2 rounds. [1]: ietf-wg-ppm/draft-ietf-ppm-dap#494 [2]: divviup/libprio-rs#683 [3]: cfrg/draft-irtf-cfrg-vdaf#281 Part of #1669
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recent VDAF changes replaced the ping-pong family of functions from VDAF-06. We now use the new ones which will appear in VDAF-07.
One consequence of this is that various aggregation protocol sub-messages now have fields of type
Message(which is defined in TLS syntax in VDAF and thus can be included in DAP's message definitions) instead of being opaque byte buffers.Additionally, we use the form
Vdaf.function_name()instead ofVDAF.function_name()to harmonize with the VDAF document.