Adding the class ReachOnlyLiquidLegionsV2Mill on the Duchy mill.

[ple13]

No description provided.

[wfa-reviewable]

This change is 

[SanjayVas]

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

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 217 at r2 (raw file):

    val request =
      SetParticipantRequisitionParamsRequest.newBuilder()

Use Kotlin DSL rather than Java builders for protobufs in new code.

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 455 at r2 (raw file):

  }

  private suspend fun completeReachOnlySetupPhaseAtAggregator(

This Mill is exclusively for RO LLv2. I don't think we need to specify this in method names.

Code quote:

ReachOnly

[renjiezh]

Reviewed 1 of 22 files at r1, all commit messages.
Reviewable status: 18 of 22 files reviewed, 2 unresolved discussions (waiting on @kungfucraig, @ple13, and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 455 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This Mill is exclusively for RO LLv2. I don't think we need to specify this in method names.

It will be helpful to use global text replacement to refactor the function names as the IDEA's refactor won't help for both Kotlin and c++ code at once.

[renjiezh]

Reviewed 14 of 22 files at r1, 7 of 7 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @kungfucraig, @ple13, and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 568 at r2 (raw file):

            }
          }
          liquidLegionsParameters = liquidLegionsSketchParameters {

nit: Could you help to rename this field to sketchParameters.

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2MillTest.kt line 1909 at r2 (raw file):

  }
}

Could we have a unit test to exercise the parsing of excessive_noise?

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/crypto/BUILD.bazel line 0 at r2 (raw file):
Does it make sense to have a separated folder for rollv2's mill test like .../daemon/mill/reachonlyliquidlegionsv2

[kungfucraig]

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

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 460 at r2 (raw file):

    val rollv2Details = token.computationDetails.reachOnlyLiquidLegionsV2
    require(AGGREGATOR == rollv2Details.role) { "invalid role for this function." }
    val (bytes, nextToken) =

What are the types of these two things? And what do they represent? I'm having a hard time understanding how we're unpacking a tuple out of the CRV, so I assume I'm missing something important. :)

---

src/main/swig/protocol/reachonlyliquidlegionsv2/reach_only_liquid_legions_v2_encryption_utility.swig line 19 at r2 (raw file):

#include "wfa/measurement/internal/duchy/protocol/liquid_legions_v2/reach_only_liquid_legions_v2_encryption_utility_wrapper.h"
%}

Can we factor our and include conversions from: https://sourcegraph.com/github.com/world-federation-of-advertisers/cross-media-measurement/-/blob/src/main/swig/protocol/liquidlegionsv2/liquid_legions_v2_encryption_utility.swig

If so, it'd be nice if the factoring out were in a different PR.

[renjiezh]

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

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 460 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

What are the types of these two things? And what do they represent? I'm having a hard time understanding how we're unpacking a tuple out of the CRV, so I assume I'm missing something important. :)

It is the trick of existingOutputOr returning the tuple of bytestring and token. Function existingOutputOr takes a token and a block. It execute the block to get the result of bytestring, than update the token with the result. Thus it returns bytestring and new_token as a pair.
The CRV operations only produce the bytestring though.

[renjiezh]

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

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 460 at r2 (raw file):

Previously, renjiezh wrote…

It is the trick of existingOutputOr returning the tuple of bytestring and token. Function existingOutputOr takes a token and a block. It execute the block to get the result of bytestring, than update the token with the result. Thus it returns bytestring and new_token as a pair.
The CRV operations only produce the bytestring though.

The name existingOutput is because that, it checks if the result is already there. Otherwise it execute the block to get the result. Yeah, the name does not imply the return type which causes the confusion.

[ple13]

Reviewable status: 18 of 22 files reviewed, 7 unresolved discussions (waiting on @kungfucraig, @renjiezh, and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 217 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Use Kotlin DSL rather than Java builders for protobufs in new code.

Fixed.

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 455 at r2 (raw file):

Previously, renjiezh wrote…

It will be helpful to use global text replacement to refactor the function names as the IDEA's refactor won't help for both Kotlin and c++ code at once.

Fixed.

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 460 at r2 (raw file):

Previously, renjiezh wrote…

The name existingOutput is because that, it checks if the result is already there. Otherwise it execute the block to get the result. Yeah, the name does not imply the return type which causes the confusion.

Thanks Renjie for the explanation!

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 568 at r2 (raw file):

Previously, renjiezh wrote…

nit: Could you help to rename this field to sketchParameters.

This change required modifying the c++ code and many other places. I think it's better to have a separate PR that changes it and to do other clean up as well.

---

src/main/swig/protocol/reachonlyliquidlegionsv2/reach_only_liquid_legions_v2_encryption_utility.swig line 19 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Can we factor our and include conversions from: https://sourcegraph.com/github.com/world-federation-of-advertisers/cross-media-measurement/-/blob/src/main/swig/protocol/liquidlegionsv2/liquid_legions_v2_encryption_utility.swig

If so, it'd be nice if the factoring out were in a different PR.

I'll check if it's possible.

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2MillTest.kt line 1909 at r2 (raw file):

Previously, renjiezh wrote…

Could we have a unit test to exercise the parsing of excessive_noise?

There are tests for the excessive noise in the PR. The only test is to check that the size of the string reading from input blob is at least 66 bytes (as its length is at least the length of ciphertext (66) plus length of crv (at least 0). The correctness of the parsing is tested with the crypto code, and also with the JNI tests.

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/crypto/BUILD.bazel line at r2 (raw file):

Previously, renjiezh wrote…

Does it make sense to have a separated folder for rollv2's mill test like .../daemon/mill/reachonlyliquidlegionsv2

The code didn't compile when I had them in the same folder. I'm not sure if there is some way around it, but putting them in a separate folder makes it compilable.

[SanjayVas]

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

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2MillTest.kt line 1 at r3 (raw file):

// Copyright 2023 The Cross-Media Measurement Authors

I'd prefer if this hadn't just blindly copied the existing R/F LLv2 Mill test as it has a lot of cruft, but I understand the desire for expediency.

[renjiezh]

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

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/crypto/ReachOnlyLiquidLegionsV2EncryptionUtilityTest.kt line 272 at r3 (raw file):

            combineElGamalPublicKeysRequest {
                curveId = CURVE_ID
                elGamalKeys.add(DUCHY_1_EL_GAMAL_KEYS.publicKey.toAnySketchElGamalPublicKey())

nit: elGamalKeys += DUCHY_1_EL_GAMAL_KEYS.publicKey.toAnySketchElGamalPublicKey()
We try to use += instead of .add()

[kungfucraig]

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

---

src/main/swig/protocol/reachonlyliquidlegionsv2/reach_only_liquid_legions_v2_encryption_utility.swig line 19 at r2 (raw file):

Previously, ple13 (Phi) wrote…

I'll check if it's possible.

We can come back and do this in a follow-up.

[kungfucraig]

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

[ple13]

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

---

src/main/swig/protocol/reachonlyliquidlegionsv2/reach_only_liquid_legions_v2_encryption_utility.swig line 19 at r2 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

We can come back and do this in a follow-up.

Sure. I will have a separate PR to clean this and other stuff up.

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2MillTest.kt line 1 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I'd prefer if this hadn't just blindly copied the existing R/F LLv2 Mill test as it has a lot of cruft, but I understand the desire for expediency.

Yeah. It is faster this way.

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/crypto/ReachOnlyLiquidLegionsV2EncryptionUtilityTest.kt line 272 at r3 (raw file):

Previously, renjiezh wrote…

nit: elGamalKeys += DUCHY_1_EL_GAMAL_KEYS.publicKey.toAnySketchElGamalPublicKey()
We try to use += instead of .add()

Fixed.

[stevenwarejones]

Reviewed 12 of 22 files at r1, 2 of 7 files at r2, 1 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ple13, @renjiezh, and @SanjayVas)

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2MillTest.kt line 1 at r3 (raw file):

Previously, ple13 (Phi) wrote…

Yeah. It is faster this way.

how much work would it be to make it an abstract test so we only have to refactor this one time in the future?

[ple13]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2MillTest.kt line 1 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

how much work would it be to make it an abstract test so we only have to refactor this one time in the future?

I think I could make it work. However, as I'm very new to kotlin, it will probably take some time. I think the optimal way is to merge this PR, so that Renjie can submit his integration test and run the benchmarking. I'll have a separate PR to clean this up. What do you think?

[stevenwarejones]

Reviewed 2 of 22 files at r1, 1 of 7 files at r2, 1 of 4 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ple13 and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 444 at r4 (raw file):

  /** Processes computation in the confirmation phase */
  private suspend fun confirmationPhase(token: ComputationToken): ComputationToken {

I'd also like refactoring of the common pieces out of this and LiquidLegionsV2Mill as a follow-up

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2MillTest.kt line 1 at r3 (raw file):

Previously, ple13 (Phi) wrote…

I think I could make it work. However, as I'm very new to kotlin, it will probably take some time. I think the optimal way is to merge this PR, so that Renjie can submit his integration test and run the benchmarking. I'll have a separate PR to clean this up. What do you think?

sgtm

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/crypto/JniReachOnlyLiquidLegionsV2EncryptionTest.kt line 33 at r4 (raw file):

          .completeReachOnlySetupPhase(CompleteReachOnlySetupPhaseRequest.getDefaultInstance())
      }
    assertThat(e1.message).contains("ECGroup::CreateGroup() - Could not create group.")

I think the practive we've been adopting is to only check the error type (ie RunTime) and not the message content. fyi @SanjayVas

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/crypto/JniReachOnlyLiquidLegionsV2EncryptionTest.kt line 41 at r4 (raw file):

          .combineElGamalPublicKeys(CombineElGamalPublicKeysRequest.getDefaultInstance())
      }
    assertThat(e2.message).contains("Keys cannot be empty")

ditto

[ple13]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2Mill.kt line 444 at r4 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

I'd also like refactoring of the common pieces out of this and LiquidLegionsV2Mill as a follow-up

Noted.

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/ReachOnlyLiquidLegionsV2MillTest.kt line 1 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

sgtm

Thanks.

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/crypto/JniReachOnlyLiquidLegionsV2EncryptionTest.kt line 33 at r4 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

I think the practive we've been adopting is to only check the error type (ie RunTime) and not the message content. fyi @SanjayVas

I asked @renjiezh about this and other similar tests. The reason is to verify that the exception originated from the JNI and checking the content helps.

---

src/test/kotlin/org/wfanet/measurement/duchy/daemon/mill/liquidlegionsv2/crypto/JniReachOnlyLiquidLegionsV2EncryptionTest.kt line 41 at r4 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

ditto

Noted.