Fix bugs in HMSS implementation.

[renjiezh]

• Bypass ShufflePhaseInput blob from SETUP_PHASE to SHUFFLE_PHASE for worker2.
• ShufflePhase correctly sorts RequisitionsList.
• VerifySecretSeed in ShufflePhase correctly interpret secret seed ciphertext.
• Handle PublicApiVersion for secret seed.
• Get non-aggregator ids from protocol config instead of SystemComputation.

[wfa-reviewable]

This change is 

[SanjayVas]

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

a discussion (no related file):
nit: "Fix bugs" is too vague. Be more specific in PR description, even if that just means adding a list of the bugs fixed after the title.

---

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/herald/HonestMajorityShareShuffleStarter.kt line 84 at r1 (raw file):

            val privateKeyHandle = TinkPrivateKeyHandle.generateHpke()
            requireNotNull(privateKeyStore) { "privateKeyStore cannot be null" }

nit: put precondition checks as early as possible. No need to wait until computationDetails is being built.

[stevenwarejones]

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

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: "Fix bugs" is too vague. Be more specific in PR description, even if that just means adding a list of the bugs fixed after the title.

I agree - please list out the defects and the remediations

---

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/herald/Herald.kt line 81 at r1 (raw file):

  private val protocolsSetupConfig: ProtocolsSetupConfig,
  private val clock: Clock,
  private val privateKeyStore: PrivateKeyStore<TinkKeyId, TinkPrivateKeyHandle>? = null,

when is null a valid use case?

---

src/main/proto/wfa/measurement/internal/duchy/protocol/honest_majority_share_shuffle.proto line 155 at r1 (raw file):

    // The list of ids of non-aggregators participating in this computation.
    repeated string non_aggregators = 3;

shouldn't we also have an aggregator field?

[kungfucraig]

Can you describe what is being fixed in the PR description?

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

[renjiezh]

Reviewable status: 10 of 12 files reviewed, 2 unresolved discussions (waiting on @ple13, @SanjayVas, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/herald/Herald.kt line 81 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

when is null a valid use case?

privateKeyStore is used for the encryption keys for secret seed. Aggregator does not need it for now.

---

src/main/proto/wfa/measurement/internal/duchy/protocol/honest_majority_share_shuffle.proto line 155 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

shouldn't we also have an aggregator field?

This non-aggregators field is only used to check whether the aggregator receives both input blob from non-aggregators.

[ple13]

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

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/mill/shareshuffle/HonestMajorityShareShuffleMill.kt line 247 at r1 (raw file):

  private fun aggregatorStub(): ComputationControlCoroutineStub {
    // The last participant is the aggregator.

nit: remove the comment line.

[stevenwarejones]

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

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/herald/Herald.kt line 81 at r1 (raw file):

Previously, renjiezh wrote…

privateKeyStore is used for the encryption keys for secret seed. Aggregator does not need it for now.

in the init, what if we check to see if the duchyId is "aggregator" or soemthing like that and throw an error if its not an aggregator but privateKeyStore is null.

---

src/main/proto/wfa/measurement/internal/duchy/protocol/honest_majority_share_shuffle.proto line 155 at r1 (raw file):

Previously, renjiezh wrote…

This non-aggregators field is only used to check whether the aggregator receives both input blob from non-aggregators.

when would an aggregator receive data from another aggregator?

[renjiezh]

Reviewable status: 10 of 12 files reviewed, 2 unresolved discussions (waiting on @ple13, @SanjayVas, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/herald/Herald.kt line 81 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

in the init, what if we check to see if the duchyId is "aggregator" or soemthing like that and throw an error if its not an aggregator but privateKeyStore is null.

Good idea. I added checks in init {} for both herald and mill to fail it earlier. But to take care of both null and non-null usage, the parameter here will still be nullable.

---

src/main/proto/wfa/measurement/internal/duchy/protocol/honest_majority_share_shuffle.proto line 155 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

when would an aggregator receive data from another aggregator?

Sorry I didn't make it very clear. This field is used for aggregator's WAIT_ON_AGGREGATION_INPUT phase to determine whether it has received all required AggregationPhaseInput from non-aggregators. This field contains two ids of non-aggregators so the aggregator just check them against the input blobs in storage.

[SanjayVas]

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

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/herald/HonestMajorityShareShuffleStarter.kt line 84 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: put precondition checks as early as possible. No need to wait until computationDetails is being built.

This is still happening inside of the DSL builder for computationDetails. I meant to move it to init.

[stevenwarejones]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @renjiezh)

[renjiezh]

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

---

src/main/kotlin/org/wfanet/measurement/duchy/daemon/herald/HonestMajorityShareShuffleStarter.kt line 84 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This is still happening inside of the DSL builder for computationDetails. I meant to move it to init.

It has been done in Herald's init while the starter is just helper functions instead of a real class.