-
Notifications
You must be signed in to change notification settings - Fork 13
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 Kingdom measurement creation to support HMSS protocol. #1404
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 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ple13 and @renjiezh)
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateMeasurements.kt
line 109 at r1 (raw file):
ProtocolConfig.ProtocolCase.HONEST_MAJORITY_SHARE_SHUFFLE -> HmssProtocolConfig.requiredExternalDuchyIds else -> emptySet()
Use an exhaustive when.
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateMeasurements.kt
line 131 at r1 (raw file):
ProtocolConfig.ProtocolCase.HONEST_MAJORITY_SHARE_SHUFFLE -> HmssProtocolConfig.requiredExternalDuchyIds.size else -> 0
Use an exhaustive when
src/main/proto/wfa/measurement/internal/kingdom/protocol_config.proto
line 183 at r1 (raw file):
// Length of each register in bytes. // // The product of `maximum_frequency` and the `number_of_edps` should be no
nit: these fields don't exist on this message. Is there a better way to document this, or is it fine to drop it here?
Code quote:
`maximum_frequency` and the `number_of_edps`
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: 7 of 9 files reviewed, 2 unresolved discussions (waiting on @ple13 and @SanjayVas)
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateMeasurements.kt
line 109 at r1 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Use an exhaustive when.
Done.
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateMeasurements.kt
line 131 at r1 (raw file):
Previously, SanjayVas (Sanjay Vasandani) wrote…
Use an exhaustive when
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 r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ple13)
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 7 of 9 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @renjiezh and @stevenwarejones)
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/HmssProtocolConfig.kt
line 1 at r2 (raw file):
// Copyright 2023 The Cross-Media Measurement Authors
2024
Code quote:
2023
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/HmssProtocolConfig.kt
line 31 at r2 (raw file):
private set fun initializeFromFlags(flags: HmssProtocolConfigFlags) {
Do we need DuchyProtocolConfig? I think in llv2, we use it to specify differential privacy params.
src/main/proto/wfa/measurement/internal/kingdom/protocol_config.proto
line 140 at r2 (raw file):
// The mechanism to generate noise by MPC nodes during computation. NoiseMechanism noise_mechanism = 2;
We need DifferentialPrivacyParams to go with noise_mechanism.
src/main/proto/wfa/measurement/internal/kingdom/protocol_config.proto
line 182 at r2 (raw file):
// Length of each register in bytes. int32 bytes_per_register = 2;
We need a field to specify the ring modulus too (e.g. uint32 ring_modulus = 3;). The modulus is needed when the computation is done over a ring which is not Z_{2^k}.
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 5 of 9 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @renjiezh)
src/main/proto/wfa/measurement/internal/kingdom/protocol_config_config.proto
line 41 at r2 (raw file):
ProtocolConfig.HonestMajorityShareShuffle protocol_config = 1; // List of required duchies for this protocol. repeated string required_external_duchy_ids = 2;
wdyt about adding max and min duchy_participatnt_count ?
d5f604b
to
a316951
Compare
a316951
to
dd88a27
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: 7 of 9 files reviewed, 5 unresolved discussions (waiting on @ple13, @SanjayVas, and @stevenwarejones)
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/HmssProtocolConfig.kt
line 1 at r2 (raw file):
Previously, ple13 (Phi) wrote…
2024
Done.
src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/HmssProtocolConfig.kt
line 31 at r2 (raw file):
Previously, ple13 (Phi) wrote…
Do we need DuchyProtocolConfig? I think in llv2, we use it to specify differential privacy params.
In llv2, DuchyProtocolConfig contains several sets of dp params for different intermediate noise. But HMSS should only have one type of noise whose dp params is specified via measurement creation. Please correct me if I misunderstand it.
src/main/proto/wfa/measurement/internal/kingdom/protocol_config.proto
line 140 at r2 (raw file):
Previously, ple13 (Phi) wrote…
We need DifferentialPrivacyParams to go with noise_mechanism.
I think output DP params are specified by the MC during measurement creation. Is this correct?
src/main/proto/wfa/measurement/internal/kingdom/protocol_config.proto
line 182 at r2 (raw file):
Previously, ple13 (Phi) wrote…
We need a field to specify the ring modulus too (e.g. uint32 ring_modulus = 3;). The modulus is needed when the computation is done over a ring which is not Z_{2^k}.
Done.
Please let me know if the description needs update.
src/main/proto/wfa/measurement/internal/kingdom/protocol_config_config.proto
line 41 at r2 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
wdyt about adding max and min duchy_participatnt_count ?
For HMSS protocol, the number of DuchyParticipants is fixed as 3. I don't see the potential change in the near future. So I think we would not config that.
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, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @renjiezh and @stevenwarejones)
src/main/proto/wfa/measurement/internal/kingdom/protocol_config.proto
line 1 at r3 (raw file):
// Copyright 2024 The Cross-Media Measurement Authors
This is not a new file, should we keep 2021?
src/main/proto/wfa/measurement/internal/kingdom/protocol_config.proto
line 184 at r3 (raw file):
int32 bytes_per_register = 2; // Modulus for computations whose ring is not Z_{2^k}.
Maybe just Secret share modulus
? (as we haven't defined k anywhere).
Code quote:
Modulus for computations whose ring is not Z_{2^k}.
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: 8 of 9 files reviewed, 3 unresolved discussions (waiting on @ple13 and @stevenwarejones)
src/main/proto/wfa/measurement/internal/kingdom/protocol_config.proto
line 1 at r3 (raw file):
Previously, ple13 (Phi) wrote…
This is not a new file, should we keep 2021?
Done.
src/main/proto/wfa/measurement/internal/kingdom/protocol_config.proto
line 184 at r3 (raw file):
Previously, ple13 (Phi) wrote…
Maybe just
Secret share modulus
? (as we haven't defined k anywhere).
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 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)
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 9 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @renjiezh)
No description provided.