-
Notifications
You must be signed in to change notification settings - Fork 16
fix: set max frequency below the ring modulus #3292
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
fix: set max frequency below the ring modulus #3292
Conversation
kungfucraig
left a comment
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.
@kungfucraig reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)
src/main/kotlin/org/wfanet/measurement/computation/KAnonymityParams.kt line 32 at r1 (raw file):
val minUsers: Int, val minImpressions: Int, val reachMaxFrequencyPerUser: Int = 126,
Can these two things reference the same constant, perhaps defined in a third file?
stevenwarejones
left a comment
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 @kungfucraig)
src/main/kotlin/org/wfanet/measurement/computation/KAnonymityParams.kt line 32 at r1 (raw file):
Previously, kungfucraig (Craig Wright) wrote…
Can these two things reference the same constant, perhaps defined in a third file?
Unfortunately, this is part of the protocol config configured in the kingdom. I'd think we'd need to think about how to maintain consistency across these configs. we could add a param for the kingdom ring modulus. I don't want to have to add that param since we are moving to TrusTee anyways.
stevenwarejones
left a comment
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 5 files reviewed, 1 unresolved discussion (waiting on @kungfucraig)
src/main/kotlin/org/wfanet/measurement/computation/KAnonymityParams.kt line 32 at r1 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
Unfortunately, this is part of the protocol config configured in the kingdom. I'd think we'd need to think about how to maintain consistency across these configs. we could add a param for the kingdom ring modulus. I don't want to have to add that param since we are moving to TrusTee anyways.
actually - went ahead and put it in a separate params file
kungfucraig
left a comment
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.
@kungfucraig reviewed 5 of 5 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones)
src/main/kotlin/org/wfanet/measurement/computation/KAnonymityParams.kt line 32 at r1 (raw file):
Previously, stevenwarejones (Steven Ware Jones) wrote…
actually - went ahead and put it in a separate params file
Thanks. I think keeping it consistent across EDPA and the Kingdom is a separate challenge, but consistency within EDPA is nice.
stevenwarejones
left a comment
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.
@stevenwarejones reviewed 1 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones)
…ny_reach_frequency
The default max frequency is limited by two things: