infrastructure: modernize vendored secp256k1 to bitcoin-core v0.6.0 - #541
Open
fpelliccioni wants to merge 1 commit into
Open
infrastructure: modernize vendored secp256k1 to bitcoin-core v0.6.0#541fpelliccioni wants to merge 1 commit into
fpelliccioni wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (185)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughECDSA signature handling now uses libsecp256k1 parsing and serialization APIs instead of opaque-layout byte copies. Related endorsement, message-signing, and elliptic-curve test vectors are updated. ChangesECDSA signature handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Replace the old (~2018 BCH fork) libsecp256k1 with bitcoin-core/secp256k1 v0.6.0. This brings the ElligatorSwift and BIP340 schnorrsig modules that the upcoming SV2 Noise handshake needs, plus recovery/extrakeys/ecdh. BCH's custom EC-Schnorr-SHA256 scheme (consensus-critical, not BIP340) is re-grafted onto the modern internals as a local module (ENABLE_MODULE_SCHNORR_BCH): - secp256k1_ecmult no longer takes an ecmult_context (now static), - fe_is_quad_var -> fe_is_square_var, - gej_has_quad_y_var (removed upstream) reimplemented as is_square(y*z), - fe_set_b32 -> fe_set_b32_limit. The scheme is unchanged; verified against the old fork's known-answer vector and all BCH-Schnorr consensus script vectors. The infrastructure ECDSA wrapper stored the opaque secp256k1_ecdsa_signature .data blob in ec_signature; that layout is implementation-defined and differs between the two libraries. The wrapper now round-trips through the stable compact (r||s) serialization APIs, so ec_signature holds the canonical compact form. Signature *verification* is byte-identical; deterministic *generation* now follows modern RFC6979, producing different-but-valid signatures, so the hardcoded ECDSA/message/endorsement golden vectors are updated accordingly. The modern build self-generates its precomputed tables, so the secp256k1-precompute tool_requires, the gen_context table-generation step and the obsolete per-module conan options are removed. secp256k1 no longer depends on GMP. Assembly selection moves to the modern SECP256K1_ASM (option secp256k1_asm=auto|off; the ASan CI build uses off).
fpelliccioni
force-pushed
the
feature/secp256k1-modernize
branch
from
July 31, 2026 16:33
43bef59 to
3e49360
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Replaces the old (~2018 BCH fork) vendored libsecp256k1 with bitcoin-core/secp256k1 v0.6.0, while re-grafting BCH's consensus-critical custom Schnorr scheme onto the modern internals.
Why
The upcoming SV2 Noise handshake needs ElligatorSwift and BIP340 schnorrsig, which the old fork does not provide. This also modernizes the ECDSA/EC core (recovery, extrakeys, ecdh) and drops the external precompute tooling.
BCH Schnorr re-graft
BCH's EC-Schnorr-SHA256 scheme (consensus-critical, not BIP340) is ported as a local module
ENABLE_MODULE_SCHNORR_BCH:secp256k1_ecmultno longer takes anecmult_context(static now)fe_is_quad_var→fe_is_square_vargej_has_quad_y_var(removed upstream) reimplemented asis_square(y·z)fe_set_b32→fe_set_b32_limitThe scheme itself is unchanged. Validated against the old fork's known-answer vector (pubkey=G, msg=0 → verify=1; tampered → 0; sign/verify round-trip) and all BCH-Schnorr consensus script vectors.
ec_signature representation
The infrastructure ECDSA wrapper used to copy the opaque
secp256k1_ecdsa_signature.datablob intoec_signature. That layout is implementation-defined and differs between the two libraries. The wrapper now round-trips through the stable compact (r‖s) serialization APIs, soec_signatureholds the canonical compact form.Build / packaging
secp256k1-precomputetool_requiresand thegen_contextgeneration step.SECP256K1_ASM(conan optionsecp256k1_asm=auto|off; the ASan CI build usesoff).Validation
All suites pass: infrastructure (456), domain / consensus+scripts (3820, ~1.0M assertions), blockchain (133), blockchain_vmlimits (21), network (85), node (96), c-api (735). Clean
conan install+ fresh CMake configure verified.Summary by CodeRabbit
Bug Fixes
Tests