-
Notifications
You must be signed in to change notification settings - Fork 25
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
Refactor reshare tests #268
Refactor reshare tests #268
Conversation
While working on private-attribution#256, I noticed that reshare tests consistently fail concurrency test. It was caused by always using RecordId(0) in a loop that terminates if reshare successfully generates unique shares. When loop needs more than 1 iteration it fails on PRSS uniqueness check. While it was possible to fix that test, I did not really like the probabilistic nature of it and decided to create 2 tests instead. One validates that reshare protocol actually communicates and another one that it is correct. It was also a good opportunity to use new test API implemented by @martinthomson in private-attribution#261. It should be used everywhere. One small change I made to the `TestWorld` to improve ergonomics changes `make_contexts` function to narrow generated contexts to some unique (within this world) step, so the following syntax is made possible ```rust for _ in 0..10 { world.semi_honest(..., |ctx, v| { ... }) }
src/protocol/sort/reshare.rs
Outdated
// run reshare protocol for all helpers except the one that does not know the input | ||
if ctx.role() == target { | ||
// test follows the reshare protocol | ||
ctx.prss().generate_fields(record_id).into() |
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.
Won't the effect be the same if you run reshare for this target? Or, are you trying to test that it doesn't need to participate?
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 tried to ensure a naive Reshare
implementation (that is - take the input and return it as output) won't pass this test. I think that was the reason for for
loop inside the test before.
src/protocol/sort/reshare.rs
Outdated
ctx.prss().generate_fields(record_id).into() | ||
} else { | ||
Reshare::new(share.clone()) | ||
.execute(&ctx, record_id, target) |
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 still takes a reference to the context, when it shouldn't.
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.
let me merge #257 and rebase this on top of that
While working on #256, I noticed that reshare tests consistently fail concurrency test. It was caused by always using RecordId(0) in a loop that terminates if reshare successfully generates unique shares. When loop needs more than 1 iteration it fails on PRSS uniqueness check.
While it was possible to fix that test, I did not really like the probabilistic nature of it and decided to create 2 tests instead. One validates that reshare protocol actually communicates and another one that it is correct.
It was also a good opportunity to use new test API implemented by @martinthomson in #261. It should be used everywhere.
One small change I made to the
TestWorld
to improve ergonomics changesmake_contexts
function to narrow generated contexts to some unique (within this world) step, so the following syntax is made possible