Page MenuHomePhabricator

[electrum] start implementing the PSBT format
ClosedPublic

Authored by PiRK on Jun 14 2024, 16:23.

Details

Reviewers
Fabien
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
Branch
psbt
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 29312
Build 58160: Build Diffelectrum-tests
Build 58159: arc lint + arc unit

Event Timeline

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

copyright header missing

electrum/electrumabc/psbt.py
139 ↗(On Diff #48242)

The walrus operator (:=) has been added in python 3.8.

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

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.

Fabien requested changes to this revision.Jun 17 2024, 08:00
Fabien added a subscriber: Fabien.

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
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.

fix a comment in the tests

This revision is now accepted and ready to land.Jun 17 2024, 09:20