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

AVM: EC math #4924

Merged
merged 23 commits into from
Aug 23, 2023
Merged

AVM: EC math #4924

merged 23 commits into from
Aug 23, 2023

Conversation

jannotti
Copy link
Contributor

@jannotti jannotti commented Dec 19, 2022

Adds five opcodes to support elliptic curve operations, on the BN254 and BLS 12-381 curves. These curves are pairing friendly, so opcodes support the normal, expected, operations (ec_add, ec_scalar_mul, ec_multi_scalar_mul) on elliptic curve points, and a ec_pairing_check operation on sets of points from associated curves (G1 and G2) allowing for checking zk-proofs.

  • Implementations
  • Specs
  • Tests on standard test vectors
  • Benchmarks for costs

Address #3253

@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Merging #4924 (7c4dc15) into master (620e9bd) will increase coverage by 0.21%.
Report is 4 commits behind head on master.
The diff coverage is 74.84%.

@@            Coverage Diff             @@
##           master    #4924      +/-   ##
==========================================
+ Coverage   55.18%   55.39%   +0.21%     
==========================================
  Files         466      466              
  Lines       65084    65642     +558     
==========================================
+ Hits        35917    36363     +446     
- Misses      26776    26820      +44     
- Partials     2391     2459      +68     
Files Changed Coverage Δ
data/transactions/logic/assembler.go 94.08% <0.00%> (-0.06%) ⬇️
data/transactions/logic/doc.go 75.00% <ø> (ø)
data/transactions/logic/fields_string.go 57.69% <50.00%> (-0.65%) ⬇️
data/transactions/logic/opcodes.go 83.46% <60.00%> (-3.32%) ⬇️
data/transactions/logic/fields.go 75.31% <72.72%> (-0.20%) ⬇️
data/transactions/logic/pairing.go 76.55% <76.27%> (+76.55%) ⬆️

... and 18 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@fabrice102 fabrice102 left a comment

Choose a reason for hiding this comment

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

We also need three more opcodes:

  • ec_map_field_to_curve: which correspond to BLS12_MAP_FP_TO_G1 / BLS12_MAP_FP_TO_G2 in https://eips.ethereum.org/EIPS/eip-2537
  • ec_is_on_curve: which checks if a point is on a curve
  • ec_is_in_subgroup: which checks if a point on the curve is in the subgroup

data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
@jannotti
Copy link
Contributor Author

jannotti commented Dec 21, 2022

Added, as ec_map_to, so it reads as ec_map_to BN254_G1

  • ec_is_on_curve: which checks if a point is on a curve

Do we need this if the operations keep checking? It's not in the EIP. Are there times a contract would want to notice something isn't on curve and do something else.

  • ec_is_in_subgroup: which checks if a point on the curve is in the subgroup

Added as ec_subgroup_check to match ec_pairing_check. Ilan wasn't able to tell (and neither could I) whether the gnark-crypto subgroup check also implicitly check "on curve" or not. Can an off curve point somehow pass the in subgroup check?

edit: I was told this is possible, so I have added the on curve check.

@HashMapsData2Value
Copy link

HashMapsData2Value commented Dec 22, 2022

I'd like to request that ec math for ed25519 also be added. I'd like ed25519 based ring signatures to be verifiable by Algorand smart contracts. https://github.com/HashMapsData2Value/mahber/blob/master/Project_Mahber.pdf

Requested opcodes:

  • ed25519 addition (takes two points and adds them)
  • ed25519 scalarmult (multiplies point with scalar)
  • ed25519 valid point (verify point on curve)
  • ed25519 hash to point (hashes input directly to point on curve)

I believe everything needed is already conveniently inside of the internal LibSodium fork.

Regarding the hash to point, the chain that inspired me to suggest ring signatures (CryptoNote/Monero) implemented a hash_to_ec here/here. But I believe that LibSodium's crypto_core_ed25519_from_uniform can be used for this. It implements the Elligator 2 map, which accepts a 32 byte value (e.g., the compressed point of a public key like the ed25519_verify opcode) and returns a point on the curve. Also checks small order. Someone wrote a python implementation of ring signatures using LibSodium's crypto_core_ed25519_from_uniform.

Would be grateful if someone with more expertise than me could weigh in. But I think we could see some pretty cool things with ring signatures.

@jannotti
Copy link
Contributor Author

jannotti commented Jan 4, 2023

I'd like to request that ec math for ed25519 also be added.

I don't think this would be very hard (but different enough that I'd want it in another follow-up PR). However, I wonder if ring signatures can be implemented on these curves instead, if the intent is to do the entire implementation in AVM anyway. (It's possible that would be slower though, I don't know. We'd have to benchmark all these operations.)

@HashMapsData2Value
Copy link

HashMapsData2Value commented Jan 4, 2023

I'd like to request that ec math for ed25519 also be added.

I don't think this would be very hard (but different enough that I'd want it in another follow-up PR). However, I wonder if ring signatures can be implemented on these curves instead, if the intent is to do the entire implementation in AVM anyway. (It's possible that would be slower though, I don't know. We'd have to benchmark all these operations.)

I suspect it would be slower and I think require 2x the storage since points in ed25519 can be represented with only 32 bytes (like the ed25519_verify opcode expects). Benchmarks would be required.

EDIT: I saw the opcode costs just now. Assuming bn254_G1 the cost for verifying 1 element in a ring signature is ec_scalar_mul(100)+ec_add(10)+ec_scalar_mul(100)+ec_scalar_mul(100)+ec_map_to_g(100)+ec_add(10) + ec_scalar_mul (100) + keccak256(130) -> 650 cost. That's enough to fit in one transaction, which is quite good actually.

bild

@jannotti
Copy link
Contributor Author

jannotti commented Jan 9, 2023

EDIT: I saw the opcode costs just now. Assuming bn254_G1 the cost for verifying 1 element in a ring signature is ec_scalar_mul(100)+ec_add(10)+ec_scalar_mul(100)+ec_scalar_mul(100)+ec_map_to_g(100)+ec_add(10) + ec_scalar_mul (100) + keccak256(130) -> 650 cost. That's enough to fit in one transaction, which is quite good actually.

Unfortunately, no, these operations are way more expensive. I had those numbers in before benchmarking. After benchmarking, you'll see the only way these things are going to work is in logicsigs which have way more compute allowed.

The numbers (and comments) may actually be 2x too high right now, and I need to resolve some surprising behavior as I figure out how scalar_mul and pairing_check scale with multiple inputs.

Copy link
Contributor

@fabrice102 fabrice102 left a comment

Choose a reason for hiding this comment

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

Made some comments on the documentation only.
I'll review the code later.

data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
data/transactions/logic/pairing_test.go Outdated Show resolved Hide resolved
data/transactions/logic/pairing_test.go Outdated Show resolved Hide resolved
data/transactions/logic/pairing_test.go Show resolved Hide resolved
data/transactions/logic/pairing_test.go Show resolved Hide resolved
data/transactions/logic/pairing.go Outdated Show resolved Hide resolved
data/transactions/logic/pairing.go Outdated Show resolved Hide resolved
iten-alg and others added 3 commits August 1, 2023 12:48
Moved benchmarks into pairing_test and improved them

Removed the restriction of using 32 byte scalars, but but with a 32
byte _maximum_, so that costs can be calculated with that assumption.
@jannotti jannotti self-assigned this Aug 2, 2023
@ismael-elatifi
Copy link

Hello @jannotti ,
I want to implement Algorand smart contracts which need to verify ZK-SNARK proofs.
I think this work should enable that.
Do you know approximately when ZK-SNARK verification on-chain should be possible on test net ?

@jannotti
Copy link
Contributor Author

jannotti commented Aug 7, 2023

I think this work should enable that.

I think so too. That is the intention. If you'd like to try it out and give feedback to ensure that's true, we'd love that. You can use sandbox to run a local network using this branch.

Do you know approximately when ZK-SNARK verification on-chain should be possible on test net ?

Once the PR is accepted, it will go to test net wth the next consensus update. They seem to happen every few months, and the last one was about a month ago. So, optimistically, it might be on test net in a couple months. But that assumes we get the PR in soon. Having direct feedback on whether the opcodes here are sufficient for a real use case would be great! What proving system do you intend to use?

Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

I am taking some quick pass on it, encountering some questions about cost, which I am unsure about whether it has been finalized.

data/transactions/logic/README.md Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
data/transactions/logic/TEAL_opcodes.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

I only have eval_test and pairing_test pending on my read stack. The implementation seems reasonable by using gnark API.

data/transactions/logic/opcodes.go Show resolved Hide resolved
ledger/apptxn_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Still poking through tests, but here are my comments so far

data/transactions/logic/doc.go Outdated Show resolved Hide resolved
data/transactions/logic/fields.go Outdated Show resolved Hide resolved
data/transactions/logic/pairing.go Show resolved Hide resolved
data/transactions/logic/pairing.go Outdated Show resolved Hide resolved
data/transactions/logic/pairing_test.go Outdated Show resolved Hide resolved
data/transactions/logic/pairing_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bbroder-algo bbroder-algo left a comment

Choose a reason for hiding this comment

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

Audited code to provide ZK verification capabilities in AVM.

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Looks good to me

@jannotti jannotti merged commit 828c1df into algorand:master Aug 23, 2023
10 checks passed
@ismael-elatifi
Copy link

ismael-elatifi commented Aug 24, 2023

Hi @jannotti,
Well done for the merge of this important work !
Will there eventually be tests added proving that we can actually implement ZK proof verification with AVM/TEAL ?
They would also serve as a great example for people wanting to do the same 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants