[electrum] fix get_tx_derivations for regular keystores and imported keys keystores
ClosedPublic
Actions

Authored by PiRK on Oct 4 2023, 08:56.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCdde0959297be: [electrum] fix get_tx_derivations for regular keystores and imported keys…

Summary

In D14569 I forgot to make sure that keystores also provided pubkeys as bytes in keypairs. As a result, I broke transaction signing for most wallet types.

Fix this by making get_tx_derivations return pubkeys as bytes. These returned pubkeys are then used in SoftwareKeyStore.sign_transaction to build the keypairs before calling tx.sign, and now they are in the correct type.

For ImportedKeyStore, fix the get_pubkey_derivation method to correctly deal with x_pubkey as bytes. This was missed in D14455

Test Plan

python test_runner.py

Send a transaction with the software for a regular BIP39 wallet, a multisig wallet, a hardware wallet and an imported privkey wallet

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Oct 4 2023, 08:56

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 4 2023, 08:56

PiRK requested review of this revision.Oct 4 2023, 08:56

Harbormaster completed remote builds in B25255: Diff 42524.Oct 4 2023, 08:58

Build Bitcoin ABC Diffs / Diff Testing (electrum-tests) failed.

Tail of the build log:

-- Configuring done
-- Generating done
-- Build files have been written to: /work/abc-ci-builds/electrum-tests
[1/2] link test_runner.py
[2/2] Run Electrum ABC unit tests...
FAILED: electrum/CMakeFiles/check-electrum 
cd /work/abc-ci-builds/electrum-tests/electrum && /usr/bin/python3.9 ./test_runner.py
[secp256k1] warning: libsecp256k1 library failed to load
Testing `setup.py --version`: OK

[ecc] info: libsecp256k1 library not available, falling back to python-ecdsa. This means signing operations will be slower. Try running:

  $  contrib/make_secp

(You need to be running from the git sources for contrib/make_secp to be available)
...................................................................................................F...F.........127.0.0.1 - - [04/Oct/2023 08:58:18] "GET /invoice HTTP/1.1" 503 -
.127.0.0.1 - - [04/Oct/2023 08:58:19] "GET / HTTP/1.1" 200 -
.127.0.0.1 - - [04/Oct/2023 08:58:19] "GET /invoice HTTP/1.1" 200 -
127.0.0.1 - - [04/Oct/2023 08:58:19] "POST /pay HTTP/1.1" 200 -
.127.0.0.1 - - [04/Oct/2023 08:58:20] "GET / HTTP/1.1" 200 -
.s.........................................................................................................................................s.........sss.........................................s.s...
======================================================================
FAIL: test_sign (electrumabc.tests.test_keystore.TestBip32KeyStore)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/work/electrum/electrumabc/tests/test_keystore.py", line 87, in test_sign
    self.assertEqual(txin.signatures, [expected_sig])
AssertionError: Lists differ: [b'0E\x02!\x00\xf5\x18B\x90\xac\xcf\x9a=o=\n\[133 chars]KlA'] != [b'0D\x02 j\xdd\xcc\xcer\x9b\xa8u\xed\xa4{\x8[158 chars]9bA']

First differing element 0:
b'0E\x02!\x00\xf5\x18B\x90\xac\xcf\x9a=o=\n\[132 chars]6KlA'
b'0D\x02 j\xdd\xcc\xcer\x9b\xa8u\xed\xa4{\x8[157 chars]x9bA'

- [b'0E\x02!\x00\xf5\x18B\x90\xac\xcf\x9a=o=\n\x8a?R\xda\xe1b)K\\\xc6\x12q'
-  b'\x93\xe5\\F<\x94!&\xfa\x02 O|q\xca\\\xbc`\x13v\xca68\xff\xe2\x1a<\x9b'
-  b'\x01\xd8.$x\xb6\xa6\x9cd\x9c\xf02\xf6KlA']

======================================================================
FAIL: test_sign (electrumabc.tests.test_keystore.TestImportedKeyStore)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/work/electrum/electrumabc/tests/test_keystore.py", line 58, in test_sign
    self.assertEqual(txin.signatures, [expected_sig])
AssertionError: Lists differ: [b"0E\x02!\x00\x93\x11\x88 \x99o\x7f\x93\xf[171 chars]6ZA"] != [b'0D\x02 ITg\xc2\x94\x9d\x8e7]km<l^!\x1c\x[128 chars]abA']

First differing element 0:
b"0E\x02!\x00\x93\x11\x88 \x99o\x7f\x93\xf[170 chars]86ZA"
b'0D\x02 ITg\xc2\x94\x9d\x8e7]km<l^!\x1c\x[127 chars]xabA'

- [b'0E\x02!\x00\x93\x11\x88 \x99o\x7f\x93\xf0\xc9w\x03\xd8\x9d\xbc'
-  b"\x1c\x02\x1b\xe3\xe0\xe3p\x92x'FP\r@\x01z\xa3\x02 \x1b\xe2\x14\x89\xd6"
-  b'b\xedZ\xbb\xf0\xa8c>8x\xd7\xb0s\x8ff\x08\xe2\xfa\xf6\xaa\xf4\xd7\xa1\x19'
-  b'\xaf\x86ZA']

----------------------------------------------------------------------
Ran 311 tests in 12.224s

FAILED (failures=2, skipped=7)
ninja: build stopped: cannot make progress due to previous errors.
Build electrum-tests failed with exit code 1

Back to your queue, the test is failing

This revision now requires changes to proceed.Oct 4 2023, 09:08

no need to verify the sig in the test, it is done via keystore.sign_transaction -> Transaction.sign -> Transaction._sign_txin -> Transaction.verify_signature

Harbormaster completed remote builds in B25257: Diff 42527.Oct 4 2023, 10:27

Fabien accepted this revision.Oct 4 2023, 10:28

This revision is now accepted and ready to land.Oct 4 2023, 10:28

Closed by commit rABCdde0959297be: [electrum] fix get_tx_derivations for regular keystores and imported keys… (authored by PiRK). · Explain WhyOct 4 2023, 11:36

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABCdde0959297be: [electrum] fix get_tx_derivations for regular keystores and imported keys….

PiRK mentioned this in D14593: [electrum] fix OldKeyStore.get_pubkey_derivation.Oct 4 2023, 12:53

PiRK mentioned this in D14594: [electrum] progress converting keystore.py to using bytes.Oct 4 2023, 13:55

PiRK mentioned this in rABC6cb98fda1e23: [electrum] fix OldKeyStore.get_pubkey_derivation.Oct 5 2023, 03:41

PiRK mentioned this in rABC8d71c7c66119: [electrum] progress converting keystore.py to using bytes.