Add Shared Input support in interactive TX construction

[optout21]

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]

👋 Thanks for assigning @dunxen 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]

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

[wpaulino]

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.

[optout21]

I think I need help with that

[wpaulino]

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,

[wpaulino]

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

[dunxen]

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

[wpaulino]

I don't think we need to do any of this, can't we rely on prevtx_outpoints to know if it was already added?

[optout21]

I don't know how can we know whether an OutPoint in prevtx_outpoints is shared or not?

[wpaulino]

I don't think we need to care if it is shared or not, just that it hasn't been added twice

[wpaulino]

Same here, use prevtx_outpoints to see if the input was added

[wpaulino]

I'm not convinced we need this NegotiationInput abstraction, it seems to complicate things a good bit. Can we just keep inputs_to_contribute as we had it, and use expected_shared_funding_input along with is_initiator to know if we have to add it in maybe_send_message? Seems like the same could be done for expected_shared_funding_output, I'm not sure why the OutputOwned abstraction is needed.

[wpaulino]

Hm, I guess we do need it, but I managed to clean up how the shared funding output works and think we could do something similar for the shared funding input.

Here's a diff of the changes I made that can be applied to main (it doesn't fix tests): https://gist.github.com/wpaulino/d8e6282ecd63b250a6cc5820b25c7ca7

[optout21]

Probably this could be simplified, after some considerations your diff looks right, but I need to try it with the tests.

[optout21]

I've hacked the tests to work with your change, and they are generally OK.

[optout21]

In your change, the semantics for the shared_funding_output is:

• if we are the initiator, we want this funding output, and we will add it as an output in the negotiation
• if we are the acceptor, we want this funding output, and we expect the initiator to add it

[optout21]

There are a few cases which will not be possible to express with this change:

• adding multiple shared outputs
• acceptor adding a shared output.

As these are invalid combinations, it is fine.

[wpaulino]

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