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

Adds Prio3 a set of verifiable distributed aggregation functions. #522

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented Dec 17, 2024

Prio3 supports several variants for aggregating data measurements in a privacy preserving manner.
This implementation is compliant with v13 of CRFG draft VDAF https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-vdaf-13#name-prio3

Prio3 supports several variants for aggregating data measurements
in a privacy preserving manner.
This implementation is compliant with v13 of CRFG draft VDAF
https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-vdaf-13#name-prio3
@armfazh armfazh added the new feature New functionality or module label Dec 17, 2024
@armfazh armfazh self-assigned this Dec 17, 2024
Copy link
Contributor

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

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

This looks really good!

package math

import "math/bits"

Copy link
Contributor

Choose a reason for hiding this comment

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

This could use a comment explaining what twoN and N are.

@@ -0,0 +1,2 @@
// Package vdaf provides verifiable distributed aggregation functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add reference to draft?

Comment on lines +19 to +21
// Fp lists the functionality that a prime field element must have denoting
// methods with a pointer receiver.
type Fp[E Elt] interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this functionality overlap with the functionality of finite fields used in elliptic curves? If so, would it be worth pulling out the overlapping functionality into an interface?

IsZero() bool
// Returns true if the element is the neutral multiplicative element.
IsOne() bool
// Returns true if the element is equivalent to x.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The word "equivalent" makes me think of the "$\equiv$" symbol, which makes me think "equal modulo $p$"

Suggested change
// Returns true if the element is equivalent to x.
// Returns true if the equal is equivalent to x.

IsOne() bool
// Returns true if the element is equivalent to x.
IsEqual(x *E) bool
// Set the element to the neutral additive element.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Set the element to the neutral additive element.
// Set the element to the neutral multiplicative element.

@@ -0,0 +1,200 @@
// Package mhcv is a VDAF for aggregating bounded vectors of Booleans into buckets.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, perhaps say what mhcv stands for.

Suggested change
// Package mhcv is a VDAF for aggregating bounded vectors of Booleans into buckets.
// Package mhcv is a VDAF for aggregating vectors of Booleans with bounded weight.

Comment on lines +92 to +101
m := new(flpMultiHotCountVec)
if length == 0 {
panic("length cannot be zero")
}
if maxWeight > length {
panic("maxWeight cannot be greater than length")
}
if chunkLen == 0 {
panic("chunkLen cannot be zero")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure panicking is appropriate. Suppose the parameters for MHCV are provided by the peer in some higher level protocol: to avoid panic, we basically have to check these conditions outside of this function before attempting to construct the FLP.

I think we should return an error here instead.

Here and elsewhere.

Comment on lines +163 to +166

testMarshal(t, prepStates[0], &params)
testMarshal(t, &outboundPrepShares[0], &params)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth testing all prep state/shares and not just the first, as they might have different formats depending on whether the aggregator is the leader or a helper.

return err
}

v[i], ok = isInRange(&b)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure you've implemented the masking properly: see next_vec() in the draft. The idea is that if the modulus is 1 or more bits smaller than the 2 ** ENCODED_SIZE, then we'll clear those higher bits before doing the range check.

This may not apply to either Field64 or Field128. If not, we should at least document this in a way that makes it harder for us to make this mistake with different field parameters.

Comment on lines +61 to +62
// RandomSHA3 samples an element from a SHA3 state.
RandomSHA3(*sha3.State) error
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this used? Does it work differently than the same function on vectors?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New functionality or module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants