ECDSA secp256k1 by algorandskiy · Pull Request #2852 · algorand/go-algorand

algorandskiy · 2021-09-07T22:19:12Z

Summary

ecdsa_verify, ecdsa_pk_decompress, ecdsa_pk_recover opcodes.
crypto/secp256k1 sources from go-ethereum 1.10.7 committed into the repo.
costs calculated (see below).
so far only secp256k1 curve is supported.
ecdsa_verify and ecdsa_pk_recover work with two-component keys (two 32 bytes long numbers) and not a single 33-bytes compressed or 65-bytes uncompressed keys.

Test Plan

Added new tests.

Cost calculation

Made as a ratio of ed25519verify cost

    BenchmarkEcDsa/ecdsa_verify-8              17341             67335 ns/op
    BenchmarkEcDsa/ecdsa_pk_decompress-8       42854             25714 ns/op
    BenchmarkEcDsa/ecdsa_pk_recover-8          15337             78249 ns/op
    
    BenchmarkEd25519Verifyx1-8         15244             75253 ns/op

Reviewer Notes

Most of the code is crypto/secp256k1 import from go-ethereum, please ignore.

tsachiherman · 2021-09-08T14:23:01Z

Could you fork the eth secp256k1 to algorand/secp256k1 and use it from there ? ( and have the eth as upstream ).
( so that we won't be depending on the eth repo directly )

jasonpaulos

This is a great start. I have some comments on opcode details, but things are looking good in general.

codecov-commenter · 2021-09-09T01:37:49Z

Codecov Report

Merging #2852 (a727dd7) into master (6afa86b) will decrease coverage by 0.04%.
The diff coverage is 41.80%.

@@            Coverage Diff             @@
##           master    #2852      +/-   ##
==========================================
- Coverage   47.34%   47.29%   -0.05%     
==========================================
  Files         351      355       +4     
  Lines       56523    56889     +366     
==========================================
+ Hits        26758    26906     +148     
- Misses      26758    26948     +190     
- Partials     3007     3035      +28

Impacted Files	Coverage Δ
crypto/secp256k1/panic_cb.go	`0.00% <0.00%> (ø)`
data/transactions/logic/doc.go	`82.75% <ø> (ø)`
crypto/secp256k1/curve.go	`14.61% <14.61%> (ø)`
data/transactions/logic/fields_string.go	`5.18% <16.66%> (+0.53%)`	⬆️
crypto/secp256k1/secp256.go	`42.25% <42.25%> (ø)`
data/transactions/logic/assembler.go	`80.32% <45.00%> (-0.35%)`	⬇️
data/transactions/logic/eval.go	`88.38% <66.35%> (-1.18%)`	⬇️
data/transactions/logic/fields.go	`81.57% <66.66%> (-2.01%)`	⬇️
crypto/secp256k1/scalar_mult_cgo.go	`77.77% <77.77%> (ø)`
data/transactions/logic/opcodes.go	`100.00% <100.00%> (ø)`
... and 12 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 6afa86b...a727dd7. Read the comment docs.

Cost calculated relative to ed25519verify BenchmarkEcDsa/ecdsa_verify-8 17341 67335 ns/op BenchmarkEcDsa/ecdsa_pk_decompress-8 42854 25714 ns/op BenchmarkEcDsa/ecdsa_pk_recover-8 15337 78249 ns/op BenchmarkEd25519Verifyx1-8 15244 75253 ns/op

algorandskiy · 2021-09-09T01:59:19Z

Could you fork the eth secp256k1 to algorand/secp256k1 and use it from there ?

Done.
Edit: Well, vendored instead. @jasonpaulos Do we plan to expose ECDSA in SDKs? If so then makes sense forking instead of vendoring (to re-import to go-sdk at least)

jannotti

I don't feel qualified to comment on the crypto itself, but I've commented where I could.

Thanks for adding work to document fields. I'll will need that to finish up tx_field spec.

jannotti · 2021-09-10T14:46:16Z

 | `getbyte` | pop a byte-array A and integer B. Extract the Bth byte of A and push it as an integer |
 | `setbyte` | pop a byte-array A, integer B, and small integer C (between 0..255). Set the Bth byte of A to C, and push the result |
 | `concat` | pop two byte-arrays A and B and join them, push the result |
+| `ecdsa_verify c` | for (data A, signature B, C and pubkey D, E) verify the signature of the data against the pubkey => {0 or 1} |


This is confusing because c is the name of the field (the immediate) and also (I think) the name you're using to describe the stack value. I'd pick a new name for the immediate.

Also need to say what the immediate is/does.

jannotti · 2021-09-10T14:46:47Z

+| `ecdsa_pk_recover c` | for (data A, recovery id B, signature C, D) recover a public compressed key => [*... stack*, X, Y] |
+| `ecdsa_pk_decompress c` | decompress pubkey A into components X, Y => [*... stack*, X, Y] |


Earlier comment applies to these too.

jannotti · 2021-09-10T14:51:30Z

+	"ed25519verify":       "The 32 byte public key is the last element on the stack, preceded by the 64 byte signature at the second-to-last element on the stack, preceded by the data which was signed at the third-to-last element on the stack.",
+	"ecdsa_verify":        "The 32 byte Y-component of a public key is the last element on the stack, preceded by X-component of a pubkey, preceded by S and R components of a signature, preceded by the data that is fifth element on the stack. The signed data must be 32 bytes long, and signatures in lower-S form are only accepted.",
+	"ecdsa_pk_decompress": "The 33 byte public key in a compressed form to be decompressed into X and Y (top) components.",
+	"ecdsa_pk_recover":    "S (top) and R elements of a signature, recovery id and data (bottom) are expected on the stack and used to deriver a public key in compressed format. The signed data must be 32 bytes long.",


I think we should extend our convention for naming stack arguments. So for a five argument opcode, you can just refer to A,B,C,D,&E. (A is the deepest, E is top-of-stack). I think that will simplify the text.

I agree but in this extended section we do not refer A-E at all, only in short description.

jasonpaulos

This is nearly there, I just have some minor suggestions.

Additionally, could you also add this test I made to verify compatibility with ethereum signatures? https://gist.github.com/jasonpaulos/47166d44c13e4efb18c7a1a09f4ac969

jasonpaulos · 2021-09-09T21:11:58Z

+	"ed25519verify":       "for (data A, signature B, pubkey C) verify the signature of (\"ProgData\" || program_hash || data) against the pubkey => {0 or 1}",
+	"ecdsa_verify":        "for (data A, signature B, C and pubkey D, E) verify the signature of the data against the pubkey => {0 or 1}",
+	"ecdsa_pk_decompress": "decompress pubkey A into components X, Y => [*... stack*, X, Y]",
+	"ecdsa_pk_recover":    "for (data A, recovery id B, signature C, D) recover a public compressed key => [*... stack*, X, Y]",


This should not say compressed key

jasonpaulos · 2021-09-09T21:17:36Z

-	"app_params_get":    "params: Txn.ForeignApps offset or an app id that appears in Txn.ForeignApps. Return: did_exist flag (1 if exist and 0 otherwise), value.",
-	"log":               "`log` can be called up to MaxLogCalls times in a program, and log up to a total of 1k bytes.",
+	"ed25519verify":       "The 32 byte public key is the last element on the stack, preceded by the 64 byte signature at the second-to-last element on the stack, preceded by the data which was signed at the third-to-last element on the stack.",
+	"ecdsa_verify":        "The 32 byte Y-component of a public key is the last element on the stack, preceded by X-component of a pubkey, preceded by S and R components of a signature, preceded by the data that is fifth element on the stack. The signed data must be 32 bytes long, and signatures in lower-S form are only accepted.",


Could you mention that Y, X, S, and R are big-endian encoded?

jasonpaulos · 2021-09-09T21:20:40Z

+	"ed25519verify":       "The 32 byte public key is the last element on the stack, preceded by the 64 byte signature at the second-to-last element on the stack, preceded by the data which was signed at the third-to-last element on the stack.",
+	"ecdsa_verify":        "The 32 byte Y-component of a public key is the last element on the stack, preceded by X-component of a pubkey, preceded by S and R components of a signature, preceded by the data that is fifth element on the stack. The signed data must be 32 bytes long, and signatures in lower-S form are only accepted.",
+	"ecdsa_pk_decompress": "The 33 byte public key in a compressed form to be decompressed into X and Y (top) components.",
+	"ecdsa_pk_recover":    "S (top) and R elements of a signature, recovery id and data (bottom) are expected on the stack and used to deriver a public key in compressed format. The signed data must be 32 bytes long.",


compressed should not be mentioned here either

jasonpaulos · 2021-09-10T16:30:37Z

+		pass bool
+	}{
+		{r, true},
+		{rTampered, false},


I would also recommend altering the message to make sure verification fails

tolikzinovyev · 2021-09-10T18:02:40Z

Could we maybe have crypto/secp256k1 in a separate repo?

algorandskiy

All reviews addressed except A, B, C,... naming in extended doc.

Additionally, could you also add this test I made to verify compatibility with ethereum signatures? https://gist.github.com/jasonpaulos/47166d44c13e4efb18c7a1a09f4ac969

Added.

algorandskiy · 2021-09-11T01:25:05Z

 | `getbyte` | pop a byte-array A and integer B. Extract the Bth byte of A and push it as an integer |
 | `setbyte` | pop a byte-array A, integer B, and small integer C (between 0..255). Set the Bth byte of A to C, and push the result |
 | `concat` | pop two byte-arrays A and B and join them, push the result |
+| `ecdsa_verify c` | for (data A, signature B, C and pubkey D, E) verify the signature of the data against the pubkey => {0 or 1} |


algorandskiy · 2021-09-11T01:25:10Z

+| `ecdsa_pk_recover c` | for (data A, recovery id B, signature C, D) recover a public compressed key => [*... stack*, X, Y] |
+| `ecdsa_pk_decompress c` | decompress pubkey A into components X, Y => [*... stack*, X, Y] |


algorandskiy · 2021-09-11T01:29:28Z

+	"ed25519verify":       "for (data A, signature B, pubkey C) verify the signature of (\"ProgData\" || program_hash || data) against the pubkey => {0 or 1}",
+	"ecdsa_verify":        "for (data A, signature B, C and pubkey D, E) verify the signature of the data against the pubkey => {0 or 1}",
+	"ecdsa_pk_decompress": "decompress pubkey A into components X, Y => [*... stack*, X, Y]",
+	"ecdsa_pk_recover":    "for (data A, recovery id B, signature C, D) recover a public compressed key => [*... stack*, X, Y]",


algorandskiy · 2021-09-11T01:37:33Z

-	"app_params_get":    "params: Txn.ForeignApps offset or an app id that appears in Txn.ForeignApps. Return: did_exist flag (1 if exist and 0 otherwise), value.",
-	"log":               "`log` can be called up to MaxLogCalls times in a program, and log up to a total of 1k bytes.",
+	"ed25519verify":       "The 32 byte public key is the last element on the stack, preceded by the 64 byte signature at the second-to-last element on the stack, preceded by the data which was signed at the third-to-last element on the stack.",
+	"ecdsa_verify":        "The 32 byte Y-component of a public key is the last element on the stack, preceded by X-component of a pubkey, preceded by S and R components of a signature, preceded by the data that is fifth element on the stack. The signed data must be 32 bytes long, and signatures in lower-S form are only accepted.",


algorandskiy · 2021-09-11T01:38:14Z

+	"ed25519verify":       "The 32 byte public key is the last element on the stack, preceded by the 64 byte signature at the second-to-last element on the stack, preceded by the data which was signed at the third-to-last element on the stack.",
+	"ecdsa_verify":        "The 32 byte Y-component of a public key is the last element on the stack, preceded by X-component of a pubkey, preceded by S and R components of a signature, preceded by the data that is fifth element on the stack. The signed data must be 32 bytes long, and signatures in lower-S form are only accepted.",
+	"ecdsa_pk_decompress": "The 33 byte public key in a compressed form to be decompressed into X and Y (top) components.",
+	"ecdsa_pk_recover":    "S (top) and R elements of a signature, recovery id and data (bottom) are expected on the stack and used to deriver a public key in compressed format. The signed data must be 32 bytes long.",


algorandskiy · 2021-09-11T01:55:50Z

+	"ed25519verify":       "The 32 byte public key is the last element on the stack, preceded by the 64 byte signature at the second-to-last element on the stack, preceded by the data which was signed at the third-to-last element on the stack.",
+	"ecdsa_verify":        "The 32 byte Y-component of a public key is the last element on the stack, preceded by X-component of a pubkey, preceded by S and R components of a signature, preceded by the data that is fifth element on the stack. The signed data must be 32 bytes long, and signatures in lower-S form are only accepted.",
+	"ecdsa_pk_decompress": "The 33 byte public key in a compressed form to be decompressed into X and Y (top) components.",
+	"ecdsa_pk_recover":    "S (top) and R elements of a signature, recovery id and data (bottom) are expected on the stack and used to deriver a public key in compressed format. The signed data must be 32 bytes long.",


I agree but in this extended section we do not refer A-E at all, only in short description.

algorandskiy · 2021-09-11T02:18:31Z

+		pass bool
+	}{
+		{r, true},
+		{rTampered, false},


algorandskiy · 2021-09-11T02:23:09Z

Could we maybe have crypto/secp256k1 in a separate repo?

This is quite small and follows the approach of keeping crypto stuff under `crypto/' in go-algorand.

Since the merge of #2852 go mod tidy and go mod vendor see paths in github.com/ethereum/go-ethereum/crypto/secp256k1/libsecp256k1 as a dependency, and go mod tidy adds it to go.mod.

* Add secp256k1 module from go-ethereum 1.10.7 * ECDSA secp256k1 implementation Cost calculated relative to ed25519verify BenchmarkEcDsa/ecdsa_verify-8 17341 67335 ns/op BenchmarkEcDsa/ecdsa_pk_decompress-8 42854 25714 ns/op BenchmarkEcDsa/ecdsa_pk_recover-8 15337 78249 ns/op BenchmarkEd25519Verifyx1-8 15244 75253 ns/op

Since the merge of algorand#2852 go mod tidy and go mod vendor see paths in github.com/ethereum/go-ethereum/crypto/secp256k1/libsecp256k1 as a dependency, and go mod tidy adds it to go.mod.

algorandskiy added the Team Scytale label Sep 7, 2021

algorandskiy requested review from ahangsu and jasonpaulos September 7, 2021 22:19

algorandskiy self-assigned this Sep 7, 2021

algorandskiy marked this pull request as draft September 7, 2021 22:19

algorandskiy force-pushed the pavel/ecdsa branch from c46dd16 to efb72e2 Compare September 7, 2021 22:22

barnjamin reviewed Sep 8, 2021

View reviewed changes

Comment thread data/transactions/logic/eval.go Outdated

algorandskiy force-pushed the pavel/ecdsa branch 2 times, most recently from 28019e7 to 1da24d5 Compare September 8, 2021 14:08

jasonpaulos reviewed Sep 8, 2021

View reviewed changes

Comment thread data/transactions/logic/opcodes.go Outdated

Comment thread data/transactions/logic/eval.go Outdated

Comment thread data/transactions/logic/eval.go Outdated

Comment thread data/transactions/logic/eval.go Outdated

Comment thread data/transactions/logic/doc.go Outdated

algorandskiy force-pushed the pavel/ecdsa branch from 1da24d5 to 688fbb9 Compare September 9, 2021 01:02

algorandskiy added 6 commits September 8, 2021 21:42

Add secp256k1 module from go-ethereum 1.10.7

88a95d4

Fix linters for imported go-ethereum code

046b451

ECDSA secp256k1 implementation

40bd676

ECDSA opcodes tests

8ae3494

Swap arg / ret order

895ef09

Set costs for ecdsa opcodes

bb53759

Cost calculated relative to ed25519verify BenchmarkEcDsa/ecdsa_verify-8 17341 67335 ns/op BenchmarkEcDsa/ecdsa_pk_decompress-8 42854 25714 ns/op BenchmarkEcDsa/ecdsa_pk_recover-8 15337 78249 ns/op BenchmarkEd25519Verifyx1-8 15244 75253 ns/op

algorandskiy force-pushed the pavel/ecdsa branch from 688fbb9 to bb53759 Compare September 9, 2021 01:55

algorandskiy requested review from jasonpaulos and tsachiherman September 9, 2021 01:59

algorandskiy marked this pull request as ready for review September 9, 2021 01:59

algorandskiy requested a review from jannotti September 9, 2021 02:01

jannotti reviewed Sep 10, 2021

View reviewed changes

jasonpaulos reviewed Sep 10, 2021

View reviewed changes

Address CR notes

a727dd7

algorandskiy commented Sep 11, 2021

View reviewed changes

jasonpaulos approved these changes Sep 11, 2021

View reviewed changes

jannotti merged commit 8f743a9 into algorand:master Sep 11, 2021

cce mentioned this pull request Sep 13, 2021

Change import paths used in crypto/secp256k1/dummy.go #2881

Merged

onetechnical mentioned this pull request Sep 15, 2021

go-algorand 3.0.0-beta #2893

Merged

algorandskiy deleted the pavel/ecdsa branch September 21, 2021 16:55

algojack mentioned this pull request Sep 24, 2021

go-algorand 3.0.1-stable #2949

Merged

ahangsu mentioned this pull request Apr 1, 2022

Enhancement: Re-enable fillBytes method in ABI and eval.go implementation #3856

Merged

		\| `ecdsa_pk_recover c` \| for (data A, recovery id B, signature C, D) recover a public compressed key => [... stack, X, Y] \|
		\| `ecdsa_pk_decompress c` \| decompress pubkey A into components X, Y => [... stack, X, Y] \|

Conversation

algorandskiy commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Cost calculation

Reviewer Notes

Uh oh!

Uh oh!

tsachiherman commented Sep 8, 2021

Uh oh!

jasonpaulos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

algorandskiy commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jannotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jasonpaulos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tolikzinovyev commented Sep 10, 2021

Uh oh!

algorandskiy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

algorandskiy commented Sep 7, 2021 •

edited

Loading

codecov-commenter commented Sep 9, 2021 •

edited

Loading

algorandskiy commented Sep 9, 2021 •

edited

Loading