Skip to content

psbt: pass in NULL for script if script_len == 0 - #192

Closed
niftynei wants to merge 1 commit into
ElementsProject:masterfrom
niftynei:nifty/fixup_input_parse
Closed

psbt: pass in NULL for script if script_len == 0#192
niftynei wants to merge 1 commit into
ElementsProject:masterfrom
niftynei:nifty/fixup_input_parse

Conversation

@niftynei

Copy link
Copy Markdown
Contributor

wally_tx_output_init_alloc will fail if the script_len == 0 but the
buffer passed in for the script is not NULL; consequently if
script_len is 0 we should pass in a NULL pointer for the script.

@niftynei

niftynei commented May 21, 2020

Copy link
Copy Markdown
Contributor Author

on second thought, maybe a failure here is desirable behaviour? the current c-lightning implementation abuses the witness-utxo field as a placeholder for the input amount, so in some cases the scriptPubkey is set to NULL.

@glslang

glslang commented May 23, 2020

Copy link
Copy Markdown
Contributor

see BYTES_VALID in transaction.c. that's the usual check in wally. an error is preferable.
EDIT: it's possible psbt is missing thischeck in many other places.

@rustyrussell

Copy link
Copy Markdown
Contributor

see BYTES_VALID in transaction.c. that's the usual check in wally. an error is preferable.
EDIT: it's possible psbt is missing thischeck in many other places.

Yes, see #191

`wally_tx_output_init_alloc` will fail if the script_len == 0 but the
buffer passed in for the script is not NULL; consequently if
`script_len` is 0 we should pass in a NULL pointer for the script.
@niftynei
niftynei force-pushed the nifty/fixup_input_parse branch from 10385e2 to 17a18fc Compare May 27, 2020 01:58
@niftynei

Copy link
Copy Markdown
Contributor Author

see BYTES_VALID in transaction.c.

Yes I have seen BYTES_VALID, that's the motivation for this patch. 😆

@rustyrussell

Copy link
Copy Markdown
Contributor

Hmm, in this case, it's invalid to have a zero-length witness script, so the return is correct (note that #191 has this exact same behavior, despite reworking this code).

But that's actually indep of whether script is NULL or not (as it is in this path).

@jgriffiths

Copy link
Copy Markdown
Contributor

Closing this in favour of #210 which includes/supersedes it. Please re-open if something you need got missed in the merge.

@jgriffiths jgriffiths closed this Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants