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

Add register_count into HMSS requisition fulfillment header. #203

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

renjiezh
Copy link
Contributor

@renjiezh renjiezh commented Mar 7, 2024

register_count is calculated by EDPs during sketch generation(All EDP should get the same value based on ProtocolConfig and vid model). So it is not included in ProtocolConfig but provided by RequisitionFulfillmentRequest.

@renjiezh renjiezh requested review from ple13 and kungfucraig March 7, 2024 21:26
@wfa-reviewable
Copy link

This change is Reviewable

@renjiezh renjiezh requested a review from SanjayVas March 7, 2024 21:26
Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

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


src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto line 289 at r1 (raw file):

// Sketch parameters for a Honest Majority Shuffle Based Secret Sharing
// protocol.
message ShareShuffleSketchParams {

Do you think it's worth adding a comment about how the size of the sketch is arrived at? Something like:

"The size of the sketch is calculated by the EDP and is determined by the sampling interval and the size of the population being measured. It is expected that EDPs will calculate the length in exactly the same way. Support libraries are provided for this in the any-sketch repo."

@SanjayVas Wdyt?

Copy link
Contributor

@ple13 ple13 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @renjiezh and @SanjayVas)

@SanjayVas SanjayVas requested a review from kungfucraig March 8, 2024 19:08
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 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kungfucraig and @renjiezh)


src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto line 289 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Do you think it's worth adding a comment about how the size of the sketch is arrived at? Something like:

"The size of the sketch is calculated by the EDP and is determined by the sampling interval and the size of the population being measured. It is expected that EDPs will calculate the length in exactly the same way. Support libraries are provided for this in the any-sketch repo."

@SanjayVas Wdyt?

SGTM with two the slight modifications:

  1. We don't use EDP abbreviation in the API and instead reference the DataProvider resource.
  2. Instead of just mentioning support libraries are available, reference the library. e.g. "See foo::bar::Baz in the any-sketch library"

src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto line 290 at r1 (raw file):

// protocol.
message ShareShuffleSketchParams {
  reserved 1;

You're technically safe to renumber in this case because this message has never been used in released code, but this is also fine.

Copy link
Member

@kungfucraig kungfucraig 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: all files reviewed, 2 unresolved discussions (waiting on @renjiezh and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto line 289 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

SGTM with two the slight modifications:

  1. We don't use EDP abbreviation in the API and instead reference the DataProvider resource.
  2. Instead of just mentioning support libraries are available, reference the library. e.g. "See foo::bar::Baz in the any-sketch library"

Unfortunately said libraries do not exist yet, but maybe we can posit a directory to reference now? Might I suggest: https://github.com/world-federation-of-advertisers/any-sketch-java/tree/main/src/main/java/org/wfanet/hmss

@renjiezh renjiezh force-pushed the renjiez-share-shuffle-sketch-params-update branch from dba470a to ee9b7ed Compare March 12, 2024 20:44
@renjiezh renjiezh force-pushed the renjiez-share-shuffle-sketch-params-update branch from ee9b7ed to 76f682e Compare March 12, 2024 20:46
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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @kungfucraig, @ple13, and @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/protocol_config.proto line 289 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Unfortunately said libraries do not exist yet, but maybe we can posit a directory to reference now? Might I suggest: https://github.com/world-federation-of-advertisers/any-sketch-java/tree/main/src/main/java/org/wfanet/hmss

Comment added on register_count in RequisitionFulfillmentRequest. I did not mention the library. It will be covered in rollout instruction doc.

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@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 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @renjiezh)

@renjiezh renjiezh merged commit bde0777 into main Mar 13, 2024
4 checks passed
@renjiezh renjiezh deleted the renjiez-share-shuffle-sketch-params-update branch March 13, 2024 17:32
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.

6 participants