Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABC2673549a2fd8: [electrum] replace match_decoded function with more explicit functions

Summary

The match_decoded function is surprising, it matches all push opcodes with OP_PUSHDATA4. Also its name is not very descriptive.

Replace it with more explicit functions: is_push_opcode, matches_p2pk_scriptsig, matches_p2pkh_scriptsig, matches_p2sh_multisig_scriptsig, matches_multisig_redeemscript

Document the Script.get_ops classmethod that produces the input for these functions.

Test Plan

python test_runner.py

python -m electrumabc.transaction

The additional tests added in D14458, D14459 and D14461 also pass (after rebasing on this)

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 24992
Build 49572: Build Diff	electrum-tests
Build 49571: arc lint + arc unit

Event Timeline

PiRK created this revision.Sep 11 2023, 10:45

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 11 2023, 10:45

PiRK requested review of this revision.Sep 11 2023, 10:45

Harbormaster completed remote builds in B24987: Diff 42129.Sep 11 2023, 10:46

PiRK added a child revision: D14458: [electrum] implement scriptSig parsing for TxInput.Sep 11 2023, 10:49

Fabien requested changes to this revision.Sep 11 2023, 12:01

Fabien added a subscriber: Fabien.

Fabien added inline comments.

electrum/electrumabc/transaction.py
330 ↗	(On Diff #42129)	and OP_RESERVED. Shouldn't you also manage the other opcodes ? It's trivial to do so and the function would have no surprising side effect. Actually another useful option is to give a minimal data size, so you can check it's a signature, an pubkey, a hash etc. The only thing the dismissed opcodes have in common is that their size is 1
350 ↗	(On Diff #42129)	that's the opposite
359 ↗	(On Diff #42129)	matches_p2sh_ecdsa_multisig_scriptsig

This revision now requires changes to proceed.Sep 11 2023, 12:01

review: fix nits and make is_push_opcode more precise

Harbormaster completed remote builds in B24992: Diff 42137.Sep 11 2023, 16:27

comment nits

Harbormaster completed remote builds in B24994: Diff 42140.Sep 11 2023, 18:13

Fabien requested changes to this revision.Sep 11 2023, 20:18

Fabien added inline comments.

electrum/electrumabc/transaction.py
329 ↗	(On Diff #42140)	It's a good candidate for some doctest
330 ↗	(On Diff #42140)
333 ↗	(On Diff #42140)	Since min_data_size is an int, isn't it more correct to use == 0 here ?
335 ↗	(On Diff #42140)
341 ↗	(On Diff #42140)	dito

This revision now requires changes to proceed.Sep 11 2023, 20:18

PiRK added inline comments.Sep 12 2023, 06:18

electrum/electrumabc/transaction.py
333 ↗	(On Diff #42140)	I was convinced that PEP8 had a rule about this, but apparently I overinterpreted the rule that says "do not compare `x == True/False`.