-
Notifications
You must be signed in to change notification settings - Fork 31
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
VDAF-06+ ping-pong topology #683
Conversation
fb613a5
to
db78177
Compare
I might still need to tweak this as the corresponding Janus change evolves, but I think this is ready to be reviewed. Note there's a corresponding PR to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the in-line comments, one higher-level comment:
Currently, in Janus' experimental implementation of this change (as well as in the pre-ping-pong aggregation implementation), the Leader does not store the next prep_share
to send between steps of an aggregation job; we store only the prep_state
& prep_msg
, and compute the prep_share
in-memory when the job is picked up to continue onto the next step. Effectively, the Janus implementation stops its aggregation job steps after the prep_shares_to_prep
call inside ping_pong_transition
inside ping_pong_continued
, before the following prep_next
.
I don't see a way to use these functions to implement these semantics: PingPongTopologyPrivate::transition
will always compute the prep share, and it will be included in the Message
that is returned. So if the Leader calls ping_pong_continued
in the same aggregation job step, the Leader will end up storing an extra (Leader) prep_share
to durable storage; if the Leader calls ping_pong_continued
in the next aggregation job step, the Leader will end up storing an extra (Helper) prep_share
to durable storage.
I'm not sure the best way to address this; I'd fairly-strongly prefer not to store unnecessary data to durable storage. OTOH, splitting ping_pong_continued
up in a way that just-so-happens to be what Janus does might be too inflexible for other implementations. Janus could always choose not to use these functions at all, of course, but that would be somewhat wasteful of this generic ping-pong implementation.
edit: see David's https://github.com/divviup/janus/blob/6dbab170ca9176b55661a1607b0c5cf0593ea527/core/src/task/message_sizes.rs#L173-L236 for formulas on how much extra space this would use, per in-progress report aggregation -- N.B. these are only for Prio3, which will never end up storing an in-progress report aggregation since it will terminate in a single network request/aggregation job step, but may be useful to estimate how many bytes per report aggregation we waste by storing an unnecessary prep_share
.
Good points about the size of objects we store between prep steps. I'm going to do a comparison between what we store in a |
Using David's branch, I worked up a table of sizes for prepare messages, prepare message shares, prepare states and output shares for various instantiations of Prio3.
Crucially we observe that message shares and output shares are much bigger than the other objects. Then, by code inspection in Janus, we work out the worst-case storage usage for the leader and helper, depending on the number of rounds in the VDAF. As a reminder, the reason that finished helpers must store anything is usualy to record the most recent response to the leader to handle replays. In the case of a 1-round VDAF, that response consists of the prepare message.
* The waiting 2-round leader stores the output share because it does not accumulate until the helper has also finished Now let's plug in sizes from an 8 bit
For 1-round VDAFs, the leader and helper both come out ahead. However both aggregators lose in the 2-round VDAF case, because they have to store a prepare message from round n-1 and a prepare share from round n. Since many of these objects can be computed from each other, I think we can do better. Note 1: In this state, the helper stores:
It could instead store:
Then, it can recompute round 1 prepare state and round 2 helper prepare state. Note 2: In this state, the leader stores:
It could instead store:
I need to tinker with this, but I think that I can make the |
Put another way: I think I can refactor these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think I have it. I've refactored PingPongTopology::transition
into a new struct Transition
which contains the necessary values to compute transition()
, namely a PrepareState
and a PrepareMessage
. Now, implementations that want to store state to a DB between preparation rounds can store the encoded Transition
, while those that don't care can do vdaf.continued()?.evaluate()?
and get the (State, Message)
tuple as above.
Besides avoiding the the storage hit @branlwyd discussed, I think this approach will also allow Janus to stop dealing with the report_aggregations.prep_msg
column, because the stores Transition
now contains enough information to reconstruct it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think I have it. I've refactored
PingPongTopology::transition
into a newstruct Transition
which contains the necessary values to computetransition()
, namely aPrepareState
and aPrepareMessage
. Now, implementations that want to store state to a DB between preparation rounds can store the encodedTransition
, while those that don't care can dovdaf.continued()?.evaluate()?
and get the(State, Message)
tuple as above.
I like this abstraction. The only note I have is that a DAP implementation's stored state type will now be called Transition
; this transition can fallibly compute a State
. This naming scheme might be a bit confusing to folks new to this API, but since we're stuck with State
to match the VDAF spec, I think we can't do much better here. (raising this in case someone else has a better idea)
Besides avoiding the the storage hit @branlwyd discussed, I think this approach will also allow Janus to stop dealing with the
report_aggregations.prep_msg
column, because the storesTransition
now contains enough information to reconstruct it.
I think report_aggregations.prep_state
and report_aggregations.prep_msg
will be merged into something like report_aggregations.transition
, since the new ping-pong API deals in types that merge the prep_state
& prep_msg
.
src/topology/ping_pong.rs
Outdated
.prepare_preprocess([inbound_prep_share, prep_share]) | ||
.map_err(PingPongError::VdafPreparePreprocess)?; | ||
|
||
Ok(Transition { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: does the Helper get a similar reduction in storage via use of the Transition
abstraction, or is the usage here for API consistency (or another reason unrelated to storage reduction)? I think it's a good choice either way, I just haven't thought as much about the Helper case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. When the helper is in the waiting state, it conceptually holds:
- prepare state for round n
- prepare message that got it to round n
- prepare share for round n+1
Using Transition
, it only needs to store:
- prepare state for round n-1
- prepare message to get to round n
...and then it can recompute the round n prepare state and round n+1 prepare share. Prepare shares are large (see table in my comment above) so this is a meaningful gain for the helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this abstraction. The only note I have is that a DAP implementation's stored state type will now be called Transition; this transition can fallibly compute a State. This naming scheme might be a bit confusing to folks new to this API, but since we're stuck with State to match the VDAF spec, I think we can't do much better here. (raising this in case someone else has a better idea)
I agree. I think that when we decide how to slice this up into functions and types, there is a tension here between the objectives of doing something that is useful or convenient for implementations and doing something that lines up recognizably with the spec. If we want to do better here, I think we'd have to revisit the decision to put the ping-pong topology into VDAF. If it were in DAP, then a project like Janus has more freedom.
I think report_aggregations.prep_state and report_aggregations.prep_msg will be merged into something like report_aggregations.transition, since the new ping-pong API deals in types that merge the prep_state & prep_msg.
Yes, that's what's panning out in divviup/janus#1741 (which I have not yet updated to reflect the latest changes in this PR). I had hoped that last_prep_step
could also go away, but the Transition
doesn't quite capture everything we need there. I think we can do better with last_prep_step
, but I'll pursue that discussion in the Janus PR, and in fact may punt improvements to last_prep_step
to a later change since janus/1741 is going to be rather large and complex as it is.
e3acf8e
to
4115e7e
Compare
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work. I see no issues in the implementation, all my comments are relatively minor. One high-level thing to carefully consider: Which parts of the new API surface are essential, and which aren't?
029c87c
to
cd8eec8
Compare
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
8ff8e52
to
702ac75
Compare
d473fa9
to
1059856
Compare
Implements the ping-pong topology introduced in VDAF-06, though the implementation here is based on the revised ping-pong interface in a yet-unpublished VDAF version. We add a `topology` module, on the premise that we might someday add new topologies and their implementations there, and `topology::ping_pong`. This also adds an implementation of a dummy VDAF, brought over from Janus ([1]). `vdaf::dummy` is only compiled if the `test-util` Cargo feature is enabled. The dummy VDAF implements the `vdaf::{Vdaf, Aggregator, Client, Collector}` and provides associated types for output shares, prepare shares, etc., but it doesn't do anything except return success, making it useful for testing higher-level constructions like ping-pong. Finally, we replace the derived `std::fmt::Debug` implementations on a few `prio3` and `poplar1` associated types so that they redact fields that are either sensitive secrets or just too big to be worth printing when debugging. This is so that we can provide `Debug` impls on new types in `topology::ping_pong` without pulling in crate `derivative`, which would require us to do 9,000+ lines of audits. [1]: https://github.com/divviup/janus
add tests for encode/decode messages move max rounds into the dummy vdaf prep closure
- update various doccomments - remove Role enum and provide distinct leader_continued, helper_continued - rename various structs to `PingPongWhatever` for consistency with Prio3, Poplar1 modules - rename leader_initialize, helper_initialize to leader_initialized, helper_initialized to align with continued
Rebasing means a couple methods need an agg param argument, since `Vdaf::prepare_shares_to_prepare_message` needs agg param.
1059856
to
a2076f9
Compare
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 DAP-06. This change depends on the implementation of the VDAF ping-pong topology added to crate `prio` in [1], which in turn conforms to the specification in VDAF-07. In the ping-pong topology, each DAP-layer step of aggregation now spans two VDAF rounds. An aggregator will use the prepare message it gets from its peer to advance by one VDAF round, and then can use the prepare share it just computed along with the peer's prepare share to advance by another. This incurs some changes to what intermediate values are stored by aggregators. In the case where a leader is continuing/waiting, it will have computed a prepare state, a prepare message for the current round and a prepare share for the next round. The naive implementation would store all three objects in the database, significantly increasing the per-report storage use. To mitigate this, the leader stores a `prio::topology::ping_pong::PingPongTransition`, 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. On the helper side, there's no way around storing the prepare share: we store the most recently computed `PrepareResp` so that we can handle aggregation jobs idempotently. But to avoid storing prepare messages twice, the continuing/waiting helper stores just a prepare state and a `PingPongMessage`. 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]: divviup/libprio-rs#683 Co-authored-by: Tim Geoghegan <[email protected]> Part of #1669
src/topology/ping_pong.rs
Outdated
/// Error running prepare_step | ||
#[error("vdaf.prepare_step {0}")] | ||
VdafPrepareStep(VdafError), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be renamed as well to refer to prepare_next
.
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 DAP-06. This change depends on the implementation of the VDAF ping-pong topology added to crate `prio` in [1], which in turn conforms to the specification in VDAF-07. In the ping-pong topology, each DAP-layer step of aggregation now spans two VDAF rounds. An aggregator will use the prepare message it gets from its peer to advance by one VDAF round, and then can use the prepare share it just computed along with the peer's prepare share to advance by another. This incurs some changes to what intermediate values are stored by aggregators. In the case where a leader is continuing/waiting, it will have computed a prepare state, a prepare message for the current round and a prepare share for the next round. The naive implementation would store all three objects in the database, significantly increasing the per-report storage use. To mitigate this, the leader stores a `prio::topology::ping_pong::PingPongTransition`, 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. On the helper side, there's no way around storing the prepare share: we store the most recently computed `PrepareResp` so that we can handle aggregation jobs idempotently. But to avoid storing prepare messages twice, the continuing/waiting helper stores just a prepare state and a `PingPongMessage`. 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]: divviup/libprio-rs#683 Co-authored-by: Tim Geoghegan <[email protected]> Part of #1669
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 DAP-06. This change depends on the implementation of the VDAF ping-pong topology added to crate `prio` in [1], which in turn conforms to the specification in VDAF-07. In the ping-pong topology, each DAP-layer step of aggregation now spans two VDAF rounds. An aggregator will use the prepare message it gets from its peer to advance by one VDAF round, and then can use the prepare share it just computed along with the peer's prepare share to advance by another. This incurs some changes to what intermediate values are stored by aggregators. In the case where a leader is continuing/waiting, it will have computed a prepare state, a prepare message for the current round and a prepare share for the next round. The naive implementation would store all three objects in the database, significantly increasing the per-report storage use. To mitigate this, the leader stores a `prio::topology::ping_pong::PingPongTransition`, 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. On the helper side, there's no way around storing the prepare share: we store the most recently computed `PrepareResp` so that we can handle aggregation jobs idempotently. But to avoid storing prepare messages twice, the continuing/waiting helper stores just a prepare state and a `PingPongMessage`. 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]: divviup/libprio-rs#683 Co-authored-by: Tim Geoghegan <[email protected]> Part of #1669 Co-authored-by: Brandon Pitman <[email protected]>
Implements the ping-pong topology introduced in VDAF-06, though the
implementation here is based on the revised ping-pong interface in a
yet-unpublished VDAF version.
We add a
topology
module, on the premise that we might someday add newtopologies and their implementations there, and
topology::ping_pong
.This also adds an implementation of a dummy VDAF, brought over from
Janus (1).
vdaf::dummy
is only compiled if thetest-util
Cargofeature is enabled. The dummy VDAF implements the
vdaf::{Vdaf, Aggregator, Client, Collector}
traits and provides associated types for outputshares, prepare shares, etc., but it doesn't do anything except return
success, making it useful for testing higher-level constructions like
ping-pong.
Finally, we replace the derived
std::fmt::Debug
implementations on afew
prio3
andpoplar1
associated types so that they redact fieldsthat are either sensitive secrets or just too big to be worth printing
when debugging. This is so that we can provide
Debug
impls on newtypes in
topology::ping_pong
without pulling in cratederivative
,which would require us to do 9,000+ lines of audits.