Add secret share generator adapter.

[ple13]

No description provided.

[wfa-reviewable]

This change is 

[renjiezh]

Reviewed 18 of 22 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

[SanjayVas]

Add jni interface

This doesn't actually add any JNI libraries. Those are in any-sketch-java. I personally believe the adapters also belong there, but it's probably too much work to change that now.

Note that "JNI interface" is redundant as it expands to "Java native interface interface"

Reviewed 12 of 22 files at r1, 3 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ple13)

---

src/main/cc/crypto/shuffle.h line 25 at r2 (raw file):

#include "wfa/frequency_count/secret_share.pb.h"

namespace wfa::measurement::common::crypto {

The namespace does not match the directory path. Looking at the existing code, the wfa namespace appears to be implied such that src/main/cc maps to wfa. This is not correct, but it is at least consistent for this repo.

Suggestion:

wfa::crypto

---

src/main/proto/wfa/frequency_count/share_shuffle_sketch.proto line 27 at r2 (raw file):

option java_outer_classname = "ShareShuffleSketchProto";

// The data share of HonestMajorityShareShuffle protocol.

@kungfucraig Are we intentionally associating this to the protocol? I thought this repo was meant to remain protocol-agnostic and just include primitives.

If we're trying to remain generic, then I think this should be something like VectorOfCountsSketch.

[kungfucraig]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ple13 and @SanjayVas)

---

src/main/proto/wfa/frequency_count/share_shuffle_sketch.proto line 27 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

@kungfucraig Are we intentionally associating this to the protocol? I thought this repo was meant to remain protocol-agnostic and just include primitives.

If we're trying to remain generic, then I think this should be something like VectorOfCountsSketch.

I'd call it "FrequencyVector." I don't think there's any harm in saying this is specific for frequency estimation.

Keeping this protocol-agnostic makes sense to me.

On the other hand, I'm not philosophically opposed to having protocol specific stuff in this repo. I suppose we can cross that bridge when we get there though.

[ple13]

I've edited the name.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SanjayVas)

---

src/main/cc/crypto/shuffle.h line 25 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The namespace does not match the directory path. Looking at the existing code, the wfa namespace appears to be implied such that src/main/cc maps to wfa. This is not correct, but it is at least consistent for this repo.

Done.

---

src/main/proto/wfa/frequency_count/share_shuffle_sketch.proto line 27 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

I'd call it "FrequencyVector." I don't think there's any harm in saying this is specific for frequency estimation.

Keeping this protocol-agnostic makes sense to me.

On the other hand, I'm not philosophically opposed to having protocol specific stuff in this repo. I suppose we can cross that bridge when we get there though.

I've changed it to FrequencyVector.

[SanjayVas]

Reviewed 4 of 22 files at r1, 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ple13)

---

src/test/cc/frequency_count/secret_share_generator_adapter_test.cc line 34 at r3 (raw file):

constexpr int kRingModulus = 128;

TEST(SecretShareJavaAdapterTest, EmptyFrequencyVectorFails) {

nit: I'd personally drop the Java here, as this is technically a language-agnostic adapter for using serialized protobuf request/response messages. It just happens to be that our only use case is Java via JNI.

Code quote:

Java

---

src/main/cc/frequency_count/secret_share_generator.h line 12 at r3 (raw file):

// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and

Just wanted to flag this for the future: the file names here are inconsistent with typical patterns for object-oriented languages. A object noun like the file name in such languages are typically for types (e.g. classes), but this file does not have a type declaration matching that name. For a file containing bare functions, typically the naming would just reference the grouping (e.g. 'secret_shares' or 'secret_share_generation) or the single function if there's just one (e.g. generate_secret_shares`).

Similarly, an adapter would generally be a class that follows the Adapter pattern.

See absl for examples, such as str_join.h which declares absl::StrJoin overloads, or numbers.h which declares various functions for numbers.

This need not be handled in this PR as there are already existing cases in this repo that follow the anti-pattern.

[ple13]

Reviewable status: 15 of 22 files reviewed, 1 unresolved discussion (waiting on @renjiezh and @SanjayVas)

---

src/test/cc/frequency_count/secret_share_generator_adapter_test.cc line 34 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: I'd personally drop the Java here, as this is technically a language-agnostic adapter for using serialized protobuf request/response messages. It just happens to be that our only use case is Java via JNI.

Done.

---

src/main/cc/frequency_count/secret_share_generator.h line 12 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Just wanted to flag this for the future: the file names here are inconsistent with typical patterns for object-oriented languages. A object noun like the file name in such languages are typically for types (e.g. classes), but this file does not have a type declaration matching that name. For a file containing bare functions, typically the naming would just reference the grouping (e.g. 'secret_shares' or 'secret_share_generation) or the single function if there's just one (e.g. generate_secret_shares`).

Similarly, an adapter would generally be a class that follows the Adapter pattern.

See absl for examples, such as str_join.h which declares absl::StrJoin overloads, or numbers.h which declares various functions for numbers.

This need not be handled in this PR as there are already existing cases in this repo that follow the anti-pattern.

Thanks. I've updated the file names for secret_share_generator.* as it's simple to do so.

[SanjayVas]

Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones)

[stevenwarejones]

Reviewed 8 of 22 files at r1, 1 of 4 files at r2, 5 of 10 files at r3, 7 of 7 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ple13)

[kungfucraig]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ple13)

[SanjayVas]

Reviewable status: all files reviewed, 1 unresolved discussion

---

src/main/cc/frequency_count/secret_share_generator_adapter.h line 30 at r4 (raw file):

namespace wfa::frequency_count {

absl::StatusOr<std::string> SecretShareFrequencyVector(

Shouldn't this name match the function being called?

Suggestion:

GenerateSecretShares