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

chore: misc comments for PR83 #86

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

themighty1
Copy link
Collaborator

This PR adds comments to clarify:

  • we are proving the authenticity of the circuit output
  • other minor points

Comment on lines +338 to +339
/// This trait provides methods for the evaluator to prove the authenticity of the evaluated garbled
/// circuit's output.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These traits are meant to be protocol generic, ie no mention of garbled circuits here. (I know its in the garble crate, but these will be moving)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. Can we keep the garbling terminology now and when we move, we will adapt the wording to be generic?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't agree we should do that, the traits are already generic and I don't think coupling the trait docs to DEAP semantics helps any more than just understanding how DEAP implements them

async fn verify(
&mut self,
values: &[ValueRef],
expected_values: &[Value],
) -> Result<(), VerifyError>;
}

/// This trait provides methods for decoding values.
/// This trait provides methods for decoding output values.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These don't have to be output values, input values can be decoded as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't think of any concrete scenarios where there would be a need to decode input values. I think we should try to keep the comments less abstract where possible (it will cause less abstraction fatigue for the reader). Later, when there is a real use-case for using these traits on input values, we can modify the comments accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We do it in @th4s latest PR to prove substrings of plaintext

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, ok, thx, goot to know. then i'll revisit this PR in light of that PR later on.

@@ -332,7 +327,7 @@ impl DEAP {
Ok(())
}

/// Sends a commitment to the provided values, proving them to the follower upon finalization.
/// Sends a commitment to the provided output values, deferring the actual proving until finalization.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Sends a commitment to the provided output values, deferring the actual proving until finalization.
/// Sends a commitment to the provided values, deferring the actual proving until finalization.

@@ -354,7 +349,7 @@ impl DEAP {
Ok(())
}

/// Receives a commitment to the provided values, and stores it until finalization.
/// Receives a commitment to the provided output values, and stores it until finalization.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Receives a commitment to the provided output values, and stores it until finalization.
/// Receives a commitment to the provided values, and stores it until finalization.

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.

2 participants