test: refactor: deduplicate legacy ECDSA signing for tx inputs #28025

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202307-test-deduplicate_legacy_input_signing changing 4 files +24 −25

theStack commented at 3:35 PM on July 3, 2023: contributor
There are several instances in functional tests and the framework (MiniWallet, feature_block.py, p2p_segwit.py) where we create a legacy ECDSA signature for a certain transaction's input by doing the following steps:
1. calculate the LegacySignatureHash with the desired sighash type
2. create the actual digital signature by calling ECKey.sign_ecdsa on the signature message hash calculated above
3. put the DER-encoded result as CScript data push into tx input's scriptSig
Create a new helper sign_input_legacy which hides those details and takes only the necessary parameters (tx, input index, relevant scriptPubKey, private key, sighash type [SIGHASH_ALL by default]). For further convenience, the signature is prepended to already existing data-pushes in scriptSig, in order to avoid rehashing the transaction after calling the new signing function.

test: refactor: deduplicate legacy ECDSA signing for tx inputs

There are several instances in functional tests and the framework
(MiniWallet, feature_block.py, p2p_segwit.py) where we create a legacy
ECDSA signature for a certain transaction's input by doing the following
steps:
    1) calculate the `LegacySignatureHash` with the desired sighash type
    2) create the actual digital signature by calling `ECKey.sign_ecdsa`
       on the signature message hash calculated above
    3) put the DER-encoded result as CScript data push into
       tx input's scriptSig

Create a new helper `sign_input_legacy` which hides those details and
takes only the necessary parameters (tx, input index, relevant
scriptPubKey, private key, sighash type [SIGHASH_ALL by default]). For
further convenience, the signature is prepended to already existing
data-pushes in scriptSig, in order to avoid rehashing the transaction
after calling the new signing function.

5cf44275c8

DrahtBot commented at 3:35 PM on July 3, 2023: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK dimitaracev, pinheadmz, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on Jul 3, 2023
fanquake requested review from pinheadmz on Jul 4, 2023
dimitaracev commented at 8:14 PM on July 4, 2023: contributor

ACK 5cf4427
in test/functional/test_framework/script.py:692 in 5cf44275c8
```
 688 | @@ -689,6 +689,16 @@ def LegacySignatureHash(*args, **kwargs):
 689 |      else:
 690 |          return (hash256(msg), err)
 691 |  
 692 | +def sign_input_legacy(tx, input_index, input_scriptpubkey, privkey, sighash_type=SIGHASH_ALL):
```
pinheadmz commented at 7:52 PM on July 5, 2023:

Is it worth checking things like the input index exists and sighash is only one byte? Can we safely assume that tx.vin[input_index].scriptSig will always be a bytes object? (Even if it is empty?)

theStack commented at 12:09 AM on July 6, 2023:

Seems like the input index is checked in LegacySignatureMsg (which is in turn called by LegacySignatureHash): https://github.com/bitcoin/bitcoin/blob/bc4f6b13feb29146b7e10e86f93dc7f6fb6937f2/test/functional/test_framework/script.py#L638-L639 For sighash_type it might be nice to add a type annotation, though I don't know if the byte range (0..255) can be specified exactly (currently that value is not set explicitly, as all our call-sites use the default SIGHASH_ALL). As for tx.vin[input_index].scriptSig: it's default-initialized with an empty byte-string b'' (see CTxIn.__init__), and if the user sets it to a CScript instance that's also okay, as this is derived from bytes, so I think we can safely assume that the type is okay.
pinheadmz approved

Type	Reviewers
ACK	dimitaracev, pinheadmz, achow101

pinheadmz commented at 7:58 PM on July 5, 2023: member

ACK 5cf44275c8ca8c32d238f37f717d78e9823f44c2

Reviewed all code. Built and ran tests locally. Nice clean up!

Also - do you think we need a segwitV0 function like this as well? As far as I can grep there's only 2 such signing operations in p2p_segwit.py so maybe that's more work than its worth.

<details><summary>Show Signature</summary>

pinheadmz's public key is on keybase

</details>

theStack commented at 12:13 AM on July 6, 2023: contributor

Also - do you think we need a segwitV0 function like this as well? As far as I can grep there's only 2 such signing operations in p2p_segwit.py so maybe that's more work than its worth.

Good point -- I think it would be nice to introduce similar functions for other signature types (e.g. sign_input_segwitv0) for consistency reasons. Might be worth a follow-up, together with some other improvements pointed out in #28025 (review) (type annotations, input range checks etc.)? If anyone wants to tackle that, feel free to ping me as reviewer.
achow101 commented at 9:16 PM on July 11, 2023: member

ACK 5cf44275c8ca8c32d238f37f717d78e9823f44c2
achow101 merged this on Jul 11, 2023
achow101 closed this on Jul 11, 2023
theStack deleted the branch on Jul 11, 2023
theStack referenced this in commit 83d7cfd542 on Jul 25, 2023
achow101 referenced this in commit 8247a8db69 on Sep 20, 2023
Frank-GER referenced this in commit cc3afd7929 on Sep 27, 2023
Retropex referenced this in commit 52e33cdf56 on Oct 4, 2023
bitcoin locked this on Jul 10, 2024

Contributors

Labels

Linked (view graph)

#28154 test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs

test: refactor: deduplicate legacy ECDSA signing for tx inputs #28025

Reviews