Details

Reviewers

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

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 24910
Build 49408: Build Diff	electrum-tests
Build 49407: arc lint + arc unit

Event Timeline

PiRK created this revision.Aug 30 2023, 08:24

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 30 2023, 08:24

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

Harbormaster completed remote builds in B24899: Diff 42005.Aug 30 2023, 08:27

PiRK edited the summary of this revision. (Show Details)Aug 30 2023, 08:29

PiRK added a child revision: D14436: [electrum] use an IntEnum for script types.Aug 30 2023, 14:45

rebase and add comment for DUMMY_TXINPUT size

Harbormaster completed remote builds in B24910: Diff 42021.Aug 30 2023, 15:54

rebase

Harbormaster completed remote builds in B24913: Diff 42025.Aug 30 2023, 19:20

bytesofman added a subscriber: bytesofman.Aug 30 2023, 19:57

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

PiRK added inline comments.Aug 31 2023, 09:18

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.