Details

Reviewers

bytesofman
Fabien

Group Reviewers

Restricted Project

Commits

rABC26c62cc78b2c: [electrum] refactor private key class to better abstract ecdsa library usage
rABC1d7770150082: Fix permission on numerous files.

Summary

The goal of this refactoring is to better encapsulate internals of the ecdsa library in a single module (in anticipation for removing that dependency later), and improve the general code quality.

The ECKey class is renamed into ECPrivkey and now inherits ECPubkey. The code of the ECPrivkey is massaged into a better shape. The constructor now checks that the key is exactly 32 bytes, and for creating a key from a shorter amount of data (used for wallet file encryption) a special classmethod from_arbitrary_size_secret is introduced with the previous behavior of normalizing the shorter key data to 32 bytes.

The MySigningKey class, which is a wrapper around ecdsa.SigningKey, is now an implementation details of ecc.py, and is no longer used in transaction.py and unit tests.

This diff is a big chunck of what remains to be split out of D16301.

Depends on D16678

Test Plan

python test_runner.py

Run the GUI, sign and verify a message, check that the sig is still identical to the one generated before this diff.
Sign, send and receive a transaction.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

electrum_crypto

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30073
Build 59680: Build Diff	electrum-tests
Build 59679: arc lint + arc unit

Event Timeline

PiRK created this revision.Aug 23 2024, 16:14

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 23 2024, 16:14

Harbormaster completed remote builds in B30073: Diff 49336.Aug 23 2024, 16:16

PiRK added a child revision: D16677: [electrum] move .encrypt method to ECPubkey class.Aug 23 2024, 19:16

PiRK removed a child revision: D16677: [electrum] move .encrypt method to ECPubkey class.Aug 23 2024, 19:19

extracted the moving of .encrypt -> ECPubkey into its own "move-mostly" diff so that the changes are easier to spot in this one

Harbormaster completed remote builds in B30076: Diff 49339.Aug 23 2024, 19:23

PiRK removed parent revisions: D16677: [electrum] move .encrypt method to ECPubkey class, D16675: [electrum] don't use ecdsa for basic bytes <-> int conversion.Aug 23 2024, 19:54

move the mostly unrelated fix in ECPubkey.encrypt_message to its own diff (D16678)

Harbormaster completed remote builds in B30078: Diff 49341.Aug 23 2024, 19:57

PiRK edited the summary of this revision. (Show Details)Aug 23 2024, 20:06

Alexander1978-nice added a commit: rABC1d7770150082: Fix permission on numerous files..Aug 24 2024, 19:36

PiRK published this revision for review.Aug 26 2024, 06:42

this silently breaks fusion

  File "~/bitcoin-abc/electrum/electrumabc_plugins/fusion/fusion.py", line 50, in <module>
    from electrumabc.ecc import public_key_from_private_key
ImportError: cannot import name 'public_key_from_private_key' from 'electrumabc.ecc' (~/bitcoin-abc/electrum/electrumabc/ecc.py)

fix fusion.py

Harbormaster completed remote builds in B30086: Diff 49351.Aug 26 2024, 11:57

PiRK added a child revision: D16681: [electrum] properly handle point at infinity and point not on curve.Aug 26 2024, 15:08

rebase

Harbormaster completed remote builds in B30097: Diff 49367.Aug 27 2024, 07:08

I see this has been rebased but I am running into the same issue, arc patch 16676 fails with a cherry pick error

bytesofman mentioned this in D16681: [electrum] properly handle point at infinity and point not on curve.Aug 28 2024, 21:16

rebase

Harbormaster completed remote builds in B30128: Diff 49406.Aug 29 2024, 09:38

I can't get the test plan to pass but I'm also not sure it's really an issue.

Here's what I do.

Open ElectrumABC on my desktop (I'm one minor version behind the latest)
Sign message thisisatestmessage with address in wallet
Signature generated
Close electrumABC

Then open ElectrumABC from ./electrum-abc from patching this diff and repeat. I get a different signature. But the original signature also verifies.

Likewise I can verify the ./electrum-abc signature from this diff in my normal desktop ElectrumABC.

In D16676#378433, @bytesofman wrote:

I can't get the test plan to pass but I'm also not sure it's really an issue.

It should not be an issue as long as the signature verifies (and this is reasonably well covered in the unit tests, with some deterministic hardcoded signatures), but I don't really understand how this change could affect the produces signature, and why it could change for you but not for me. So it is a bit odd.

confirmed my different signatures were from not having built libsecp

i think we can get rid of the 32 magic number since we have that as a constant now? other magic numbers are probably ok in this diff since these are already in the codebase, should be handled with a specific diff if the impact is warranted

electrum/electrumabc/ecc.py
356 ↗	(On Diff #49406)	can we use the now-defined constant for this 32?
358 ↗	(On Diff #49406)	same
374 ↗	(On Diff #49406)	same
380 ↗	(On Diff #49406)	not really clear to a dev coming across this what "legacy reasons" refers to, tho not sure if that is important beyond the explicit "don't use this for new things"
381 ↗	(On Diff #49406)	magic 32
391 ↗	(On Diff #49406)	magic 32
426 ↗	(On Diff #49406)	why 4 looks like the previous code also has a lot of this stuff hardcoded so not really a blocker

This revision now requires changes to proceed.Aug 29 2024, 15:31

PiRK added inline comments.Aug 30 2024, 07:24

electrum/electrumabc/ecc.py
426 ↗	(On Diff #49406)	Yeah it took me a while to figure this one out, and I'm not sure if it is worth a comment or a CONSTANT. This is only ever going to be in that signing function. https://bitcoin.stackexchange.com/questions/38351/ecdsa-v-r-s-what-is-v/38909#38909 In Bitcoin, for message signatures, we use a trick called public key recovery. The fact is that if you have the full R point (not just its X coordinate) and s, and a message, you can compute for which public key this would be a valid signature. What this allows is to 'verify' a message with an address, without needing to know the full key (we just do public key recovery on the signature, and then hash the recovered key and compare it with the address). However, this means we need the full R coordinates. There can be up to 4 different points with a given "X coordinate modulo n". (2 because each X coordinate has two possible Y coordinates, and 2 because r+n may still be a valid X coordinate). That number between 0 and 3 we call the recovery id, or recid.

replace a few 32's with PRIVATE_KEY_BYTECOUNT, clarify legacy use for arbitrary size secret

Harbormaster completed remote builds in B30134: Diff 49414.Aug 30 2024, 07:38

Fabien accepted this revision.Aug 30 2024, 20:07

Fabien added a subscriber: Fabien.

Fabien added inline comments.

electrum/electrumabc/ecc.py
406 ↗	(On Diff #49414)	You can make signature much faster by skipping this check, but let's do this in another diff as it's not new behavior

bytesofman accepted this revision.Sep 2 2024, 21:11

This revision is now accepted and ready to land.Sep 2 2024, 21:11

Closed by commit rABC26c62cc78b2c: [electrum] refactor private key class to better abstract ecdsa library usage (authored by PiRK). · Explain WhySep 3 2024, 06:19

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC26c62cc78b2c: [electrum] refactor private key class to better abstract ecdsa library usage.

[electrum] refactor private key class to better abstract ecdsa library usage
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 49336

electrum/electrumabc/address.py

electrum/electrumabc/avalanche/primitives.py

electrum/electrumabc/bip32.py

electrum/electrumabc/bitcoin.py

electrum/electrumabc/commands.py

electrum/electrumabc/ecc.py

electrum/electrumabc/keystore.py

electrum/electrumabc/paymentrequest.py

electrum/electrumabc/storage.py

electrum/electrumabc/tests/test_bitcoin.py

electrum/electrumabc/tests/test_ecc.py

electrum/electrumabc/tests/test_schnorr.py

electrum/electrumabc/transaction.py

electrum/electrumabc/wallet.py

electrum/electrumabc_gui/qt/main_window.py

electrum/electrumabc_plugins/cosigner_pool/qt.py

[electrum] refactor private key class to better abstract ecdsa library usageClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 49336

electrum/electrumabc/address.py

electrum/electrumabc/avalanche/primitives.py

electrum/electrumabc/bip32.py

electrum/electrumabc/bitcoin.py

electrum/electrumabc/commands.py

electrum/electrumabc/ecc.py

electrum/electrumabc/keystore.py

electrum/electrumabc/paymentrequest.py

electrum/electrumabc/storage.py

electrum/electrumabc/tests/test_bitcoin.py

electrum/electrumabc/tests/test_ecc.py

electrum/electrumabc/tests/test_schnorr.py

electrum/electrumabc/transaction.py

electrum/electrumabc/wallet.py

electrum/electrumabc_gui/qt/main_window.py

electrum/electrumabc_plugins/cosigner_pool/qt.py

[electrum] refactor private key class to better abstract ecdsa library usage
ClosedPublic
Actions

Revision Contents
Changeset List