Page MenuHomePhabricator
Differential D14472

[electrum] use TxInput (almost) everywhere in transaction.py
ClosedPublic
Actions

Authored by PiRK on Sep 12 2023, 15:13.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCcff5b05affcb: [electrum] use TxInput (almost) everywhere in transaction.py
Summary

This makes the Transaction class use TxInput everywhere internally, except for a few remaining methods that work with fetched_inputs which still need to be converted.

Input setters and their callsites are also converted:

  • Transaction.add_inputs
  • Transaction.set_inputs
  • Transaction.update_input
  • Transaction.from_io

As of this diff, sweep uses TxInput objects to estimate the tx size, so the test needs to be adjusted to have properly formed scriptsigs that can be parsed by TxInput.

Depends on D14510

Test Plan

python test_runner.py

Send and receive transaction with the application.
Send transactions with hardware wallet.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
txinput_transaction
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25065
Build 49718: Build Diffelectrum-tests
Build 49717: arc lint + arc unit

Event Timeline

PiRK created this revision.Sep 12 2023, 15:13
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 12 2023, 15:13
Harbormaster completed remote builds in B25004: Diff 42161.Sep 12 2023, 15:14
teamcity added a comment.Sep 12 2023, 15:14
Build Bitcoin ABC Diffs / Diff Testing (electrum-tests) failed.

Tail of the build log:

KeyError: 'x_pubkeys'

======================================================================
ERROR: test_sweep_dust (electrumabc.tests.test_wallet.TestSweep)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/work/electrum/electrumabc/tests/test_wallet.py", line 262, in test_sweep_dust
    sweep([WIF1], network, config, recipient)
  File "/work/electrum/electrumabc/wallet.py", line 227, in sweep
    fee = config.estimate_fee(tx.estimated_size())
  File "/work/electrum/electrumabc/util.py", line 272, in <lambda>
    return lambda *args, **kw_args: do_profile(args, kw_args)
  File "/work/electrum/electrumabc/util.py", line 267, in do_profile
    o = func(*args, **kw_args)
  File "/work/electrum/electrumabc/transaction.py", line 1355, in estimated_size
    if self.is_complete() and self.raw is not None:
  File "/work/electrum/electrumabc/transaction.py", line 1388, in is_complete
    s, r = self.signature_count()
  File "/work/electrum/electrumabc/transaction.py", line 1379, in signature_count
    for txin in self.inputs():
  File "/work/electrum/electrumabc/transaction.py", line 831, in inputs
    return [txin.to_coin_dict() for txin in self.txinputs()]
  File "/work/electrum/electrumabc/transaction.py", line 831, in <listcomp>
    return [txin.to_coin_dict() for txin in self.txinputs()]
  File "/work/electrum/electrumabc/transaction.py", line 372, in to_coin_dict
    d["type"] = self.type.name
AttributeError: 'NoneType' object has no attribute 'name'

======================================================================
FAIL: test_tx_nonminimal_scriptSig (electrumabc.tests.test_transaction.TestTransaction)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/work/electrum/electrumabc/tests/test_transaction.py", line 351, in test_tx_nonminimal_scriptSig
    self.assertEqual(
AssertionError: '66020177ae3273d874728667b6a24e0a1c0200079119f3d0c294da40f0e85d34' != 'e64808c1eb86e8cab68fcbd8b7f3b01f8cc8f39bd05722f1cf2d7cd9b35fb4e3'
- 66020177ae3273d874728667b6a24e0a1c0200079119f3d0c294da40f0e85d34


======================================================================
FAIL: test_tx_unsigned (electrumabc.tests.test_transaction.TestTransaction)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/work/electrum/electrumabc/tests/test_transaction.py", line 203, in test_tx_unsigned
    self.assertEqual(tx.estimated_size(), len(signed_blob) // 2 + 1)
AssertionError: 172 != 192

======================================================================
FAIL: test_sweep (electrumabc.tests.test_wallet.TestSweep)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/work/electrum/electrumabc/tests/test_wallet.py", line 224, in test_sweep
    self.assertEqual(inputs[0].type, ScriptType.p2pkh)
AssertionError: None != <ScriptType.p2pkh: 0>

----------------------------------------------------------------------
Ran 287 tests in 10.974s

FAILED (failures=3, errors=2, skipped=7)
ninja: build stopped: cannot make progress due to previous errors.
Build electrum-tests failed with exit code 1
PiRK updated this revision to Diff 42249.Sep 18 2023, 13:36

rebase onto a lot of intermediate diffs

Harbormaster completed remote builds in B25065: Diff 42249.Sep 18 2023, 13:45
teamcity added a comment.Sep 18 2023, 13:45
Build Bitcoin ABC Diffs / Diff Testing (electrum-tests) failed.

Tail of the build log:

-- Found Boost: /usr/lib/x86_64-linux-gnu/cmake/Boost-1.74.0/BoostConfig.cmake (found suitable version "1.74.0", minimum required is "1.64")  
-- Found Event component pthreads: /usr/lib/x86_64-linux-gnu/libevent_pthreads.so
-- Found Event: /usr/include (found suitable version "2.1.12-stable", minimum required is "2.0.22") found components: pthreads 
-- Found MiniUPnPc component miniupnpc: /usr/lib/x86_64-linux-gnu/libminiupnpc.so
-- Found MiniUPnPc: /usr/include/miniupnpc (found suitable version "2.2.1", minimum required is "1.9")  
-- Found NATPMP component natpmp: /usr/lib/x86_64-linux-gnu/libnatpmp.so
-- Found NATPMP: /usr/include   
-- 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.1n")  
-- 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
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)
.........................................................................................................127.0.0.1 - - [18/Sep/2023 13:45:22] "GET /invoice HTTP/1.1" 503 -
.127.0.0.1 - - [18/Sep/2023 13:45:22] "GET / HTTP/1.1" 200 -
.127.0.0.1 - - [18/Sep/2023 13:45:22] "GET /invoice HTTP/1.1" 200 -
127.0.0.1 - - [18/Sep/2023 13:45:22] "POST /pay HTTP/1.1" 200 -
.127.0.0.1 - - [18/Sep/2023 13:45:23] "GET / HTTP/1.1" 200 -
.s..................Traceback (most recent call last):
  File "/work/electrum/electrumabc_plugins/trezor/trezor.py", line 29, in <module>
    import trezorlib
ModuleNotFoundError: No module named 'trezorlib'
...................................................................................F............................s.........sss......................................s.s.
======================================================================
FAIL: test_tx_unsigned (electrumabc.tests.test_transaction.TestTransaction)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/work/electrum/electrumabc/tests/test_transaction.py", line 207, in test_tx_unsigned
    self.assertEqual(tx.serialize(), unsigned_blob)
AssertionError: '0100[257 chars]fffff0118e43201000000001976a914e158fb15c888037[33 chars]0700' != '0100[257 chars]fffffd8e43201000000000118e43201000000001976a91[49 chars]0700'
Diff is 996 characters long. Set self.maxDiff to None to see it.

----------------------------------------------------------------------
Ran 291 tests in 11.588s

FAILED (failures=1, skipped=7)
ninja: build stopped: cannot make progress due to previous errors.
Build electrum-tests failed with exit code 1
PiRK updated this revision to Diff 42273.Sep 19 2023, 15:36
PiRK edited the summary of this revision. (Show Details)
PiRK edited the test plan for this revision. (Show Details)

rebase, minor improvements (assertions for type of inputs, more uses of inputs() converted in Transaction) and fixes (hardware wallets)

Harbormaster completed remote builds in B25082: Diff 42273.Sep 19 2023, 15:37
PiRK updated this revision to Diff 42276.Sep 19 2023, 15:59
PiRK edited the summary of this revision. (Show Details)

rebase and:

  • remove unused sign_schnorr argument in sweep_preparations (I think at some point I passed this to TxInput.from_coin_dict)
  • remove a debugging comment in test_transaction
  • document Transaction.inputs() (mention that it is deprecated)
Harbormaster completed remote builds in B25083: Diff 42276.Sep 19 2023, 16:00
PiRK published this revision for review.Sep 19 2023, 16:00
PiRK added inline comments.Sep 19 2023, 16:04
electrum/electrumabc/transaction.py
1451 ↗(On Diff #42276)

Cleaning this is on my TODO list, but I need to dive into the code related to fetched_inputs first.

Fabien requested changes to this revision.Sep 20 2023, 09:28
Fabien added a subscriber: Fabien.

It seems to me the prev_tx change should be its own diff

electrum/electrumabc/tests/test_transaction.py
20 ↗(On Diff #42276)

?

769 ↗(On Diff #42276)

I don't think it really matters but is there any benefit in using a tuple instead of a list ? It's ro anyway

This revision now requires changes to proceed.Sep 20 2023, 09:28
PiRK updated this revision to Diff 42290.Sep 20 2023, 13:18
PiRK edited the summary of this revision. (Show Details)

rebase after doing the prev_tx change in another diff

PiRK edited the summary of this revision. (Show Details)Sep 20 2023, 13:19
PiRK added a parent revision: D14510: [electrum] add prev_tx to TxInput.
Harbormaster completed remote builds in B25096: Diff 42290.Sep 20 2023, 13:20
PiRK planned changes to this revision.Sep 20 2023, 13:21

i think there was a rebase incident

PiRK updated this revision to Diff 42292.Sep 20 2023, 13:27

fix typo (i'm pretty sure I fixed this before, but maybe it got lost in an aborted rebase)
Rerun all the interactive tests to be sure.

Harbormaster completed remote builds in B25098: Diff 42292.Sep 20 2023, 13:29
Fabien accepted this revision.Sep 20 2023, 14:25
Fabien added inline comments.
electrum/electrumabc/transaction.py
410 ↗(On Diff #42292)

ouch

This revision is now accepted and ready to land.Sep 20 2023, 14:25
Closed by commit rABCcff5b05affcb: [electrum] use TxInput (almost) everywhere in transaction.py (authored by PiRK). · Explain WhySep 20 2023, 15:26
This revision was automatically updated to reflect the committed changes.
PiRK added a commit: rABCcff5b05affcb: [electrum] use TxInput (almost) everywhere in transaction.py.
PiRK mentioned this in D14521: [electrum] fix OutPoint.__str__.Sep 21 2023, 15:38
PiRK mentioned this in rABC75b8d794395d: [electrum] fix OutPoint.__str__.Sep 21 2023, 16:07
PiRK mentioned this in D14810: [electrum] fix signature updating in fusion code.Nov 17 2023, 15:21
PiRK mentioned this in rABCfe2b435dc1e9: [electrum] fix signature updating in fusion code.Nov 20 2023, 11:32

Revision Contents
Changeset List

PathSize
electrum/
electrumabc/
4 lines
14 lines
6 lines
2 lines
tests/
17 lines
32 lines
63 lines
28 lines
electrumabc_gui/
qt/
10 lines
electrumabc_plugins/
digitalbitbox/
4 lines

Diff 42249

View Options

electrum/electrumabc/coinchooser.py

Loading...
View Options

electrum/electrumabc/commands.py

Loading...
View Options

electrum/electrumabc/consolidate.py

Loading...
View Options

electrum/electrumabc/paymentrequest.py

Loading...
View Options

electrum/electrumabc/tests/test_transaction.py

Loading...
View Options

electrum/electrumabc/tests/test_wallet.py

Loading...
View Options

electrum/electrumabc/transaction.py

Loading...
View Options

electrum/electrumabc/wallet.py

Loading...
View Options

electrum/electrumabc_gui/qt/main_window.py

Loading...
View Options

electrum/electrumabc_plugins/digitalbitbox/digitalbitbox.py

Loading...