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

Use hash_to_curve to derive global parameters #251

Merged
merged 44 commits into from
Jul 15, 2021

Conversation

howardwu
Copy link
Contributor

@howardwu howardwu commented Jul 10, 2021

Motivation

Use hash_to_curve to derive global parameters.

Pedersen CRH (& compressed variant) updates:

  • Removes usage of an RNG from the setup method in favor of hash_to_curve
  • Updates group trait to ProjectiveCurve

Bowe Hopwood Pedersen CRH updates:

  • Removes usage of an RNG from the setup method in favor of hash_to_curve
  • Reduces in-memory parameter size by 50%

Pedersen commitment (& compressed variant) updates:

  • Removes usage of an RNG from the setup method in favor of hash_to_curve
  • Updates group trait to ProjectiveCurve
  • Reduces serialized commitment parameter size by 50%

Schnorr signature updates:

Group encryption updates:

  • Removes usage of an RNG from the setup method in favor of hash_to_curve
  • Removes SG signature group and SignatureScheme implementation/variant
  • Adds CryptoRng trait to methods where Rng is used

DPC changes:

  • Extracts account commitment, encryption, and signature parameters out of DPC logic internals
  • Accounts now utilize FromBytes(ToBytes()) to convert between encryption key and signature key

Parameter changes:

  • Removes account commitment, account encryption, account signature, record commitment, inner circuit ID CRH, local data commitment, local data CRH, and serial number nonce CRH parameters files and RNG-based generation scripts

Related PRs

Builds on #241

@howardwu howardwu changed the base branch from master to testnet2 July 10, 2021 02:03
@howardwu howardwu added the staging Staging label Jul 10, 2021
@howardwu howardwu changed the base branch from testnet2 to feat/hash_to_curve July 12, 2021 06:51
Base automatically changed from feat/hash_to_curve to testnet2 July 14, 2021 04:51
@howardwu
Copy link
Contributor Author

I have ignored those single integration tests for testnet1 and 2 given the other integration tests will synthesize the circuits from scratch and verify transaction generation works. We can resample these parameters in another PR.

@weikengchen
Copy link
Contributor

Got it. I am going through the files (40% done). Will finish soon.

@howardwu
Copy link
Contributor Author

Great, I'm debugging the Merkle tree gadget at the moment, will keep file changes minimal!

Copy link
Contributor

@weikengchen weikengchen left a comment

Choose a reason for hiding this comment

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

I went through all the files. Everything looks good, though I haven't yet found why the Merkle tree test is failing.

@howardwu
Copy link
Contributor Author

Same, can't seem to isolate the issue. The fact that the Pedersen compressed CRH Merkle Tree variant passes, yet the Pedersen CRH MT variant fails seems odd.

The compressed CRH calls the standard CRH as a subroutine and invokes to_affine() after. Given the failure is that the X Coordinate Conditional Equality/conditionally_equals fails, could it be that the comparison is comparing two different representations that technically are equivalent (e.g. unreduced form)?

@weikengchen
Copy link
Contributor

I think I know why. Two ProjectiveCurve elements, even if they are the same, may not be "Eq".

@weikengchen
Copy link
Contributor

weikengchen commented Jul 15, 2021

Unconfirmed. But see this code:

            let previous_is_left = AllocatedBit::alloc(&mut cs.ns(|| format!("previous_is_left_{}", i)), || {
                Ok(&previous_hash == left_hash)
            })?
            .into();

If two ProjectiveCurve "gadgets" are not Eq even if they are the same, then it would always target the wrong root path.

@weikengchen
Copy link
Contributor

Wait nope, that is not right. They should be affine gadgets...let me think about it.

@howardwu
Copy link
Contributor Author

I just checked it does seem to be AffineGadgets.

Related, but unrelated, shouldn't this be a ProjectiveGadget for performance?

@weikengchen
Copy link
Contributor

Nope. Inside curves, Affine is better.

@weikengchen
Copy link
Contributor

But I would say previous_hash == left_hash does seem to be dangerous.

@howardwu
Copy link
Contributor Author

I definitely want to get to the bottom of this issue, however want to propose one way to remedy this issue.

Ray has PR #249 which we could rebase to with a more generalized and refreshed implementation of Merkle tree.

@weikengchen
Copy link
Contributor

I agree. So merge this to testnet2 first and then see if #249 can fix it?

@howardwu
Copy link
Contributor Author

I think that works.

By the way, your suspicion is correct. If I add the following println!:

            println!("{}: {} {}", i, previous_hash == *left_hash, previous_hash == *right_hash);

The output looks as follows:

0: true false
1: false false
2: false false
3: false false

@weikengchen
Copy link
Contributor

Actually let me have a try on this:

impl<P: ShortWeierstrassParameters, F: PrimeField, FG: FieldGadget<P::BaseField, F>> AllocGadget<SWProjective<P>, F>
    for AffineGadget<P, F, FG>

I have concerns that the implementation is not correct.

@weikengchen
Copy link
Contributor

Ah, nope, in our cases I would be Edwards curve. Let me check a little bit.

@weikengchen
Copy link
Contributor

I think we can narrow it down: either the parameters are not actually the same or there is an issue in allocating the tree path. The second is very likely because it explains why the compressed version passes.

@howardwu
Copy link
Contributor Author

Agreed, it should be from a lower level of abstraction than Merkle Tree gadget given other variants of this test are passing.

In the meantime, I'll follow the proposed course of merging this into testnet2 and migrating to #249 and testing the version there.

@weikengchen
Copy link
Contributor

weikengchen commented Jul 15, 2021

Ok. I think this might be the explanation:

In the native version, when computing MIDDLE nodes in the Merkle tree, we to_bytes previous layers' hashes, which are projective curve elements. In twisted Edwards, this means that we to_bytes [x, y, t, z].

In the constraint version, it to_bytes affine curve elements, which is only [x, y].

We need to change the native version.

@howardwu howardwu merged commit d32f62d into testnet2 Jul 15, 2021
@howardwu howardwu deleted the feat/hash_to_parameters branch July 15, 2021 01:58
@howardwu howardwu restored the feat/hash_to_parameters branch July 15, 2021 01:59
@howardwu
Copy link
Contributor Author

That sounds like the culprit! It would also explain why the compressed gadget variant passes. Given we reduce to affine and use the x-coordinate on it's own, this issue wouldn't arise then.

@weikengchen
Copy link
Contributor

weikengchen commented Jul 15, 2021

The fix would be easy: just change the output type, in the native world, if of Bowe-Hopwood (uncompressed) CRH and Pedersen (uncompressed) CRH from G to G::Affine. And then change the corresponding code to get the compilation passing.

@weikengchen
Copy link
Contributor

Should I make and push a fix to testnet2, or you will fix it?

@howardwu
Copy link
Contributor Author

That sounds like a good idea, feel free to make the fix and push to testnet2!

@howardwu howardwu deleted the feat/hash_to_parameters branch January 15, 2022 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
staging Staging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants