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 8 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 7 times, most recently from 7d82dd4 to e42b525 Compare December 4, 2024 06:49
@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.

@samkim-crypto
Copy link
Author

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.

Okay, I rekeyed the existing feature gate and then put the cost behind that feature gate.

Yeah the original plan was to backport, but I think we will have to wait until the next backport meeting.

native_cost: 0,
core_bpf_migration_feature: None,
}
)
// DO NOT ADD MORE ENTRIES TO THIS MAP
Copy link

Choose a reason for hiding this comment

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

Ha apparently this warning wasn't sufficient? We can't add more entries to this map without risking a consensus failure because after #3799, any changes will impact the default compute unit limit we calculate for transactions which don't specify a limit explicitly. The specific issues is that any tx instruction which invokes a "builtin program" (i.e. an entry in this map) will be allocated 3k cu's rather than 200k cu's. If you revert this change the rest of the PR looks ok to me.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I did see the warning and asked about it, but I seemed to have misinterpreted the answers that I got 😅. I will revert!

Choose a reason for hiding this comment

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

Please let's land #3866 and eliminate this footgun.

Choose a reason for hiding this comment

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

#3948 to document cost model issue with precompile

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.

4 participants