Remove --enable-module-experimental from libsecp256k1 config in tests

Simplexum · Jun 29, 2023 · 5654a4c · 5654a4c
1 parent 231c527
commit 5654a4c
Show file tree

Hide file tree

Showing 10 changed files with 66 additions and 153 deletions.
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
@@ -31,8 +31,9 @@ jobs:
         elif [ "$RUNNER_OS" == "macOS" ]; then
           brew install automake libtool boost pkg-config libevent
         fi
-        sh -c 'git clone https://github.com/bitcoin/secp256k1.git /tmp/libsecp256k1 && cd /tmp/libsecp256k1 && git checkout v0.3.2 && ./autogen.sh && ./configure --disable-coverage --disable-benchmark --disable-tests --disable-exhaustive-tests --enable-module-recovery --enable-module-ecdh --enable-experimental --enable-module-schnorrsig && make && sudo make install'
-        sh -c 'git clone https://github.com/bitcoin/bitcoin.git /tmp/bitcoin && cd /tmp/bitcoin && git checkout v25.0 && ./autogen.sh && ./configure --without-qtdbus --without-qrencode --without-miniupnpc --disable-tests --disable-wallet --disable-zmq --with-libs --disable-util-cli --disable-util-tx --disable-util-wallet --disable-bench --without-daemon --without-gui --disable-fuzz --disable-ccache --disable-static --with-system-libsecp256k1 && make && sudo make install'
+        SECP256K1_VERSION=$(grep -A1 '!LIBSECP256K1_VERSION_MARKER_DO_NOT_MOVE_OR_EDIT!' README.md |tail -n 1|sed -E 's/`(v[0-9\.]+)`/\1/g')
+        sh -c "git clone https://github.com/bitcoin/secp256k1.git /tmp/libsecp256k1 && cd /tmp/libsecp256k1 && git checkout $SECP256K1_VERSION && ./autogen.sh && ./configure --disable-coverage --disable-benchmark --disable-tests --disable-exhaustive-tests --enable-module-recovery --enable-module-ecdh --enable-module-schnorrsig && make && sudo make install"
+        sh -c "git clone https://github.com/bitcoin/bitcoin.git /tmp/bitcoin && cd /tmp/bitcoin && git checkout v25.0 && ./autogen.sh && ./configure --without-qtdbus --without-qrencode --without-miniupnpc --disable-tests --disable-wallet --disable-zmq --with-libs --disable-util-cli --disable-util-tx --disable-util-wallet --disable-bench --without-daemon --without-gui --disable-fuzz --disable-ccache --disable-static --with-system-libsecp256k1 && make && sudo make install"
         python -m pip install flake8 pytest coverage mypy typing-extensions types-contextvars
     - name: Lint with flake8
       shell: bash
@@ -42,6 +43,5 @@ jobs:
       run: ./run_mypy.sh
     - name: Test with pytest
       env:
-        PYTHON_BITCOINTX_ALLOW_LIBSECP256K1_EXPERIMENTAL_MODULES_USE: 1
         LD_LIBRARY_PATH: /usr/local/lib
       run: pytest
diff --git a/README.md b/README.md
@@ -46,25 +46,22 @@ the library and v1.0.0 release in particular, and also has some code examples.
 - [libsecp256k1](https://github.com/bitcoin-core/secp256k1)
 - [libbitcoinconsensus](https://github.com/bitcoin/bitcoin/blob/master/doc/shared-libraries.md) (optional, for consensus-compatible script verification)
 
-It is recommended to build the libsecp256k1 library by hand, using the following commit:
+Tests use the following libsecp256k1 version:
 
-[//]: # (!LIBSECP256K1_COMMIT_MARKER_DO_NOT_MOVE_OR_EDIT! this marker is used by automatic tests to extract the commit hash that is in the following line from this README.md, and use it to run tests with this specific version of libsecp256k1)
-`7006f1b97fd8dbf4ef75771dd7c15185811c3f50`
+[//]: # (!LIBSECP256K1_VERSION_MARKER_DO_NOT_MOVE_OR_EDIT! this marker is used by automatic tests to extract the version that is in the following line from this README.md, and use it to run tests with this specific version of libsecp256k1)
+`v0.3.2`
 
 Libsecp256k1 is not linked as a git submodule in python-bitcointx git repository, because python-bitcointx
 can still be used with other versions of libsecp256k1 as long as experimental modules with unstable ABI
-of are not used, or are compatible with the vesion from the commit listed above. Please note that the ABI
-even for non-experimental modules of libsecp256k1 has no guarantees of not changing, as that library has no
-'release' version as of date.
+of are not used, or are compatible with the vesion listed above.
 
 While allowing dynamic linkage with libsecp256k1 adds these complications, it is at the same time allows
 more flexibility for advanced uses. For example, one can use libsecp256k1-zkp instead of libsecp256k1 to
 have access to zero-knowledge-proof related functions, as is done by python-elementstx package.
 
-For best results, use the version that corresponds to the commit hash listed above, as it is the commit
-that python-bitcointx automatic tests use to build libsecp256k1. Then make sure that this version of the
-library is loaded by python-bitcointx, by using `bitcointx.set_custom_secp256k1_path()`
-or `LD_LIBRARY_PATH ` environment variable.
+For best results, use the version listed above, as it is the version that python-bitcointx automatic tests
+use to build libsecp256k1. Then make sure that this version of the library is loaded by python-bitcointx,
+by using `bitcointx.set_custom_secp256k1_path()` or `LD_LIBRARY_PATH ` environment variable.
 
 ## Installation
 

diff --git a/bitcointx/__init__.py b/bitcointx/__init__.py
@@ -310,33 +310,6 @@ def select_chain_params(params: Union[str, ChainParamsBase,
     return prev_params, params
 
 
-def allow_secp256k1_experimental_modules() -> None:
-    """
-    python-bitcointx uses libsecp256k1 via ABI using ctypes, using dynamic
-    library loading. This means that the correctness of the calls to secp256k1
-    functions depend on these function definitions in python-bitcointx code
-    to be in-sync with the actual C language definitions in the library when
-    it was compiled. Libsecp256k1 ABI is mostly stable, but still has no
-    officially released version at the moment. But for schnorr signatures
-    and x-only pubkeys, we have to use the modules of libsecp256k1 that are
-    currently marked as 'experimental'. These modules being experimental
-    mean that their ABI can change at any moment. Therefore, to use
-    python-bitcointx with this functionality, it is highly recommended to
-    compile your own version libsecp256k1 using the git commit that is
-    recommended for particular version of python-bitcointx, and then make
-    sure that python-bitcointx will load that exact version of libsecp256k1
-    with set_custom_secp256k1_path() or LD_LIBRARY_PATH environment variable.
-
-    But since using experimental modules with some over version of libsecp256k1
-    may lead to crashes, using them is disabled by default. One have to
-    explicitly enable this by calling this function, or setting the
-    environment variable
-    "PYTHON_BITCOINTX_ALLOW_LIBSECP256K1_EXPERIMENTAL_MODULES_USE" to the
-    value "1".
-    """
-    bitcointx.util._allow_secp256k1_experimental_modules = True
-
-
 def set_custom_secp256k1_path(path: str) -> None:
     """Set the custom path that will be used to load secp256k1 library
     by bitcointx.core.secp256k1 module. For the calling of this
@@ -362,14 +335,6 @@ class ChainParamsContextVar(bitcointx.util.ContextVarsCompat):
 
 _chain_params_context = ChainParamsContextVar(params=BitcoinMainnetParams())
 
-_secp256k1_experimental_modues_enable_var = os.environ.get(
-    'PYTHON_BITCOINTX_ALLOW_LIBSECP256K1_EXPERIMENTAL_MODULES_USE')
-
-if _secp256k1_experimental_modues_enable_var is not None:
-    assert _secp256k1_experimental_modues_enable_var in ("0", "1")
-    if int(_secp256k1_experimental_modues_enable_var):
-        allow_secp256k1_experimental_modules()
-
 
 __all__ = (
     'ChainParamsBase',
@@ -384,5 +349,4 @@ class ChainParamsContextVar(bitcointx.util.ContextVarsCompat):
     'find_chain_params',
     'set_custom_secp256k1_path',
     'get_custom_secp256k1_path',
-    'allow_secp256k1_experimental_modules',
 )
diff --git a/bitcointx/core/key.py b/bitcointx/core/key.py
@@ -57,15 +57,10 @@ class KeyDerivationFailException(RuntimeError):
     pass
 
 
-def _experimental_module_unavailable_error(msg: str, module_name: str) -> str:
+def _module_unavailable_error(msg: str, module_name: str) -> str:
     return (
         f'{msg} handling functions from libsecp256k1 is not available. '
-        f'You should use newer version of secp256k1 library, '
-        f'configure it with --enable-experimental and '
-        f'--enable-module-{module_name}, and also enable the use '
-        f'of functions from libsecp256k1 experimental modules by '
-        f'python-bitcointx (see docstring for '
-        f'allow_secp256k1_experimental_modules() function)'
+        f'configure it libsecp256k1 with --enable-module-{module_name}'
     )
 
 
@@ -266,8 +261,7 @@ def _sign_schnorr_internal(  # noqa
 
         if not secp256k1_has_schnorrsig:
             raise RuntimeError(
-                _experimental_module_unavailable_error(
-                    'schnorr signature', 'schnorrsig'))
+                _module_unavailable_error('schnorr signature', 'schnorrsig'))
 
         if aux is not None:
             ensure_isinstance(aux, (bytes, bytearray), 'aux')
@@ -1988,8 +1982,7 @@ def __new__(cls: Type[T_XOnlyPubKey],
 
         if not secp256k1_has_xonly_pubkeys:
             raise RuntimeError(
-                _experimental_module_unavailable_error(
-                    'x-only pubkey', 'extrakeys'))
+                _module_unavailable_error('x-only pubkey', 'extrakeys'))
 
         if len(keydata) in (32, 0):
             ensure_isinstance(keydata, bytes, 'x-only pubkey data')
@@ -2032,8 +2025,7 @@ def is_null(self) -> bool:
     def verify_schnorr(self, hash: bytes, sigbytes: bytes) -> bool:
         if not secp256k1_has_schnorrsig:
             raise RuntimeError(
-                _experimental_module_unavailable_error(
-                    'schnorr signature', 'schnorrsig'))
+                _module_unavailable_error('schnorr signature', 'schnorrsig'))
 
         ensure_isinstance(sigbytes, (bytes, bytearray), 'signature')
         ensure_isinstance(hash, (bytes, bytearray), 'hash')

diff --git a/bitcointx/core/secp256k1.py b/bitcointx/core/secp256k1.py
@@ -185,43 +185,36 @@ def _add_function_definitions(_secp256k1: ctypes.CDLL) -> None:
         _secp256k1.secp256k1_ecdh.restype = ctypes.c_int
         _secp256k1.secp256k1_ecdh.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_void_p, ctypes.c_void_p]
 
-    if bitcointx.util._allow_secp256k1_experimental_modules:
-        if getattr(_secp256k1, 'secp256k1_xonly_pubkey_parse', None):
-            secp256k1_has_xonly_pubkeys = True
-            _secp256k1.secp256k1_xonly_pubkey_parse.restype = ctypes.c_int
-            _secp256k1.secp256k1_xonly_pubkey_parse.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p]
-            _secp256k1.secp256k1_xonly_pubkey_tweak_add_check.restype = ctypes.c_int
-            _secp256k1.secp256k1_xonly_pubkey_tweak_add_check.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_int, ctypes.c_char_p, ctypes.c_char_p]
-            _secp256k1.secp256k1_xonly_pubkey_tweak_add.restype = ctypes.c_int
-            _secp256k1.secp256k1_xonly_pubkey_tweak_add.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_char_p]
-            _secp256k1.secp256k1_xonly_pubkey_from_pubkey.restype = ctypes.c_int
-            _secp256k1.secp256k1_xonly_pubkey_from_pubkey.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.POINTER(ctypes.c_int), ctypes.c_char_p]
-            _secp256k1.secp256k1_xonly_pubkey_serialize.restype = ctypes.c_int
-            _secp256k1.secp256k1_xonly_pubkey_serialize.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p]
-            _secp256k1.secp256k1_keypair_create.restype = ctypes.c_int
-            _secp256k1.secp256k1_keypair_create.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p]
-            _secp256k1.secp256k1_keypair_xonly_pub.restype = ctypes.c_int
-            _secp256k1.secp256k1_keypair_xonly_pub.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.POINTER(ctypes.c_int), ctypes.c_char_p]
-            _secp256k1.secp256k1_keypair_xonly_tweak_add.restype = ctypes.c_int
-            _secp256k1.secp256k1_keypair_xonly_tweak_add.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p]
-
-        # Note that we check specifically for secp256k1_schnorrsig_sign_custom
-        # to avoid incompatibility with earlier version of libsecp256k1.
-        # Before secp256k1_schnorrsig_sign_custom was itroduced,
-        # secp256k1_schnorrsig_sign had different signature, and using it
-        # with this signature will result in segfault.
-        # Supporting older versions of libsecp256k1 will be burdensome, and given
-        # that currently schnorrsig module is experimental, even the current
-        # signature can change unexpectedly. Such is the woes of being lightweight
-        # in linking C libraries, as we cannot look into C headers as CFFI would
-        # do. We will need to wait for libsecp256k1 ABI stabilization for this
-        # potential problem go go away.
-        if getattr(_secp256k1, 'secp256k1_schnorrsig_sign_custom', None):
-            secp256k1_has_schnorrsig = True
-            _secp256k1.secp256k1_schnorrsig_verify.restype = ctypes.c_int
-            _secp256k1.secp256k1_schnorrsig_verify.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_int, ctypes.c_char_p]
-            _secp256k1.secp256k1_schnorrsig_sign.restype = ctypes.c_int
-            _secp256k1.secp256k1_schnorrsig_sign.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_char_p]
+    if getattr(_secp256k1, 'secp256k1_xonly_pubkey_parse', None):
+        secp256k1_has_xonly_pubkeys = True
+        _secp256k1.secp256k1_xonly_pubkey_parse.restype = ctypes.c_int
+        _secp256k1.secp256k1_xonly_pubkey_parse.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p]
+        _secp256k1.secp256k1_xonly_pubkey_tweak_add_check.restype = ctypes.c_int
+        _secp256k1.secp256k1_xonly_pubkey_tweak_add_check.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_int, ctypes.c_char_p, ctypes.c_char_p]
+        _secp256k1.secp256k1_xonly_pubkey_tweak_add.restype = ctypes.c_int
+        _secp256k1.secp256k1_xonly_pubkey_tweak_add.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_char_p]
+        _secp256k1.secp256k1_xonly_pubkey_from_pubkey.restype = ctypes.c_int
+        _secp256k1.secp256k1_xonly_pubkey_from_pubkey.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.POINTER(ctypes.c_int), ctypes.c_char_p]
+        _secp256k1.secp256k1_xonly_pubkey_serialize.restype = ctypes.c_int
+        _secp256k1.secp256k1_xonly_pubkey_serialize.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p]
+        _secp256k1.secp256k1_keypair_create.restype = ctypes.c_int
+        _secp256k1.secp256k1_keypair_create.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p]
+        _secp256k1.secp256k1_keypair_xonly_pub.restype = ctypes.c_int
+        _secp256k1.secp256k1_keypair_xonly_pub.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.POINTER(ctypes.c_int), ctypes.c_char_p]
+        _secp256k1.secp256k1_keypair_xonly_tweak_add.restype = ctypes.c_int
+        _secp256k1.secp256k1_keypair_xonly_tweak_add.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p]
+
+    # Note that we check specifically for secp256k1_schnorrsig_sign_custom
+    # to avoid incompatibility with earlier version of libsecp256k1.
+    # Before secp256k1_schnorrsig_sign_custom was itroduced,
+    # secp256k1_schnorrsig_sign had different signature, and using it
+    # with this signature will result in segfault.
+    if getattr(_secp256k1, 'secp256k1_schnorrsig_sign_custom', None):
+        secp256k1_has_schnorrsig = True
+        _secp256k1.secp256k1_schnorrsig_verify.restype = ctypes.c_int
+        _secp256k1.secp256k1_schnorrsig_verify.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_int, ctypes.c_char_p]
+        _secp256k1.secp256k1_schnorrsig_sign.restype = ctypes.c_int
+        _secp256k1.secp256k1_schnorrsig_sign.argtypes = [ctypes.c_void_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_char_p]
 
 
 class _secp256k1_context:

diff --git a/bitcointx/tests/test_key.py b/bitcointx/tests/test_key.py
@@ -17,8 +17,6 @@
 import warnings
 import hashlib
 
-import bitcointx.util
-
 from bitcointx.core.key import CKey, CPubKey, XOnlyPubKey
 from bitcointx.core import x
 from bitcointx.core.secp256k1 import secp256k1_has_pubkey_negate
@@ -164,8 +162,6 @@ def test_signature_grind(self) -> None:
         self.assertTrue(small_sig_found)
 
     def test_schnorr(self) -> None:
-        if not bitcointx.util._allow_secp256k1_experimental_modules:
-            self.skipTest("secp256k1 experimental modules are not available")
         # adapted from reference code of BIP340
         # at https://github.com/bitcoin/bips/blob/master/bip-0340/reference.py
         with open(os.path.dirname(__file__) + '/data/schnorr-sig-test-vectors.csv', 'r') as fd:

diff --git a/bitcointx/tests/test_scripteval.py b/bitcointx/tests/test_scripteval.py
@@ -23,8 +23,6 @@
 
 from binascii import unhexlify
 
-import bitcointx.util
-
 from bitcointx.core import (
     coins_to_satoshi, x, ValidationError,
     CTxOut, CTxIn, CTransaction, COutPoint, CTxWitness, CTxInWitness
@@ -354,10 +352,6 @@ def test_script_bitcoinconsensus(self) -> None:
         # test with default-loaded handle
         self._do_test_bicoinconsensus(None, test_data_iterator)
 
-    @unittest.skipIf(
-        not bitcointx.util._allow_secp256k1_experimental_modules,
-        "secp256k1_experimental_module is not available or not enabled"
-    )
     def test_script_bitcoinconsensus_taproot_scripts(self) -> None:
         if not self._bitcoinconsensus_handle:
             self.skipTest("bitcoinconsensus library is not available")