Skip to content

Secp256r1 precompile: switch to BoringSSL #8437

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 1, 2025
Merged

Secp256r1 precompile: switch to BoringSSL #8437

merged 5 commits into from
Apr 1, 2025

Conversation

alexb5dh
Copy link
Contributor

@alexb5dh alexb5dh commented Mar 28, 2025 •
edited
Loading

Changes

  • Switches to the secp256r1 package using BoringSSL for signature verification (see Use BoringSSL crypto module secp256r1-bindings#6)
  • Adds a bunch of random test cases
  • Small fixes for Benchmarks solution:
    • Broken build because of missing projects
    • Precompile benchmarks runnable only from specific directory
    • 2 baseline methods in single benchmark

Related:

Types of changes

What types of changes does your code introduce?

  • Optimization

Testing

Requires testing

  • Yes

If yes, did you write tests?

  • Yes

Documentation

Requires documentation update

  • No

Requires explanation in Release Notes

  • No

@alexb5dh alexb5dh self-assigned this Mar 28, 2025
@alexb5dh alexb5dh marked this pull request as ready for review March 28, 2025 14:09
@derrix060 derrix060 requested a review from Copilot March 28, 2025 15:13
Copilot
Copilot AI reviewed Mar 28, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request implements the switch to the secp256r1 package using BoringSSL for signature verification and updates associated benchmarks and tests.

  • Replaces manual ECDsa-based signature verification with a call to Secp256r1.VerifySignature.
  • Updates benchmark input file handling and adjusts the benchmark attributes.
  • Adds new test cases using random ECDSA inputs.

Reviewed Changes

Copilot reviewed 4 out of 7 changed files in this pull request and generated 2 comments.

File Description
src/Nethermind/Nethermind.Precompiles.Benchmark/PrecompileBenchmarkBase.cs Uses Path.Combine with AppContext.BaseDirectory for computing the inputs directory.
src/Nethermind/Nethermind.Evm/Precompiles/Secp256r1Precompile.cs Removes the manual signature verification logic and its input length check, replacing it with a call to Secp256r1.VerifySignature.
src/Nethermind/Nethermind.Evm.Test/Secp256r1PrecompileTests.cs Adds new test cases and a method for generating random valid signature inputs.
src/Nethermind/Nethermind.Evm.Benchmark/StaticCallBenchmarks.cs Updates the benchmark attribute by removing the Baseline flag.
Files not reviewed (3)
  • src/Nethermind/Benchmarks.slnx: Language not supported
  • src/Nethermind/Directory.Packages.props: Language not supported
  • src/Nethermind/Nethermind.Evm/Nethermind.Evm.csproj: Language not supported
Comments suppressed due to low confidence (1)

src/Nethermind/Nethermind.Evm.Benchmark/StaticCallBenchmarks.cs:122

  • The removal of the Baseline attribute might affect benchmark comparisons if a reference point is needed. Confirm that this change is intentional and that performance metrics are still comparable across benchmarks.
[Benchmark]

@alexb5dh
Copy link
Contributor Author

alexb5dh commented Mar 28, 2025 •
edited
Loading

Windows 10

Type Input Mean Error StdDev Gas Throughput Throughput CI-Lower Throughput CI-Upper Allocated Alloc Ratio
Secp256r1Benchmark Invalid 53.20 us 0.091 us 0.130 us 3450 64.85 MGas/s 64.93 MGas/s 64.76 MGas/s - NA
Secp256r1Benchmark Valid1 53.28 us 0.104 us 0.153 us 3450 64.75 MGas/s 64.85 MGas/s 64.66 MGas/s - NA
Secp256r1Benchmark Valid2 53.29 us 0.163 us 0.239 us 3450 64.74 MGas/s 64.89 MGas/s 64.59 MGas/s - NA
Secp256r1Benchmark Valid3 53.68 us 0.123 us 0.177 us 3450 64.27 MGas/s 64.38 MGas/s 64.16 MGas/s - NA
EcRecoverBenchmark ValidKey 57.73 us 1.063 us 1.559 us 3000 51.96 MGas/s 52.69 MGas/s 51.25 MGas/s 416 B 1.00

Ubuntu 22 (WSL)

Type Input Mean Error StdDev Gas Throughput Throughput CI-Lower Throughput CI-Upper Allocated Alloc Ratio
Secp256r1Benchmark Invalid 64.01 us 2.163 us 3.101 us 3450 53.89 MGas/s 55.30 MGas/s 52.56 MGas/s 1 B 1.00
Secp256r1Benchmark Valid1 62.86 us 1.940 us 2.720 us 3450 54.88 MGas/s 56.18 MGas/s 53.64 MGas/s - NA
Secp256r1Benchmark Valid2 60.55 us 0.555 us 0.760 us 3450 56.98 MGas/s 57.38 MGas/s 56.59 MGas/s 1 B 1.00
Secp256r1Benchmark Valid3 61.24 us 0.725 us 1.040 us 3450 56.34 MGas/s 56.84 MGas/s 55.84 MGas/s - NA
EcRecoverBenchmark ValidKey 49.87 us 1.097 us 1.641 us 3000 60.16 MGas/s 61.17 MGas/s 59.18 MGas/s 416 B 1.00

*Allocated doesn't take unmanaged memory into account

@alexb5dh alexb5dh merged commit a42b9c9 into master Apr 1, 2025
80 checks passed
@alexb5dh alexb5dh deleted the feature/secp256r1-boringssl branch April 1, 2025 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@benaadams benaadams benaadams approved these changes

@LukaszRozmej LukaszRozmej LukaszRozmej approved these changes

Assignees

@alexb5dh alexb5dh

Labels
performance is good
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexb5dh @benaadams @LukaszRozmej