-
Notifications
You must be signed in to change notification settings - Fork 2
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
Update FulfillRequisitionRequest and ProtocolConfig for ShuffleBasedSecretSharing protocol #191
Conversation
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.
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @ple13, @renjiezh, and @SanjayVas)
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 35 at r2 (raw file):
// Request message for `FulfillRequisition` method. message FulfillRequisitionRequest {
What do you think about adding a field for the protocol config being used?
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.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @kungfucraig, @ple13, and @SanjayVas)
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 35 at r2 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
What do you think about adding a field for the protocol config being used?
Thanks.
Covered that in this PR.
9623fe4
to
e751973
Compare
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.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @kungfucraig, @ple13, and @SanjayVas)
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 211 at r4 (raw file):
// The maximum frequency to reveal in the histogram. This is also the EDP // frequency cap. int32 maximum_frequency = 2;
LLv2's protocol config deprecated this field as it is in measurement_spec already. However, for ShuffleBasedSecretSharing we want to validate ProtocolConfig. Do we want to also compare this field?
@kungfucraig @SanjayVas
e751973
to
567e55b
Compare
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.
Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @kungfucraig and @ple13)
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 211 at r4 (raw file):
Previously, renjiezh wrote…
LLv2's protocol config deprecated this field as it is in measurement_spec already. However, for ShuffleBasedSecretSharing we want to validate ProtocolConfig. Do we want to also compare this field?
@kungfucraig @SanjayVas
We shouldn't include this field here in ProtocolConfig for the same reason we dropped it from LLv2: it's specified in MeasurementSpec. Drop this field.
08a2204
to
5112176
Compare
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.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @kungfucraig and @ple13)
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 211 at r4 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
We shouldn't include this field here in ProtocolConfig for the same reason we dropped it from LLv2: it's specified in MeasurementSpec. Drop this field.
Done.
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.
Reviewed 2 of 2 files at r3, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kungfucraig and @renjiezh)
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 69 at r7 (raw file):
message ShuffleBasedSecretSharing { // The seed can be expanded into a deterministic blob using the same PRNG. bytes seed = 1;
Does seed
stand for both blob/seed?
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 72 at r7 (raw file):
} // Protocol specified values. oneof protocol {
Why do we use oneof when there's only 1 option?
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.
Reviewed 2 of 2 files at r3, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @renjiezh and @SanjayVas)
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 205 at r7 (raw file):
// Configuration for the Shuffle Based Secret Sharing Protocol. message ShuffleBasedSecretSharing {
Can we call this: HonestMajoritySecretShareShuffle?
I think having HM in the name is helpful.
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 286 at r7 (raw file):
message SecretSharingSketchParams { // The size of the sketch. int64 sketch_size = 1;
Should we call this register count?
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 290 at r7 (raw file):
// Length of each item in bits. #_EDPs * max_frequency should be no more than // 2^bit_per_item. int32 bit_per_item = 2;
Let's make this "bytes_per_item," and in the future if we need bit level granularity we can make a oneof that has the option of bits or bytes. It will keep the implementation simpler to assume bytes.
@ple13 do we need to specify Z_p in this structure?
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 62 at r7 (raw file):
// The protocol config of the Computation. This is used to validate that // EDPs and the MPC are using the same protocol config. // Required for ShuffleBasedSecretSharing protocol.
Should we say "not required for LLv2 protocols?" Then we can assume that going forward we'll always expect this.
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.
Reviewable status: 1 of 3 files reviewed, 8 unresolved discussions (waiting on @kungfucraig, @renjiezh, and @SanjayVas)
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 290 at r7 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
Let's make this "bytes_per_item," and in the future if we need bit level granularity we can make a oneof that has the option of bits or bytes. It will keep the implementation simpler to assume bytes.
@ple13 do we need to specify Z_p in this structure?
For the frequency protocol, we use the ring 2^{byte_per_item*8}. Later on, if we want a separate protocol for reach, we will need to use a prime and need to specify one.
1d298bf
to
0732d41
Compare
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.
Reviewable status: 1 of 3 files reviewed, 8 unresolved discussions (waiting on @kungfucraig, @ple13, and @SanjayVas)
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 205 at r7 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
Can we call this: HonestMajoritySecretShareShuffle?
I think having HM in the name is helpful.
Done.
Renamed to HonesMajorityShareShuffle
. Internal APIs will use the same name.
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 286 at r7 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
Should we call this register count?
Done.
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 290 at r7 (raw file):
Previously, ple13 (Phi) wrote…
For the frequency protocol, we use the ring 2^{byte_per_item*8}. Later on, if we want a separate protocol for reach, we will need to use a prime and need to specify one.
Done.
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 62 at r7 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
Should we say "not required for LLv2 protocols?" Then we can assume that going forward we'll always expect this.
Done.
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 69 at r7 (raw file):
Previously, ple13 (Phi) wrote…
Does
seed
stand for both blob/seed?
Added comment to make it clearer.
The requisition data is either a seed from the header or blob of chunks .
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 72 at r7 (raw file):
Previously, ple13 (Phi) wrote…
Why do we use oneof when there's only 1 option?
Each requisition could contains protocol specified values, e.g,. seed
. As we might have multiple protocols, oneof fits the usage.
Ideally, each fulfillment can have both values in header and blobs in following chunks. The protocol field should provides information about both of them. However, we omitted the info regarding chunks previously.
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.
Reviewed 1 of 2 files at r9, all commit messages.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @ple13 and @SanjayVas)
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 290 at r7 (raw file):
Previously, renjiezh wrote…
Done.
nit: bytes_per_item
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.
Reviewed 1 of 2 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ple13 and @renjiezh)
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 288 at r10 (raw file):
int64 register_count = 1; // Length of each item in bytes. #_EDPs * max_frequency should be no more than
What is "item" in this case? Is this a known term w.r.t sketch creation?
Code quote:
item
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 289 at r10 (raw file):
// Length of each item in bytes. #_EDPs * max_frequency should be no more than // 2^(bytes_per_item*8).
Reference specific messages and fields. The abbreviation "EDP" is not used in the API.
Suggestion:
// Length of each item in bytes.
//
// The product of `maximum_frequency` and the `nonce_hashes` count from the `MeasurementSpec` should be no more than 2 ^ (`bytes_per_item` * 8).
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 69 at r7 (raw file):
Previously, renjiezh wrote…
Added comment to make it clearer.
The requisition data is either a seed from the header or blob of chunks .
To clarify, this means that you'll either have a seed specified in the header and no chunks or no seed specified in the header and a set of chunks, correct?
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 63 at r10 (raw file):
// EDPs and the MPC are using the same protocol config. // // NOT required for LiquidLegionV2 protocols.
This is implying it will never be required for those protocols, where I thought we wanted to make this field unconditionally required in the future.
Maybe rephrase this to make it clear that this is required for the HMShareShuffle protocol and may eventually be required for all protocols.
Code quote:
NOT required for LiquidLegionV2 protocols
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 72 at r10 (raw file):
// // If the seed is not specified or empty, it means the requisition is // fulfilled by a blob of chunks.
The first line of each of each message/field doc should be a summary fragment1 which should describe the message/field itself.
Leave out all mention of a blob here, since that's not part of the API. Also note that for singular proto3 scalar fields, "not specified" is the same as "set to default value". Therefore there's no reason to include "or empty" for a bytes
field.
Suggestion:
// Header for the HonestMajorityShareShuffle protocol.
message HonestMajorityShareShuffle {
// <explanation of what the seed is and where it comes from, e.g. if it's pre-shared across DataProviders>
//
// If specified then there should be no subsequent requests with `body_chunk`s.
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 92 at r10 (raw file):
// the combined `Duchy` ElGamal public keys. The only alignment requirement // is by bytes: a chunk might begin or end in the middle of a single // register.
Need to update this comment as I believe it differs for the new protocol.
Code quote:
// The format of the data depends on the corresponding `MeasurementSpec`. If
// the `Requisition` is for an encrypted sketch, this is the register
// data as documented in the `Sketch` message (sketch.proto) encrypted using
// the combined `Duchy` ElGamal public keys. The only alignment requirement
// is by bytes: a chunk might begin or end in the middle of a single
// register.
Footnotes
-
This is from the Kotlin style guide, but applies to all languages ↩
4d7cd7b
to
dfe74f0
Compare
dfe74f0
to
d3b9052
Compare
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.
Reviewable status: 1 of 3 files reviewed, 7 unresolved discussions (waiting on @ple13 and @SanjayVas)
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 288 at r10 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
What is "item" in this case? Is this a known term w.r.t sketch creation?
Renamed to register
src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto
line 289 at r10 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Reference specific messages and fields. The abbreviation "EDP" is not used in the API.
Done.
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 69 at r7 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
To clarify, this means that you'll either have a seed specified in the header and no chunks or no seed specified in the header and a set of chunks, correct?
For our new protocol, yes.
But it supports both appearances of seed
(along with other values if needed) and chunks.
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 63 at r10 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
This is implying it will never be required for those protocols, where I thought we wanted to make this field unconditionally required in the future.
Maybe rephrase this to make it clear that this is required for the HMShareShuffle protocol and may eventually be required for all protocols.
Done.
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 72 at r10 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
The first line of each of each message/field doc should be a summary fragment1 which should describe the message/field itself.
Leave out all mention of a blob here, since that's not part of the API. Also note that for singular proto3 scalar fields, "not specified" is the same as "set to default value". Therefore there's no reason to include "or empty" for a
bytes
field.
Done.
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 92 at r10 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Need to update this comment as I believe it differs for the new protocol.
Done.
Footnotes
-
This is from the Kotlin style guide, but applies to all languages ↩
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.
Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ple13 and @renjiezh)
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 72 at r10 (raw file):
nit
From the linked style guide:
This is a fragment–a noun phrase or verb phrase, not a complete sentence. It does not begin with "
A `Foo` is a...
"
This means the the summary fragment shouldn't read "The seed is..." but rather e.g. "A random seed which is..."
src/main/proto/wfa/measurement/api/v2alpha/requisition_fulfillment_service.proto
line 63 at r11 (raw file):
// EDPs and the MPC are using the same protocol config. // // Required for HonestMajorityShareShuffle protocol. LiquidLegionsV2
nit: indicate that it will be required for all protocols. We don't need to enumerate them, and the Direct protocol doesn't involve calling this method.
Code quote:
LiquidLegionsV2
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.
Reviewed 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @renjiezh)
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @renjiezh)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @renjiezh)
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.
Reviewed 1 of 2 files at r3, 1 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @renjiezh)
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.
Reviewed 1 of 2 files at r11, 1 of 1 files at r12.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @renjiezh)
For shuffle based secret sharing protocol, requisitions are fulfilled by EDPs with either chunks of data (a blob) or a seed. The seed can be expanded into a deterministic blob with the same PRNG by workers.
The change is back-compatible.
Also added ProtocolConfig in FulfillRequisitionRequest for this issue(world-federation-of-advertisers/cross-media-measurement#1329).