Page MenuHomePhabricator
Differential D16676

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

Authored by PiRK on Fri, Aug 23, 16:14.

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK created this revision.Fri, Aug 23, 16:14
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Aug 23, 16:14
Harbormaster completed remote builds in B30073: Diff 49336.Fri, Aug 23, 16:16
PiRK added a child revision: D16677: [electrum] move .encrypt method to ECPubkey class.Fri, Aug 23, 19:16
PiRK removed a child revision: D16677: [electrum] move .encrypt method to ECPubkey class.Fri, Aug 23, 19:19
PiRK updated this revision to Diff 49339.Fri, Aug 23, 19:20
PiRK edited the summary of this revision. (Show Details)
PiRK added a parent revision: D16677: [electrum] move .encrypt method to ECPubkey class.

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.Fri, Aug 23, 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.Fri, Aug 23, 19:54
PiRK updated this revision to Diff 49341.Fri, Aug 23, 19:54
PiRK edited the summary of this revision. (Show Details)
PiRK added a parent revision: D16678: [electrum] fix the max value for the ephemeral exponent.

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

Harbormaster completed remote builds in B30078: Diff 49341.Fri, Aug 23, 19:57
PiRK edited the summary of this revision. (Show Details)Fri, Aug 23, 20:06
Alexander1978-nice added a commit: rABC1d7770150082: Fix permission on numerous files..Sat, Aug 24, 19:36
PiRK published this revision for review.Mon, Aug 26, 06:42
PiRK planned changes to this revision.Mon, Aug 26, 11: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)
PiRK updated this revision to Diff 49351.Mon, Aug 26, 11:55

fix fusion.py

Harbormaster completed remote builds in B30086: Diff 49351.Mon, Aug 26, 11:57
PiRK added a child revision: D16681: [electrum] properly handle point at infinity and point not on curve.Mon, Aug 26, 15:08
PiRK updated this revision to Diff 49367.Tue, Aug 27, 07:05

rebase

Harbormaster completed remote builds in B30097: Diff 49367.Tue, Aug 27, 07:08
bytesofman added a subscriber: bytesofman.Tue, Aug 27, 19:27

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.Wed, Aug 28, 21:16
PiRK updated this revision to Diff 49406.Thu, Aug 29, 09:36

rebase

Harbormaster completed remote builds in B30128: Diff 49406.Thu, Aug 29, 09:38
bytesofman added a comment.Thu, Aug 29, 14:26

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.

  1. Open ElectrumABC on my desktop (I'm one minor version behind the latest)
  2. Sign message thisisatestmessage with address in wallet
  3. Signature generated
  4. 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.

PiRK added a comment.Thu, Aug 29, 15:00
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.

bytesofman requested changes to this revision.Thu, Aug 29, 15:31

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.Thu, Aug 29, 15:31
PiRK added inline comments.Fri, Aug 30, 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.

PiRK updated this revision to Diff 49414.Fri, Aug 30, 07:35

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

Harbormaster completed remote builds in B30134: Diff 49414.Fri, Aug 30, 07:38
Fabien accepted this revision.Fri, Aug 30, 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.Mon, Sep 2, 21:11
This revision is now accepted and ready to land.Mon, Sep 2, 21:11
Closed by commit rABC26c62cc78b2c: [electrum] refactor private key class to better abstract ecdsa library usage (authored by PiRK). · Explain WhyTue, Sep 3, 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.

Revision Contents
Changeset List

PathSize
electrum/
electrumabc/
6 lines
avalanche/
4 lines
26 lines
16 lines
6 lines
191 lines
13 lines
4 lines
11 lines
tests/
4 lines
9 lines
4 lines
29 lines
4 lines
electrumabc_plugins/
cosigner_pool/
4 lines
fusion/
10 lines

Diff 49442

View Options

electrum/electrumabc/address.py

Loading...
View Options

electrum/electrumabc/avalanche/primitives.py

Loading...
View Options

electrum/electrumabc/bip32.py

Loading...
View Options

electrum/electrumabc/bitcoin.py

Loading...
View Options

electrum/electrumabc/commands.py

Loading...
View Options

electrum/electrumabc/ecc.py

Loading...
View Options

electrum/electrumabc/keystore.py

Loading...
View Options

electrum/electrumabc/paymentrequest.py

Loading...
View Options

electrum/electrumabc/storage.py

Loading...
View Options

electrum/electrumabc/tests/test_bitcoin.py

Loading...
View Options

electrum/electrumabc/tests/test_ecc.py

Loading...
View Options

electrum/electrumabc/tests/test_schnorr.py

Loading...
View Options

electrum/electrumabc/transaction.py

Loading...
View Options

electrum/electrumabc/wallet.py

Loading...
View Options

electrum/electrumabc_plugins/cosigner_pool/qt.py

Loading...
View Options

electrum/electrumabc_plugins/fusion/fusion.py

Loading...