psbt: fix PSBT mutation in the changeset

During the changeset calculation after the `openchannel2_sign`
hook.

So this commit patch 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](#6672).
- Noted a potential optimization where only a subpart of the PSBT
needs to be cloned, as the mutation is specific to inputs.

Link: #6672
Reported-by: @devrandom
Suggested-by: Ken Sedgwick <[email protected]>
Co-Developed-by: Ken Sedgwick <[email protected]>
Signed-off-by: Vincenzo Palazzo <[email protected]>

bitcoin/psbt.c

@@ -36,7 +36,7 @@ struct wally_psbt *create_psbt(const tal_t *ctx, size_t num_inputs, size_t num_o
 	return psbt;
 }
 
-struct wally_psbt *clone_psbt(const tal_t *ctx, struct wally_psbt *psbt)
+struct wally_psbt *clone_psbt(const tal_t *ctx, const struct wally_psbt *psbt)
 {
 	struct wally_psbt *clone;
 	tal_wally_start();

bitcoin/psbt.h

@@ -48,7 +48,7 @@ struct wally_psbt *new_psbt(const tal_t *ctx,
  * @ctx - allocation context
  * @psbt - psbt to be cloned
  */
-struct wally_psbt *clone_psbt(const tal_t *ctx, struct wally_psbt *psbt);
+struct wally_psbt *clone_psbt(const tal_t *ctx, const struct wally_psbt *psbt);
 
 /**
  * psbt_is_finalized - Check if tx is ready to be extracted

bitcoin/test/run-tx-bitcoin_tx_2of2_input_witness_weight.c

@@ -45,7 +45,7 @@ struct amount_asset amount_sat_to_asset(struct amount_sat *sat UNNEEDED, const u
 struct amount_sat amount_tx_fee(u32 fee_per_kw UNNEEDED, size_t weight UNNEEDED)
 { fprintf(stderr, "amount_tx_fee called!\n"); abort(); }
 /* Generated stub for clone_psbt */
-struct wally_psbt *clone_psbt(const tal_t *ctx UNNEEDED, struct wally_psbt *psbt UNNEEDED)
+struct wally_psbt *clone_psbt(const tal_t *ctx UNNEEDED, const struct wally_psbt *psbt UNNEEDED)
 { fprintf(stderr, "clone_psbt called!\n"); abort(); }
 /* Generated stub for fromwire */
 const u8 *fromwire(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, void *copy UNNEEDED, size_t n UNNEEDED)

common/psbt_open.c

@@ -494,20 +494,21 @@ bool psbt_output_to_external(const struct wally_psbt_output *output)
 	return !(!result);
 }
 
+/* FIXME: both PSBT should be const */
 bool psbt_contribs_changed(struct wally_psbt *orig,
 			   struct wally_psbt *new)
 {
-	assert(orig->version == 2 && new->version == 2);
-
 	struct psbt_changeset *cs;
 	bool ok;
+
+	assert(orig->version == 2 && new->version == 2);
+
 	cs = psbt_get_changeset(NULL, orig, new);
 
 	ok = tal_count(cs->added_ins) > 0 ||
 	    tal_count(cs->rm_ins) > 0 ||
 	    tal_count(cs->added_outs) > 0 ||
 	    tal_count(cs->rm_outs) > 0;
-
 	tal_free(cs);
 	return ok;
 }

lightningd/dual_open_control.c

@@ -933,12 +933,19 @@ 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,