Proposal of refactoring of code for utxo tx signature creation.
History of the problem
Historically, signing code started with simple signing of 'iguana' private key policy (single key). Later code was added for trezor hierarchical deterministic (HD) wallet with user interaction support (to answer the device requests). Also software HD wallet support was added (in the part with no user interaction). Currently HD wallets, software and hardware are supported for withdrawals only (no swaps supported yet).
Reasons to refactor
Reasons that require refactoring are that our signing features (iguana, trezor, hd wallet) may be implemented differently and use various helper objects and params, sometimes duplicate.
Architectural issues
Architectural issues can be summarised as use of different objects for similar tasks (leading to lack of code reuse).
For utxo signing we have a TransactionInputSigner object which contains transaction fields, including inputs and outputs. This object can build signature hash and sign it. Inputs in the TransactionInputSigner are objects of UnsignedTransactionInput type which are extended to have not only the usual previous outpoints but other fields from previous transactions needed for signing, like amount and prev_script.
To sign with software key we have a function with_key_pair::sign_tx() which accepts TransactionInputSigner and calls p2pkh_spend() which in turn calls calc_and_sign_sighash() which actually signs one input. However, calc_and_sign_sighash() has a TransactionInputSigner param with prev_script in inputs but also gets prev_script as a dedicated output_script param. Thus we have duplication here. But as we set prev_script in inputs only for p2pkh outputs, we still need output_script in calc_and_sign_sighash() for other cases, like for p2sh we need redeem_script.
As a result prev_script in UnsignedTransactionInput is set empty in some cases, which is not good.
Suggestion: add other prev script types into an input objects (like UnsignedTransactionInput in TransactionInputSigner or in later suggested SpendingInputInfo) as a enum and eliminate output_script param in calc_and_sign_sighash().
Although calc_and_sign_sighash() is a low-level function it is used by its own (without sign_tx() on top) to sign swap tx where we need pure signature value produced by calc_and_sign_sighash() to create a custom input for the swap tx.
Suggestion: again, maybe eliminate output_script param in calc_and_sign_sighash() and use TransactionInputSigner
For signing Trezor new helper objects were added, SpendingInputInfo and SendingOutputInfo. In partilular SpendingInputInfo is used to pass the derivation_path to derive signing key in Trezor.
Maybe we could use the same SpendingInputInfo to derive signing key in the software HD wallet code, but instead in the interactive InitUtxoWithdraw sign_tx() we does not derive key and use the default activation_key. This is commented as 'InitUtxoWithdraw is used for hardware wallets only'.
Suggestion: maybe better to derive keys for the software HD wallet too (as for Trezor)
Suggestion: maybe use SpendingInputInfo for the software HD wallet for key derivation, like for Trezor?
Again, we don't derive keys in InitUtxoWithdraw but we do derive keys in StandardUtxoWithdraw new() and pass the derived key in sign_tx.
Suggestion: could we use SpendingInputInfo and derive the key with its help in all cases (trezor, hd wallet, InitUtxoWithdraw and StandardUtxoWithdraw, and possibly in swaps?
When we need to sign a tx we create TransactionInputSigner with a vec of inputs of UnsignedTransactionInput type.
Actual signing of one input is done in a low-level calc_and_sign_sighash() which gets TransactionInputSigner with a vec of inputs of UnsignedTransactionInput. However to sign just one input we do not need signing data in each input, but only for the one which is signed. So we may have to pass the rest of inputs with empty signing data (prev_script, amount)
Suggestion: maybe remove signing data from UnsignedTransactionInput and add them into SpendingInputInfo? Then we could create and pass only one SpendingInputInfo used for signing (instead of vector) into calc_and_sign_sighash(), so we will never need to create vectors of inputs with empty signing data. Extend SpendingInputInfo with signing data and use it for all signing cases.
There is yet another struct P2SHSpendingTxInput which also is used to sign a tx and contains the tx itself and data for signing.
Suggestion: study if it is possible to eliminate P2SHSpendingTxInput and use a unified struct (TransactionInputSigner with SpendingInputInfo, if we agree with other suggestions here).
Naming issues
IMHO 'unsigned_tx' or 'unsigned' does not look like a good name for var of TransactionInputSigner type, 'tx_signer' seems to be better. In some places we use also 'signer' name, let's use a unified name everywhere.
Same name sign_tx() is used in several cases for utxo signing and that's confusing. F.e. we have sign_tx() in StandardUtxoWithdraw which calls with_key_pair::sign_tx(). Also we have sign_tx() in InitUtxoWithdraw which calls sign_tx() in UtxoSignerOps, which in turn calls with_key_pair::sign_tx() or sign_tx() in TrezorTxSigner.
Suggestion: use more distinguished names for lower level functions, like sign_tx_with_policy in UtxoSignerOps, sign_tx_impl in with_key_pair mod.
We have p2sh_spend() function and there is also spend_p2sh() in slp.rs, what does not look good IMHO. Maybe in slp it could be renamed to build_p2sh_spending_tx().
Proposal of refactoring of code for utxo tx signature creation.
History of the problem Historically, signing code started with simple signing of 'iguana' private key policy (single key). Later code was added for trezor hierarchical deterministic (HD) wallet with user interaction support (to answer the device requests). Also software HD wallet support was added (in the part with no user interaction). Currently HD wallets, software and hardware are supported for withdrawals only (no swaps supported yet).
Reasons to refactor Reasons that require refactoring are that our signing features (iguana, trezor, hd wallet) may be implemented differently and use various helper objects and params, sometimes duplicate.
Architectural issues Architectural issues can be summarised as use of different objects for similar tasks (leading to lack of code reuse).
For utxo signing we have a TransactionInputSigner object which contains transaction fields, including inputs and outputs. This object can build signature hash and sign it. Inputs in the TransactionInputSigner are objects of UnsignedTransactionInput type which are extended to have not only the usual previous outpoints but other fields from previous transactions needed for signing, like amount and prev_script.
To sign with software key we have a function with_key_pair::sign_tx() which accepts TransactionInputSigner and calls p2pkh_spend() which in turn calls calc_and_sign_sighash() which actually signs one input. However, calc_and_sign_sighash() has a TransactionInputSigner param with prev_script in inputs but also gets prev_script as a dedicated output_script param. Thus we have duplication here. But as we set prev_script in inputs only for p2pkh outputs, we still need output_script in calc_and_sign_sighash() for other cases, like for p2sh we need redeem_script. As a result prev_script in UnsignedTransactionInput is set empty in some cases, which is not good. Suggestion: add other prev script types into an input objects (like UnsignedTransactionInput in TransactionInputSigner or in later suggested SpendingInputInfo) as a enum and eliminate output_script param in calc_and_sign_sighash().
Although calc_and_sign_sighash() is a low-level function it is used by its own (without sign_tx() on top) to sign swap tx where we need pure signature value produced by calc_and_sign_sighash() to create a custom input for the swap tx. Suggestion: again, maybe eliminate output_script param in calc_and_sign_sighash() and use TransactionInputSigner
For signing Trezor new helper objects were added, SpendingInputInfo and SendingOutputInfo. In partilular SpendingInputInfo is used to pass the derivation_path to derive signing key in Trezor. Maybe we could use the same SpendingInputInfo to derive signing key in the software HD wallet code, but instead in the interactive InitUtxoWithdraw sign_tx() we does not derive key and use the default activation_key. This is commented as 'InitUtxoWithdraw is used for hardware wallets only'. Suggestion: maybe better to derive keys for the software HD wallet too (as for Trezor) Suggestion: maybe use SpendingInputInfo for the software HD wallet for key derivation, like for Trezor?
Again, we don't derive keys in InitUtxoWithdraw but we do derive keys in StandardUtxoWithdraw new() and pass the derived key in sign_tx. Suggestion: could we use SpendingInputInfo and derive the key with its help in all cases (trezor, hd wallet, InitUtxoWithdraw and StandardUtxoWithdraw, and possibly in swaps?
When we need to sign a tx we create TransactionInputSigner with a vec of inputs of UnsignedTransactionInput type. Actual signing of one input is done in a low-level calc_and_sign_sighash() which gets TransactionInputSigner with a vec of inputs of UnsignedTransactionInput. However to sign just one input we do not need signing data in each input, but only for the one which is signed. So we may have to pass the rest of inputs with empty signing data (prev_script, amount) Suggestion: maybe remove signing data from UnsignedTransactionInput and add them into SpendingInputInfo? Then we could create and pass only one SpendingInputInfo used for signing (instead of vector) into calc_and_sign_sighash(), so we will never need to create vectors of inputs with empty signing data. Extend SpendingInputInfo with signing data and use it for all signing cases.
There is yet another struct P2SHSpendingTxInput which also is used to sign a tx and contains the tx itself and data for signing. Suggestion: study if it is possible to eliminate P2SHSpendingTxInput and use a unified struct (TransactionInputSigner with SpendingInputInfo, if we agree with other suggestions here).
Naming issues IMHO 'unsigned_tx' or 'unsigned' does not look like a good name for var of TransactionInputSigner type, 'tx_signer' seems to be better. In some places we use also 'signer' name, let's use a unified name everywhere.
Same name sign_tx() is used in several cases for utxo signing and that's confusing. F.e. we have sign_tx() in StandardUtxoWithdraw which calls with_key_pair::sign_tx(). Also we have sign_tx() in InitUtxoWithdraw which calls sign_tx() in UtxoSignerOps, which in turn calls with_key_pair::sign_tx() or sign_tx() in TrezorTxSigner. Suggestion: use more distinguished names for lower level functions, like sign_tx_with_policy in UtxoSignerOps, sign_tx_impl in with_key_pair mod.
We have p2sh_spend() function and there is also spend_p2sh() in slp.rs, what does not look good IMHO. Maybe in slp it could be renamed to build_p2sh_spending_tx().