Update libsecp256k1 subtree to current master #29169

fanquake commented at 4:07 PM on January 3, 2024: member

This includes changes from the 0.4.1 release: https://github.com/bitcoin-core/secp256k1/releases/tag/v0.4.1.

The point multiplication algorithm used for ECDH operations (module ecdh) was replaced with a slightly faster one.

Optional handwritten x86_64 assembly for field operations was removed because modern C compilers are able to output more efficient assembly. This change results in a significant speedup of some library functions when handwritten x86_64 assembly is enabled (--with-asm=x86_64 in GNU Autotools, -DSECP256K1_ASM=x86_64 in CMake), which is the default on x86_64. Benchmarks with GCC 10.5.0 show a 10% speedup for secp256k1_ecdsa_verify and secp256k1_schnorrsig_verify.

DrahtBot commented at 4:07 PM on January 3, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, jonasnick
Concept ACK	real-or-random
Stale ACK	sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

sipa commented at 4:10 PM on January 3, 2024: member

Obvious ACK c13a17c6996442f04635bdf70ee8f06bf6854ff6 is obvious.

fanquake commented at 4:11 PM on January 3, 2024: member

cc @real-or-random @jonasnick

hebasto commented at 4:29 PM on January 3, 2024: member

Shouldn't the subtree be synced to the v0.4.1 tag instead of the current master branch?

fanquake commented at 4:30 PM on January 3, 2024: member

Shouldn't the subtree be synced to the v0.4.1 tag instead of the current master branch?

Why? It makes 0 difference.

fanquake commented at 4:31 PM on January 3, 2024: member

I would also prefer not to introduce some policy about only using tags, or similar, because we want to retain the ability to update the subtree to any commit we'd like, at any point.

hebasto approved

hebasto commented at 4:35 PM on January 3, 2024: member

ACK c13a17c6996442f04635bdf70ee8f06bf6854ff6, I've updated the secp256k1 to its current master branch (efe85c70a2e357e3605a8901a9662295bae1001f) locally and got zero diff with this PR.

However, the PR title and description are a bit misleading as they mention "0.4.1 release", which implies (at least for me) syncing to the v0.4.1 tag.

jonasnick approved

jonasnick commented at 8:05 PM on January 3, 2024: contributor

ACK c13a17c6996442f04635bdf70ee8f06bf6854ff6 no difference to my locally checked out version.

real-or-random approved

real-or-random commented at 9:27 PM on January 3, 2024: contributor

Concept ACK c13a17c6996442f04635bdf70ee8f06bf6854ff6 I haven't checked that the subtree is correct but no objections to update to this commit

However, the PR title and description are a bit misleading as they mention "0.4.1 release", which implies (at least for me) syncing to the v0.4.1 tag.

True, this updates to the merge commit of the "cleanup" PR after the release. But yeah, it really doesn't matter in the end, and I think there's no need to invalidate the ACKs here.

luke-jr commented at 10:44 PM on January 3, 2024: member

It makes a minor difference of reporting the correct version number. I agree with @hebasto on using the tag, but also agree with @fanquake on not having a policy that we must use tags.

maflcko added the label DrahtBot Guix build requested on Jan 4, 2024

fanquake commented at 11:32 AM on January 4, 2024: member

It makes a minor difference of reporting the correct version number.

What do you mean by/where are we reporting the version number?

sipa commented at 1:24 PM on January 4, 2024: member

What do you mean by/where are we reporting the version number?

The commit title, "Update secp256k1 subtree to upstream release 0.4.1", makes it sound like it's updating to the exact 0.4.1 tag. I agree it's fine to update to master, and that we shouldn't need a policy of only using tagged releases, but it'd be better if the commit title said "Update secp256k1 subtree to current master", or "to commit after 0.4.1".

real-or-random commented at 2:17 PM on January 4, 2024: contributor

What do you mean by/where are we reporting the version number?

Plus, ./configure --help in the subtree mentions libsecp256k1 0.4.2-dev [...], but that's really a minor thing.

Squashed 'src/secp256k1/' changes from 199d27cea3..efe85c70a2

efe85c70a2 Merge bitcoin-core/secp256k1#1466: release cleanup: bump version after 0.4.1
4b2e06f460 release cleanup: bump version after 0.4.1
1ad5185cd4 Merge bitcoin-core/secp256k1#1465: release: prepare for 0.4.1
672053d801 release: prepare for 0.4.1
1a81df826e Merge bitcoin-core/secp256k1#1380: Add ABI checking tool for release process
74a4d974d5 doc: Add ABI checking with `check-abi.sh` to the Release Process
e7f830e32c Add `tools/check-abi.sh`
77af1da9f6 Merge bitcoin-core/secp256k1#1455: doc: improve secp256k1_fe_set_b32_mod doc
3928b7c383 doc: improve secp256k1_fe_set_b32_mod doc
5e9a4d7aec Merge bitcoin-core/secp256k1#990: Add comment on length checks when parsing ECDSA sigs
4197d667ec Merge bitcoin-core/secp256k1#1431: Add CONTRIBUTING.md
0e5ea62207 CONTRIBUTING: add some coding and style conventions
e2c9888eee Merge bitcoin-core/secp256k1#1451: changelog: add entry for "field: Remove x86_64 asm"
d2e36a2b81 changelog: add entry for "field: Remove x86_64 asm"
1a432cb982 README: update first sentence
0922a047fb docs: move coverage report instructions to CONTRIBUTING
76880e4015 Add CONTRIBUTING.md including scope and guidelines for new code
d3e29db8bb Merge bitcoin-core/secp256k1#1450: Add group.h ge/gej equality functions
04af0ba162 Replace ge_equals_ge[,j] calls with group.h equality calls
60525f6c14 Add unit tests for group.h equality functions
a47cd97d51 Add group.h ge/gej equality functions
10e6d29b60 Merge bitcoin-core/secp256k1#1446: field: Remove x86_64 asm
07687e811d Merge bitcoin-core/secp256k1#1393: Implement new policy for VERIFY_CHECK and #ifdef VERIFY (issue #1381)
bb4672342e remove VERIFY_SETUP define
a3a3e11acd remove unneeded VERIFY_SETUP uses in ECMULT_CONST_TABLE_GET_GE macro
a0fb68a2e7 introduce and use SECP256K1_SCALAR_VERIFY macro
cf25c86d05 introduce and use SECP256K1_{FE,GE,GEJ}_VERIFY macros
5d89bc031b remove superfluous `#ifdef VERIFY`/`#endif` preprocessor conditions
c2688f8de9 redefine VERIFY_CHECK to empty in production (non-VERIFY) mode
5814d8485c Merge bitcoin-core/secp256k1#1438: correct assertion for secp256k1_fe_mul_inner
c1b4966410 Merge bitcoin-core/secp256k1#1445: bench: add --help option to bench_internal
f07cead0ca build: Don't call assembly an optimization
2f0762fa8f field: Remove x86_64 asm
1ddd76af0a bench: add --help option to bench_internal
e72103932d Merge bitcoin-core/secp256k1#1441: asm: add .note.GNU-stack section for non-exec stack
ea47c82e01 Merge bitcoin-core/secp256k1#1442: Return temporaries to being unsigned in secp256k1_fe_sqr_inner
dcdda31f2c Tighten secp256k1_fe_mul_inner's VERIFY_BITS checks
10271356c8 Return temporaries to being unsigned in secp256k1_fe_sqr_inner
33dc7e4d3e asm: add .note.GNU-stack section for non-exec stack
c891c5c2f4 Merge bitcoin-core/secp256k1#1437: ci: Ignore internal errors of snapshot compilers
8185e72d29 ci: Ignore internal errors in snapshot compilers
40f50d0fbd Merge bitcoin-core/secp256k1#1184: Signed-digit based ecmult_const algorithm
8e2a5fe908 correct assertion for secp256k1_fe_mul_inner
355bbdf38a Add changelog entry for signed-digit ecmult_const algorithm
21f49d9bec Remove unused secp256k1_scalar_shr_int
115fdc7232 Remove unused secp256k1_wnaf_const
aa9f3a3c00 ecmult_const: add/improve tests
4d16e90111 Signed-digit based ecmult_const algorithm
ba523be067 make SECP256K1_SCALAR_CONST reduce modulo exhaustive group order
2140da9cd5 Add secp256k1_scalar_half for halving scalars (+ tests/benchmarks).
1f1bb78b7f Merge bitcoin-core/secp256k1#1430: README: remove CI badge
5dab0baa80 README: remove CI badge
b314cf2833 Merge bitcoin-core/secp256k1#1426: ci/cirrus: Add native ARM64 jobs
fa4d6c76b6 ci/cirrus: Add native ARM64 persistent workers
ee7aaf213e Merge bitcoin-core/secp256k1#1395: tests: simplify `random_fe_non_zero` (remove loop limit and unneeded normalize)
ba9cb6f378 Merge bitcoin-core/secp256k1#1424: ci: Bump major versions for docker actions
d9d80fd155 ci: Bump major versions for docker actions
4fd00f4bfe Merge bitcoin-core/secp256k1#1422: cmake: Install `libsecp256k1.pc` file
421d84855a ci: Align Autotools/CMake `CI_INSTALL` directory names
9f005c60d6 cmake: Install `libsecp256k1.pc` file
2262d0eaab ci/cirrus: Bring back skeleton .cirrus.yml without jobs
b10ddd2bd2 Merge bitcoin-core/secp256k1#1416: doc: Align documented scripts with CI ones
49be5be9e8 Merge bitcoin-core/secp256k1#1390: tests: Replace counting_illegal_callbacks with CHECK_ILLEGAL_VOID
cbf3053ff1 Merge bitcoin-core/secp256k1#1417: release cleanup: bump version after 0.4.0
9b118bc7fb release cleanup: bump version after 0.4.0
70303643cf tests: add CHECK_ERROR_VOID and use it in scratch tests
f8d7ea68df tests: Replace counting_illegal_callbacks with CHECK_ILLEGAL_VOID
b0f7bfedc9 doc: Do not mention soname in CHANGELOG.md "ABI Compatibility" section
bd9d98d353 doc: Align documented scripts with CI ones
a1d52e3e12 tests: remove unnecessary test in run_ec_pubkey_parse_test
875b0ada25 tests: remove unnecessary set_illegal_callback
c45b7c4fbb refactor: introduce testutil.h (deduplicate `random_fe_`, `ge_equals_` helpers)
dc5514144f tests: simplify `random_fe_non_zero` (remove loop limit and unneeded normalize)
e02f313b1f Add comment on length checks when parsing ECDSA sigs

git-subtree-dir: src/secp256k1
git-subtree-split: efe85c70a2e357e3605a8901a9662295bae1001f

29fde0223a

Update secp256k1 subtree to latest master e2cdeb5925

fanquake renamed this:
~~Update libsecp256k1 subtree for 0.4.1 release~~
Update libsecp256k1 subtree to current master
on Jan 4, 2024

fanquake force-pushed on Jan 4, 2024

fanquake commented at 2:55 PM on January 4, 2024: member

I've updated to commit message to make it clear that this isn't exactly the 0.4.1 tag.

Plus, ./configure --help in the subtree mentions

I was thinking more like output from bitcoind or similar.

hebasto approved

hebasto commented at 3:06 PM on January 4, 2024: member

re-ACK e2cdeb592596432039d21f4c819d45f1e46d65ef

DrahtBot requested review from real-or-random on Jan 4, 2024

DrahtBot requested review from sipa on Jan 4, 2024

DrahtBot requested review from jonasnick on Jan 4, 2024

real-or-random approved

real-or-random commented at 3:34 PM on January 4, 2024: contributor

Concept ACK https://github.com/bitcoin/bitcoin/commit/c13a17c6996442f04635bdf70ee8f06bf6854ff6 I haven't checked that the subtree is correct but no objections to update to this commit

DrahtBot removed review request from jonasnick on Jan 4, 2024

DrahtBot requested review from jonasnick on Jan 4, 2024

DrahtBot removed review request from jonasnick on Jan 4, 2024

DrahtBot requested review from jonasnick on Jan 4, 2024

jonasnick commented at 3:42 PM on January 4, 2024: contributor

reACK e2cdeb592596432039d21f4c819d45f1e46d65ef

DrahtBot requested review from real-or-random on Jan 4, 2024

DrahtBot commented at 4:36 PM on January 4, 2024: contributor

Guix builds (on x86_64)

File	commit 65c05db660b2ca1d0076b0d8573a6760b3228068<br>(master)	commit b313658062531fc53e9bb79fa85cec1da85d8682<br>(master and this pull)
SHA256SUMS.part	`d97ad5d47c095d44...`	`2710c9aa57b739b8...`
*-aarch64-linux-gnu-debug.tar.gz	`c3db617f3148f66f...`	`c9b01ade848b9b76...`
*-aarch64-linux-gnu.tar.gz	`b2cf69e9adb2ed95...`	`fdd77f64130ddad2...`
*-arm-linux-gnueabihf-debug.tar.gz	`924b3b035081a1df...`	`07056c0ce8a08731...`
*-arm-linux-gnueabihf.tar.gz	`086a7297ab18d363...`	`48f72b1789c49c76...`
*-arm64-apple-darwin-unsigned.tar.gz	`7fb7559d02277a55...`	`0c96ec7045459b49...`
*-arm64-apple-darwin-unsigned.zip	`4de0cca0c1ad3765...`	`0de35b33b859498f...`
*-arm64-apple-darwin.tar.gz	`ef3406f2e73ac212...`	`ddb1f32e01dc2570...`
*-powerpc64-linux-gnu-debug.tar.gz	`7d4f91457cf2b2fb...`	`683fc572b205fc7d...`
*-powerpc64-linux-gnu.tar.gz	`001a8faa69f350c8...`	`2c27aee60d711f26...`
*-powerpc64le-linux-gnu-debug.tar.gz	`8cc12ee8e740aa9b...`	`0778c777069d9c1d...`
*-powerpc64le-linux-gnu.tar.gz	`b0b5df8e35ce5d72...`	`5d5160e4771bb0d3...`
*-riscv64-linux-gnu-debug.tar.gz	`bf6762a840dc9b6a...`	`8076f50fb7e6e247...`
*-riscv64-linux-gnu.tar.gz	`cb09fb0705923f3e...`	`04761fb0f0885a82...`
*-x86_64-apple-darwin-unsigned.tar.gz	`388d2d01d87e50c1...`	`d3090c1261dd4a04...`
*-x86_64-apple-darwin-unsigned.zip	`6089e05883e95fa2...`	`c11c87f069c60885...`
*-x86_64-apple-darwin.tar.gz	`dc4a5cb0b8bd0c60...`	`b8452be38558c11c...`
*-x86_64-linux-gnu-debug.tar.gz	`adf8ebf8576ee97d...`	`58f44af28be56e00...`
*-x86_64-linux-gnu.tar.gz	`598868ef1a2e0b40...`	`4c81fe48e76521d6...`
*.tar.gz	`9038887954759e98...`	`cef78a6e192c7aec...`
guix_build.log	`8e4b8d8a1a0d9049...`	`d2e1fb2a3fcae379...`
guix_build.log.diff		`612e9f2abc01baab...`

DrahtBot removed the label DrahtBot Guix build requested on Jan 4, 2024

glozow merged this on Jan 4, 2024

glozow closed this on Jan 4, 2024

fanquake deleted the branch on Jan 4, 2024

jamesob commented at 2:32 PM on January 17, 2024: contributor

Oddly enough, this change benched out as being slightly slower (~4.8%) than the commit before it in master.

Not a huge regression, but not what I expected. Compiled with gcc 10.2.1.

ibd local range dbcache=4000 667200 697200

#29169 vs. f8ca1357 (absolute)

bench name	x	#29169	f8ca1357
ibd.local.range.dbcache=4000.667200.697200.total_secs	2	13522.9734 (± 42.0521)	12901.7487 (± 6.9377)
ibd.local.range.dbcache=4000.667200.697200.peak_rss_KiB	2	4431630.0000 (± 5274.0000)	4431262.0000 (± 1938.0000)
ibd.local.range.dbcache=4000.667200.697200.cpu_kernel_secs	2	462.5950 (± 1.8850)	459.9700 (± 0.5700)
ibd.local.range.dbcache=4000.667200.697200.cpu_user_secs	2	47645.5000 (± 175.3900)	45210.2300 (± 37.1900)

#29169 vs. f8ca1357 (relative)

bench name	x	#29169	f8ca1357
ibd.local.range.dbcache=4000.667200.697200.total_secs	2	1.0481504324562638	1
ibd.local.range.dbcache=4000.667200.697200.peak_rss_KiB	2	1.0000830463195360	1
ibd.local.range.dbcache=4000.667200.697200.cpu_kernel_secs	2	1.0057068939278648	1
ibd.local.range.dbcache=4000.667200.697200.cpu_user_secs	2	1.0538654636351110	1

real-or-random commented at 3:51 PM on January 17, 2024: contributor

Hm, this is really not expected.

Compiled with gcc 10.2.1.

You could see if gcc 10.5.0 makes a difference. AFAIU this is used in the official builds currently (until #27897 lands), and this was one of our considerations.

If that doesn't help, please benchmark libsecp256k1 with (https://github.com/bitcoin-core/secp256k1/commit/10e6d29b60c3931e327bc18e6c50cea78296b1ba) and without https://github.com/bitcoin-core/secp256k1/pull/1446 (https://github.com/bitcoin-core/secp256k1/commit/07687e811d1c9700e6fe9d658aef080e3568c0f1), see the PR for instructions and also consider setting SECP256K1_BENCH_ITERS=200000 or another large value.

jamesob commented at 6:24 PM on January 18, 2024: contributor

Quick update:

I did confirm (on another machine) that more contemporary versions of gcc (12.2.0 in this case) do show a speed up of about 4%, with slightly more time spent in kernel:

#29169 vs. f8ca1357 (relative)

bench name	x	#29169	f8ca1357
ibd.local.range.dbcache=4000.667200.697200.total_secs	2	1.0000000000000000	1.0400910327330894
ibd.local.range.dbcache=4000.667200.697200.peak_rss_KiB	2	1.0000000000000000	1.0018178271964506
ibd.local.range.dbcache=4000.667200.697200.cpu_kernel_secs	2	1.0225739275301957	1.0000000000000000
ibd.local.range.dbcache=4000.667200.697200.cpu_user_secs	2	1.0000000000000000	1.0711547761286089

I also confirmed that the gcc 10 result was not a fluke; a rerun again showed 5% regression. I'm rerunning with gcc 12 on that same machine and will post the results when they're up. I guess this is all expected since the libsecp change was advertised to be contingent on improvements in gcc.

jamesob commented at 2:19 PM on January 19, 2024: contributor

Oddly enough, even with gcc 12.2.0, the regression on the first machine holds. Artifacts here: https://gist.github.com/jamesob/1f4456f1f9bafcabb392210bbfe9f240

Will run the secp benches in isolation.

jamesob commented at 2:53 PM on January 19, 2024: contributor

If that doesn't help, please benchmark libsecp256k1 with (bitcoin-core/secp256k1@10e6d29) and without bitcoin-core/secp256k1#1446 (bitcoin-core/secp256k1@07687e8), see the PR for instructions and also consider setting SECP256K1_BENCH_ITERS=200000 or another large value.

These benchmarks are indeed slower on the first host when compiling with debian gcc 12.2.0-14. Data here: https://gist.github.com/jamesob/52133933b9728ab0f677563ef6bc554e

sipa commented at 3:33 PM on January 19, 2024: member

Wow, bizarre, but not impossible.

It seems that the handwritten asm code we had was faster on your CPU, but is slower on all CPUs we tested on.

Yours seems to be a fairly old architecture (2nd gen Intel Core, first released in 2011), so it's not impossible there are differences.

bitcoin locked this on Jan 18, 2025