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

Enable secp256r1 in Antehandlers #8515

Closed
4 tasks done
robert-zaremba opened this issue Feb 4, 2021 · 3 comments · Fixed by #8786
Closed
4 tasks done

Enable secp256r1 in Antehandlers #8515

robert-zaremba opened this issue Feb 4, 2021 · 3 comments · Fixed by #8786
Assignees
Milestone

Comments

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Feb 4, 2021

Summary

In #7718 we are adding secp256r1 to the SDK.
In #8415 we implemented address generation algorithm for custom schemes.

Here, we want to enable secp256r1 in CheckTx validation of antehandlers.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Mar 3, 2021

For the gas cost I propose:

  • benchmark the secp256r1 against cgo secp256k1
  • instead of creating a new parameter for gas price, set the gas fee as a factor of secp256k1 gas fee based on the benchmark above.

@robert-zaremba robert-zaremba mentioned this issue Mar 4, 2021
9 tasks
@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Mar 4, 2021

benchmarking current implementation:

BenchmarkSig/secp256k1     4334   277167 ns/op   4128 B/op   79 allocs/op
BenchmarkSig/secp256r1    10000   108769 ns/op   1672 B/op   33 allocs/op

Based on the results above the factor would be 2x. However I propose to discount it and
use 1.2x because we are comparing a highly optimized cgo implementation of secp256k1 to
a go stdlib secp256r1. In other benchmark we found that ported secp256k1 to stdlib based
implementation wold be 20x slower.

@robert-zaremba
Copy link
Collaborator Author

I wrote the comment above while being sick. the secp256r1 is 2.7 times faster, not slower. So will use 0.5 factor for the gas fee compared to secp256k1.

@mergify mergify bot closed this as completed in #8786 Mar 4, 2021
@aaronc aaronc removed the in progress label Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants