Skip to content
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

Event generation using test_mpc and IPA integration test + migrate to HTTP2 #647

Merged
merged 13 commits into from
May 22, 2023

Conversation

akoshelev
Copy link
Collaborator

@akoshelev akoshelev commented May 13, 2023

There are there major changes that this PR brings:

  • Implement event generator as Iterator<TestRawDataRecord>. for millions of events that is more efficient than the existing approach. It takes about 3 seconds to generate 1 million events (excluding IO)
  • Add new gen-ipa-inputs command
  • IPA integration test that uses gen-ipa-inputs to generate inputs and run semi-honest IPA.

--edit
Unintended change, but IPA integration tests were failing (see #650) on HTTP 1.1, so I had to include changing clients to use HTTP2


#[test]
fn https_semi_honest_ipa() {
test_ipa(&[4433, 4434, 4435], IpaSecurityModel::SemiHonest, true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit annoying - I know that there is no guaranteed way to pick an available port (at least on MacOS, on Linux it works pretty reliable with SO_REUSEPORT and rust appears to be using it), but even best-effort approach with TcpListener should work here. Right now it is not possible to test this in parallel

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this when I wrote the test... a possible strategy I came up with is to create TcpListeners in the test, pass them to the helper processes as /dev/stdin, and add an option to the helper that does TcpListener::from_raw_fd rather than bind a socket. I didn't do this at the time because it seemed like a fair amount of work, and it seemed useful to have at least one test that exercises the standard flow for binding the socket on startup. But if we're adding more tests that follow this framework, it becomes more important to do this work. I'll see if I can get this done next week.

@akoshelev
Copy link
Collaborator Author

integration tests hanging is not good: https://github.com/private-attribution/ipa/actions/runs/4964162687/jobs/8884058950?pr=647, opening an issue for that

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akoshelev and @andyleiserson should work out who gets to merge and who get to rebase :)

src/bin/test_mpc.rs Outdated Show resolved Hide resolved
src/cli/csv.rs Outdated Show resolved Hide resolved
Comment on lines +106 to +108
// even bit vector takes too long to initialize for MAX_USER_ID. Need a sparse structure
// here
used: HashSet<UserId>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need to track this? I mean, the problem here is that we're essentially stateless, but you could create user IDs in monotonically increasing sequence. A PRP would allow you to have the identifiers that you use appear to be randomly distributed in the target range...

src/test_fixture/event_gen.rs Show resolved Hide resolved
type Item = TestRawDataRecord;

fn next(&mut self) -> Option<Self::Item> {
const USERS_IN_FLIGHT: usize = 10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a little small for larger event counts. It's arbitrary, but I'd have said that you would expect hundreds of people for a million records. (It's no extra work for IPA, but I'm not sure how much we care about fidelity.)

@akoshelev
Copy link
Collaborator Author

@akoshelev and @andyleiserson should work out who gets to merge and who get to rebase :)

@andyleiserson lets merge your changes first

@akoshelev akoshelev changed the title Event generation using test_mpc and IPA integration test Event generation using test_mpc and IPA integration test + migrate to HTTP2 May 18, 2023
@richajaindce
Copy link
Contributor

@akoshelev and @andyleiserson should work out who gets to merge and who get to rebase :)

@andyleiserson lets merge your changes first

There are 2 PRs which would need to be rebased based on what gets landed first (#654 also) :P
How about, we land this since it is already approved?

@andyleiserson
Copy link
Collaborator

How about, we land this since it is already approved?

That's fine with me.

@akoshelev
Copy link
Collaborator Author

I am on vacation until Tuesday PT time, so I'll merge it to unblock you folks. Sorry @andyleiserson for the rebase

@akoshelev akoshelev merged commit 8e625e6 into private-attribution:main May 22, 2023
@akoshelev akoshelev deleted the test-mpc-inputs branch May 22, 2023 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants