[Splice] Should abort if received `tx_add_input` for the shared input and `prevtx` is not empty

[remyers]

The code below from interactivetx.c enforces the interactive-tx handling rule that if the receiving node sets tlvs->shared_input_txid, then it must match the txid of the shared input. But it does not fail when tlvs->shared_input_txid is not set, but ictx->shared_outpoint is the same as the added shared input and prevtx has been set.

A check must be made when prevtx is set to make sure when adding the shared output, prevtx has not been set. We do this in Eclair here.

			/* For our shared input only, we will fill in prevtx */
			if (ictx->shared_outpoint && tlvs->shared_input_txid) {
				if (!bitcoin_txid_eq(tlvs->shared_input_txid,
						     &ictx->shared_outpoint->txid))
					return tal_fmt(ctx, "funding_txid value"
						       " %s unrecognized."
						       " Should be %s",
						       fmt_bitcoin_txid(ctx, tlvs->shared_input_txid),
						       fmt_bitcoin_txid(ctx, &ictx->shared_outpoint->txid));
				if (!ictx->funding_tx)
					return tal_fmt(ctx, "Internal error"
						       " did not set"
						       " interactivetx"
						       " funding_tx");
				tx = ictx->funding_tx;
			}

I found this missing check while investigating why during interop testing Eclair is returning InvalidSharedInput when clightning initiates the splice. If I can confirm clightning is sending an add_tx_input with prevtx set for the shared input, I'll link a new issue.
cc: @ddustin

[ddustin]

Ah that makes total sense. Great find.

…

On Thu, Jan 23, 2025 at 11:10 AM Richard Myers ***@***.***> wrote: The code below from interactivetx.c <https://github.com/ElementsProject/lightning/blob/305c3779856889bc498fb4f232115997eb0d7fa4/common/interactivetx.c#L483> enforces the interactive-tx handling rule <https://github.com/lightning/bolts/blob/aece0d8f42310d7e7d128905cf18eb0414c02459/02-peer-protocol.md#the-tx_add_input-message-2> that if the receiving node sets tlvs->shared_input_txid, then it must match the txid of the shared input. But it does not fail when tlvs->shared_input_txid is not set, but ictx->shared_outpoint is the same as the added shared input and prevtx has been set. A check must be made when prevtx is set to make sure when adding the shared output, prevtx has not been set. We do this in Eclair here <https://github.com/ACINQ/eclair/blob/3249f2b6077564030411c27d20ac08b30e0e2545/eclair-core/src/main/scala/fr/acinq/eclair/channel/fund/InteractiveTxBuilder.scala#L549> . /* For our shared input only, we will fill in prevtx */ if (ictx->shared_outpoint && tlvs->shared_input_txid) { if (!bitcoin_txid_eq(tlvs->shared_input_txid, &ictx->shared_outpoint->txid)) return tal_fmt(ctx, "funding_txid value" " %s unrecognized." " Should be %s", fmt_bitcoin_txid(ctx, tlvs->shared_input_txid), fmt_bitcoin_txid(ctx, &ictx->shared_outpoint->txid)); if (!ictx->funding_tx) return tal_fmt(ctx, "Internal error" " did not set" " interactivetx" " funding_tx"); tx = ictx->funding_tx; } I found this missing check while investigating why during interop testing Eclair is returning InvalidSharedInput when clightning initiates the splice. If I can confirm clightning is sending an add_tx_input with prevtx set for the shared input, I'll link a new issue. cc: @ddustin <https://github.com/ddustin> — Reply to this email directly, view it on GitHub <#8030>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACQ5F747O6KP6OQ675SZHT2MEH7DAVCNFSM6AAAAABVXWZ5WKVHI2DSMVQWIX3LMV43ASLTON2WKOZSHAYDOMZRHA2DQMY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[ddustin]

Added a PR for the stricter conditions on the peer tx_add_input: #8031

If I can confirm clightning is sending an add_tx_input with prevtx set for the shared input, I'll link a new issue.

There are only two instances where towire_tx_add_input is called for splicing.

One is when the shared_outpoint txid matches what would have been sent.

The code looks like it's sending NULL for prevtx bytes and sends the tlv (with txid in it)

lightning/common/interactivetx.c

Lines 236 to 238 in 305c377

	msg = towire_tx_add_input(NULL, cid, serial_id,
	NULL, in->input.index,
	in->input.sequence, tlvs);

The other code sends prevtx bytes and sends no tlv

lightning/common/interactivetx.c

Lines 240 to 242 in 305c377

	msg = towire_tx_add_input(NULL, cid, serial_id,
	prevtx, in->input.index,
	in->input.sequence, NULL);

In my reading of the code I can't see any way it would ever send both.

[remyers]

Added a PR for the stricter conditions on the peer tx_add_input: #8031

If I can confirm clightning is sending an add_tx_input with prevtx set for the shared input, I'll link a new issue.

There are only two instances where towire_tx_add_input is called for splicing.

One is when the shared_outpoint txid matches what would have been sent.

The code looks like it's sending NULL for prevtx bytes and sends the tlv (with txid in it)

lightning/common/interactivetx.c

Lines 236 to 238 in 305c377
msg = towire_tx_add_input(NULL, cid, serial_id,
NULL, in->input.index,
in->input.sequence, tlvs);

The other code sends prevtx bytes and sends no tlv

lightning/common/interactivetx.c

Lines 240 to 242 in 305c377
msg = towire_tx_add_input(NULL, cid, serial_id,
prevtx, in->input.index,
in->input.sequence, NULL);

In my reading of the code I can't see any way it would ever send both.

It looks like the problem is that ictx->shared_outpoint is null in send_next. I've attached the relevant log section where you can see from the extra logging I added before it checks ictx->shared_output on L230 that itctx->shared_ouput == null.

Where it says "Adding splice input" it's taking the 2nd branch where it adds prevtx but in this case for the shared input because ictx->shared_output was null and so that check was never done.

splice_test_1.log

From what I can see, the splice_initiator_user_update function in channeld.c does not set the shared_outpoint when it should.

These are the commands I used to produce this:

$ export PSBT_SPLICE_INIT=$(bob-clightning-cli splice_init $CHANNEL_ID 100001 | jq -r '.psbt')
$ bob-clightning-cli splice_update $CHANNEL_ID $PSBT_SPLICE_INIT
{
   "code": 362,
   "message": "Peer aborted for reason: ABORT channel dd468a1bd8dedbaabb90f5aff0f8f6ab63cb37c94623773729b2366af7a0ced2: invalid shared tx_add_input (serial_id=d7a14a9d1dc9756a)"
}

If I fix the code so that ictx->shared_outpoint is set, it properly executes the right branch for the shared output, but I need to also change the line that initialized the tlv (L232)

struct tlv_tx_add_input_tlvs *tlvs = tal(tmpctx, struct tlv_tx_add_input_tlvs);

and replace it with:

struct tlv_tx_add_input_tlvs *tlvs = tlv_tx_add_input_tlvs_new(tmpctx);

That get's me farther, but the next line in the script fails:

$ bob-clightning-cli signpsbt -k psbt="$PSBT_SPLICE_UPDATE"
{
   "code": -1,
   "message": "No wallet inputs to sign"
}

I'll hand it back to you to debug that, perhaps my testing script is incorrect? I thought I followed the man page.