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

[secp256r1] Add CU costs #3826

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

samkim-crypto
Copy link

@samkim-crypto samkim-crypto commented Nov 28, 2024

Problem

The secp256r1 precompile (#3152) was added, but the compute cost has not been assigned.

Summary of Changes

Added builtin default costs and a static cost in the cost-model for the secp256r1 precompile.

For the static cost, I benchmarked the existing three precompiles on my dev machine.

For each transaction length, secp256k1 signatures took at most 3x the time compared to ed25519. The assigned compute units for secp256k1 is about 3x that of ed25519, which seemed to make sense.

  • ed25519 is assigned 76 CUs
  • secp256k1 is assigned 223 CUs

Secp256r1 signatures take at most 2x the time compared to ed25519. It would be safe to assign CU about 2x that of ed25519, so I added 160 CUs. Please let me know if there are better ways to approximate the static costs.

     Running benches/ed25519_instructions.rs (/home/sol/src/agave/target/release/deps/ed25519_instructions-cfe374ce3ffee123)

running 4 tests
test bench_ed25519_len_032 ... bench:      56,574.71 ns/iter (+/- 996.98)
test bench_ed25519_len_128 ... bench:      56,811.95 ns/iter (+/- 455.62)
test bench_ed25519_len_32k ... bench:     109,535.23 ns/iter (+/- 9,382.32)
test bench_ed25519_len_max ... bench:     161,679.60 ns/iter (+/- 16,034.62)

test result: ok. 0 passed; 0 failed; 0 ignored; 4 measured; 0 filtered out; finished in 9.42s

     Running benches/secp256k1_instructions.rs (/home/sol/src/agave/target/release/deps/secp256k1_instructions-affd9783d2efd736)

running 4 tests
test bench_secp256k1_len_032 ... bench:     168,551.79 ns/iter (+/- 655.38)
test bench_secp256k1_len_256 ... bench:     168,978.40 ns/iter (+/- 5,021.86)
test bench_secp256k1_len_32k ... bench:     251,410.30 ns/iter (+/- 1,157.67)
test bench_secp256k1_len_max ... bench:     334,033.15 ns/iter (+/- 2,017.68)

test result: ok. 0 passed; 0 failed; 0 ignored; 4 measured; 0 filtered out; finished in 10.35s

     Running benches/secp256r1_instructions.rs (/home/sol/src/agave/target/release/deps/secp256r1_instructions-31b57b3d05aff528)

running 4 tests
test bench_secp256r1_len_032 ... bench:     111,565.93 ns/iter (+/- 253.62)
test bench_secp256r1_len_256 ... bench:     112,417.49 ns/iter (+/- 278.93)
test bench_secp256r1_len_32k ... bench:     130,337.03 ns/iter (+/- 1,029.24)
test bench_secp256r1_len_max ... bench:     165,244.45 ns/iter (+/- 17,834.96)

test result: ok. 0 passed; 0 failed; 0 ignored; 4 measured; 0 filtered out; finished in 8.33s

Fixes #

@samkim-crypto samkim-crypto force-pushed the secp256r1/cost branch 6 times, most recently from 68e7050 to 7d82dd4 Compare December 4, 2024 00:23
@samkim-crypto samkim-crypto marked this pull request as ready for review December 4, 2024 09:32
@samkim-crypto samkim-crypto requested a review from a team as a code owner December 4, 2024 09:32
Copy link

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for adding it.

Will this be backported to 2.0/2.1? If it is released after CU limits are enforced on Replay, it will need a feature gate since it changes transaction's CU cost.

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.

2 participants