Add the secret share generator function.

[ple13]

No description provided.

[wfa-reviewable]

This change is 

[kungfucraig]

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @renjiezh and @SanjayVas)

[renjiezh]

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

---

src/main/cc/any_sketch/crypto/secret_share.h line 39 at r2 (raw file):

  virtual absl::StatusOr<SecretShare> GenerateSecretShares(
      const SecretShareParameter& secret_share_parameter,
      const std::vector<uint32_t>& input) = 0;

I think it is preferable to use absl::Span for read-only arrays.

---

src/main/cc/any_sketch/crypto/secret_share.cc line 92 at r2 (raw file):

  for (int i = 0; i < input.size(); i++) {
    share_vector[i] =
        SubMod(input[i], share_vector[i], secret_share_parameter.modulus());

Do we need to check input < modulus? SumMod has assumed that but it could be wrong by the caller.

---

src/main/cc/any_sketch/crypto/secret_share.cc line 106 at r2 (raw file):

std::unique_ptr<SecretShareGenerator> CreateSecretShareGenerator() {
  return absl::WrapUnique(new SecretShareGeneratorImpl());

A comment here to explain the usage of new like:

 // Using `new` to access a non-public constructor.

---

src/main/cc/math/open_ssl_uniform_random_generator.cc line 115 at r2 (raw file):

      (double)((1 << bit_length) - modulus) / (double)(1 << bit_length);

  // Compute the expected number of samples needed.

Can we add a comment to explain the choice of factor 2 here

[SanjayVas]

Reviewed 12 of 12 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ple13)

---

src/main/cc/any_sketch/crypto/secret_share.cc line 30 at r2 (raw file):

using wfa::math::UniformPseudorandomGenerator;

uint32_t SubMod(uint32_t x, uint32_t y, uint32_t modulus) {

Put this in an unnamed namespace for internal linkage. See https://google.github.io/styleguide/cppguide.html#Internal_Linkage

---

src/main/cc/math/uniform_pseudorandom_generator.h line 46 at r2 (raw file):

      uint64_t size) = 0;

  // Generate a vector of pseudorandom values in the range [0, modulus).

nit: summary fragment for a function should be a verb phrase.

Suggestion:

Generates

---

src/main/proto/wfa/any_sketch/secret_share.proto line 31 at r2 (raw file):

}

message SecretShare {

Just double-checking why this is a protobuf. Is it so we can use the same structure from the Java impl?

[kungfucraig]

Reviewed 8 of 12 files at r2, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @ple13 and @SanjayVas)

---

src/main/cc/any_sketch/crypto/secret_share.h line 28 at r2 (raw file):

namespace wfa::any_sketch::crypto {

class SecretShareGenerator {

Either this class should be stateful and conform to Unary function, or there should just be a namespace level function called "GenerateSecretShares." Having a class a a builder pattern for a stateless object isn't buying you anything here.

---

src/main/cc/any_sketch/crypto/secret_share.cc line 30 at r2 (raw file):

using wfa::math::UniformPseudorandomGenerator;

uint32_t SubMod(uint32_t x, uint32_t y, uint32_t modulus) {

Please document this function.

---

src/main/cc/any_sketch/crypto/secret_share.cc line 90 at r2 (raw file):

                   prng->GetUniformRandomRange(
                       input.size(), secret_share_parameter.modulus()));
  for (int i = 0; i < input.size(); i++) {

If you think it's worth reusing share_vector vs. having two vectors, one to represent the outputs and the other the random sequence, then a comment indicating that the values of share vector are initially the random sequence and are then replaced by the result of SubMod will be helpful for future readers.

---

src/main/cc/math/open_ssl_uniform_random_generator.h line 92 at r2 (raw file):

  // Generate a vector of pseudorandom values in the range [0, modulus).
  // First, we sample n random values in [0, 2^k) where k is the smallest value

"size" random values?

---

src/main/cc/math/open_ssl_uniform_random_generator.cc line 118 at r2 (raw file):

  uint64_t sample_size =
      (uint64_t)(size + 2 * failure_rate * size / (1 - failure_rate));
  std::vector<unsigned char> arr(sample_size * bytes_per_value, 0);

Any reason you can't call GetPseudradomBytes as a basis of this implementation?

---

src/main/cc/math/open_ssl_uniform_random_generator.cc line 149 at r2 (raw file):

  // In case the number of rejections is higher than expected, sample more
  // random values in the range and append to the current result.
  if (ret.size() < size) {

Why not just put a while loop around the above?

---

src/main/cc/math/uniform_pseudorandom_generator.h line 46 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: summary fragment for a function should be a verb phrase.

Also with respect to the function names, in this PR Generate and Get seem to be used as synonyms. Can the functions just use one of these verbs?

[ple13]

Reviewable status: 1 of 12 files reviewed, 12 unresolved discussions (waiting on @kungfucraig, @renjiezh, and @SanjayVas)

---

src/main/cc/math/open_ssl_uniform_random_generator.h line 92 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

"size" random values?

I updated the comment. n is a bit larger than size as a portion of values in [0, 2^k) will be rejected. I removed the algorithm description and moved it to the .cc file.

---

src/main/cc/math/open_ssl_uniform_random_generator.cc line 115 at r2 (raw file):

Previously, renjiezh wrote…

Can we add a comment to explain the choice of factor 2 here

Updated the comment. Remove the factor 2 as it's not needed.

---

src/main/cc/math/open_ssl_uniform_random_generator.cc line 118 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Any reason you can't call GetPseudradomBytes as a basis of this implementation?

Ah yeah, this is much better.

---

src/main/cc/math/open_ssl_uniform_random_generator.cc line 149 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Why not just put a while loop around the above?

Done.

---

src/main/cc/math/uniform_pseudorandom_generator.h line 46 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Also with respect to the function names, in this PR Generate and Get seem to be used as synonyms. Can the functions just use one of these verbs?

Done.

---

src/main/proto/wfa/any_sketch/secret_share.proto line 31 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Just double-checking why this is a protobuf. Is it so we can use the same structure from the Java impl?

Yes, I think this will be easier to pass data through the JNI via serialization/deserialization.

---

src/main/cc/any_sketch/crypto/secret_share.h line 28 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Either this class should be stateful and conform to Unary function, or there should just be a namespace level function called "GenerateSecretShares." Having a class a a builder pattern for a stateless object isn't buying you anything here.

Fixed. The class was removed, and a simple function is used instead.

---

src/main/cc/any_sketch/crypto/secret_share.h line 39 at r2 (raw file):

Previously, renjiezh wrote…

I think it is preferable to use absl::Span for read-only arrays.

Done.

---

src/main/cc/any_sketch/crypto/secret_share.cc line 30 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Put this in an unnamed namespace for internal linkage. See https://google.github.io/styleguide/cppguide.html#Internal_Linkage

Done.

---

src/main/cc/any_sketch/crypto/secret_share.cc line 30 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Please document this function.

Commend added.

---

src/main/cc/any_sketch/crypto/secret_share.cc line 90 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

If you think it's worth reusing share_vector vs. having two vectors, one to represent the outputs and the other the random sequence, then a comment indicating that the values of share vector are initially the random sequence and are then replaced by the result of SubMod will be helpful for future readers.

Two vectors are now used to make the code clearer.

---

src/main/cc/any_sketch/crypto/secret_share.cc line 92 at r2 (raw file):

Previously, renjiezh wrote…

Do we need to check input < modulus? SumMod has assumed that but it could be wrong by the caller.

Added the checks for the SubMod function.

---

src/main/cc/any_sketch/crypto/secret_share.cc line 106 at r2 (raw file):

Previously, renjiezh wrote…

A comment here to explain the usage of new like:

 // Using `new` to access a non-public constructor.

Class has been removed.

[renjiezh]

Reviewed 11 of 11 files at r3, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @kungfucraig and @SanjayVas)

[SanjayVas]

Reviewed 11 of 11 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @kungfucraig and @ple13)

---

src/main/cc/math/open_ssl_uniform_random_generator.cc line 91 at r3 (raw file):

}

// Rejection sampling is used to generate uniformly random values in the range

nit: the first line of a function comment should be a summary fragment, specifically a verb phrase.

[ple13]

Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on @kungfucraig, @renjiezh, and @SanjayVas)

---

src/main/cc/math/open_ssl_uniform_random_generator.cc line 91 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: the first line of a function comment should be a summary fragment, specifically a verb phrase.

Done.

[SanjayVas]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kungfucraig)

[kungfucraig]

Reviewed 9 of 11 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ple13)

[renjiezh]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ple13)

---

src/test/cc/any_sketch/crypto/secret_share_generator_test.cc line 65 at r5 (raw file):

      std::unique_ptr<UniformPseudorandomGenerator> prng,
      OpenSslUniformPseudorandomGenerator::Create(key_vec, iv_vec));

nit: ASSERT_EQ(input.size(), secret_share.share_vector.size())

[ple13]

Reviewable status: 11 of 12 files reviewed, all discussions resolved (waiting on @kungfucraig, @renjiezh, and @SanjayVas)

---

src/test/cc/any_sketch/crypto/secret_share_generator_test.cc line 65 at r5 (raw file):

Previously, renjiezh wrote…

nit: ASSERT_EQ(input.size(), secret_share.share_vector.size())

Done

[stevenwarejones]

Reviewed 1 of 12 files at r2, 8 of 11 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ple13)