fix: cgosecp256k1 verification

[tac0turtle]

Description

Closes: #10747

• update secp256k1 cgo fork,
• debug verify bytes

benchmark                     old ns/op     new ns/op     delta
BenchmarkKeyGeneration-10     407           413           +1.35%
BenchmarkSigning-10           95099         36754         -61.35%
BenchmarkVerification-10      215551        48053         -77.71%

benchmark                     old allocs     new allocs     delta
BenchmarkKeyGeneration-10     2              2              +0.00%
BenchmarkSigning-10           83             4              -95.18%
BenchmarkVerification-10      74             1              -98.65%

benchmark                     old bytes     new bytes     delta
BenchmarkKeyGeneration-10     96            96            +0.00%
BenchmarkSigning-10           5283          196           -96.29%
BenchmarkVerification-10      3537          32            -99.10%

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[tac0turtle]

Im leaning towards pushing towards a single cgo flag that we group other cgo deps on.

[tac0turtle]

would love someone to check the license related things.

[tac0turtle]

cgo verification on master compared to this pr cgo.

benchmark                     old ns/op     new ns/op     delta
BenchmarkKeyGeneration-10     410           464           +13.18%
BenchmarkSigning-10           68580         37077         -45.94%
BenchmarkVerification-10      113265        46856         -58.63%

benchmark                     old allocs     new allocs     delta
BenchmarkKeyGeneration-10     2              2              +0.00%
BenchmarkSigning-10           4              4              +0.00%
BenchmarkVerification-10      1              1              +0.00%

benchmark                     old bytes     new bytes     delta
BenchmarkKeyGeneration-10     96            96            +0.00%
BenchmarkSigning-10           196           196           +0.00%
BenchmarkVerification-10      32            32            +0.00%

[alexanderbez]

I can't comment on the changes to crypto/keys/secp256k1/internal/secp256k1/curve.go, but otherwise, this seems fine to me.

I don't like the dummy build though.

[ValarDragon]

Reviewed license, Its BSD 3 clause which AFAIK is compatible with MIT!

Will try to review code tomorrow if noone else has. BTW all the heap / alloc benchmarks will not be right because CGO, but time ones are (and very impressively better!) The 4x verification time speedup is a huge win!

[liamsi]

IMO a pointer to the orig. code and the exact version/commit this is based on in the opening comment would be great. So the paranoid can diff it locally to see no changes were made to the underlying c code for instance.
From the readme this seems to be: https://github.com/ethereum/go-ethereum/tree/8fddf27a989e246659fd018ea9be37b2b4f55326/crypto/secp256k1

[liamsi]

It is not really clear to me what yields the timing improvements.
It looks like the only changes are:

• crypto/secp256k1: fix undefined behavior in BitCurve.Add ethereum/go-ethereum#22621
• crypto/secp256k1: add checking z sign in affineFromJacobian ethereum/go-ethereum#18419

The rest is just typos / adding tests and minor changes. I've verified that this wasn't modified and indeed matches with the code in https://github.com/ethereum/go-ethereum/commits/0703ef62d388eafa177540ff722c3a0871c4979d/crypto/secp256k1

IMO, this PR is good to be merged. Independent of any perf improvements or not. Good practice to keep this up to date. Can't really speak to the licensing but sounds like this is ok as well.

[ValarDragon]

I suspect the benchmarks are native go perf vs cgo performance, otherwise we'd see something different for heap / allocs

[ValarDragon]

LGTM

Did not check the copying of libsecp directly, but assume its correct. The cosmos codebase only tests signature verification on one test vector, so we know it passes on at least one case.

We should independently file an issue to add more test vectors in secp256k1_test.go