psbt: fix PSBT mutation in the changeset

[vincenzopalazzo]

During the changeset calculation after the openchannel2_sign hook.

So this commit patches the problem with the following change:

• Addressed an issue where psbt_get_changeset was modifying the original PSBT unnecessarily.
• This modification led to problems with a different hsmd, as referenced in Issue #6672.
• Noted a potential optimization where only a subpart of the PSBT needs to be cloned, as the mutation is specific to inputs.

Fixes: #6672

[niftynei]

Real fix is to fix linearize_inputs so that it doesn't delete data...

[niftynei]

better fix would be to update linearize_input so that it doesn't mutate the inputs; that'll fix it for all calling paths rather than this one.

see
#6672 (comment)

[devrandom]

@niftynei looking at linearize_input on master -

lightning/common/psbt_open.c

Lines 66 to 74 in e1ac241

	psbt->inputs[0] = *in;
	psbt->num_inputs++;
	/* Sort the inputs, so serializing them is ok */
	wally_map_sort(&psbt->inputs[0].unknowns, 0);
	/* signatures, keypaths, etc - we dont care if they change */
	wally_psbt_input_set_final_witness(&psbt->inputs[0], NULL);

I see:

• psbt->inputs[0] = *in; - that makes a copy of the input struct bytes, copying any pointers without copying underlying memory
• wally_psbt_input_set_final_witness(&psbt->inputs[0], NULL); - this uses the SET_STRUCT macro, which in turn frees the memory originally pointed to. as far as I understand, this results in a dangling pointer in the original input struct.
• there's also the /* Hide the inputs we added comment below, but it's too late by that time

so I'm actually not sure why the code in master doesn't just crash.

but in any case, a potential fix to this and the unwanted mutation may be to do psbt->inputs[0].final_witness = NULL, so the original memory is not touched. and the same for the other four calls of the form wally_psbt_input_set_*()

[vincenzopalazzo]

Real fix is to fix linearize_inputs so that it doesn't delete data...

Yes, I put this in my FIXME comment, because I think the only way is cloning psbt inputs that get sorted in linearize_inputs (https://github.com/ElementsProject/lightning/blob/master/common/psbt_open.c#L66) but this required to change libwally

I am missing somethings? @niftynei

[ksedgwic]

I think the following change (instead) minimizes the restructuring risk by isolating the workaround to a single case:

@@ -933,18 +933,25 @@ openchannel2_signed_deserialize(struct openchannel2_psbt_payload *payload,
 		fatal("Plugin supplied PSBT that's missing required fields. %s",
 		      type_to_string(tmpctx, struct wally_psbt, psbt));
 
+	/* NOTE - The psbt_contribs_changed function nulls lots of
+	 * fields in place to compare the PSBTs. This removes the
+	 * witness stack held in final_witness.  Give it a clone of
+	 * the PSBT to hack on instead ... */
+	struct wally_psbt *psbt_clone;
+	psbt_clone = clone_psbt(tmpctx, psbt);
+
 	/* Verify that inputs/outputs are the same. Note that this is a
 	 * 'de minimus' check -- we just look at serial_ids. If you've
 	 * totally managled the data here but left the serial_ids intact,
 	 * you'll get a failure back from the peer when you send
 	 * commitment sigs */
-	if (psbt_contribs_changed(payload->psbt, psbt))
+	if (psbt_contribs_changed(payload->psbt, psbt_clone))
 		fatal("Plugin must not change psbt input/output set. "
 		      "orig: %s. updated: %s",
 		      type_to_string(tmpctx, struct wally_psbt,
 			      	     payload->psbt),
 		      type_to_string(tmpctx, struct wally_psbt,
-			      	     psbt));
+			      	     psbt_clone));
 
 	if (payload->psbt)
 		tal_free(payload->psbt);

[ksedgwic]

I think both arguments to psbt_contribs_changed should be const. They are linearized internally to facilitate normalized comparison, but I don't think a caller would expect either argument to be modified.

Specifically, VLS is having trouble with the 2nd argument being modified, not the first so this fix won't work.

[vincenzopalazzo]

I put this as FIXME because we should change some logic down.

An easy solution is to clone the input and output while we are sorting, without cloning all the PSBT

[vincenzopalazzo]

I applied the @ksedgwic patch because it is the easiest one and makes the VLS tests happy again.

Happy to revisit it, or move some future work as the one that @niftynei suggested as a followup PR

[rustyrussell]

Agree we should fix linearize, but this at least fixes the immediate problem. And we don't have clone functions for inputs, so caller needs to do this for now :(

[vincenzopalazzo]

I will work on libwally for this but I think it takes some time