Page MenuHomePhabricator

[electrum] add a TxInput class
ClosedPublic

Authored by PiRK on Aug 30 2023, 08:24.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCb4412e42c6c6: [electrum] add a TxInput class
Summary

The end goal is to replace the clumsy "coin" dictionary full of hex strings by this more lightweight object.

An intermediate goal is to use its size() method to compute/estimate transaction sizes without having to construct and serialize the tx.
This will likely yield a performance improvement for some operation and allow the coinchooser to not produce transactions that can't be relayed due to the maximum 100 kB size policy.

The __eq__ methods in OutPoint and TxInput are used for testing equality between two instances.
The __str__ methods are implemented for convenience/debugging, as they make assertEqual produce a more helpful log in case a test fails.

Depends on D14433

Test Plan

python test_runner.py

Diff Detail

Event Timeline

PiRK requested review of this revision.Aug 30 2023, 08:24

rebase and add comment for DUMMY_TXINPUT size

bytesofman added inline comments.
electrum/electrumabc/tests/test_transaction.py
575 ↗(On Diff #42025)

is this doing anything different from test to test? what is being added by including this in every test?

do all of these test txs have just one input that is the same input?

Fabien requested changes to this revision.Aug 31 2023, 08:32
Fabien added a subscriber: Fabien.
Fabien added inline comments.
electrum/electrumabc/tests/test_transaction.py
27 ↗(On Diff #42025)

Please comment: including the sighash byte

29 ↗(On Diff #42025)

the name is not very descriptive of the content

575 ↗(On Diff #42025)

that's what is in the tx hex above. You can try decoderawtransaction <hex> on your node.

electrum/electrumabc/transaction.py
612 ↗(On Diff #42025)

Create another method. There is no point having the same method do 2 totally different things based on a boolean.

This revision now requires changes to proceed.Aug 31 2023, 08:32
electrum/electrumabc/tests/test_transaction.py
575 ↗(On Diff #42025)

My thinking was that it serves as documentation, to indicate that the input does not change in between tests, but maybe the long string of zero at the beginning of the transaction is enough.
I agree that it does not add a tremendous value to do it more than once.

electrum/electrumabc/transaction.py
612 ↗(On Diff #42025)

I was hoping to transition this method to eventually end up doing only the new thing. But that can also be done in two steps, with a new method.

electrum/electrumabc/transaction.py
612 ↗(On Diff #42025)

This will be 2 steps in both case, but with 2 functions you don't introduce dirty code in the middle

add a Transaction.txinputs function instead of extending the existing Transaction.inputs

The long term goal is still to use the same everywhere, and then it can be renamed inputs when the old one can be removed

Fabien added inline comments.
electrum/electrumabc/tests/test_transaction.py
29 ↗(On Diff #42025)

SCHNORR_P2PKH_SCRIPTSIG_NBYTES

This revision is now accepted and ready to land.Aug 31 2023, 18:18
This revision was automatically updated to reflect the committed changes.