From 20cd856aa5d2834a557663ea67ee0139ad522586 Mon Sep 17 00:00:00 2001
From: Duncan Dean <git@dunxen.dev>
Date: Mon, 1 May 2023 22:52:30 +0200
Subject: [PATCH] Remove `OptionalField` and move `shutdown_scriptpubkey` into
 TLV stream

As pointed out in https://github.com/lightning/bolts/pull/754/commits/6656b70,
we can move the `shutdown_scriptpubkey` field into the TLV streams of
`OpenChannel` and `AcceptChannel` without affecting the resulting encoding.

We use `WithoutLength` encoding here to ensure that we do not encode a
length prefix along with `Script` as is normally the case.
---
 lightning/src/ln/channel.rs        | 14 +++---
 lightning/src/ln/msgs.rs           | 80 ++++--------------------------
 lightning/src/ln/shutdown_tests.rs |  7 ++-
 lightning/src/util/ser.rs          | 17 ++++++-
 lightning/src/util/ser_macros.rs   |  5 +-
 5 files changed, 40 insertions(+), 83 deletions(-)

diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 040129aab..7778b9202 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -25,7 +25,7 @@ use bitcoin::secp256k1;
 use crate::ln::{PaymentPreimage, PaymentHash};
 use crate::ln::features::{ChannelTypeFeatures, InitFeatures};
 use crate::ln::msgs;
-use crate::ln::msgs::{DecodeError, OptionalField};
+use crate::ln::msgs::DecodeError;
 use crate::ln::script::{self, ShutdownScript};
 use crate::ln::channelmanager::{self, CounterpartyForwardingInfo, PendingHTLCStatus, HTLCSource, SentHTLCId, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
 use crate::ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, htlc_success_tx_weight, htlc_timeout_tx_weight, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor, ClosingTransaction};
@@ -1314,7 +1314,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 
 		let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
 			match &msg.shutdown_scriptpubkey {
-				&OptionalField::Present(ref script) => {
+				&Some(ref script) => {
 					// Peer is signaling upfront_shutdown and has opt-out with a 0-length script. We don't enforce anything
 					if script.len() == 0 {
 						None
@@ -1326,7 +1326,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 					}
 				},
 				// Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel
-				&OptionalField::Absent => {
+				&None => {
 					return Err(ChannelError::Close("Peer is signaling upfront_shutdown but we don't get any script. Use 0-length script to opt-out".to_owned()));
 				}
 			}
@@ -2191,7 +2191,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 
 		let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
 			match &msg.shutdown_scriptpubkey {
-				&OptionalField::Present(ref script) => {
+				&Some(ref script) => {
 					// Peer is signaling upfront_shutdown and has opt-out with a 0-length script. We don't enforce anything
 					if script.len() == 0 {
 						None
@@ -2203,7 +2203,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 					}
 				},
 				// Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel
-				&OptionalField::Absent => {
+				&None => {
 					return Err(ChannelError::Close("Peer is signaling upfront_shutdown but we don't get any script. Use 0-length script to opt-out".to_owned()));
 				}
 			}
@@ -5318,7 +5318,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 			htlc_basepoint: keys.htlc_basepoint,
 			first_per_commitment_point,
 			channel_flags: if self.config.announced_channel {1} else {0},
-			shutdown_scriptpubkey: OptionalField::Present(match &self.shutdown_scriptpubkey {
+			shutdown_scriptpubkey: Some(match &self.shutdown_scriptpubkey {
 				Some(script) => script.clone().into_inner(),
 				None => Builder::new().into_script(),
 			}),
@@ -5384,7 +5384,7 @@ impl<Signer: WriteableEcdsaChannelSigner> Channel<Signer> {
 			delayed_payment_basepoint: keys.delayed_payment_basepoint,
 			htlc_basepoint: keys.htlc_basepoint,
 			first_per_commitment_point,
-			shutdown_scriptpubkey: OptionalField::Present(match &self.shutdown_scriptpubkey {
+			shutdown_scriptpubkey: Some(match &self.shutdown_scriptpubkey {
 				Some(script) => script.clone().into_inner(),
 				None => Builder::new().into_script(),
 			}),
diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs
index 779cf1528..df6a2aba3 100644
--- a/lightning/src/ln/msgs.rs
+++ b/lightning/src/ln/msgs.rs
@@ -199,8 +199,8 @@ pub struct OpenChannel {
 	pub first_per_commitment_point: PublicKey,
 	/// The channel flags to be used
 	pub channel_flags: u8,
-	/// Optionally, a request to pre-set the to-sender output's `scriptPubkey` for when we collaboratively close
-	pub shutdown_scriptpubkey: OptionalField<Script>,
+	/// A request to pre-set the to-sender output's `scriptPubkey` for when we collaboratively close
+	pub shutdown_scriptpubkey: Option<Script>,
 	/// The channel type that this channel will represent
 	///
 	/// If this is `None`, we derive the channel type from the intersection of our
@@ -241,8 +241,8 @@ pub struct AcceptChannel {
 	pub htlc_basepoint: PublicKey,
 	/// The first to-be-broadcast-by-sender transaction's per commitment point
 	pub first_per_commitment_point: PublicKey,
-	/// Optionally, a request to pre-set the to-sender output's scriptPubkey for when we collaboratively close
-	pub shutdown_scriptpubkey: OptionalField<Script>,
+	/// A request to pre-set the to-sender output's scriptPubkey for when we collaboratively close
+	pub shutdown_scriptpubkey: Option<Script>,
 	/// The channel type that this channel will represent.
 	///
 	/// If this is `None`, we derive the channel type from the intersection of
@@ -946,20 +946,6 @@ pub struct CommitmentUpdate {
 	pub commitment_signed: CommitmentSigned,
 }
 
-/// Messages could have optional fields to use with extended features
-/// As we wish to serialize these differently from `Option<T>`s (`Options` get a tag byte, but
-/// [`OptionalField`] simply gets `Present` if there are enough bytes to read into it), we have a
-/// separate enum type for them.
-///
-/// This is not exported to bindings users due to a free generic in `T`
-#[derive(Clone, Debug, PartialEq, Eq)]
-pub enum OptionalField<T> {
-	/// Optional field is included in message
-	Present(T),
-	/// Optional field is absent in message
-	Absent
-}
-
 /// A trait to describe an object which can receive channel messages.
 ///
 /// Messages MAY be called in parallel when they originate from different `their_node_ids`, however
@@ -1255,52 +1241,6 @@ impl From<io::Error> for DecodeError {
 	}
 }
 
-impl Writeable for OptionalField<Script> {
-	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		match *self {
-			OptionalField::Present(ref script) => {
-				// Note that Writeable for script includes the 16-bit length tag for us
-				script.write(w)?;
-			},
-			OptionalField::Absent => {}
-		}
-		Ok(())
-	}
-}
-
-impl Readable for OptionalField<Script> {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-		match <u16 as Readable>::read(r) {
-			Ok(len) => {
-				let mut buf = vec![0; len as usize];
-				r.read_exact(&mut buf)?;
-				Ok(OptionalField::Present(Script::from(buf)))
-			},
-			Err(DecodeError::ShortRead) => Ok(OptionalField::Absent),
-			Err(e) => Err(e)
-		}
-	}
-}
-
-impl Writeable for OptionalField<u64> {
-	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		match *self {
-			OptionalField::Present(ref value) => {
-				value.write(w)?;
-			},
-			OptionalField::Absent => {}
-		}
-		Ok(())
-	}
-}
-
-impl Readable for OptionalField<u64> {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-		let value: u64 = Readable::read(r)?;
-		Ok(OptionalField::Present(value))
-	}
-}
-
 #[cfg(not(taproot))]
 impl_writeable_msg!(AcceptChannel, {
 	temporary_channel_id,
@@ -1317,8 +1257,8 @@ impl_writeable_msg!(AcceptChannel, {
 	delayed_payment_basepoint,
 	htlc_basepoint,
 	first_per_commitment_point,
-	shutdown_scriptpubkey
 }, {
+	(0, shutdown_scriptpubkey, (option, encoding: (Script, WithoutLength))), // Don't encode length twice.
 	(1, channel_type, option),
 });
 
@@ -1338,8 +1278,8 @@ impl_writeable_msg!(AcceptChannel, {
 	delayed_payment_basepoint,
 	htlc_basepoint,
 	first_per_commitment_point,
-	shutdown_scriptpubkey
 }, {
+	(0, shutdown_scriptpubkey, (option, encoding: (Script, WithoutLength))), // Don't encode length twice.
 	(1, channel_type, option),
 	(4, next_local_nonce, option),
 });
@@ -1477,8 +1417,8 @@ impl_writeable_msg!(OpenChannel, {
 	htlc_basepoint,
 	first_per_commitment_point,
 	channel_flags,
-	shutdown_scriptpubkey
 }, {
+	(0, shutdown_scriptpubkey, (option, encoding: (Script, WithoutLength))), // Don't encode length twice.
 	(1, channel_type, option),
 });
 
@@ -2103,7 +2043,7 @@ mod tests {
 	use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
 	use crate::ln::features::{ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures};
 	use crate::ln::msgs;
-	use crate::ln::msgs::{FinalOnionHopData, OptionalField, OnionErrorPacket, OnionHopDataFormat};
+	use crate::ln::msgs::{FinalOnionHopData, OnionErrorPacket, OnionHopDataFormat};
 	use crate::routing::gossip::{NodeAlias, NodeId};
 	use crate::util::ser::{Writeable, Readable, Hostname};
 
@@ -2422,7 +2362,7 @@ mod tests {
 			htlc_basepoint: pubkey_5,
 			first_per_commitment_point: pubkey_6,
 			channel_flags: if random_bit { 1 << 5 } else { 0 },
-			shutdown_scriptpubkey: if shutdown { OptionalField::Present(Address::p2pkh(&::bitcoin::PublicKey{compressed: true, inner: pubkey_1}, Network::Testnet).script_pubkey()) } else { OptionalField::Absent },
+			shutdown_scriptpubkey: if shutdown { Some(Address::p2pkh(&::bitcoin::PublicKey{compressed: true, inner: pubkey_1}, Network::Testnet).script_pubkey()) } else { None },
 			channel_type: if incl_chan_type { Some(ChannelTypeFeatures::empty()) } else { None },
 		};
 		let encoded_value = open_channel.encode();
@@ -2478,7 +2418,7 @@ mod tests {
 			delayed_payment_basepoint: pubkey_4,
 			htlc_basepoint: pubkey_5,
 			first_per_commitment_point: pubkey_6,
-			shutdown_scriptpubkey: if shutdown { OptionalField::Present(Address::p2pkh(&::bitcoin::PublicKey{compressed: true, inner: pubkey_1}, Network::Testnet).script_pubkey()) } else { OptionalField::Absent },
+			shutdown_scriptpubkey: if shutdown { Some(Address::p2pkh(&::bitcoin::PublicKey{compressed: true, inner: pubkey_1}, Network::Testnet).script_pubkey()) } else { None },
 			channel_type: None,
 			#[cfg(taproot)]
 			next_local_nonce: None,
diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs
index ca83444a9..554502d66 100644
--- a/lightning/src/ln/shutdown_tests.rs
+++ b/lightning/src/ln/shutdown_tests.rs
@@ -33,7 +33,6 @@ use regex;
 use core::default::Default;
 
 use crate::ln::functional_test_utils::*;
-use crate::ln::msgs::OptionalField::Present;
 
 #[test]
 fn pre_funding_lock_shutdown_test() {
@@ -517,7 +516,7 @@ fn test_unsupported_anysegwit_upfront_shutdown_script() {
 	// Check script when handling an open_channel message
 	nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42, None).unwrap();
 	let mut open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
-	open_channel.shutdown_scriptpubkey = Present(anysegwit_shutdown_script.clone());
+	open_channel.shutdown_scriptpubkey = Some(anysegwit_shutdown_script.clone());
 	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel);
 
 	let events = nodes[1].node.get_and_clear_pending_msg_events();
@@ -542,7 +541,7 @@ fn test_unsupported_anysegwit_upfront_shutdown_script() {
 	let open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
 	nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), &open_channel);
 	let mut accept_channel = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
-	accept_channel.shutdown_scriptpubkey = Present(anysegwit_shutdown_script.clone());
+	accept_channel.shutdown_scriptpubkey = Some(anysegwit_shutdown_script.clone());
 	nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), &accept_channel);
 
 	let events = nodes[0].node.get_and_clear_pending_msg_events();
@@ -568,7 +567,7 @@ fn test_invalid_upfront_shutdown_script() {
 
 	// Use a segwit v0 script with an unsupported witness program
 	let mut open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
-	open_channel.shutdown_scriptpubkey = Present(Builder::new().push_int(0)
+	open_channel.shutdown_scriptpubkey = Some(Builder::new().push_int(0)
 		.push_slice(&[0, 0])
 		.into_script());
 	nodes[0].node.handle_open_channel(&nodes[1].node.get_our_node_id(), &open_channel);
diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs
index 77ee33c4f..e276e7271 100644
--- a/lightning/src/util/ser.rs
+++ b/lightning/src/util/ser.rs
@@ -29,7 +29,7 @@ use bitcoin::secp256k1::constants::{PUBLIC_KEY_SIZE, SECRET_KEY_SIZE, COMPACT_SI
 use bitcoin::secp256k1::ecdsa;
 use bitcoin::secp256k1::schnorr;
 use bitcoin::blockdata::constants::ChainHash;
-use bitcoin::blockdata::script::Script;
+use bitcoin::blockdata::script::{self, Script};
 use bitcoin::blockdata::transaction::{OutPoint, Transaction, TxOut};
 use bitcoin::consensus;
 use bitcoin::consensus::Encodable;
@@ -657,6 +657,21 @@ impl<'a, T> From<&'a Vec<T>> for WithoutLength<&'a Vec<T>> {
 	fn from(v: &'a Vec<T>) -> Self { Self(v) }
 }
 
+impl Writeable for WithoutLength<&Script> {
+	#[inline]
+	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
+		writer.write_all(self.0.as_bytes())
+	}
+}
+
+impl Readable for WithoutLength<Script> {
+	#[inline]
+	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+		let v: WithoutLength<Vec<u8>> = Readable::read(r)?;
+		Ok(WithoutLength(script::Builder::from(v.0).into_script()))
+	}
+}
+
 #[derive(Debug)]
 pub(crate) struct Iterable<'a, I: Iterator<Item = &'a T> + Clone, T: 'a>(pub I);
 
diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index 6067bcdd4..d6a03a88f 100644
--- a/lightning/src/util/ser_macros.rs
+++ b/lightning/src/util/ser_macros.rs
@@ -554,7 +554,7 @@ macro_rules! impl_writeable_msg {
 		impl $crate::util::ser::Writeable for $st {
 			fn write<W: $crate::util::ser::Writer>(&self, w: &mut W) -> Result<(), $crate::io::Error> {
 				$( self.$field.write(w)?; )*
-				$crate::encode_tlv_stream!(w, {$(($type, self.$tlvfield, $fieldty)),*});
+				$crate::encode_tlv_stream!(w, {$(($type, self.$tlvfield.as_ref(), $fieldty)),*});
 				Ok(())
 			}
 		}
@@ -726,6 +726,9 @@ macro_rules! _init_tlv_field_var {
 	($field: ident, optional_vec) => {
 		let mut $field = Some(Vec::new());
 	};
+	($field: ident, (option, encoding: ($fieldty: ty, $encoding: ident))) => {
+		$crate::_init_tlv_field_var!($field, option);
+	};
 	($field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {
 		$crate::_init_tlv_field_var!($field, option);
 	};
-- 
2.39.5