Page MenuHomePhabricator
Differential D16689

[electrum] ecc: use libsecp256k1 for sign/verify/mul/add
ClosedPublic
Actions

Authored by PiRK on Aug 28 2024, 14:24.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC813785828d83: [electrum] ecc: use libsecp256k1 for sign/verify/mul/add
Summary

Make libsecp256k1 a mandatory dependency, drop the fallback pure-python code that was only used when running from sources without libsecp256k1 compiled.

Hard fail is there is any import error or unavailable symbol in secp256k1 (lib compiled without a required module, lib from Bitcoin Core...)

This is a partial backport of electrum#5947
https://github.com/spesmilo/electrum/pull/5947/commits/ad408ea832c55458b730945832c826894dfa9386

Depends on D16687

Test Plan

python test_runner.py

Diff Detail

Event Timeline

PiRK created this revision.Aug 28 2024, 14:24
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 28 2024, 14:24
PiRK requested review of this revision.Aug 28 2024, 14:24
teamcity edited the summary of this revision. (Show Details)Aug 28 2024, 14:24
Harbormaster completed remote builds in B30115: Diff 49391.Aug 28 2024, 14:26
teamcity added a comment.Aug 28 2024, 14:26
Build Bitcoin ABC Diffs / Diff Testing (electrum-tests) failed.

Tail of the build log:

    from . import ecc, networks
  File "/work/electrum/electrumabc/ecc.py", line 42, in <module>
    from .secp256k1 import SECP256K1_EC_UNCOMPRESSED, secp256k1
  File "/work/electrum/electrumabc/secp256k1.py", line 181, in <module>
    raise ImportError("Failed to load required library libsecp256k1")
ImportError: Failed to load required library libsecp256k1


======================================================================
ERROR: electrumabc_plugins.keepkey.tests.test_keepkey (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: electrumabc_plugins.keepkey.tests.test_keepkey
Traceback (most recent call last):
  File "/usr/lib/python3.9/unittest/loader.py", line 436, in _find_test_path
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.9/unittest/loader.py", line 377, in _get_module_from_name
    __import__(name)
  File "/work/electrum/electrumabc_plugins/keepkey/tests/test_keepkey.py", line 5, in <module>
    from electrumabc.plugins import Plugins
  File "/work/electrum/electrumabc/plugins.py", line 56, in <module>
    from .bip32 import xpub_type
  File "/work/electrum/electrumabc/bip32.py", line 32, in <module>
    from . import ecc, networks
  File "/work/electrum/electrumabc/ecc.py", line 42, in <module>
    from .secp256k1 import SECP256K1_EC_UNCOMPRESSED, secp256k1
  File "/work/electrum/electrumabc/secp256k1.py", line 181, in <module>
    raise ImportError("Failed to load required library libsecp256k1")
ImportError: Failed to load required library libsecp256k1


======================================================================
ERROR: electrumabc_plugins.trezor.tests.test_trezor (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: electrumabc_plugins.trezor.tests.test_trezor
Traceback (most recent call last):
  File "/usr/lib/python3.9/unittest/loader.py", line 436, in _find_test_path
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.9/unittest/loader.py", line 377, in _get_module_from_name
    __import__(name)
  File "/work/electrum/electrumabc_plugins/trezor/tests/test_trezor.py", line 5, in <module>
    from electrumabc.plugins import Plugins
  File "/work/electrum/electrumabc/plugins.py", line 56, in <module>
    from .bip32 import xpub_type
  File "/work/electrum/electrumabc/bip32.py", line 32, in <module>
    from . import ecc, networks
  File "/work/electrum/electrumabc/ecc.py", line 42, in <module>
    from .secp256k1 import SECP256K1_EC_UNCOMPRESSED, secp256k1
  File "/work/electrum/electrumabc/secp256k1.py", line 181, in <module>
    raise ImportError("Failed to load required library libsecp256k1")
ImportError: Failed to load required library libsecp256k1


----------------------------------------------------------------------
Ran 102 tests in 0.042s

FAILED (errors=30, skipped=4)
Testing `setup.py --version`: OK

ninja: build stopped: cannot make progress due to previous errors.
Build electrum-tests failed with exit code 1
PiRK updated this revision to Diff 49392.Aug 28 2024, 14:30

remove dead/commented code (the todo is done in a couple of commits, and is not critical)

Harbormaster completed remote builds in B30116: Diff 49392.Aug 28 2024, 14:31
teamcity added a comment.Aug 28 2024, 14:31
Build Bitcoin ABC Diffs / Diff Testing (electrum-tests) failed.

Tail of the build log:

    from . import ecc, networks
  File "/work/electrum/electrumabc/ecc.py", line 42, in <module>
    from .secp256k1 import SECP256K1_EC_UNCOMPRESSED, secp256k1
  File "/work/electrum/electrumabc/secp256k1.py", line 181, in <module>
    raise ImportError("Failed to load required library libsecp256k1")
ImportError: Failed to load required library libsecp256k1


======================================================================
ERROR: electrumabc_plugins.keepkey.tests.test_keepkey (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: electrumabc_plugins.keepkey.tests.test_keepkey
Traceback (most recent call last):
  File "/usr/lib/python3.9/unittest/loader.py", line 436, in _find_test_path
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.9/unittest/loader.py", line 377, in _get_module_from_name
    __import__(name)
  File "/work/electrum/electrumabc_plugins/keepkey/tests/test_keepkey.py", line 5, in <module>
    from electrumabc.plugins import Plugins
  File "/work/electrum/electrumabc/plugins.py", line 56, in <module>
    from .bip32 import xpub_type
  File "/work/electrum/electrumabc/bip32.py", line 32, in <module>
    from . import ecc, networks
  File "/work/electrum/electrumabc/ecc.py", line 42, in <module>
    from .secp256k1 import SECP256K1_EC_UNCOMPRESSED, secp256k1
  File "/work/electrum/electrumabc/secp256k1.py", line 181, in <module>
    raise ImportError("Failed to load required library libsecp256k1")
ImportError: Failed to load required library libsecp256k1


======================================================================
ERROR: electrumabc_plugins.trezor.tests.test_trezor (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: electrumabc_plugins.trezor.tests.test_trezor
Traceback (most recent call last):
  File "/usr/lib/python3.9/unittest/loader.py", line 436, in _find_test_path
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.9/unittest/loader.py", line 377, in _get_module_from_name
    __import__(name)
  File "/work/electrum/electrumabc_plugins/trezor/tests/test_trezor.py", line 5, in <module>
    from electrumabc.plugins import Plugins
  File "/work/electrum/electrumabc/plugins.py", line 56, in <module>
    from .bip32 import xpub_type
  File "/work/electrum/electrumabc/bip32.py", line 32, in <module>
    from . import ecc, networks
  File "/work/electrum/electrumabc/ecc.py", line 42, in <module>
    from .secp256k1 import SECP256K1_EC_UNCOMPRESSED, secp256k1
  File "/work/electrum/electrumabc/secp256k1.py", line 181, in <module>
    raise ImportError("Failed to load required library libsecp256k1")
ImportError: Failed to load required library libsecp256k1


----------------------------------------------------------------------
Ran 102 tests in 0.042s

FAILED (errors=30, skipped=4)
Testing `setup.py --version`: OK

ninja: build stopped: cannot make progress due to previous errors.
Build electrum-tests failed with exit code 1
PiRK added a child revision: D16690: [electrum] use libsecp256k1 for signature conversions.Aug 28 2024, 14:36
PiRK planned changes to this revision.Aug 28 2024, 14:38

I forgot that this needs D16687 to be sorted out first

PiRK mentioned this in D16681: [electrum] properly handle point at infinity and point not on curve.Aug 30 2024, 07:02
PiRK updated this revision to Diff 49453.Sep 3 2024, 10:30

rebase

PiRK edited the summary of this revision. (Show Details)Sep 3 2024, 10:31
PiRK added a parent revision: D16687: [CI] build libsecp256k1 for electrum tests.
Harbormaster completed remote builds in B30157: Diff 49453.Sep 3 2024, 10:33
PiRK added inline comments.Sep 3 2024, 10:50
electrum/contrib/secp_HOWTO.md
79–91

As there is no longer any python fallback code for schnorr sigs, we can no longer use the BTC variant of libsecp256k1 that is usually installed via the package manager.

Fabien requested changes to this revision.Sep 4 2024, 08:09
Fabien added a subscriber: Fabien.
Fabien added inline comments.
electrum/electrumabc/schnorr.py
289

It's a constant, better fix it now and remove the import ecdsa which is confusing

electrum/electrumabc/secp256k1.py
181

We should improve this a bit and at least point to the README to explain how to install it (and what version is required, otherwise some users might just install the package from their distro and think it's a bug).

electrum/electrumabc/tests/test_schnorr.py
48

well, there is no test_slow anymore so the name doesn't make sense

electrum/electrumabc/wallet.py
3023

layout

electrum/electrumabc_plugins/fusion/compatibility.py
35

I don't understand why this is removed ?

This revision now requires changes to proceed.Sep 4 2024, 08:09
PiRK added inline comments.Sep 4 2024, 09:01
electrum/electrumabc_plugins/fusion/compatibility.py
35

it is only used below in the removed line. Both schnorr.has_fast_sign() and schnorr.has_fast_verify() should always be true now.

I guess the whole module is pretty much useless as the other check about the protobuf version is also always true now (enforced in requirements.txt). But that's unrelated to this diff.

PiRK updated this revision to Diff 49472.Sep 4 2024, 09:50
PiRK edited the summary of this revision. (Show Details)

move secp256k1_schnorr_* declarations to secp256k1 where it belongs, raise a meaningful error if the lib is missing the schnorr module, remove some extra try: except: wrapping in secp256k1.py so that the inner errors are raised with more precise error messages

Harbormaster completed remote builds in B30170: Diff 49472.Sep 4 2024, 09:56
PiRK added inline comments.Sep 4 2024, 10:06
electrum/electrumabc/schnorr.py
31–57 ↗(On Diff #49472)

moved to secp256k1.py, as everything in libsecp256k1 is now equally mandatory

Fabien requested changes to this revision.Sep 4 2024, 12:12
Fabien added inline comments.
electrum/electrumabc/schnorr.py
104 ↗(On Diff #49472)

Or remove the seclib alias if it's unused

electrum/electrumabc/secp256k1.py
75 ↗(On Diff #49472)

Please improve the message here and point the user to the relevant doc

This revision now requires changes to proceed.Sep 4 2024, 12:12
PiRK updated this revision to Diff 49476.Sep 4 2024, 13:42

better error message if the lib is not available at all

Harbormaster completed remote builds in B30174: Diff 49476.Sep 4 2024, 13:44
PiRK added inline comments.Sep 4 2024, 13:44
electrum/electrumabc/schnorr.py
104 ↗(On Diff #49472)

Usage is inconsistent so I missed that there was an alias: 9 occurences of secp256k1.secp256k1 vs 11 occurences of seclib in this module

PiRK updated this revision to Diff 49478.Sep 4 2024, 13:52

improve error message some more

Harbormaster completed remote builds in B30176: Diff 49478.Sep 4 2024, 13:54
Fabien accepted this revision.Sep 4 2024, 17:57
Fabien added inline comments.
electrum/electrumabc/schnorr.py
104 ↗(On Diff #49472)

that will make a nice follow-up

This revision is now accepted and ready to land.Sep 4 2024, 17:57
Closed by commit rABC813785828d83: [electrum] ecc: use libsecp256k1 for sign/verify/mul/add (authored by PiRK). · Explain WhySep 5 2024, 06:09
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABC813785828d83: [electrum] ecc: use libsecp256k1 for sign/verify/mul/add.
PiRK mentioned this in D16715: [electrum] consistently use the seclib alias in schnorr.py.Sep 5 2024, 06:18
PiRK mentioned this in rABCecf072cabad2: [electrum] consistently use the seclib alias in schnorr.py.Sep 5 2024, 13:22

Revision Contents
Changeset List

PathSize
electrum/
contrib/
40 lines
electrumabc/
5 lines
232 lines
161 lines
5 lines
tests/
32 lines
32 lines
electrumabc_gui/
qt/
47 lines
electrumabc_plugins/
fusion/
12 lines

Diff 49453

View Options

electrum/contrib/secp_HOWTO.md

Loading...
View Options

electrum/electrumabc/bitcoin.py

Loading...
View Options

electrum/electrumabc/ecc.py

Loading...
View Options

electrum/electrumabc/ecc_fast.py

Loading...
View Options

electrum/electrumabc/schnorr.py

Loading...
View Options

electrum/electrumabc/secp256k1.py

Loading...
View Options

electrum/electrumabc/tests/test_schnorr.py

Loading...
View Options

electrum/electrumabc/wallet.py

Loading...
View Options

electrum/electrumabc_gui/qt/__init__.py

Loading...
View Options

electrum/electrumabc_plugins/fusion/compatibility.py

Loading...