private-attribution / ipa

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

Step one of removing UpgradedSemiHonestContext #1127

Closed benjaminsavage closed 2 months ago

benjaminsavage commented 2 months ago

I don't think we should be hard-coding "UpgradedSemiHonestContext" into our protocols. That's a strange dummy type that's there so that a Semi Honest Context can get "upgraded" but it won't actually do anything.

We probably want to make our protocols not really care if it's operating in a semi-honest or a malicious context. This PR is about starting to make that change.

codecov[bot] commented 2 months ago

Codecov Report

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

Project coverage is 91.47%. Comparing base (4ea9feb) to head (7fe6b79). Report is 4 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1127 +/- ## ========================================== - Coverage 91.48% 91.47% -0.02% ========================================== Files 189 189 Lines 27267 27224 -43 ========================================== - Hits 24946 24902 -44 - Misses 2321 2322 +1 ```

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

benjaminsavage commented 2 months ago

@andyleiserson - you cool with this change?

akoshelev commented 2 months ago

I agree with the direction set by this change. It feels like we need two types of contexts:

I think @danielmasny and @andyleiserson had a plan how to get there

andyleiserson commented 2 months ago

@akoshelev had objected to the addition of SecureMul bounds at one point. Part of the reason for using UpgradedSemiHonestContext may have been to avoid other trait bounds by using a concrete type.

With MAC-based malicious security, we had some semi-honest protocols implemented for C: Context. These implementations did not overlap with the malicious implementations, because malicious shares had a dedicated type (MaliciousReplicated). The dedicated malicious share type also protected us against inadvertently using a semi-honest protocol in a malicious context.

With DZKP-based malicious security, we no longer have a dedicated share type, which means that we can't have generic semi-honest implementations for C: Context, because of impl overlap and safety concerns. This likely means additional trait bounds.

There is a question of whether we want protocols to be generic over all three of (semi-honest, MAC malicious, DZKP malicious). My current thinking is no, because:

This change uses generic C: Context in attribution and aggregation, which I don't think need to do reveals or validates, so a generic context should work. (Caveat: that's not true of the proposed aggregation implementation that reveals breakdown keys.) I'm curious what the subsequent parts of this change are. As @danielmasny noted, in some parts of the protocol, we'll need to use a bound of C: UpgradableContext.

akoshelev commented 2 months ago

@akoshelev had objected to the addition of SecureMul bounds at one point. Part of the reason for using UpgradedSemiHonestContext may have been to avoid other trait bounds by using a concrete type.

yea I wasn't excited about slapping SecureMul trait bound on most (all?) of our MPC functions. I still think we shouldn't do that.

With DZKP-based malicious security, we no longer have a dedicated share type, which means that we can't have generic semi-honest implementations for C: Context, because of impl overlap and safety concerns. This likely means additional trait bounds.

I remember discussing adding a new associated type to Context trait that will bind the share type to the context type used. Would it solve the issue with trait bounds?

There is a question of whether we want protocols to be generic over all three of (semi-honest, MAC malicious, DZKP malicious). My current thinking is no, because:

I am thinking maybe. We went down the path of making semi-honest and MAC-malicious compatible at the protocol layer and they are utterly different. I don't see why ZKP context can't have no-op upgrades and downgrades with checks, if we need it. Maybe the flexibility of writing the circuits in one unified way by asking for C: Context is worth it, I don't know.

One thing I noted is that we always start queries with SemiHonestContext, so maybe UpgradableContext is not necesary to have.