Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update SetParticipantParams for HMSS protocol #1470

Merged
merged 31 commits into from
Feb 16, 2024
Merged

Conversation

renjiezh
Copy link
Contributor

No description provided.

@wfa-reviewable
Copy link

This change is Reviewable

@renjiezh renjiezh force-pushed the renjiez-hmss-system-api branch from e365562 to b7a4eda Compare February 13, 2024 00:57
@renjiezh renjiezh marked this pull request as ready for review February 13, 2024 05:45
@renjiezh renjiezh requested a review from SanjayVas February 13, 2024 15:49
Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

    private set

  /** A set of external duchy ids that the first one must correspond to the aggregator. */

nit: Phrasing

Suggestion:

Set of external IDs of required Duchies, where the first entry must correspond to the Duchy in the aggregator role.

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateMeasurements.kt line 185 at r2 (raw file):

      }
      ProtocolConfig.ProtocolCase.HONEST_MAJORITY_SHARE_SHUFFLE -> {
        // For each EDP, insert a Requisitions for each non-aggregator.

nit

Suggestion:

For each EDP, insert a Requisition for each non-aggregator Duchy.

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/ComputationParticipantsServiceTest.kt line 513 at r2 (raw file):

    val nonUpdatedMeasurement =
      measurementsService.getMeasurementByComputationId(
        GetMeasurementByComputationIdRequest.newBuilder()

Use Kotlin DSL builder

Code quote:

GetMeasurementByComputationIdRequest.newBuilder()

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/Population.kt line 347 at r2 (raw file):

        measurementSpecSignatureAlgorithmOid = "2.9999"
        duchyProtocolConfig = duchyProtocolConfig {
          liquidLegionsV2 = DuchyProtocolConfigKt.liquidLegionsV2 {}

Why does this have anything set for LLv2?


src/main/proto/wfa/measurement/system/v1alpha/computation_participant.proto line 111 at r2 (raw file):

      //
      // If not specified, this is assumed to be the signature algorithm of the
      // accompanying certificate

We only had this for backwards-compatibility for existing fields. Since this is a new protocol, this can be required instead.

Code quote:

      // If not specified, this is assumed to be the signature algorithm of the
      // accompanying certificate

Copy link
Contributor Author

@renjiezh renjiezh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 45 of 50 files reviewed, 3 unresolved discussions (waiting on @SanjayVas)


src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/ComputationParticipantsServiceTest.kt line 513 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Use Kotlin DSL builder

Done.


src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/Population.kt line 347 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Why does this have anything set for LLv2?

Done.
Nice catch. Deleted.


src/main/proto/wfa/measurement/system/v1alpha/computation_participant.proto line 111 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

We only had this for backwards-compatibility for existing fields. Since this is a new protocol, this can be required instead.

Done.

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/ComputationParticipantsServiceTest.kt line 80 at r3 (raw file):

private val TINK_PUBLIC_KEY_SIGNATURE =
  ByteString.copyFromUtf8("This is an Tink Public Key signature.")
private val TINK_PUBLIC_KEY_SIGNATURE_ALGORTIHM_OID = "4.99"

nit: use a valid registered OID. 2.9999 is an OID specifically for examples.

Suggestion:

2.9999

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/Population.kt line 347 at r3 (raw file):

        measurementSpecSignatureAlgorithmOid = "2.9999"
        protocolConfig = protocolConfig {
          honestMajorityShareShuffle = ProtocolConfigKt.honestMajorityShareShuffle {}

nit: Prefer getDefaultInstance

Suggestion:

ProtocolConfig.HonestMajorityShareShuffle.getDefaultInstance()

src/main/proto/wfa/measurement/system/v1alpha/computation_participant.proto line 111 at r2 (raw file):

Previously, renjiezh wrote…

Done.

Add the REQUIRED field behavior annotation too.

Copy link
Contributor Author

@renjiezh renjiezh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 47 of 50 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


src/main/proto/wfa/measurement/system/v1alpha/computation_participant.proto line 111 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Add the REQUIRED field behavior annotation too.

Done.

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 45 of 50 files at r2, 2 of 5 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @renjiezh)

@renjiezh renjiezh enabled auto-merge (squash) February 16, 2024 18:41
@renjiezh renjiezh merged commit 321b373 into main Feb 16, 2024
4 checks passed
@renjiezh renjiezh deleted the renjiez-hmss-system-api branch February 16, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants