test: refactor: deduplicate legacy ECDSA signing for tx inputs #28025
Conversation
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
ACK |
pinheadmz
left a comment
There was a problem hiding this comment.
ACK 5cf4427
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.
Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 5cf44275c8ca8c32d238f37f717d78e9823f44c2
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmSlyacACgkQ5+KYS2KJ
yToilhAA0xbzuJ7W+WAuxPjb/FbZU+aOlRHm5NJrxEWJXv4AUcyykkioGt/EfOKQ
w8iBlgQzr4HHdnJeDryZ+d+4vJOdPjrF2h9HH+OLnY/WihGEpPV2dtjFqeiZchtJ
dI7mryFHtNa8mpMAW7zJZ00jGZuYd9BTcKdE2JK/izjam1JwN1l6BkLnB/6QSfXS
Twg5HZbkHboa9ojYiz6KHt3lodMH+xWdC2TVQKO5rthjZ2tVws/4AWxpsnnHk6wL
VhMgo1vKKZOF3d0f29fbXb1mPaiCfz+BBPCSZZrj+Uzvpz9eLa71vGo7npudfO9H
tvxQnL42tRdUvftmlMZh6uneBuAICrjeELvRdu0+9CO4PRlbno2BMhNS1BB1+rwO
XJgq4Tqzv5eUBywxWF97zfIXDPn/8YjSVB0YAAOBAArSmDRo78aMXdPTF7+nT07T
xCS06fDj4wi2XDD00nuB6uPVfYSHYfZJ7vSaZQlZFYVSfScblFD2xgM4kcYkmz4c
VlqpTxI3nmx6enjNvw4t8T3OeXEkO/n7shiKXJ1v6VMNbefyTZtW2833wZTbWU4O
8oZkZoWSdd1S4yzU3Q1FQm0JWscSPYiweexHf2xIP2apWxhoCW0gSOdwTfCtbJcG
ClHU26Qms4nqlLK6YvTXlaEAB1x8QCLqRr95lpbkOAv/9Iv7xlY=
=WdbY
-----END PGP SIGNATURE-----
pinheadmz's public key is on keybase
| else: | ||
| return (hash256(msg), err) | ||
|
|
||
| def sign_input_legacy(tx, input_index, input_scriptpubkey, privkey, sighash_type=SIGHASH_ALL): |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
Seems like the input index is checked in LegacySignatureMsg (which is in turn called by LegacySignatureHash):
bitcoin/test/functional/test_framework/script.py
Lines 638 to 639 in bc4f6b1
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.
Good point -- I think it would be nice to introduce similar functions for other signature types (e.g. |
|
ACK 5cf4427 |
…tx inputs 83d7cfd test: refactor: deduplicate segwitv0 ECDSA signing for tx inputs (Sebastian Falbesoner) Pull request description: This PR is a simple follow-up for #28025. It introduces a `signing_input_segwitv0` helper in order to deduplicate the following steps needed to create a segwitv0 ECDSA signature: 1. calculate the `SegwitV0SignatureHash` 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 (plus sighash byte) at the bottom of the witness stack ACKs for top commit: achow101: ACK 83d7cfd pinheadmz: code review ACK at 83d7cfd Tree-SHA512: b8e55409ddc9ddb14cfc06daeb4730d7750a4632f175f88dcac6ec4d216e71fd4a7eee325a64d6ebba3b33be50bcd30c2de7400f834c01abb67e52840d9823b6
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:
LegacySignatureHashwith the desired sighash typeECKey.sign_ecdsaon the signature message hash calculated aboveCreate a new helper
sign_input_legacywhich 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.