Circulate direct computation methodology from protocol config to measurement result.

[riemanli]

In this PR, the implementation of the API changes in CMM-API is done. The main changes are

1. new direct protocol config,
2. additional info in measurement results,
3. new enum constants in ProtocolConfig.NoiseMechanism,
4. EDP simulator choose the noise mechanism based on the overlap of its preferences and the noise mechanism options in the direct protocol config.

Any future measurement creations will contain certain direct protocol configs based on the measurement types. For the backward compatibility for existing measurements and requisitions, the empty internal direct protocol configs will be converted with the options that would have been valid at the time the Measurement was created.

EDPs are restricted to use continuous noise mechanisms by setting the validation in V2alphaPublicApiServer. Discrete Gaussian will be supported in the future.

[wfa-reviewable]

This change is 

[riemanli]

Current dependencies on/for this PR:

• main
  • PR Circulate direct computation methodology from protocol config to measurement result. #1154 👈
    • PR Store set operation compilation IDs #1100
      • PR Store measurement computation details in reporting for error bars #1184
      • PR Add internal DP params proto #1107

This comment was auto-generated by Graphite.

[SanjayVas]

Reviewed 4 of 18 files at r1, 2 of 9 files at r2, all commit messages.
Reviewable status: 6 of 21 files reviewed, 3 unresolved discussions (waiting on @renjiezh, @riemanli, and @tristanvuong2021)

---

build/repositories.bzl line 44 at r2 (raw file):

        name = "wfa_measurement_proto",
        repo = "cross-media-measurement-api",
        sha256 = "642106dd7c10b4c8820c31c3c18f54e7a5b9480adc85b9f6e58b267fd8f7a62e",

Add a DO_NOT_SUBMIT comment as a note to use the release version prior to submitting (merging).

---

src/main/kotlin/org/wfanet/measurement/integration/common/InProcessKingdom.kt line 210 at r2 (raw file):

    /** Default deadline for RPCs to internal server in milliseconds. */
    private const val DEFAULT_INTERNAL_DEADLINE_MILLIS = 30_000L
    private val measurementNoiseMechanisms: List<ProtocolConfig.NoiseMechanism> =

Despite not being a compile-time const, this should still use CONST_CASE since it's immutable and has no behavior.

Code quote:

measurementNoiseMechanisms

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 208 at r2 (raw file):

private class V2alphaFlags {
  class DirectNoiseMechanismInput {
    @CommandLine.Option(

This should just be a single option of type List<NoiseMechanism>. Picocli will convert multiple instances of the option with an enum name into the list for you.

[riemanli]

Reviewable status: 3 of 22 files reviewed, 3 unresolved discussions (waiting on @renjiezh, @SanjayVas, and @tristanvuong2021)

---

build/repositories.bzl line 44 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Add a DO_NOT_SUBMIT comment as a note to use the release version prior to submitting (merging).

Thanks for reminding. Done.

---

src/main/kotlin/org/wfanet/measurement/integration/common/InProcessKingdom.kt line 210 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Despite not being a compile-time const, this should still use CONST_CASE since it's immutable and has no behavior.

Done.

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 208 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This should just be a single option of type List<NoiseMechanism>. Picocli will convert multiple instances of the option with an enum name into the list for you.

Based on offline discussion, we decided to use Single Value Validation to exclude invalid enum constants.

[SanjayVas]

Reviewed 1 of 18 files at r1, 3 of 9 files at r2, 5 of 11 files at r3, all commit messages.
Reviewable status: 12 of 22 files reviewed, 10 unresolved discussions (waiting on @renjiezh, @riemanli, and @tristanvuong2021)

---

build/repositories.bzl line 44 at r2 (raw file):

Previously, riemanli (Rieman) wrote…

Thanks for reminding. Done.

Generally we reference the PR in the comment using a similar format as TODOs.

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 214 at r3 (raw file):

    private set

  val directNoiseMechanisms: MutableList<NoiseMechanism> = mutableListOf()

Suggestion:

lateinit var directNoiseMechanisms: List<NoiseMechanism>
  private set

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 216 at r3 (raw file):

  val directNoiseMechanisms: MutableList<NoiseMechanism> = mutableListOf()

  @CommandLine.Spec lateinit var spec: CommandLine.Model.CommandSpec // injected by picocli

Add private set to make sure nothing outside of the flags instance can change it.

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 219 at r3 (raw file):

  @CommandLine.Option(
    names = ["--direct-noise-mechanisms"],

Singular, since the caller should be specifying it multiple times to get multiple values

Suggestion:

--direct-noise-mechanism

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 220 at r3 (raw file):

  @CommandLine.Option(
    names = ["--direct-noise-mechanisms"],
    split = ",",

Don't use split. Make the caller specify it multiple times instead.

Code quote:

    split = ",",

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 252 at r3 (raw file):

      }
    }
    directNoiseMechanisms += noiseMechanisms

Suggestion:

directNoiseMechanisms = noiseMechanisms

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 124 at r3 (raw file):

import org.wfanet.measurement.kingdom.deploy.common.RoLlv2ProtocolConfig

// (-- TODO(world-federation-of-advertisers/cross-media-measurement-api/issues/160): this value

Wrong comment/TODO format for a Kotlin property.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 338 at r3 (raw file):

    if (source.hasDeterministicCountDistinct()) {
      deterministicCountDistinct = ProtocolConfigKt.DirectKt.deterministicCountDistinct {}

Prefer getDefaultInstance() as that returns a singleton.

Suggestion:

ProtocolConfig.Direct.DeterministicCountDistinct.getDefaultInstance()

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/system/v1alpha/ProtoConversions.kt line 397 at r3 (raw file):

    InternalNoiseMechanism.CONTINUOUS_GAUSSIAN,
    InternalNoiseMechanism.NOISE_MECHANISM_UNSPECIFIED,
    ProtocolConfig.NoiseMechanism.NONE,

Suggestion:

InternalNoiseMechanism

[riemanli]

Reviewable status: 8 of 22 files reviewed, 10 unresolved discussions (waiting on @renjiezh, @SanjayVas, and @tristanvuong2021)

---

build/repositories.bzl line 44 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Generally we reference the PR in the comment using a similar format as TODOs.

Do you mean this PR or the PR in the cmm-api?

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 214 at r3 (raw file):

    private set

  val directNoiseMechanisms: MutableList<NoiseMechanism> = mutableListOf()

Done.

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 216 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Add private set to make sure nothing outside of the flags instance can change it.

Done.

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 219 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Singular, since the caller should be specifying it multiple times to get multiple values

Done.

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 220 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Don't use split. Make the caller specify it multiple times instead.

Done.

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 252 at r3 (raw file):

      }
    }
    directNoiseMechanisms += noiseMechanisms

Done.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 124 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Wrong comment/TODO format for a Kotlin property.

Fixed based on the TODO format guide.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 338 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Prefer getDefaultInstance() as that returns a singleton.

Fixed this and the other empty {} DSLs

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/system/v1alpha/ProtoConversions.kt line 397 at r3 (raw file):

    InternalNoiseMechanism.CONTINUOUS_GAUSSIAN,
    InternalNoiseMechanism.NOISE_MECHANISM_UNSPECIFIED,
    ProtocolConfig.NoiseMechanism.NONE,

Done.

[SanjayVas]

Reviewed 4 of 18 files at r1, 3 of 11 files at r3, 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @renjiezh, @riemanli, and @tristanvuong2021)

---

build/repositories.bzl line 44 at r2 (raw file):

Previously, riemanli (Rieman) wrote…

Do you mean this PR or the PR in the cmm-api?

The API PR. Otherwise it's hard to track down where the commit that's referenced came from.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 124 at r3 (raw file):

Previously, riemanli (Rieman) wrote…

Fixed based on the TODO format guide.

It's still incorrect here. This is using the TODO format for a protobuf API definition, not for a Kotlin property. Kotlin properties should be documented using KDoc syntax, and the TODO shouldn't be wrapped in (--/--).

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 240 at r4 (raw file):

            } else {
              // For backward compatibility
              direct = ProtocolConfig.Direct.getDefaultInstance()

This still needs to specify the options that would have been valid at the time the Measurement was created. This means that the noise mechanisms wouldn't be restricted by the Kingdom flags.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 317 at r4 (raw file):

      ProtocolConfig.MeasurementType.IMPRESSION,
      ProtocolConfig.MeasurementType.DURATION -> {
        protocols += protocol { direct = ProtocolConfig.Direct.getDefaultInstance() }

Doesn't this need to use the same logic of reading the source protocol config if it exists (for Measurements created after this change) or else using a default value (for old Measurements)? Otherwise the returned value won't have any methodologies.

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 154 at r4 (raw file):

  private val privacyBudgetManager: PrivacyBudgetManager,
  private val trustedCertificates: Map<ByteString, X509Certificate>,
  private val directNoiseMechanism: DirectNoiseMechanism,

Do we still want to pick the noise mechanism for the simulator? Now that the options are in the protocol config, I think the simulator should pick from the options based on its own order of preference (e.g. preferring gaussian). Keep in mind that the EDP must refuse the requisition if there's no noise mechanism in the protocol config that it supports.

We can still control which mechanism is used in testing by restricting the options in the protocol config.

---

src/test/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/RequisitionsServiceTest.kt line 875 at r4 (raw file):

      ProtocolConfigKt.direct {
        noiseMechanisms += ProtocolConfig.NoiseMechanism.GEOMETRIC
        noiseMechanisms += ProtocolConfig.NoiseMechanism.DISCRETE_GAUSSIAN

nit: prefer to use valid configurations in tests, unless checking for an error. I believe that direct protocol configs wouldn't specify this value

Suggestion:

CONTINUOUS_GAUSSIAN

[riemanli]

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 154 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Do we still want to pick the noise mechanism for the simulator? Now that the options are in the protocol config, I think the simulator should pick from the options based on its own order of preference (e.g. preferring gaussian). Keep in mind that the EDP must refuse the requisition if there's no noise mechanism in the protocol config that it supports.

We can still control which mechanism is used in testing by restricting the options in the protocol config.

Thanks for bringing this aspect. Do you know how noise mechanism selection really works on the EDP side? Do EDPs pick one and only one noise mechanism or do they pick the most preferred and acceptable one from the options?

For the former scenario, directNoiseMechanism in the constructor can indicate that this is the only noise mechanism the EDP wants to use. For the latter, we can change directNoiseMechanism to a list of noise mechanisms in the order of preference.

[riemanli]

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 240 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This still needs to specify the options that would have been valid at the time the Measurement was created. This means that the noise mechanisms wouldn't be restricted by the Kingdom flags.

Thanks for pointing it out. I am still working on the requested change. I found out that the request leads to several default direct configs for different measurement types. I wonder if this is the case that we prefer using object configs. WDYT?

[riemanli]

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

---

build/repositories.bzl line 44 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The API PR. Otherwise it's hard to track down where the commit that's referenced came from.

Done.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 124 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

It's still incorrect here. This is using the TODO format for a protobuf API definition, not for a Kotlin property. Kotlin properties should be documented using KDoc syntax, and the TODO shouldn't be wrapped in (--/--).

Done.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 240 at r4 (raw file):

Previously, riemanli (Rieman) wrote…

Thanks for pointing it out. I am still working on the requested change. I found out that the request leads to several default direct configs for different measurement types. I wonder if this is the case that we prefer using object configs. WDYT?

Made the changes with hardcoded default configs to move forward. Let me know if that's ok.

To accomplish the requested change, I separated measurement type case to handle differently.

cc @renjiezh

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 317 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Doesn't this need to use the same logic of reading the source protocol config if it exists (for Measurements created after this change) or else using a default value (for old Measurements)? Otherwise the returned value won't have any methodologies.

Done.

---

src/test/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/RequisitionsServiceTest.kt line 875 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: prefer to use valid configurations in tests, unless checking for an error. I believe that direct protocol configs wouldn't specify this value

Thanks for pointing it out! Fixed.

[SanjayVas]

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @renjiezh, @riemanli, and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 124 at r3 (raw file):

Previously, riemanli (Rieman) wrote…

Done.

Still not quite correct. All KDoc begins with a summary fragment. See https://developer.android.com/kotlin/style-guide#summary_fragment

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 130 at r5 (raw file):

const val DEFAULT_MAXIMUM_FREQUENCY_DIRECT_DISTRIBUTION = 20

val DEFAULT_DIRECT_NOISE_MECHANISMS: List<NoiseMechanism> =

nit: Public symbols should have KDoc.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 402 at r5 (raw file):

    }
    InternalProtocolConfig.ProtocolCase.DIRECT -> {
      error("Direct protocol shouldn't be used when number of data providers is greater than 1.")

nit: I don't think this error message is quite accurate relative to the nearby code. I don't spot any checks for number of data providers here.

Suggestion:

error("Direct protocol cannot be used for MPC-based Measurements")

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 154 at r4 (raw file):

Previously, riemanli (Rieman) wrote…

Thanks for bringing this aspect. Do you know how noise mechanism selection really works on the EDP side? Do EDPs pick one and only one noise mechanism or do they pick the most preferred and acceptable one from the options?

For the former scenario, directNoiseMechanism in the constructor can indicate that this is the only noise mechanism the EDP wants to use. For the latter, we can change directNoiseMechanism to a list of noise mechanisms in the order of preference.

The EDP must use one of the options defined in the protocol config. If they do not support any of the specified options, they must refuse the Requisition.

Therefore the logical way to implement this would be to check the options in the protocol config and pick from the subset that the EDP supports in some preference order. If none are supported by the EDP, refuse the Requisition.

[riemanli]

Reviewable status: 23 of 27 files reviewed, 7 unresolved discussions (waiting on @iverson52000, @renjiezh, @SanjayVas, and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/api/v2alpha/ProtoConversions.kt line 356 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

What did we decide on w.r.t. population methodology? I assume there's really only one methodology since we just return the count of the specified sub-population.

It's deterministic count. Added.

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 1158 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

If the only part of the Requisition we use is the Direct protocol config, pass that as the param instead.

Done by using a data class to wrap the direct protocol config and the selected noise mechanism.

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 1217 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Use getDefaultInstance() rather than empty DSL builder for empty messages.

Done.

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 1405 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: This is okay, but don't worry about supporting arbitrary preference ordering. It would be fine to hard-code the preference order implicitly into the fulfillment logic:

• If mechanisms contains CONTINUOUS_LAPLACE, use that
• Else if mechanisms contains CONTINUOUS_GAUSSIAN, use that
• Else refuse

Would like to keep the current code if it's ok. The if-else would have many branches when taking ACDP into consideration and when options get more.

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 1410 at r7 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

More accurate comment "The direct noise mechanisms for ACDP composition in PBM for for reach and frequency in order of preference. Currently ACDP composition only supports CONTINUOUS_GAUSSIAN noise for direct measurements"

Thanks for the suggestion! Done.

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 1429 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: use @return tag for this

See https://developer.android.com/kotlin/](https://developer.android.com/kotlin/style-guide#block_tags

Done.

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 1431 at r7 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

It looks like this is called in 4 different places? We should just be able to convert once and pass it along.

Note that we technically only need to convert when calling the Noiser. I leave it to you to consider the tradeoffs of converting early (benefit is that we're already restricted to valid mechanisms for Direct protocol, with the downside that we'd may need to pass along the converted versions along with the API messages they come from).

Note that if the API gives us an invalid mechanism for the protocol, the Requisition should be refused with invalid spec since the API documentation specifically indicates that it's an error.

I converted the noise mechanism up front as suggested.

---

src/test/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulatorTest.kt line 1191 at r7 (raw file):

The options provided by Kingdom should always be [CONTINUOUS_LAPLACE, CONTINUOUS_GAUSSIAN]?

I am not sure if that's a true statement. For an EDP, it should not assume what the options are, right? The code now selects the noise mechanism based on measurement types and composition method. It throw exceptions when no valid options.

Renamed the test name to fit the test better if that solves the issue.

[iverson52000]

Reviewable status: 23 of 27 files reviewed, 7 unresolved discussions (waiting on @renjiezh, @riemanli, @SanjayVas, and @tristanvuong2021)

---

src/test/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulatorTest.kt line 1191 at r7 (raw file):

Previously, riemanli (Rieman) wrote…

The options provided by Kingdom should always be [CONTINUOUS_LAPLACE, CONTINUOUS_GAUSSIAN]?

I am not sure if that's a true statement. For an EDP, it should not assume what the options are, right? The code now selects the noise mechanism based on measurement types and composition method. It throw exceptions when no valid options.

Renamed the test name to fit the test better if that solves the issue.

Got it. Make sense. The directNoiseMechanism here could be misleading that it looks like the preferred noiseMechanism selected by EDP but it's not. Maybe rephrase it to "refuses Requisition when directNoiseMechanism option provided by Kingdom is not CONTINUOUS_GAUSSIAN and compositionMechanism is ACDP"?

[SanjayVas]

Reviewed 2 of 12 files at r7, 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @renjiezh, @riemanli, and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 145 at r8 (raw file):

)

private class RequisitionRefusalException(

This should ideally still be private to EdpSimulator since it's specific to that class. You may be able to move it to the companion object and put any functions that need to use it but not access instance state there.

See related comment on the extension function.

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 1479 at r8 (raw file):

 *
 * @return [DirectNoiseMechanism] when there is a matched.
 * @throws [RequisitionRefusalException] with SPEC_INVALID when no matched noise mechanism.

This shouldn't be thrown by an extension method that's not specific to the EdpSimulator class.

Either

1. Move this into the companion object of EdpSimulator so it's clear it's specific to the class (as opposed to any other type that might end up defined in this file).
2. Go back to returning null, and make sure the calling code throws the exception if the returned value is null. e.g.

   val directNoiseMechanism: DirectNoiseMechanism = noiseMechanism.toDirectNoiseMechanism() ?: throw RequisitionRefusalException(...)

[riemanli]

Reviewable status: 25 of 27 files reviewed, 4 unresolved discussions (waiting on @iverson52000, @renjiezh, @SanjayVas, and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 145 at r8 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This should ideally still be private to EdpSimulator since it's specific to that class. You may be able to move it to the companion object and put any functions that need to use it but not access instance state there.

See related comment on the extension function.

Thanks for the suggestions. Moved it back.

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 1479 at r8 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This shouldn't be thrown by an extension method that's not specific to the EdpSimulator class.

Either

1. Move this into the companion object of EdpSimulator so it's clear it's specific to the class (as opposed to any other type that might end up defined in this file).
2. Go back to returning null, and make sure the calling code throws the exception if the returned value is null. e.g.

   val directNoiseMechanism: DirectNoiseMechanism = noiseMechanism.toDirectNoiseMechanism() ?: throw RequisitionRefusalException(...)

Did a variant of (2). The conversion method returns a nullable DirectNoiseMechanism. Since the noise mechanism selection process would throw an exception when there is no matched, I simply filtered out null from the options. Please see the changes.

---

src/test/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulatorTest.kt line 1191 at r7 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

Got it. Make sense. The directNoiseMechanism here could be misleading that it looks like the preferred noiseMechanism selected by EDP but it's not. Maybe rephrase it to "refuses Requisition when directNoiseMechanism option provided by Kingdom is not CONTINUOUS_GAUSSIAN and compositionMechanism is ACDP"?

SGTM. Done.

[SanjayVas]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @iverson52000, @renjiezh, and @tristanvuong2021)

[iverson52000]

Reviewed 2 of 18 files at r1, 4 of 9 files at r2, 2 of 11 files at r3, 3 of 7 files at r4, 1 of 4 files at r5, 5 of 12 files at r7, 2 of 4 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @renjiezh, @SanjayVas, @stevenwarejones, and @tristanvuong2021)

[stevenwarejones]

Reviewed 1 of 18 files at r1, 4 of 9 files at r2, 1 of 11 files at r3, 2 of 7 files at r4, 1 of 8 files at r6, 8 of 12 files at r7, 1 of 4 files at r8, 1 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @renjiezh, @riemanli, @SanjayVas, and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 237 at r9 (raw file):

        NoiseMechanism.CONTINUOUS_GAUSSIAN -> {}
        NoiseMechanism.GEOMETRIC,
        NoiseMechanism.DISCRETE_GAUSSIAN -> {

why are we also not wanting to allow these?

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/system/v1alpha/ProtoConversions.kt line 394 at r9 (raw file):

    InternalNoiseMechanism.DISCRETE_GAUSSIAN -> NoiseMechanism.DISCRETE_GAUSSIAN
    InternalNoiseMechanism.CONTINUOUS_LAPLACE,
    InternalNoiseMechanism.CONTINUOUS_GAUSSIAN,

why are these invalid?

[riemanli]

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

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/system/v1alpha/ProtoConversions.kt line 394 at r9 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

why are these invalid?

Good question. The code before this change only used discrete types of noises as shown in

cross-media-measurement/src/main/proto/wfa/measurement/system/v1alpha/computation.proto

Lines 110 to 117 in 60dea18

	enum NoiseMechanism {
	// Default value if state is omitted.
	NOISE_MECHANISM_UNSPECIFIED = 0;
	// Geometric.
	GEOMETRIC = 1;
	// Discrete Gaussian.
	DISCRETE_GAUSSIAN = 2;
	}

In order not to change the behavior by adding new types, the new types should be treated as errors.

cc @renjiezh

[riemanli]

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

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 237 at r9 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

why are we also not wanting to allow these?

Discrete types of noise are not supported for direct measurement.

[stevenwarejones]

Reviewed 3 of 18 files at r1, 1 of 11 files at r3, 1 of 7 files at r4, 1 of 4 files at r5, 1 of 4 files at r8, 1 of 2 files at r9.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jiayu-google, @renjiezh, @SanjayVas, and @tristanvuong2021)

[stevenwarejones]

Reviewed 7 of 7 files at r10.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jiayu-google, @renjiezh, @riemanli, @SanjayVas, and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 237 at r9 (raw file):

Previously, riemanli (Rieman) wrote…

Discrete types of noise are not supported for direct measurement.

as discussed, maybe add a note to support discrete later

[riemanli]

Reviewable status: 26 of 27 files reviewed, 1 unresolved discussion (waiting on @iverson52000, @jiayu-google, @renjiezh, @SanjayVas, @stevenwarejones, and @tristanvuong2021)

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/common/server/V2alphaPublicApiServer.kt line 237 at r9 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

as discussed, maybe add a note to support discrete later

Done

[riemanli]

Reviewed 4 of 18 files at r1, 4 of 9 files at r2, 2 of 11 files at r3, 1 of 7 files at r4, 1 of 8 files at r6, 5 of 12 files at r7, 1 of 4 files at r8, 1 of 2 files at r9, 7 of 7 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiayu-google, @renjiezh, @SanjayVas, and @tristanvuong2021)