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

introduce secp operators #303

Merged
merged 5 commits into from
Jun 8, 2023
Merged

introduce secp operators #303

merged 5 commits into from
Jun 8, 2023

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Jun 5, 2023

This is a softfork activated by ENABLE_SECP_OPERATORS. It's based on @cameroncooper 's patch which implements verification of secp256k1 and secp256r1 signatures.

The two new operators are:

name opcode cost description
secp256k1_verify 0x0cf84f00 850000 Verifies a signature given a public key, message digest and signature
secp256r1_verify 0x1c3a8f00 1850000 Verifies a signature given a public key, message digest and signature

The message digest parameter must be the sha256 of the message to sign.

This is best reviewed one commit at a time.

@arvidn arvidn force-pushed the secp2 branch 2 times, most recently from 6f78a67 to 6b8585a Compare June 5, 2023 15:21
@coveralls-official
Copy link

coveralls-official bot commented Jun 5, 2023

Pull Request Test Coverage Report for Build 5203783675

  • 60 of 65 (92.31%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 92.837%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/chia_dialect.rs 12 13 92.31%
src/secp_ops.rs 45 46 97.83%
src/f_table.rs 0 3 0.0%
Totals Coverage Status
Change from base Build 5203270196: 0.3%
Covered Lines: 5042
Relevant Lines: 5431

💛 - Coveralls

@arvidn arvidn marked this pull request as ready for review June 5, 2023 16:27
@arvidn
Copy link
Contributor Author

arvidn commented Jun 5, 2023

@rostislav pointed out that there's a reason for k1 to be about twice as fast as r1. so maybe it would make sense to reflect that in the cost

richardkiss
richardkiss previously approved these changes Jun 5, 2023
Copy link
Contributor

@richardkiss richardkiss left a comment

Choose a reason for hiding this comment

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

Seems good

@arvidn arvidn force-pushed the secp2 branch 2 times, most recently from ec20a10 to f47c57b Compare June 7, 2023 18:49
@arvidn arvidn requested a review from richardkiss June 7, 2023 19:00
Copy link
Contributor

@richardkiss richardkiss left a comment

Choose a reason for hiding this comment

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

I haven't looked too closely since last time. But I thought I'd approve it anyway since my understanding is only the costs have changed.

@arvidn arvidn merged commit 8959ff1 into main Jun 8, 2023
@arvidn arvidn deleted the secp2 branch June 8, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants