Skip to content

secp256k1: drop pure-Python fallback, add reproducible Docker build environment#135

Open
kdmukai wants to merge 7 commits into
diybitcoinhardware:masterfrom
kdmukai:feature/libsecp-builder
Open

secp256k1: drop pure-Python fallback, add reproducible Docker build environment#135
kdmukai wants to merge 7 commits into
diybitcoinhardware:masterfrom
kdmukai:feature/libsecp-builder

Conversation

@kdmukai

@kdmukai kdmukai commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Replace the pure-Python secp256k1 fallback (src/embit/util/py_secp256k1.py) with a hard libsecp256k1 requirement at import time, and add docker/ as a fully-pinned build environment that replaces the prebuilt-binary distribution path removed in #92e016b.

Motivation

The pure-Python implementation diverges from libsecp behavior. See #125 discussion r3243767405: the two backends produce different results on the same Liquid call sites because the ctypes path mutates through a C pointer while the Python path tries to mutate an immutable bytes. Maintaining two implementations of the same primitives is not sustainable, and this PR continues the direction set by 92e016b.

Changes

  • Delete src/embit/util/py_secp256k1.py.
  • Rewrite src/embit/util/secp256k1.py to fail fast at import with Libsecp256k1NotAvailable (a subclass of ImportError) when no backend resolves. The underlying load error is chained as __cause__.
  • New docker/ directory (Dockerfile, build.sh, README) providing a reproducible build environment for linux/amd64, armv6l (Pi Zero), armv7l, aarch64, and windows/amd64. Pinning details in docker/README.md.
  • CI: extend the existing tests job to build via the container, add a tests-macos native build job, add a cross-compile-validation job that builds all five targets and publishes a SHA256SUMS artifact and workflow step-summary.
  • Tests: drop the cross-backend parity assertions in test_bindings.py (replaced with self-consistency checks). Update the skip messages on the existing @skipUnless(hasattr(secp256k1, ...)) gates in test_ecdh.py, test_liquid.py, test_threading.py to remove references to the deleted pure-Python fallback. The gates still serve their original purpose: skipping cleanly when the loaded library lacks an optional module.
  • Docs: README.md, secp256k1/README.md, docker/README.md updated. One-line comment in secp256k1/Makefile about the future upstream swap.

Deliberately out of scope

  • The secp256k1-zkp submodule pointer is not bumped.
  • ctypes_secp256k1.py is not modified. The wrapper's existing legacy privkey_* and pre-BIP340 schnorrsig_* bindings continue to work against the currently pinned zkp commit, but will fail against modern zkp or upstream. Adding a seckey_* fallback and a Schnorr ABI probe is a prerequisite for any submodule bump and belongs in a follow-up PR.

Verification

# Pull in the secp256k1-zkp submodule.
git submodule update --init --recursive

# Native libsecp build + stage at the loader-preferred path.
cd secp256k1 && make && cd ..
mkdir -p secp256k1/secp256k1-zkp/.libs
cp secp256k1/build/libsecp256k1_*.{dylib,so} secp256k1/secp256k1-zkp/.libs/libsecp256k1.${EXT}

# Full test suite against the new wrapper.
pytest

# Build the container and cross-compile every supported target.
# --user keeps output files owned by your shell user (essential on Linux).
docker build -t embit-libsecp docker/
docker run --rm --user "$(id -u):$(id -g)" \
    -v "$PWD":/embit embit-libsecp all

# Action-pin policy.
python tools/verify_github_actions_pins.py --self-test

# Reproducibility check: compare these hashes to the SHA256SUMS that CI's
# cross-compile-validation job emits at the top of its workflow run.
sha256sum secp256k1/build/libsecp256k1_*

Locally on Apple Silicon: 96 tests pass; Docker image builds and produces working binaries; action-pin policy is green. The local-vs-CI SHA256SUMS comparison can only be exercised once this PR's CI run completes.

Delete py_secp256k1.py and require libsecp256k1 at import time. The
wrapper raises Libsecp256k1NotAvailable (an ImportError subclass) when
the backend can't load.

Add docker/ as a fully-pinned build environment for libsecp256k1, with
CI jobs that build via the container, build natively on macOS, and
publish SHA256SUMS for the cross-compiled targets.

The zkp submodule pointer and the ctypes wrapper bindings are deliberately
untouched.
Copilot AI review requested due to automatic review settings June 13, 2026 17:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR makes libsecp256k1 a hard runtime requirement on CPython, removes the pure-Python secp256k1 fallback, and adds a Docker-based (reproducible) build workflow plus CI coverage for building/staging the required native library.

Changes:

  • Replace “optional ctypes backend + Python fallback” with a fail-fast import error (Libsecp256k1NotAvailable) when bindings can’t be loaded.
  • Add Docker build tooling/docs and CI jobs to build/stage libsecp256k1 for supported targets.
  • Update tests to skip cleanly when optional secp256k1 modules (ECDH / Liquid-ZKP extras) are missing.

Reviewed changes

Copilot reviewed 12 out of 14 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
src/embit/util/secp256k1.py Centralizes backend selection and enforces fail-fast behavior when bindings aren’t loadable.
src/embit/util/py_secp256k1.py Removes the pure-Python fallback implementation.
tests/tests/test_bindings.py Updates bindings tests to target ctypes backend only (no Python parity).
tests/tests/test_ecdh.py Adds capability-based skipping for optional ECDH module.
tests/tests/test_liquid.py Adds capability-based skipping for Liquid/ZKP-required symbols.
docker/Dockerfile Introduces pinned, reproducible cross-toolchain build image for libsecp256k1.
docker/build.sh Adds a build wrapper to invoke the Makefile for named targets and stage artifacts.
docker/README.md Documents Docker build workflow, targets, and reproducibility properties.
secp256k1/README.md Updates native build + staging instructions and clarifies “no fallback on CPython”.
.github/workflows/ci.yml Builds/stages libsecp256k1 in CI (Linux+macOS) and validates cross-compiled outputs.
.gitignore Ignores local build outputs (secp256k1/build/) and ccache state (.ccache/).
README.md Updates installation/runtime expectations and points to the new build paths.
secp256k1/Makefile Adds clarifying comments about the zkp fork dependency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread docker/build.sh Outdated
Comment thread docker/build.sh
Comment thread tests/tests/test_bindings.py Outdated
Comment thread tests/tests/bindings/test_bindings.py
Comment thread tests/tests/bindings/test_bindings.py
Comment thread src/embit/util/secp256k1.py Outdated
Comment thread src/embit/util/secp256k1.py Outdated
Comment thread src/embit/util/secp256k1.py Outdated
Comment thread .github/workflows/ci.yml Outdated
Copilot AI review requested due to automatic review settings June 13, 2026 18:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 15 changed files in this pull request and generated 5 comments.

Comment thread docker/build.sh Outdated
Comment thread docker/build.sh
Comment thread src/embit/util/secp256k1.py Outdated
Comment thread tests/tests/bindings/test_bindings.py
Comment thread tests/tests/test_bindings.py Outdated
Copilot AI review requested due to automatic review settings June 13, 2026 19:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 15 changed files in this pull request and generated 6 comments.

Comment thread .github/workflows/ci.yml Outdated
Comment thread src/embit/util/secp256k1.py Outdated
Comment thread docker/Dockerfile Outdated
Comment thread tests/tests/test_bindings.py Outdated
Comment thread tests/tests/test_bindings.py Outdated
Comment thread src/embit/util/secp256k1.py
@kdmukai kdmukai marked this pull request as ready for review June 13, 2026 20:57
@kdmukai kdmukai requested review from miketlk and odudex as code owners June 13, 2026 20:57
Copilot AI review requested due to automatic review settings June 13, 2026 20:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 19 changed files in this pull request and generated 2 comments.

Comment thread src/embit/util/secp256k1.py Outdated
Comment thread docker/build.sh Outdated

@odudex odudex left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

tACK
Great improvement, using THE single source of true is good for reliability and management, especially on future updates.

Some AI nit catches:
1. The ec_privkey_tweak_mul gate is inert (Medium).
secp256k1.py:80-82 prunes ec_privkey_tweak_mul when the C symbol is missing, but _init() dereferences it unconditionally at ctypes_secp256k1.py:188 (core block, not a try/except like the optional modules). So on an upstream libsecp ≥ v0.7.0 that dropped the symbol, line 188 raises AttributeError → the whole ctypes_secp256k1 import fails → secp256k1.py raises Libsecp256k1NotAvailable, and the pruning loop never runs. In the exact scenario the gate documents, test_liquid doesn't skip cleanly — collection fails outright. No impact today (the pinned zkp has the symbol), but the selector-side gate alone is misleading. Suggest folding it into the same follow-up that wraps line 188 in try/except, or a one-line comment that the entry is inert until then.

2. build.sh all leaves a wrong-arch .so staged (Low).
Each target stages to .libs/libsecp256k1.so (build.sh:170); in all the loop runs amd64 → armv6l → armv7l → aarch64, so the staged .so ends up aarch64. A dev who runs docker run … all on x86_64 then pytest hits a wrong-arch lib at the loader-first path; the loader falls through (OSError caught), so with no system libsecp the import fails. CI is unaffected (tests uses single-target amd64; cross-compile-validation reads build/). Suggest having all skip staging / stage only the native arch, or a note in docker/README.md.

3. test_liquid gates on ec_pubkey_tweak_mul, which is never pruned (Low).
_LIQUID_REQUIRED_FUNCS (test_liquid.py:20) lists ec_pubkey_tweak_mul, but only the privkey variant is in _OPTIONAL_SYMBOLS, and pubkey_tweak_mul is set unconditionally in _init() — so the hasattr is always True. Harmless, but the comment "the tweak_mul operations upstream secp256k1 deprecated" overstates it: upstream removed only the seckey/privkey variant; secp256k1_ec_pubkey_tweak_mul still exists.

Verified clean: exception chaining + Libsecp256k1NotAvailable-before-ImportError ordering; selector pruning for the genuinely-optional ecdh/schnorr/recovery/zkp symbols; replacement of cross-backend parity with self-consistency + 1·G KAT; action pinning (third-party SHA-pinned, first-party tag-pinned per policy); CI --user ownership, deterministic fixed-order sha256sum, file | grep arch assertions failing correctly under bash -eo pipefail, submodules: recursive everywhere; PE32 --no-insert-timestamp reproducibility.

Recommendation: address #1 (at least a comment) before merge; #2 and #3 are optional polish.

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.

3 participants