Add Shared Input support in interactive TX construction #3842

optout21 · 2025-06-10T15:40:49Z

In interactive TX construction, add support for shared input:

if we expect a shared input, make sure that the remote node provides it
if we provide a shared input, make sure that it's accounted appropriately

Additionally, the prevtx field of the TxAddInput message is changed to Optional, as it should not be set for the shared input (it cannot, as the full funding transaction is not stored on the acceptor side) (spec discussion: lightning/bolts#1160 (comment))

To be used by splicing, see #3736 .

Note: this PR does not include splicing negotiation.

ldk-reviews-bot · 2025-06-10T15:40:52Z

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

wpaulino · 2025-06-11T23:32:15Z

lightning/src/ln/msgs.rs

 	prevtx_out,
 	sequence,
 }, {
-	(0, shared_input_txid, option), // `funding_txid`
+	(0, prevtx, option),
+	(1, shared_input_txid, option), // `funding_txid`


This is actually TLV 0, and there's no TLV for prevtx

Looks like we'll need to unroll the serialization macro. We need to read prevtx_len and only if it's 0, do we need to read the shared_input_txid TLV.

I think I need help with that

Something like this:

diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 76db8e67a..6f43ca8c1 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -29,7 +29,7 @@ use bitcoin::hash_types::Txid; use bitcoin::script::ScriptBuf; use bitcoin::secp256k1::ecdsa::Signature; use bitcoin::secp256k1::PublicKey; -use bitcoin::{secp256k1, Witness}; +use bitcoin::{secp256k1, Transaction, Witness}; use crate::blinded_path::payment::{ BlindedPaymentTlvs, ForwardTlvs, ReceiveTlvs, UnauthenticatedReceiveTlvs, @@ -2668,15 +2668,59 @@ impl_writeable_msg!(SpliceLocked, { splice_txid, }, {}); -impl_writeable_msg!(TxAddInput, { - channel_id, - serial_id, - prevtx_out, - sequence, -}, { - (0, prevtx, option), - (1, shared_input_txid, option), // `funding_txid` -}); +impl Writeable for TxAddInput { + fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> { + self.channel_id.write(w)?; + self.serial_id.write(w)?; + if let Some(prevtx) = self.prevtx.as_ref() { + debug_assert!(self.shared_input_txid.is_none()); + prevtx.write(w)?; + } else { + debug_assert!(self.shared_input_txid.is_some()); + 0u16.write(w)?; + } + self.prevtx_out.write(w)?; + self.sequence.write(w)?; + + if let Some(shared_input_txid) = self.shared_input_txid.as_ref() { + encode_tlv_stream!(w, { (0, shared_input_txid, required) }); + } else { + encode_tlv_stream!(w, {}); + } + + Ok(()) + } +} +impl LengthReadable for TxAddInput { + fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> { + let channel_id = Readable::read(r)?; + let serial_id = Readable::read(r)?; + let prevtx_len = <u16 as Readable>::read(r)?; + let mut prevtx = None; + if prevtx_len > 0 { + let mut tx_reader = FixedLengthReader::new(r, prevtx_len as u64); + let tx: Transaction = Readable::read(&mut tx_reader)?; + if tx_reader.bytes_remain() { + return Err(DecodeError::BadLengthDescriptor); + } + prevtx = + Some(TransactionU16LenLimited::new(tx).map_err(|_| DecodeError::InvalidValue)?); + } + let prevtx_out = Readable::read(r)?; + let sequence = Readable::read(r)?; + + let mut shared_input_txid = None; + if prevtx_len > 0 { + decode_tlv_stream!(r, {}); + } else { + decode_tlv_stream!(r, { + (0, shared_input_txid, required), + }); + } + + Ok(Self { channel_id, serial_id, prevtx, prevtx_out, sequence, shared_input_txid }) + } +} impl_writeable_msg!(TxAddOutput, { channel_id,

Thanks, implemented.

Related to my previous comment, using a custom serialization isn't needed because we can implement serialization for Option<TransactionU16LenLimited> instead of directly on TransactionU16LenLimited.

However, in order to do so we need to address #3842 (comment) to placate the compiler. I've done both in https://github.com/jkczyz/rust-lightning/commits/pr-3842-fix/, which drops the HEAD commit on your branch. Feel free to cherrypick the top two commits and interleave them / squash with your commits accordingly.

I see, I'll do.

wpaulino · 2025-06-11T23:35:30Z

lightning/src/ln/msgs.rs

-	/// malleable.
-	pub prevtx: TransactionU16LenLimited,
+	/// malleable. Omitted for shared input.
+	pub prevtx: Option<TransactionU16LenLimited>,


We could make this an enum along with shared_input_txid, but I'm not sure what's a good name to call it

Probably not a good suggestion, but how about just TxInputOrigin with Txid and PrevTx variants?

I think it's a good idea to keep the message struct field names close to the names in the spec.

Discussed this offline with @wpaulino. While ideally we could use an enum to represent the correct semantics, in practice we don't want to fail parsing if they are incorrect. Otherwise, we'd disconnect rather than simply abandoning the negotiation.

lightning/src/ln/interactivetxs.rs

ldk-reviews-bot · 2025-06-14T00:00:19Z

🔔 1st Reminder

Hey @dunxen @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-14T00:00:21Z

🔔 1st Reminder

Hey @dunxen @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-16T00:01:15Z

🔔 2nd Reminder

Hey @dunxen @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-16T00:01:16Z

🔔 2nd Reminder

Hey @dunxen @jkczyz! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

optout21 · 2025-06-16T12:45:46Z

Great, can you push those changes here then and rework the shared funding input work to follow a similar approach?

I managed to do it: first I refactored the shared output support as discussed, then added shared input support in a similar style. Although this PR does not include spicing, I also checked the changes with splicing (shared input is used there).

lightning/src/ln/interactivetxs.rs

lightning/src/ln/channel.rs

wpaulino · 2025-06-16T17:38:04Z

lightning/src/ln/interactivetxs.rs

+					};
+					let prev_output = TxOut {
+						value: Amount::from_sat(shared_funding_input.1),
+						script_pubkey: txin.script_sig.to_p2wsh(),


The script_sig is initialized to empty right above this. We should just pass in the actual TxOut and track it as part of shared_funding_input.

Right, but... In the shared case, the TxAddInput messages does not contain prev tx or prev txout. In shared_funding_input we track the prev OutPoint only. Are you suggesting to place a TxOut there? But in case of the acceptor, we don't have that to initialize it. I think in this case we just don't have to create a TxOut. Maybe we can make that optional in SharedOwnedInput?

wpaulino · 2025-06-16T17:38:28Z

lightning/src/ln/interactivetxs.rs

@@ -513,8 +534,8 @@ fn is_serial_id_valid_for_counterparty(holder_is_initiator: bool, serial_id: Ser
 impl NegotiationContext {
 	fn new(
 		holder_node_id: PublicKey, counterparty_node_id: PublicKey, holder_is_initiator: bool,
-		shared_funding_output: (TxOut, u64), tx_locktime: AbsoluteLockTime,
-		feerate_sat_per_kw: u32,
+		shared_funding_input: Option<(OutPoint, u64, u64)>, shared_funding_output: (TxOut, u64),


Same comment here about maybe using SharedOwnedInput for shared_funding_input

wpaulino · 2025-06-16T18:04:39Z

lightning/src/ln/interactivetxs.rs

+			if let Some(shared_funding_input) = &self.shared_funding_input {
+				let value = shared_funding_input.1;
+				let local_owned = shared_funding_input.2;
+				// Sanity check
+				if local_owned > value {
+					return Err(AbortReason::InvalidLowFundingInputValue);
+				}
+				let prev_output = TxOut {
+					value: Amount::from_sat(value),
+					script_pubkey: txin.script_sig.to_p2wsh(),
+				};
+				(
+					prev_outpoint,
+					InputOwned::Shared(SharedOwnedInput::new(txin, prev_output, local_owned)),
+				)
+			} else {
+				return Err(AbortReason::MissingFundingInput);
+			}


Wasn't all of this already done when we initialized the InteractiveTxConstructor?

wpaulino · 2025-06-16T18:11:52Z

lightning/src/ln/interactivetxs.rs

+					..Default::default()
+				};
+				let single_input =
+					SingleOwnedInput { input: txin, prev_tx: prevtx.clone(), prev_output };


We shouldn't need to carry around the prev_tx anymore, seems like InteractiveTxInput::input should be of a different type

lightning/src/ln/interactivetxs.rs

wpaulino · 2025-06-16T18:16:44Z

lightning/src/ln/interactivetxs.rs

+		total_input_satoshis = total_input_satoshis.saturating_add(shared_input);
+		if is_initiator {
+			our_funding_inputs_weight =
+				our_funding_inputs_weight.saturating_add(P2WSH_INPUT_WEIGHT_LOWER_BOUND);


This needs to be the weight of the full witness (see FUNDING_TRANSACTION_WITNESS_WEIGHT) plus the base input weight

wpaulino · 2025-06-16T18:20:12Z

lightning/src/ln/interactivetxs.rs

+	// If there is a shared input, account for it,
+	// and for the initiator also consider the fee
+	if let Some(shared_input) = shared_input {
+		total_input_satoshis = total_input_satoshis.saturating_add(shared_input);


Is this supposed to be the full channel value of the shared input, or just the locally owned portion of it?

The locally owned portion, as this calculation is for ensuring that the contribution is enough to cover the (locally owned) outputs and fees.

I have some doubts about the possible rounding errors due to the msat->sat conversion.

optout21 · 2025-06-17T03:49:12Z

@wpaulino thanks for the comments. I will process them (I've re-requested review accidentally, please ignore that).

ldk-reviews-bot · 2025-06-18T00:01:39Z

🔔 3rd Reminder

Hey @dunxen @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-18T00:01:40Z

🔔 3rd Reminder

Hey @dunxen @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-06-19T03:11:00Z

🔔 1st Reminder

Hey @dunxen @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

graphite-app · 2025-06-19T08:13:56Z

lightning/src/ln/msgs.rs

+		if prevtx_len > 0 {
+			decode_tlv_stream!(r, {});
+		} else {
+			decode_tlv_stream!(r, {
+				(0, shared_input_txid, required),
+			});
+		}


There appears to be a logic mismatch between the read and write implementations for TxAddInput.

In the write method, the presence of shared_input_txid determines whether prevtx is written:

if let Some(prevtx) = self.prevtx.as_ref() { debug_assert!(self.shared_input_txid.is_none()); prevtx.write(w)?; } else { debug_assert!(self.shared_input_txid.is_some()); 0u16.write(w)?; }

However, in the read_from_fixed_length_buffer method, the decision to decode the TLV stream with or without shared_input_txid is based on prevtx_len > 0:

if prevtx_len > 0 { decode_tlv_stream!(r, {}); } else { decode_tlv_stream!(r, { (0, shared_input_txid, required), }); }

For consistency, the read logic should match the write logic. Consider checking for the presence of a shared input rather than just the length of prevtx. This would ensure that the TLV stream is decoded correctly in all cases.

Suggested change

if prevtx_len > 0 {

decode_tlv_stream!(r, {});

} else {

decode_tlv_stream!(r, {

(0, shared_input_txid, required),

});

}

if prevtx_len > 0 {

decode_tlv_stream!(r, {});

shared_input_txid = None;

} else {

decode_tlv_stream!(r, {

(0, shared_input_txid, required),

});

}

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

This simplifies tracking separately the expected and actual shared output. In the initiator case, we can just provide the shared output separately, instead of including it within other outputs, and marking which one is the output. We can use the same field for the intended shared output in the initiator case, and the expected one in the acceptor case.

codecov · 2025-06-19T08:28:59Z

Codecov Report

Attention: Patch coverage is 86.65710% with 93 lines in your changes missing coverage. Please review.

Project coverage is 90.92%. Comparing base (30ab411) to head (96236df).
Report is 58 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/interactivetxs.rs	91.99%	41 Missing and 8 partials ⚠️
lightning/src/ln/msgs.rs	40.00%	34 Missing and 8 partials ⚠️
lightning/src/ln/channel.rs	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3842      +/-   ##
==========================================
+ Coverage   89.95%   90.92%   +0.97%     
==========================================
  Files         164      164              
  Lines      131981   142768   +10787     
  Branches   131981   142768   +10787     
==========================================
+ Hits       118718   129812   +11094     
+ Misses      10586    10350     -236     
+ Partials     2677     2606      -71

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

graphite-app · 2025-06-19T08:31:05Z

lightning/src/ln/interactivetxs.rs

+					};
+					let prev_output = TxOut {
+						value: Amount::from_sat(shared_funding_input.1),
+						script_pubkey: txin.script_sig.to_p2wsh(),


There appears to be an issue with the script_pubkey generation for shared inputs. The code is using txin.script_sig.to_p2wsh(), but script_sig is empty by default in a newly created TxIn. This would create an incorrect P2WSH script that doesn't match the actual funding output.

For shared inputs, the script_pubkey should either be derived from the actual funding script or passed as a parameter to ensure it correctly matches the previous output being spent. Consider adding the funding script as a parameter to the shared input constructor or deriving it from other available data.

Suggested change

script_pubkey: txin.script_sig.to_p2wsh(),

script_pubkey: funding_script.to_p2wsh(),

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

lightning/src/ln/interactivetxs.rs

optout21 mentioned this pull request Jun 10, 2025

[Splicing] Tx negotiation during splicing #3736

Open

optout21 force-pushed the shared-input branch from 660dd6b to f63c8a4 Compare June 11, 2025 18:53

optout21 marked this pull request as ready for review June 11, 2025 19:03

optout21 requested review from jkczyz, wpaulino and dunxen June 11, 2025 19:03

wpaulino reviewed Jun 12, 2025

View reviewed changes

optout21 force-pushed the shared-input branch from f63c8a4 to 9151a72 Compare June 16, 2025 12:43

optout21 force-pushed the shared-input branch 5 times, most recently from 166e364 to b2e0da8 Compare June 16, 2025 13:47

wpaulino reviewed Jun 16, 2025

View reviewed changes

optout21 requested a review from wpaulino June 17, 2025 03:10

optout21 force-pushed the shared-input branch from 967dfa3 to 5fb333d Compare June 19, 2025 08:12

graphite-app bot reviewed Jun 19, 2025

View reviewed changes

optout21 force-pushed the shared-input branch from 5fb333d to 151a2ec Compare June 19, 2025 08:19

graphite-app bot reviewed Jun 19, 2025

View reviewed changes

optout21 added 5 commits June 19, 2025 12:52

fix Use SharedOwnedOutput for shared_funding_output

c96b44f

Add Shared Input support

31416e8

Fix tx-add-input serialization

86efd14

fix Minor changes, error enum, comments

702bf59

fix Include vout check as well in OutPoint check for shared input

96236df

optout21 force-pushed the shared-input branch from cafc821 to 96236df Compare June 19, 2025 10:59

	script_pubkey: txin.script_sig.to_p2wsh(),
	script_pubkey: funding_script.to_p2wsh(),

Add Shared Input support in interactive TX construction #3842

Are you sure you want to change the base?

Add Shared Input support in interactive TX construction #3842

Conversation

optout21 commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ldk-reviews-bot commented Jun 14, 2025

Uh oh!

ldk-reviews-bot commented Jun 14, 2025

Uh oh!

ldk-reviews-bot commented Jun 16, 2025

Uh oh!

ldk-reviews-bot commented Jun 16, 2025

Uh oh!

optout21 commented Jun 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optout21 commented Jun 17, 2025

Uh oh!

ldk-reviews-bot commented Jun 18, 2025

Uh oh!

ldk-reviews-bot commented Jun 18, 2025

Uh oh!

ldk-reviews-bot commented Jun 19, 2025

Uh oh!

graphite-app bot Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

optout21 commented Jun 10, 2025 •

edited

Loading

ldk-reviews-bot commented Jun 10, 2025 •

edited

Loading

codecov bot commented Jun 19, 2025 •

edited

Loading