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

Merge main into develop #277

Merged
merged 17 commits into from
Jul 30, 2019
Merged

Merge main into develop #277

merged 17 commits into from
Jul 30, 2019

Conversation

hdevalence
Copy link
Contributor

This updates the develop branch with R1CS code to include the extra validation changes made in the main branch for 1.0.2.

@hdevalence
Copy link
Contributor Author

Ah, looks like this doesn't quite work, because of the lack of error conversions...

This follows the same logic as in the rangeproof case: we don't have an a
priori reason to think that there's a security problem with allowing these
points to be the identity, but this is an area where people make mistakes in
under-specification of assumptions, etc., and there is no valid reason for
these points to be the identity, so this works as a defense-in-depth mechanism.

The extra validation is not applied to the value commitments, in order to allow
commitments to the zero value with zero blinding factor.
In the long term, I think that it would be better to combine R1CSError with
ProofError, if we can do that without breaking stability.  If not, we should
refactor the TranscriptProtocol so that it returns a TranscriptError type that
can be converted into either a ProofError or an R1CSError.
@hdevalence hdevalence force-pushed the merge-main-into-develop branch from b5960b8 to 29fad78 Compare June 4, 2019 22:57
@hdevalence
Copy link
Contributor Author

Updated. There's an issue with the R1CS benchmarks -- it looks like they fell out of sync with the RandomizedConstraintSystem / RandomizableConstraintSystem change.

In the long term, I think that it would be better to combine R1CSError with ProofError, if we can do that without breaking stability. If not, we should refactor the TranscriptProtocol so that it returns a TranscriptError type that can be converted into either a ProofError or an R1CSError. For now I would prefer to keep the "bad conversion" and then remove it later by one of those two means.

@rubdos
Copy link
Contributor

rubdos commented Jun 5, 2019

Have you considered using something like failure for the exported error types? In my experience it aids with three things:

  1. Rapid writing of new errors using bail!() et al., if you're prototyping something new.
  2. Easily returning "generic" errors using the Error struct and ? ...
  3. ... while it still allows to return non-generic errors (if performance might be at stake).

@hdevalence
Copy link
Contributor Author

Yep, we are using failure, but we don't want to use Error because it's not as lightweight as we'd like. I think that the correct fix is to add a TranscriptError type.

@hdevalence
Copy link
Contributor Author

hdevalence commented Jul 24, 2019

We should probably update and merge this branch before merging #280. I think the things left to do here are:

  • Resolve the error-heirarchy issue (probably by creating a new TranscriptError that can be converted to a ProofError or an R1CSError)
  • Fix the R1CS benchmarks

@hdevalence
Copy link
Contributor Author

Fixed the R1CS benchmarks.

@hdevalence
Copy link
Contributor Author

Moved the error item into #286, let's fix it there instead of here.

Copy link
Collaborator

@oleganza oleganza left a comment

Choose a reason for hiding this comment

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

👍

@hdevalence hdevalence merged commit a8d4fc2 into develop Jul 30, 2019
@hdevalence hdevalence deleted the merge-main-into-develop branch July 30, 2019 19:15
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.

3 participants