Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCeb0a68ec66d0: [electrum] start implementing the PSBT format

Summary

PSBT is a standardized format for partially signed transactions that will eventually replace the current Electrum specific format used in the app for multisig and offline signing purposes. And the new Ledger app for bitcoin works with this format as well.

This implement basic serialization and deserialization of a Partially Signed Bitcoin Transaction file. For now there is no interpretation of the data except for a few enums defining the expected key types, just parsing of the sections (keypair maps) and keypairs within the sections.

This is inspired by https://github.com/spesmilo/electrum/blob/13d9677e534cc82aeace3d428a6bc78eadb0e1d4/electrum/transaction.py#L1277

The unit test is using the same data as the node's psbt_wallet_tests::psbt_updater_test

Test Plan

python test_runner.py

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

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

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

PiRK requested review of this revision.Jun 14 2024, 16:23

electrum/electrumabc/psbt.py
139 ↗	(On Diff #48242)	The walrus operator (:=) has been added in python 3.8.

Harbormaster completed remote builds in B29298: Diff 48242.Jun 14 2024, 16:25

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

Tail of the build log:

-- Found NATPMP component natpmp: /usr/lib/x86_64-linux-gnu/libnatpmp.so
-- Found NATPMP: /usr/include   
-- Performing Test fuzz_target_builds_without_main_fuzz
-- Performing Test fuzz_target_builds_without_main_fuzz - Failed
-- Found BerkeleyDB component CXX: /usr/lib/x86_64-linux-gnu/libdb_cxx-5.3.so
-- Found BerkeleyDB: /usr/include (found suitable version "5.3.28", minimum required is "5.3") found components: CXX 
-- Found SQLite3: /usr/include (found suitable version "3.34.1", minimum required is "3.7.17") 
-- Found ZeroMQ component zmq: /usr/lib/x86_64-linux-gnu/libzmq.so
-- Found ZeroMQ: /usr/include (found suitable version "4.3.4", minimum required is "4.1.5")  
-- Could NOT find Protobuf (missing: Protobuf_DIR)
-- Found Protobuf: /usr/lib/x86_64-linux-gnu/libprotobuf.so;-pthread (found version "3.12.4") 
-- Found OpenSSL: /usr/lib/x86_64-linux-gnu/libcrypto.so (found version "1.1.1w")  
-- Looking for EVP_MD_CTX_new
-- Looking for EVP_MD_CTX_new - found
-- Found QREncode component qrencode: /usr/lib/x86_64-linux-gnu/libqrencode.so
-- Found QREncode: /usr/include   
-- Configuring native build in /work/abc-ci-builds/electrum-tests/native
-- 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
[secp256k1] warning: loading from /work/electrum/electrumabc/libsecp256k1.so.0 failed with: /work/electrum/electrumabc/libsecp256k1.so.0: cannot open shared object file: No such file or directory
[secp256k1] warning: loading from libsecp256k1.so.0 failed with: libsecp256k1.so.0: cannot open shared object file: No such file or directory
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)
........................E..................................................................................................................127.0.0.1 - - [14/Jun/2024 16:24:46] "GET /invoice HTTP/1.1" 503 -
.127.0.0.1 - - [14/Jun/2024 16:24:47] "GET / HTTP/1.1" 200 -
.127.0.0.1 - - [14/Jun/2024 16:24:47] "GET /invoice HTTP/1.1" 200 -
127.0.0.1 - - [14/Jun/2024 16:24:47] "POST /pay HTTP/1.1" 200 -
.127.0.0.1 - - [14/Jun/2024 16:24:47] "GET / HTTP/1.1" 200 -
...s.........................................................................................................................................s.........sss.............................s.s....
======================================================================
ERROR: test_raises_deserializationerror (electrumabc.tests.test_avalanche.TestAvalancheDelegationFromHex)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/work/electrum/electrumabc/tests/test_avalanche.py", line 709, in test_raises_deserializationerror
    Delegation.from_hex("aabbcc")
  File "/work/electrum/electrumabc/serialize.py", line 58, in from_hex
    return cls.deserialize(BytesIO(bytes.fromhex(hex_str)))
  File "/work/electrum/electrumabc/avalanche/delegation.py", line 147, in deserialize
    levels = deserialize_sequence(stream, Level)
  File "/work/electrum/electrumabc/serialize.py", line 139, in deserialize_sequence
    for _ in range(size):
TypeError: 'NoneType' object cannot be interpreted as an integer

----------------------------------------------------------------------
Ran 328 tests in 23.844s

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

add copyright header, misc comments and doc fixups

Harbormaster completed remote builds in B29302: Diff 48246.Jun 14 2024, 19:31

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

Tail of the build log:

-- Found NATPMP component natpmp: /usr/lib/x86_64-linux-gnu/libnatpmp.so
-- Found NATPMP: /usr/include   
-- Performing Test fuzz_target_builds_without_main_fuzz
-- Performing Test fuzz_target_builds_without_main_fuzz - Failed
-- Found BerkeleyDB component CXX: /usr/lib/x86_64-linux-gnu/libdb_cxx-5.3.so
-- Found BerkeleyDB: /usr/include (found suitable version "5.3.28", minimum required is "5.3") found components: CXX 
-- Found SQLite3: /usr/include (found suitable version "3.34.1", minimum required is "3.7.17") 
-- Found ZeroMQ component zmq: /usr/lib/x86_64-linux-gnu/libzmq.so
-- Found ZeroMQ: /usr/include (found suitable version "4.3.4", minimum required is "4.1.5")  
-- Could NOT find Protobuf (missing: Protobuf_DIR)
-- Found Protobuf: /usr/lib/x86_64-linux-gnu/libprotobuf.so;-pthread (found version "3.12.4") 
-- Found OpenSSL: /usr/lib/x86_64-linux-gnu/libcrypto.so (found version "1.1.1w")  
-- Looking for EVP_MD_CTX_new
-- Looking for EVP_MD_CTX_new - found
-- Found QREncode component qrencode: /usr/lib/x86_64-linux-gnu/libqrencode.so
-- Found QREncode: /usr/include   
-- Configuring native build in /work/abc-ci-builds/electrum-tests/native
-- 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
[secp256k1] warning: loading from /work/electrum/electrumabc/libsecp256k1.so.0 failed with: /work/electrum/electrumabc/libsecp256k1.so.0: cannot open shared object file: No such file or directory
[secp256k1] warning: loading from libsecp256k1.so.0 failed with: libsecp256k1.so.0: cannot open shared object file: No such file or directory
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)
........................E..................................................................................................................127.0.0.1 - - [14/Jun/2024 19:30:28] "GET /invoice HTTP/1.1" 503 -
.127.0.0.1 - - [14/Jun/2024 19:30:28] "GET / HTTP/1.1" 200 -
.127.0.0.1 - - [14/Jun/2024 19:30:29] "GET /invoice HTTP/1.1" 200 -
127.0.0.1 - - [14/Jun/2024 19:30:29] "POST /pay HTTP/1.1" 200 -
.127.0.0.1 - - [14/Jun/2024 19:30:29] "GET / HTTP/1.1" 200 -
...s.........................................................................................................................................s.........sss.............................s.s....
======================================================================
ERROR: test_raises_deserializationerror (electrumabc.tests.test_avalanche.TestAvalancheDelegationFromHex)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/work/electrum/electrumabc/tests/test_avalanche.py", line 709, in test_raises_deserializationerror
    Delegation.from_hex("aabbcc")
  File "/work/electrum/electrumabc/serialize.py", line 58, in from_hex
    return cls.deserialize(BytesIO(bytes.fromhex(hex_str)))
  File "/work/electrum/electrumabc/avalanche/delegation.py", line 147, in deserialize
    levels = deserialize_sequence(stream, Level)
  File "/work/electrum/electrumabc/serialize.py", line 139, in deserialize_sequence
    for _ in range(size):
TypeError: 'NoneType' object cannot be interpreted as an integer

----------------------------------------------------------------------
Ran 328 tests in 34.922s

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

I broke another test when I changed how read_compact_size handles errors.

rebase onto D16336

Harbormaster completed remote builds in B29304: Diff 48248.Jun 14 2024, 20:20

Not really requesting changes here but I have questions

electrum/electrumabc/psbt.py
124 ↗	(On Diff #48248)	What if val_size is zero ?
171 ↗	(On Diff #48248)	Not sure about the PSBT_SEPARATOR here, see comment below
192 ↗	(On Diff #48248)	Wouldn't make sense to use the separator here, with PSBT_SEPARATOR.join() ? This would avoid the extra separator at the end of the last section which seems redundant with the end of stream

This revision now requires changes to proceed.Jun 17 2024, 08:00

PiRK added inline comments.Jun 17 2024, 08:58

electrum/electrumabc/psbt.py
124 ↗	(On Diff #48248)	That should achieve the expected behavior, val will just be the empty byte string. >>> from io import BytesIO >>> stream = BytesIO(b"spam") >>> stream.read(0) b''
192 ↗	(On Diff #48248)	I agree that it seems redundant, but the PSBT spec seems to clearly always require the final separator. See https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki <psbt> := <magic> <global-map> <input-map>* <output-map>* <magic> := 0x70 0x73 0x62 0x74 0xFF <global-map> := <keypair>* 0x00 <input-map> := <keypair>* 0x00 <output-map> := <keypair>* 0x00 I want to make sure that the format is as compliant as possible with the spec to maximize compatibility with other software.