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

runtime-sdk: add p384 signing & verification #1519

Merged
merged 3 commits into from
Oct 12, 2023
Merged

runtime-sdk: add p384 signing & verification #1519

merged 3 commits into from
Oct 12, 2023

Conversation

nhynes
Copy link
Contributor

@nhynes nhynes commented Oct 3, 2023

No description provided.

@nhynes nhynes force-pushed the nhynes/bump-crypto-deps branch 3 times, most recently from bc35522 to 4dc1196 Compare October 4, 2023 12:34
@nhynes nhynes force-pushed the nhynes/bump-crypto-deps branch 3 times, most recently from 1d45270 to a700acf Compare October 4, 2023 16:34
Base automatically changed from nhynes/bump-crypto-deps to main October 4, 2023 19:39
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #1519 (5e530c0) into main (6590ae6) will increase coverage by 0.04%.
The diff coverage is 64.64%.

@@            Coverage Diff             @@
##             main    #1519      +/-   ##
==========================================
+ Coverage   60.45%   60.50%   +0.04%     
==========================================
  Files         138      139       +1     
  Lines        9894     9957      +63     
==========================================
+ Hits         5981     6024      +43     
- Misses       3871     3891      +20     
  Partials       42       42              
Files Coverage Δ
...ime-sdk/modules/evm/src/precompile/confidential.rs 80.64% <100.00%> (+0.31%) ⬆️
runtime-sdk/src/crypto/signature/secp256k1.rs 66.66% <100.00%> (-0.94%) ⬇️
runtime-sdk/src/crypto/signature/secp256r1.rs 53.70% <33.33%> (+5.21%) ⬆️
runtime-sdk/src/crypto/signature/mod.rs 72.48% <77.77%> (+0.61%) ⬆️
runtime-sdk/src/crypto/signature/secp384r1.rs 53.70% <53.70%> (ø)

Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

If you want this to be accessible from EVM, make sure to also update the gas costs in the confidential precompiles. Ideally you would add some benchmarks to establish proper costs (see how it was done for the others).

Otherwise looks good.

@nhynes
Copy link
Contributor Author

nhynes commented Oct 5, 2023

The benches look like

test precompile::confidential::test::bench_sign_secp256r1_prehashed_sha256   ... bench:     288,916 ns/iter (+/- 1,086)
test precompile::confidential::test::bench_sign_secp384r1_prehashed_sha384   ... bench:   1,214,954 ns/iter (+/- 6,736)
test precompile::confidential::test::bench_verify_secp256r1_prehashed_sha256 ... bench:     277,456 ns/iter (+/- 1,579
)
test precompile::confidential::test::bench_verify_secp384r1_prehashed_sha384 ... bench:   1,178,009 ns/iter (+/- 4,977
)

and

test precompile::confidential::test::bench_sign_secp256r1_prehashed_sha256   ... bench:     248,305 ns/iter (+/- 184)
test precompile::confidential::test::bench_sign_secp384r1_prehashed_sha384   ... bench:   1,163,157 ns/iter (+/- 3,854)
test precompile::confidential::test::bench_verify_secp256r1_prehashed_sha256 ... bench:     233,778 ns/iter (+/- 1,733)
test precompile::confidential::test::bench_verify_secp384r1_prehashed_sha384 ... bench:   1,127,003 ns/iter (+/- 25,798)

and so forth, with about 4.8x being the max I've seen

@nhynes nhynes force-pushed the nhynes/p384 branch 2 times, most recently from fbc3978 to ff72bb4 Compare October 5, 2023 18:18
@nhynes nhynes requested a review from kostko October 5, 2023 19:07
@peternose
Copy link
Contributor

so about 4.8x as slow

Did you mean 4.2x?

@nhynes nhynes force-pushed the nhynes/p384 branch 2 times, most recently from 1633780 to dac06fb Compare October 6, 2023 09:43
@nhynes nhynes requested a review from peternose October 6, 2023 09:44
@nhynes
Copy link
Contributor Author

nhynes commented Oct 6, 2023

Did you mean 4.2x?

It looks that way from the one sample, but it's highly variable, and sometimes I get 4.8x, so I took the max so that we can safely reduce it later.

@nhynes nhynes enabled auto-merge October 12, 2023 20:46
@nhynes nhynes merged commit a895d38 into main Oct 12, 2023
27 checks passed
@nhynes nhynes deleted the nhynes/p384 branch October 12, 2023 21:09
github-actions bot added a commit that referenced this pull request Oct 12, 2023
…hynes/p384

runtime-sdk: add p384 signing & verification a895d38
github-actions bot added a commit that referenced this pull request Oct 12, 2023
…/nhynes/p384

runtime-sdk: add p384 signing & verification a895d38
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.

None yet

3 participants