Skip to content

bench_ecmult: add benchmark for ecmult_const_xonly #1668

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

Merged

Conversation

theStack
Copy link
Contributor

In the course of trying to understand the x-only ECDH benchmark results in the silent payments PR (#1519 (comment)), I noticed that we don't have a benchmark yet for ecmult_const_xonly, so I added one.

Results on my machine:

$ ./build/bin/bench_ecmult
Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)

ecmult_gen                    ,    12.3       ,    12.6       ,    13.2
ecmult_const                  ,    27.2       ,    28.6       ,    30.9
ecmult_const_xonly            ,    29.3       ,    30.4       ,    32.6

...

I was a bit surprised to see that the x-only variant is slower than the regular ecmult_const function, but on the other hand the former allows to skip pubkey deserialization (i.e. determining y with a square root calculation, which is rather expensive), so that has to be taken account for a fair comparison in use-cases like ECDH (see also benchmark commit in #1198). Having an isolated ecmult benchmark for it still seems to make sense, imho. Note that the teardown function is a bit hacky; since we don't have the parity of the x-only point multiplication results, the full point multiplication ones are calculated and the x coordinates are compared, and bench_ecmult_teardown_helper isn't used.

@stratospher
Copy link
Contributor

oh interesting to see the result! I'm getting something similar on my machine too. Looks like ~54% of ecmult_const_xonly's execution time is spent in ecmult_const (inside the ecmult_const_xonly).

with d = NULL

also I ran the benchmarks in b62b7b7 -bench_ecdh was few microseconds slower/bench_ecdh_xonly was few microseconds faster. But when I remove secp256k1_ec_pubkey_parse from bench_ecdh, I'm seeing a similar performance trend in the ecdh, ecdh_xonly benchmark - bench_ecdh_xonly is a few microseconds slower/bench_ecdh is few microseconds faster.

Benchmark                     ,    Min(us)    ,    Avg(us)    ,    Max(us)

ecdh                          ,    23.5       ,    24.1       ,    26.0    
ecdh_xonly                    ,    25.0       ,    25.1       ,    25.2  

@real-or-random
Copy link
Contributor

real-or-random commented Mar 31, 2025

@stratospher Just out of curiosity, how did you generate this nice graph?

@stratospher
Copy link
Contributor

CLion has a 1 click option to generate flame graphs. 😊

@sipa
Copy link
Contributor

sipa commented May 12, 2025

ACK cc71265

@theStack theStack force-pushed the add-ecmult_const_xonly-benchmark branch from cc71265 to 0544537 Compare May 14, 2025 15:46
Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

ACK 0544537

@jonasnick jonasnick merged commit 9fab425 into bitcoin-core:master May 14, 2025
117 checks passed
@theStack theStack deleted the add-ecmult_const_xonly-benchmark branch May 14, 2025 20:03
vmta added a commit to umkoin/umkoin that referenced this pull request May 22, 2025
9fab42525 Merge bitcoin-core/secp256k1#1668: bench_ecmult: add benchmark for ecmult_const_xonly
05445377f bench_ecmult: add benchmark for ecmult_const_xonly
bb597b3d3 Merge bitcoin-core/secp256k1#1670: tests: update wycheproof files
d73ed9947 tests: update wycheproof files
4187a4664 Merge bitcoin-core/secp256k1#1492: tests: Add Wycheproof ECDH vectors
e266ba11a tests: Add Wycheproof ECDH vectors
13906b715 Merge bitcoin-core/secp256k1#1669: gitignore: Add Python cache files
c1bcb0327 gitignore: Add Python cache files
70f149b9a Merge bitcoin-core/secp256k1#1662: bench: add ellswift to bench help output
6b3fe51fb bench: add ellswift to bench help output
d84bb83e2 Merge bitcoin-core/secp256k1#1661: configure: Show exhaustive tests in summary
3f54ed8c1 Merge bitcoin-core/secp256k1#1659: include: remove WARN_UNUSED_RESULT for functions always returning 1
20b05c9d3 configure: Show exhaustive tests in summary
e56716a3b Merge bitcoin-core/secp256k1#1660: ci: Fix exiting from ci.sh on error
d87c3bc58 ci: Fix exiting from ci.sh on error
1b6e08153 include: remove WARN_UNUSED_RESULT for functions always returning 1
2abb35b03 Merge bitcoin-core/secp256k1#1657: tests: remove unused uncounting_illegal_callback_fn
51907fa91 tests: remove unused uncounting_illegal_callback_fn
a7a511714 Merge bitcoin-core/secp256k1#1359: Fix symbol visibility issues, add test for it
13ed6f65d Merge bitcoin-core/secp256k1#1593: Remove deprecated `_ec_privkey_{negate,tweak_add,tweak_mul}` aliases from API
d1478763a build: Drop no longer needed  `-fvisibility=hidden` compiler option
8ed1d83d9 ci: Run `tools/symbol-check.py`
41d32ab2d test: Add `tools/symbol-check.py`
88548058b Introduce `SECP256K1_LOCAL_VAR` macro
03bbe8c61 Merge bitcoin-core/secp256k1#1655: gha: Print all *.log files, in a separate action
59860bcc2 gha: Print all *.log files, in a separate action
4ba1ba2af Merge bitcoin-core/secp256k1#1647: cmake: Adjust diagnostic flags for `clang-cl`
abd25054a Merge bitcoin-core/secp256k1#1656: musig: Fix clearing of pubnonces
961ec25a8 musig: Fix clearing of pubnonces
318608238 Merge bitcoin-core/secp256k1#1614: Add _ge_set_all_gej and use it in musig for own public nonces
6c2a39daf Merge bitcoin-core/secp256k1#1639: Make static context const
37d2c60be Remove deprecated _ec_privkey_{negate,tweak_add,tweak_mul} aliases
432ac5770 Make static context const
1b1fc0934 Merge bitcoin-core/secp256k1#1642: Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize`
c0d9480fb Merge bitcoin-core/secp256k1#1654: use `EXIT_` constants over magic numbers for indicating program execution status
13d389629 CONTRIBUTING: mention that `EXIT_` codes should be used
c85558172 test, bench, precompute_ecmult: use `EXIT_...` constants for `main` return values
965393fce examples: use `EXIT_...` constants for `main` return values
2e3bf1365 Merge bitcoin-core/secp256k1#1646: README: add instructions for verifying GPG signatures
b682dbcf8 README: add instructions for verifying GPG signatures
00774d072 Merge bitcoin-core/secp256k1#1650: schnorrsig: clear out masked secret key in BIP-340 nonce function
a82287fb8 schnorrsig: clear out masked secret key in BIP-340 nonce function
4c50d73dd ci: Add new "Windows (clang-cl)" job
84c0bd1f7 cmake: Adjust diagnostic flags for clang-cl
f79f46c70 Merge bitcoin-core/secp256k1#1641: doc: Improve cmake instructions in README
2ac9f558c doc: Improve cmake instructions in README
182359476 Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize`
64228a648 musig: Use _ge_set_all_gej for own public nonces
300aab1c0 tests: Improve _ge_set_all_gej(_var) tests
365f274ce group: Simplify secp256k1_ge_set_all_gej
d3082ddea group: Add constant-time secp256k1_ge_set_all_gej

git-subtree-dir: src/secp256k1
git-subtree-split: 9fab4252567661574cc9f6f97a057884f8129ff2
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.

5 participants