arkworks-rs / snark

Interfaces for Relations and SNARKs for these relations
https://www.arkworks.rs
Apache License 2.0
776 stars 209 forks source link

Improve curve naming and documentation #213

Closed Pratyush closed 4 years ago

Pratyush commented 4 years ago

This PR renames some modules relating to the concrete curves that are implemented in zexe.

This is a breaking change, but the change can be 99% automated away via the following script:

grep -rli 'edwards_bls12' * | xargs -i@ sed -i 's/edwards_bls12/ed_on_bls12_377/g' @
grep -rli 'jubjub' * | xargs -i@ sed -i 's/jubjub/ed_on_bls12_381/g' @
grep -rli 'edwards_sw6' * | xargs -i@ sed -i 's/edwards_sw6/ed_on_cp6_782/g' @
grep -rli 'sw6' * | xargs -i@ sed -i 's/sw6/cp6_782/g' @
grep -rli 'SW6' * | xargs -i@ sed -i 's/SW6/CP6_782/g' @
grep -rli 'JubJubAffine' * | xargs -i@ sed -i 's/JubJubAffine/EdwardsAffine/g' @
grep -rli 'JubJubProjective' * | xargs -i@ sed -i 's/JubJubProjective/EdwardsProjective/g' @
grep -rli 'JubJubParameters' * | xargs -i@ sed -i 's/JubJubParameters/EdwardsParameters/g' @
grep -rli 'JubJubGadget' * | xargs -i@ sed -i 's/JubJubGadget/EdwardsGadget/g' @
burdges commented 4 years ago

I take it nobody singled out any Edwards curve for BW6-761 yet?

Pratyush commented 4 years ago

Yeah, that was something that needs to be discussed: edwards_on_cp782 is actually also edwards_on_bw761, because the base field for this edwards curve is just the base field of BLS12-377. Suggestions for a better name would be much appreciated =)

burdges commented 4 years ago

lol of course :P

I donno, maybe edwards_alongside_bls12_377 or edwards_on_bls12_377_base

kobigurk commented 4 years ago

Yeah, that was something that needs to be discussed: edwards_on_cp782 is actually also edwards_on_bw761, because the base field for this edwards curve is just the base field of BLS12-377. Suggestions for a better name would be much appreciated =)

Yay, naming :)

Maybe we can use the following as conventions:

Since both SW6 and BW6 both have a main purpose of having 377 as an inner curve, rather than being good in their own right, maybe better naming for them would be cp6_782_over_bls12_377 and bw6_761_over_bls12_377, and correspondingly the Edwards curve would be edwards_on_bls12_377.

I'm not super satisfied with the exact prepositions, but maybe it's a start :)

burdges commented 4 years ago

I think A_on_B cannot mean they share a base field, so you need words like adjacent, alongside, beside, parallel, cobased, etc. for that relationship, not really happy with shorter words like "with" and "by".

Algebraic geometers say "over k" for the base field being k, so A_over_B could only mean the base field of A is the scalar field of B, the opposite of what you wrote.

It follows that edwards_on_X and edwards_over_X should exactly the same thing, with edwards_over_X being more in line with algebraic geometry, and edwards_on_X being slightly shorter.

You could say "under" except really one need not place multiple full curve names, so cp6_under_bls12_377 or bw6_under_bls12_377 work, but really just cp6_782 and bw6_761 play nicer with google, and so does jubjub.

I suppose Edwards with the same base field as X gets used largely for selecting verifier keys for X so maybe some word that expresses that, but nothing short comes to mind.

kobigurk commented 4 years ago

I think A_on_B cannot mean they share a base field, so you need words like adjacent, alongside, beside, parallel, cobased, etc. for that relationship, not really happy with shorter words like "with" and "by".

Algebraic geometers say "over k" for the base field being k, so A_over_B could only mean the base field of A is the scalar field of B, the opposite of what you wrote.

It follows that edwards_on_X and edwards_over_X should exactly the same thing, with edwards_over_X being more in line with algebraic geometry, and edwards_on_X being slightly shorter.

You could say "under" except really one need not place multiple full curve names, so cp6_under_bls12_377 or bw6_under_bls12_377 work, but really just cp6_782 and bw6_761 play nicer with google, and so does jubjub.

I suppose Edwards with the same base field as X gets used largely for selecting verifier keys for X so maybe some word that expresses that, but nothing short comes to mind.

I agree with all the points... But I wouldn't be hung up about "algebraic geometers say", it's a different context here, and so I think we have some freedom to re-use terms.

I'm pro easy googling and easy naming for the matter at hand!

burdges commented 4 years ago

Apologies if I sounded harsh about "over" but maybe best to avoid another additive vs multiplicative notation issue. lol

Pratyush commented 4 years ago

Another solution would be to also re-export edwards_over_cp782 as edwards_over_bw761. This is a bit redundant, and does hide the fact that they're the same curve, so I'm not super pleased with the idea, but just thought I'd throw it out there.

The real issue here is that we're talking about curves being "on top" of other curves, but really they're "on top" of specific fields. Finding a nice way to denote this would resolve our issues =)

Pratyush commented 4 years ago

Actually thinking over it a bit more, I think it's fine to just do the re-export. Obviously, it doesn't scale, but for the time being it's the cleanest solution. I've noted in documentation that edwards_on_bw6_761 is the same as edwards_on_cp6_782

yelhousni commented 4 years ago

cp6_782 is a good name. For Edwards curves, maybe we can make names shorter (e.g. ed_bls12_377) as we are assuming for other curves people know what bls, bw and cp stand for. For the _on_, _in_, _over_ ..., I think neither says explicitly that the base field of one is equal to the scalar field of the other and we didn't respect this rule in bls12, cp6, bw6 naming otherwise bls12_377 should be named bls12_on_bw6_761 or bls12_on_cp6_782 (terrible names). I think that explaining this relationship between curves in comments and/or readme is enough and that curves can have independent names as they can be used independently.

Pratyush commented 4 years ago

Thanks for the suggestion re: using ed_on; I updated the code to use that.