Page MenuHomePhabricator

[electrum] implement scriptSig parsing for TxInput
ClosedPublic

Authored by PiRK on Sep 7 2023, 10:48.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC7e2809b5198c: [electrum] implement scriptSig parsing for TxInput
Summary

This should bring TxInput to feature-parity with the existing input dicts in the case of deserialized transactions.

A couple of functions and a constant are converted to bytes, to avoid unnecessary hex decoding in the new function. All exisiting callsites for these functions are adjusted to expect bytes.

Signatures and keys are stored as bytes (can be trivially converted to hex if necessary)

Coinbase inputs have an empty list of signatures and num_sig == 0, rather than not defining the attributes.

Redundant fields are not implemented:

  • "num_sig" can be trivially deduced from the number of signature
  • "redeemScript" is always the same as scriptSig, as far as I can tell
  • p2pk inputs don't have a pubkey or an address

For now the TxInput.parse_scriptSig method duplicates the logic in the parse_scriptSig function. Same for TxInput.get_sorted_pubkeys and Transaction.get_sorted_pubkeys. This is done intentionally, to demonstrate that the results are the same (via the added test).

The next commit will remove the old function.

Depends on D14466

Test Plan

python test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Sep 7 2023, 10:48
PiRK planned changes to this revision.Sep 7 2023, 11:01

I need to fix the error with multisig wallets that shows in the next commit. Not sure in which commit I made a mistake.

test deserializing a partially signed multisig input, clarify that num_sig is the number of REQUIRED sigs (including missing ones for partial txs)

A couple of functions and a constant are converted to bytes, to avoid unnecessary hex decoding in the new function. All exisiting callsites for these functions are adjusted to expect bytes.

Signatures and keys are stored as bytes (can be trivially converted to hex if necessary)

This diff introduces a few conversions to hex -- are these new conversions less costly than hex decoding? Why was everything in hex before ... ? Was it just easier to write it that way, was hex decoding unanticipated until later features came in?

This diff introduces a few conversions to hex -- are these new conversions less costly than hex decoding? Why was everything in hex before ... ? Was it just easier to write it that way, was hex decoding unanticipated until later features came in?

The end goal here is to have bytes everywhere for internal data structures, which will ultimately make everything faster and less costly, and convert to hex only for screen display or file IO (exporting coins to JSON...)
To avoid making one giant impossible to review diff, the conversions to hex are necessary when interfacing converted function to functions that still work with hex. This should be transitory.

I don't know why the choice was made to work mostly with hex, but it didn't make much sense to start with. I may have been influenced by Python 2's bad design (string and bytes were the same type, so working with bytes was a bit more confusing, you never knew if you were dealing with raw data or hex data.

The tests are the one place where hex is still going to be used, just for convenience and readability ("coffeedeadbeef" vs b"\xco\xff\xee\xde\xad\xbe\xef").

remove unused attribute TxInput._num_sig. I forgot to remove this when I choose the use len(self.signatures) to compute this

Fabien added inline comments.
electrum/electrumabc/transaction.py
168 ↗(On Diff #42119)

Do we really need to store both ? Seems like the xpubkey could be cheaply computed from pubkey and type already, that would save some memory

electrum/electrumabc/transaction.py
168 ↗(On Diff #42119)

The code dealing with x_pubkeys is confusing, sometimes (often) it is just a pubkey, sometimes it is a xpub and a derivation path. In most cases, it seems only the pubkey is really used, except in the hardware wallet related code (which has very poor test coverage) and for partially signed transactions which serialize the tx using the xpub + derivation (with 0xff prefix).

If we want to store just one it would be simpler to just store the x_pubkey. But then I don't think computing the pubkey is always cheap, and I don't like storing normal pubkey while calling them xpubkey.

I think for now it is best to mimic the existing behavior as closely as possible, until I get a better grip on the xpubkey thing.

Maybe what would make sense is to only store the xpubkey when it is actually different from the pubkey. It will require more codechanges, though, because of all the code that reads x_pubkeys and assumes that it will contain a pubkey when the xpub is not necessary.

PiRK planned changes to this revision.Sep 8 2023, 11:00
PiRK added inline comments.
electrum/electrumabc/transaction.py
247 ↗(On Diff #42119)

This assertion seems to be wrong, I managed to make it raise when opening an unsigned transaction from file.

electrum/electrumabc/transaction.py
168 ↗(On Diff #42119)

OK, no blocker here (it's not different from the actual situation)

I figured out the issue, the assertion is wrong. There can be N elements in the signatures list for partially signed transactions (some of them None).

I tested only a 2-of-2 multisig, I need to add a test for an unsigned 2-of-3 multisig or any scheme with M < N.

For fully signed transactions (the ones that work with decoderawtransaction) the assertion was correct.

electrum/electrumabc/transaction.py
211 ↗(On Diff #42124)

Discussed offline, this doesn't do what the name implies and is a genuine disgusting piece of code.

236 ↗(On Diff #42124)

No support for schnorr multisig ?

249 ↗(On Diff #42124)

I guess there is no reason to parse unspendable txs here right ?

PiRK edited the summary of this revision. (Show Details)

rebase and add dependency on D14466

electrum/electrumabc/transaction.py
236 ↗(On Diff #42124)

No. It has never been implemented as far as I can tell.

electrum/electrumabc/transaction.py
234 ↗(On Diff #42139)
241 ↗(On Diff #42139)

Will that fail in partially signed txs ?

electrum/electrumabc/transaction.py
241 ↗(On Diff #42139)

No, because partially signed txs have all "n" signatures (some of thel NO_SIGNATURE (b"\xFF").

Fully signed transactions will have "m" signatures.

This revision is now accepted and ready to land.Sep 12 2023, 07:22