private-attribution / ipa

A raw implementation of Interoperable Private Attribution
MIT License
40 stars 23 forks source link

Adding a few useful helper functions #1109

Closed benjaminsavage closed 3 months ago

benjaminsavage commented 3 months ago

We will definitely need these two helper functions at the next stage. I've added a few tests for them that illustrate how they will be used and ensure the expected behavior.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.16%. Comparing base (6cc97cd) to head (8810146).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1109 +/- ## ========================================== + Coverage 91.10% 91.16% +0.06% ========================================== Files 187 187 Lines 26342 26457 +115 ========================================== + Hits 23998 24120 +122 + Misses 2344 2337 -7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

andyleiserson commented 3 months ago

Is there a way to use left and right instead of .0 and .1? I understand not wanting to use Replicated / AdditiveShare since that's not really what these are. Maybe return a simple helper type something like struct LeftRight<T> { left: T, right: T } instead of a tuple? Or just deconstruct the return value as (foo_left, foo_right) immediately.

It might be possible to use the StdArray wrapper type for the proofs (instead of [F; P]), which would provide pre-built support for generation with PRSS, subtraction for gen_other_proof_share, and sending over channels. Maybe ShareArray is the better name for StdArray that I wish I had thought of earlier.

benjaminsavage commented 3 months ago

These are all great suggestions. @danielmasny and I can follow up with them.

benjaminsavage commented 3 months ago

@andyleiserson - I started trying to use StdArray, but I got stuck along the way. It immediately started spreading and before long the whole file needed StdArray throughout, not just in this one function.

Changing gen_other_proof_share was easy, but then I needed to change gen_artefacts_from_recursive_step which meant changing gen_challenge_and_recurse to accept StdArray, and I couldn't make it work there because compute_hash wants something it can iterate through and serialize, and I got stuck there.

In general though, are we sure there's a big benefit to this? Vanilla arrays are nice a readable, and the only benefit we seem to get from StdArray is that you can't access the elements.

andyleiserson commented 2 months ago

I think it would be okay to add some things like From, Into, and Deref to StdArray. It wasn't needed for vectorization, but if we're using it for other things, as you found, it's hard to do much with it without some conversions.

Adding them to StdArray doesn't directly make them available to vectorized code in most cases, because the vectorized code is using the SharedValueArray trait rather than StdArray directly.

andyleiserson commented 2 months ago

This is what I had in mind: https://github.com/private-attribution/ipa/commit/a9587f827189ed4aaba1afdb4f47da563230042e