-
Notifications
You must be signed in to change notification settings - Fork 155
Fixup borked PSBT signing + finalize #203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2542,7 +2542,7 @@ int wally_sign_psbt( | |
| struct wally_tx_input *txin = &psbt->tx->inputs[i]; | ||
| unsigned char sighash[SHA256_LEN], *scriptcode, wpkh_sc[WALLY_SCRIPTPUBKEY_P2PKH_LEN]; | ||
| size_t scriptcode_len; | ||
| bool match = false, comp = false; | ||
| bool match = false, comp = false, already_signed = false; | ||
| uint32_t sighash_type = WALLY_SIGHASH_ALL; | ||
|
|
||
| if (!input->keypaths) { | ||
|
|
@@ -2568,6 +2568,23 @@ int wally_sign_psbt( | |
| continue; | ||
| } | ||
|
|
||
| /* Make sure we don't already have a sig for this input ?! */ | ||
| if (input->partial_sigs) { | ||
| for (j = 0; j < input->partial_sigs->num_items; j++) { | ||
| struct wally_partial_sigs_item *item = &input->partial_sigs->items[j]; | ||
| if (memcmp((char *)item->pubkey, (char *)uncomp_pubkey, EC_PUBLIC_KEY_UNCOMPRESSED_LEN) == 0 | ||
| || memcmp((char *)item->pubkey, (char *)pubkey, EC_PUBLIC_KEY_LEN) == 0) { | ||
| already_signed = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /* We've already got a partial sig for this pubkey on this input */ | ||
| if (already_signed) { | ||
| continue; | ||
| } | ||
|
|
||
| /* Sighash type */ | ||
| if (input->sighash_type > 0) { | ||
| sighash_type = input->sighash_type; | ||
|
|
@@ -2697,7 +2714,9 @@ int wally_finalize_psbt(struct wally_psbt *psbt) | |
| for (i = 0; i < psbt->num_inputs; ++i) { | ||
| struct wally_psbt_input *input = &psbt->inputs[i]; | ||
| struct wally_tx_input *txin = &psbt->tx->inputs[i]; | ||
| unsigned char *out_script = NULL; /* Script that determines how we should finalize this input, typically output script */ | ||
| /* Script for this input. originally set to the input's scriptPubKey, but in the case of a p2sh/p2wsh | ||
| * input, it will be eventually be set to the unhashed script, if known */ | ||
| unsigned char *out_script = NULL; | ||
| size_t out_script_len, type; | ||
| bool witness = false, p2sh = false;; | ||
|
|
||
|
|
@@ -2706,15 +2725,22 @@ int wally_finalize_psbt(struct wally_psbt *psbt) | |
| continue; | ||
| } | ||
|
|
||
| if (input->redeem_script) { | ||
| out_script = input->redeem_script; | ||
| out_script_len = input->redeem_script_len; | ||
| p2sh = true; | ||
| /* Note that if we patch libwally to supply the non-witness utxo tx field (tx) for | ||
| * witness inputs also, we'll need a different way to signal p2sh-p2wpkh scripts */ | ||
| if (input->witness_utxo && input->witness_utxo->script_len > 0) { | ||
| out_script = input->witness_utxo->script; | ||
| out_script_len = input->witness_utxo->script_len; | ||
| witness = true; | ||
| } else if (input->non_witness_utxo && input->non_witness_utxo->num_outputs > txin->index) { | ||
| struct wally_tx_output out = input->non_witness_utxo->outputs[txin->index]; | ||
| out_script = out.script; | ||
| out_script_len = out.script_len; | ||
| } | ||
| if (input->redeem_script) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This if block can be swapped with the one above. Then the |
||
| out_script = input->redeem_script; | ||
| out_script_len = input->redeem_script_len; | ||
| p2sh = true; | ||
| } | ||
| if (input->witness_script) { | ||
| out_script = input->witness_script; | ||
| out_script_len = input->witness_script_len; | ||
|
|
@@ -2735,7 +2761,7 @@ int wally_finalize_psbt(struct wally_psbt *psbt) | |
| case WALLY_SCRIPT_TYPE_P2WPKH: { | ||
| struct wally_partial_sigs_item *partial_sig; | ||
| unsigned char script_sig[WALLY_SCRIPTSIG_P2PKH_MAX_LEN]; | ||
| size_t script_sig_len, pubkey_len = EC_PUBLIC_KEY_UNCOMPRESSED_LEN; | ||
| size_t written, script_sig_len, pubkey_len = EC_PUBLIC_KEY_UNCOMPRESSED_LEN; | ||
|
|
||
| if (!input->partial_sigs || input->partial_sigs->num_items != 1) { | ||
| /* Must be single key, single sig */ | ||
|
|
@@ -2757,6 +2783,16 @@ int wally_finalize_psbt(struct wally_psbt *psbt) | |
| if ((ret = wally_witness_p2wpkh_from_der(partial_sig->pubkey, pubkey_len, partial_sig->sig, partial_sig->sig_len, &input->final_witness)) != WALLY_OK) { | ||
| return ret; | ||
| } | ||
| if (input->redeem_script) { | ||
| /* P2SH wrapped witness requires final scriptsig of pushing the redeemScript */ | ||
| script_sig_len = varint_get_length(input->redeem_script_len) + input->redeem_script_len; | ||
| input->final_script_sig = wally_malloc(script_sig_len); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May be worth explicitly checking the value returned by wally_malloc for null and returning WALLY_ENOMEM
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is copied from https://github.com/ElementsProject/libwally-core/blob/master/src/psbt.c#L2860-L2868, which will also need to be cleaned up
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the interest of getting my PRs in c-lightning unblocked, we should merge this PR and clean up the malloc problems in a second fixup
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, i went through and updated all |
||
| if ((ret = wally_script_push_from_bytes(input->redeem_script, input->redeem_script_len, 0, input->final_script_sig, script_sig_len, &written)) != WALLY_OK) { | ||
| wally_free(input->final_script_sig); | ||
| return ret; | ||
| } | ||
| input->final_script_sig_len = written; | ||
| } | ||
| } | ||
| break; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
turns out
uncrustifywants it this way, so reverted this suggested change.