Skip to content

Conversation

@algoidurovic
Copy link
Contributor

ECDSA opcodes only support the Secp256k1 curve. The goal of this PR is to extend the opcodes to support the Secp256r1 curve as well. Note: the PkRecover opcode is not covered here.

Testing included correctness and benchmark tests. Benchmark tests indicate a significant slowdown when the Secp256r1 curve is used, meaning the current opcode cost may not be viable.

Added some helper test functions.

@jasonpaulos
Copy link
Contributor

From doc.go for ecdsa_verify:

The signed data must be 32 bytes long, and signatures in lower-S form are only accepted

I suspect these constraints might not be true for the secp256r1 curve. And if that's the case we should document it (or decide to enforce them).

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2022

Codecov Report

Merging #3495 (f1984c0) into master (93a372d) will decrease coverage by 0.10%.
The diff coverage is 21.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3495      +/-   ##
==========================================
- Coverage   47.57%   47.46%   -0.11%     
==========================================
  Files         370      370              
  Lines       60060    60213     +153     
==========================================
+ Hits        28572    28579       +7     
- Misses      28178    28323     +145     
- Partials     3310     3311       +1     
Impacted Files Coverage Δ
data/transactions/logic/assembler.go 77.15% <0.00%> (-1.22%) ⬇️
data/transactions/logic/doc.go 77.41% <ø> (ø)
data/transactions/logic/opcodes.go 100.00% <ø> (ø)
data/transactions/logic/fields_string.go 5.80% <10.00%> (+0.32%) ⬆️
data/transactions/logic/eval.go 82.18% <23.80%> (-3.51%) ⬇️
data/transactions/logic/fields.go 76.08% <57.14%> (-1.57%) ⬇️
agreement/cryptoVerifier.go 75.17% <0.00%> (-2.13%) ⬇️
agreement/proposalManager.go 96.07% <0.00%> (-1.97%) ⬇️
catchup/service.go 69.62% <0.00%> (-0.50%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93a372d...f1984c0. Read the comment docs.

@michaeldiamant
Copy link
Contributor

Closing because:

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.

5 participants