[electrum] move some methods from Transaction to TxInput
ClosedPublic
Actions

Authored by PiRK on Sep 15 2023, 16:00.

Details

Reviewers

bytesofman

Group Reviewers

Restricted Project

Commits

rABC470667730709: [electrum] move some methods from Transaction to TxInput

Summary

Remove some code duplication and move code that works purely on inputs to the TxInput class.
Keep some shortcuts/helpers in Transaction. We can remove them (and avoid a dict -> TxInput conversion) when Transaction entirely works with TxInputs instead of coin dicts.

The multisig_script function can be converted to using and returning bytes to reduce the number of needed conversions, now that it is mainly called in TxInput

Depends on D14493

Test Plan

python test_runner.py

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25059
Build 49706: Build Diff	electrum-tests
Build 49705: arc lint + arc unit

Event Timeline

PiRK created this revision.Sep 15 2023, 16:00

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 15 2023, 16:00

PiRK requested review of this revision.Sep 15 2023, 16:00

Harbormaster completed remote builds in B25056: Diff 42231.Sep 15 2023, 16:02

PiRK edited the summary of this revision. (Show Details)Sep 15 2023, 16:02

do we need to preserve sign_schnorr as a param in so many places? could we just have electrumabc always sign schnorr unless there was some tx type that didn't support it?

electrum/electrumabc/transaction.py
946 ↗	(On Diff #42231)	are we ultimately converting everything away from `.hex()` and to `bytes`? Is this change a consequence of the specific improvement targeted in this diff? Is it a general benefit? Still some other instances of `.hex()` in this file.

PiRK added inline comments.Sep 16 2023, 08:41

electrum/electrumabc/transaction.py
946 ↗	(On Diff #42231)	Yes, we are ultimately removing the use of hex strings in the codebase, and only converting `hex <-> bytes` when reading/writing json or saving transactions to files. `bytes` use only half as much memory and are faster to work with, so this will improve the performance. Also there is already a lot of places that do internal conversions back and forth, because currently the codebase is not consistent, so picking one everywhere is a benefit in itself, no matter which one we pick.

In D14494#325399, @bytesofman wrote:

do we need to preserve sign_schnorr as a param in so many places? could we just have electrumabc always sign schnorr unless there was some tx type that didn't support it?

There are still some wallet types that don't support schnorr signing, such as hardware wallets and multisig wallets.

rebase

Harbormaster completed remote builds in B25059: Diff 42234.Sep 16 2023, 08:59

bytesofman accepted this revision.Sep 16 2023, 14:27

This revision is now accepted and ready to land.Sep 16 2023, 14:27

Closed by commit rABC470667730709: [electrum] move some methods from Transaction to TxInput (authored by PiRK). · Explain WhySep 18 2023, 05:47

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC470667730709: [electrum] move some methods from Transaction to TxInput.