Implementation of the initialization, setup, and execution phase of the reach-only protocol.

[ple13]

Implement the phases of the new sparse reach only protocol:

1. Initialization phase: Duchies sample local El Gamal keypair.
2. Setup phase:
   a) Non-aggregators add noise registers to the crv and shuffle the modified crv. They encrypt their excessive noise using the composite El Gamal public key. They send the crv and the excessive noise ciphertext to the aggregator.
   b) Aggregator: Waits for the crv and the excessive noise ciphertexts from non-aggregators. It adds noise to the crv, shuffles it, and encrypts its excessive noise with the composite El Gamal key. It then combines the excessive noise ciphertexts by adding them together. It sends the modified crv and the excessive noise ciphertext to the next worker.
3. Execution phase: Duchies collaborate to decrypt, randomize the register indices, and decrypt the excessive noise ciphertext. The aggregator can count the number of distinct registers, obtain the total amount of excessive noise, then estimate the reach based on the available information.

[wfa-reviewable]

This change is 

[SanjayVas]

Reviewing for C++ style rather than protocol correctness.

Reviewed 1 of 16 files at r1, 1 of 5 files at r2, all commit messages.
Reviewable status: 2 of 16 files reviewed, 3 unresolved discussions (waiting on @kungfucraig, @ple13, and @renjiezh)

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.h line 49 at r2 (raw file):

// the deterministically encrypted results.
absl::StatusOr<std::vector<std::string>> GetBlindedRegisterIndexes(
    absl::string_view data, ProtocolCryptor& protocol_cryptor);

Do all of these methods only use a non-const reference to ProtocolCryptor because the methods are marked as non-const solely due to needing to mutate mutex_? That's one of the few cases where the mutable keyword is acceptable.

This can be handled in a separate PR as it doesn't directly relate to this one, but I'd recommend fixing in a "pre-factoring" PR before this one so we can avoid introducing new functions that unnecessarily have the ability to mutate the ProtocolCryptor instance.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/liquid_legions_v2_encryption_utility_helper.cc line 25 at r2 (raw file):

using ::wfa::measurement::internal::duchy::ElGamalPublicKey;

::wfa::any_sketch::crypto::ElGamalPublicKey ToAnysketchElGamalKey(

nit: "AnySketch" is treated as two words (e.g. the repo name is any-sketch).

Suggestion:

ToAnySketchElGamalKey

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/liquid_legions_v2_encryption_utility_helper.cc line 33 at r2 (raw file):

}

ElGamalPublicKey ToCmmsElGamalKey(

This naming implies to me that it's converting to a CMMS public API ElGamalPublicKey, whereas this is converting to a Duchy-internal message.

Suggestion:

ToDuchyElGamalKey

[renjiezh]

Reviewed all commit messages.
Reviewable status: 2 of 16 files reviewed, 3 unresolved discussions (waiting on @kungfucraig, @ple13, and @SanjayVas)

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.h line 49 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Do all of these methods only use a non-const reference to ProtocolCryptor because the methods are marked as non-const solely due to needing to mutate mutex_? That's one of the few cases where the mutable keyword is acceptable.

This can be handled in a separate PR as it doesn't directly relate to this one, but I'd recommend fixing in a "pre-factoring" PR before this one so we can avoid introducing new functions that unnecessarily have the ability to mutate the ProtocolCryptor instance.

The methods in ProtocolCryptor are intrinsically non-const because the implementation are based on operations from private_join_and_compute, which always change the context_ for randomness. (correct me if I am wrong or not precise.)
So I guess it is not feasible to use const &ProtocolCryptor.

[kungfucraig]

Reviewed 2 of 16 files at r1, all commit messages.
Reviewable status: 4 of 16 files reviewed, 5 unresolved discussions (waiting on @ple13, @renjiezh, and @SanjayVas)

---

src/main/proto/wfa/measurement/internal/duchy/protocol/reach_only_liquid_legions_v2_encryption_methods.proto line 65 at r2 (raw file):

  // Public Key of the composite ElGamal cipher. Used to encrypt the excessive
  // noise (which is zero) when noise_parameters is not available.
  ElGamalPublicKey composite_el_gamal_public_key = 5;

Why is this called "composite key?" It's just the aggregator's key, right?

---

src/main/proto/wfa/measurement/internal/duchy/protocol/reach_only_liquid_legions_v2_encryption_methods.proto line 157 at r2 (raw file):

  RegisterNoiseGenerationParameters noise_parameters = 8;
  // The mechanism used to generate noise in previous phases.
  LiquidLegionsV2NoiseConfig.NoiseMechanism noise_mechanism = 9;

Why are these two fields being added vs. the aggregator adding it's noise at the end of the setup phase?

[ple13]

Reviewable status: 3 of 16 files reviewed, 4 unresolved discussions (waiting on @kungfucraig, @renjiezh, and @SanjayVas)

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.h line 49 at r2 (raw file):

Previously, renjiezh wrote…

The methods in ProtocolCryptor are intrinsically non-const because the implementation are based on operations from private_join_and_compute, which always change the context_ for randomness. (correct me if I am wrong or not precise.)
So I guess it is not feasible to use const &ProtocolCryptor.

I tried to add the const keyword, then the compiler complained about the mutex_. The context_ will stay the same after it has been initialized, and does not need to be changed. The same context_ is used to generate different randomness.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/liquid_legions_v2_encryption_utility_helper.cc line 25 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: "AnySketch" is treated as two words (e.g. the repo name is any-sketch).

Done.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/liquid_legions_v2_encryption_utility_helper.cc line 33 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This naming implies to me that it's converting to a CMMS public API ElGamalPublicKey, whereas this is converting to a Duchy-internal message.

This is the full ElGamal public key, which is shared across all computation parties (EDPs, Kingdom, Duchies). But you're right that the current usage only involves the duchies (as the function is currently only used in the unit duchy tests).

---

src/main/proto/wfa/measurement/internal/duchy/protocol/reach_only_liquid_legions_v2_encryption_methods.proto line 65 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Why is this called "composite key?" It's just the aggregator's key, right?

This is the combined key from all the duchies (non-aggregator + aggregator). Using the composite key, a non-aggregator can send the pair (crv, noise_ciphertext) to the aggregator in the setup phase without leaking any information.

---

src/main/proto/wfa/measurement/internal/duchy/protocol/reach_only_liquid_legions_v2_encryption_methods.proto line 157 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Why are these two fields being added vs. the aggregator adding it's noise at the end of the setup phase?

The aggregator needs to know the noise_mechanism and the noise_parameters to compute a) the max blind histogram noise (to generate the lookup table) and b) the number of baseline reach noise to subtract it from the register count.

[renjiezh]

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

[SanjayVas]

Reviewed 7 of 16 files at r1, 3 of 5 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kungfucraig and @ple13)

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.h line 49 at r2 (raw file):
You would need to add the mutable specifier to the mutex_ field to indicate that it can be changed even by const methods. Use of this specifier is rare, but a mutex field is one of the few cases where it would be valid.

Mutable is used to specify that the member does not affect the externally visible state of the class (as often used for mutexes, memo caches, lazy evaluation, and access instrumentation).

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.cc line 147 at r3 (raw file):

absl::StatusOr<ElGamalEcPointPair> GetEcPointPairFromString(
    absl::string_view str, int curve_id) {
  std::unique_ptr<Context> context(new Context);

Prefer make_unique over using raw new. See https://abseil.io/tips/126

That said, it looks like the Context doesn't outlive this function scope since its ownership is never passed. You may be able to stack allocate it instead.

Context context;
ASSIGN_OR_RETURN(ECGroup ec_group, ECGroup::Create(curve_id, &context));

Suggestion:

auto context = std::make_unique<Context>();

---

src/main/cc/wfa/measurement/common/crypto/protocol_cryptor.h line 75 at r3 (raw file):

  // Encrypts an integer with the full composite ElGamal Key.
  virtual absl::StatusOr<std::string>
  EncryptIntegerWithCompositElGamalAndWriteToString(int64_t value) = 0;

nit: "Write" seems to imply that one would pass the string to be written to as opposed to returning the string. For consistency with other methods in this class, leave out the "with". Finally, you don't need to indicate the parameter type in the name (though you may wish to leave it in this case for consistency).

Suggestion:

EncryptToStringCompositeElGamal

---

src/main/cc/wfa/measurement/common/crypto/protocol_cryptor.cc line 196 at r3 (raw file):

    ciphertext.replace(0, kBytesPerEcPoint, temp);
    ASSIGN_OR_RETURN(temp, zero_ec.e.ToBytesCompressed());
    ciphertext.replace(kBytesPerEcPoint, kBytesPerEcPoint, temp);

nit: prefer scoped initializer to avoid variable reuse¹. This pattern is generally preferred over the ASSIGN_OR_RETURN macro, but we already use that macro broadly so consider consistency.

Suggestion:

    if (absl::StatusOr<std::string> result = zero_ec.u.ToBytesCompressed(); result.ok()) {
      ciphertext.replace(0, kBytesPerEcPoint, *result);
    else {
      return result.status();
    }
    if (absl::StatusOr<std::string> result = zero_ec.e.ToBytesCompressed(); result.ok()) {
      ciphertext.replace(kBytesPerEcPoint, kBytesPerEcPoint, *result);
    } else {
      return result.status();
    }
    

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/liquid_legions_v2_encryption_utility_helper.cc line 33 at r2 (raw file):

Previously, ple13 (Phi) wrote…

This is the full ElGamal public key, which is shared across all computation parties (EDPs, Kingdom, Duchies). But you're right that the current usage only involves the duchies (as the function is currently only used in the unit duchy tests).

"Duchy" here is referring to the namespace of the type, which parallels the use of "AnySketch" in the other function. You could also go more verbose and use "DuchyInternal" if you think that's clearer.

Footnotes

1. https://abseil.io/tips/181 ↩

[kungfucraig]

Reviewed all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ple13)

---

src/main/proto/wfa/measurement/internal/duchy/protocol/reach_only_liquid_legions_v2_encryption_methods.proto line 157 at r2 (raw file):

Previously, ple13 (Phi) wrote…

The aggregator needs to know the noise_mechanism and the noise_parameters to compute a) the max blind histogram noise (to generate the lookup table) and b) the number of baseline reach noise to subtract it from the register count.

Got it! Thanks.

[kungfucraig]

Reviewed 6 of 16 files at r1, 3 of 5 files at r2, 5 of 5 files at r3.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @ple13 and @SanjayVas)

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.cc line 71 at r3 (raw file):

}

absl::StatusOr<std::vector<std::string>> GetRollv2BlindedRegisterIndexes(

Should this use the multithread helper?

cc: @renjiezh

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.cc line 147 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Prefer make_unique over using raw new. See https://abseil.io/tips/126

That said, it looks like the Context doesn't outlive this function scope since its ownership is never passed. You may be able to stack allocate it instead.

Context context;
ASSIGN_OR_RETURN(ECGroup ec_group, ECGroup::Create(curve_id, &context));

If possible, prefer the stack allocation.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/BUILD.bazel line 12 at r3 (raw file):

_INCLUDE_PREFIX = "/src/main/cc"

cc_library(

Can we mark this target test only?

https://bazel.build/reference/be/common-definitions#common.testonly

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/liquid_legions_v2_encryption_utility_helper.h line 1 at r3 (raw file):

// Copyright 2023 The Cross-Media Measurement Authors

It looks like these functions are only used in tests. Can you rename this file to end with "test_helper"?

@SanjayVas is that the convention you want to use?

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility.cc line 184 at r3 (raw file):

  ASSIGN_OR_RETURN(int64_t noise_registers_count,
                   distributed_noiser.GenerateNoiseComponent());
  // Make sure that there is at least one publisher noise added.

Can you comment in the file as to why this is important?

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility.cc line 573 at r3 (raw file):

        break;
      }
    }

Can we have a check that will throw an error if we do not successfully lookup the value?

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility.cc line 594 at r3 (raw file):

  // there exist publisher noise and padding noise.
  if (request.has_noise_parameters()) {
    non_empty_register_count -= 2;

Weren't there 2 registers per Duchy added? And weren't those factored into the excessive noise count anyway?

---

src/main/proto/wfa/measurement/internal/duchy/protocol/reach_only_liquid_legions_v2_encryption_methods.proto line 67 at r3 (raw file):

  ElGamalPublicKey composite_el_gamal_public_key = 5;
  // The attached encrypted excessive noises. Only for the aggregator.
  bytes serialized_excessive_noise_ciphertext = 6;

Can you mention that there will be one noise element for each non-aggregator worker?

---

src/test/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility_test.cc line 96 at r3 (raw file):

}

// The ReachOnlyTest generates cipher keys for 3 duchies, and the combined

Can you note in a comment which duchy is the aggregator?

---

src/test/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility_test.cc line 253 at r3 (raw file):

                     CompleteReachOnlySetupPhaseAtAggregator(
                         complete_reach_only_setup_phase_request_3));
    EXPECT_THAT(

Should you check something about the excessive noise?

[SanjayVas]

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

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/liquid_legions_v2_encryption_utility_helper.h line 1 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

It looks like these functions are only used in tests. Can you rename this file to end with "test_helper"?

@SanjayVas is that the convention you want to use?

The naming need not have "test" in it so long as it's under a testing Bazel package and the BUILD target has the testonly attribute set to True.

[ple13]

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @kungfucraig, @renjiezh, and @SanjayVas)

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.h line 49 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

You would need to add the mutable specifier to the mutex_ field to indicate that it can be changed even by const methods. Use of this specifier is rare, but a mutex field is one of the few cases where it would be valid.

Mutable is used to specify that the member does not affect the externally visible state of the class (as often used for mutexes, memo caches, lazy evaluation, and access instrumentation).

Thanks. Let me file a bug to refactor the class ProtocolCryptor.

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.cc line 71 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Should this use the multithread helper?

cc: @renjiezh

Yes, this should be done with multithread helper. The function has been updated.

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.cc line 147 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

If possible, prefer the stack allocation.

Replaced it with stack allocation instead of pointer.

---

src/main/cc/wfa/measurement/common/crypto/protocol_cryptor.h line 75 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: "Write" seems to imply that one would pass the string to be written to as opposed to returning the string. For consistency with other methods in this class, leave out the "with". Finally, you don't need to indicate the parameter type in the name (though you may wish to leave it in this case for consistency).

Fixed. I used EncryptIntegerToStringCompositeElGamal to make it explicit that we are encrypting an integer.

---

src/main/cc/wfa/measurement/common/crypto/protocol_cryptor.cc line 196 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: prefer scoped initializer to avoid variable reuse¹. This pattern is generally preferred over the ASSIGN_OR_RETURN macro, but we already use that macro broadly so consider consistency.

Fixed.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/BUILD.bazel line 12 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Can we mark this target test only?

https://bazel.build/reference/be/common-definitions#common.testonly

Done. This has been moved into testing folder and marked with testonly attribute.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/liquid_legions_v2_encryption_utility_helper.h line 1 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The naming need not have "test" in it so long as it's under a testing Bazel package and the BUILD target has the testonly attribute set to True.

The files are now moved into the testing folder and has testonly attribute.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/liquid_legions_v2_encryption_utility_helper.cc line 33 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

"Duchy" here is referring to the namespace of the type, which parallels the use of "AnySketch" in the other function. You could also go more verbose and use "DuchyInternal" if you think that's clearer.

Fixed.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility.cc line 184 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Can you comment in the file as to why this is important?

Done.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility.cc line 573 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Can we have a check that will throw an error if we do not successfully lookup the value?

Done.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility.cc line 594 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Weren't there 2 registers per Duchy added? And weren't those factored into the excessive noise count anyway?

They add noises with the same fixed ID (kPublisherNoiseRegisterId and kPaddingNoiseRegisterId), so the number of unique register IDs won't increase.

---

src/main/proto/wfa/measurement/internal/duchy/protocol/reach_only_liquid_legions_v2_encryption_methods.proto line 67 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Can you mention that there will be one noise element for each non-aggregator worker?

Done.

---

src/test/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility_test.cc line 96 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Can you note in a comment which duchy is the aggregator?

Done.

---

src/test/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility_test.cc line 253 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Should you check something about the excessive noise?

I added the size check for the excessive noise.

Footnotes

1. https://abseil.io/tips/181 ↩

[ple13]

Reviewable status: 3 of 23 files reviewed, 13 unresolved discussions (waiting on @kungfucraig, @renjiezh, and @SanjayVas)

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility.cc line 594 at r3 (raw file):

Previously, ple13 (Phi) wrote…

They add noises with the same fixed ID (kPublisherNoiseRegisterId and kPaddingNoiseRegisterId), so the number of unique register IDs won't increase.

I'm sorry I misread it. Currently we only add noise in the setup phase if the noise_parameters are set. So we check the noise_parameters before subtracting 2. However, I think the second check is not very intuitive and it's better to add 1 additional publisher/padding noise in any case, and just subtract 2 at the end. I'll update the code to reflect this change.

[kungfucraig]

Reviewed 19 of 20 files at r4, all commit messages.
Reviewable status: 22 of 23 files reviewed, 5 unresolved discussions (waiting on @renjiezh and @SanjayVas)

[SanjayVas]

Reviewed 20 of 20 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ple13)

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.h line 31 at r4 (raw file):

using ::wfa::measurement::common::crypto::CompositeType;
using ::wfa::measurement::internal::duchy::protocol::liquid_legions_v2::

A common crypto package shouldn't depend on an internal Duchy package. Move one or the other.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility.cc line 577 at r4 (raw file):

      }
    }
    // Throws an error if the decryption fails.

nit: we never throw anything

Suggestion:

Returns

[ple13]

Reviewable status: 16 of 23 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @SanjayVas)

---

src/main/cc/wfa/measurement/common/crypto/encryption_utility_helper.h line 31 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

A common crypto package shouldn't depend on an internal Duchy package. Move one or the other.

I moved the functions Get[Rollv2]BlindedRegisterIndexes into the [reach_only_]liquid_ligions_v2_encryption_utility.cc respectively.

---

src/main/cc/wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility.cc line 577 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: we never throw anything

Done.

[SanjayVas]

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

[stevenwarejones]

Reviewed 2 of 16 files at r1, 12 of 20 files at r4, 8 of 9 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ple13)

[benjaminkreuter]

This looks OK to me as far as protocol correctness goes.

[benjaminkreuter]

Reviewed 2 of 16 files at r1, 12 of 20 files at r4, 9 of 9 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ple13)

[ple13]

Thanks Ben!

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

[ple13]

Reviewed 1 of 16 files at r1, 9 of 20 files at r4, 8 of 9 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ple13)