Fix upgradable_required fields to actually be required in lower level macros

When using lower level macros such as read_tlv_stream, upgradable_required
fields have been treated as regular options. This is incorrect, they should
either be upgradable_options or treated as required fields.

Author: valentinewallace

lightning/src/chain/channelmonitor.rs

@@ -49,7 +49,7 @@ use crate::chain::onchaintx::OnchainTxHandler;
 use crate::chain::package::{CounterpartyOfferedHTLCOutput, CounterpartyReceivedHTLCOutput, HolderFundingOutput, HolderHTLCOutput, PackageSolvingData, PackageTemplate, RevokedOutput, RevokedHTLCOutput};
 use crate::chain::Filter;
 use crate::util::logger::Logger;
-use crate::util::ser::{Readable, ReadableArgs, RequiredWrapper, MaybeReadable, Writer, Writeable, U48};
+use crate::util::ser::{Readable, ReadableArgs, RequiredWrapper, MaybeReadable, UpgradableRequired, Writer, Writeable, U48};
 use crate::util::byte_utils;
 use crate::util::events::Event;
 #[cfg(anchors)]
@@ -454,19 +454,15 @@ impl MaybeReadable for OnchainEventEntry {
 		let mut transaction = None;
 		let mut block_hash = None;
 		let mut height = 0;
-		let mut event = None;
+		let mut event = UpgradableRequired(None);
 		read_tlv_fields!(reader, {
 			(0, txid, required),
 			(1, transaction, option),
 			(2, height, required),
 			(3, block_hash, option),
 			(4, event, upgradable_required),
 		});
-		if let Some(ev) = event {
-			Ok(Some(Self { txid, transaction, height, block_hash, event: ev }))
-		} else {
-			Ok(None)
-		}
+		Ok(Some(Self { txid, transaction, height, block_hash, event: _init_tlv_based_struct_field!(event, upgradable_required) }))
 	}
 }
 

lightning/src/chain/onchaintx.rs

@@ -36,7 +36,7 @@ use crate::chain::keysinterface::WriteableEcdsaChannelSigner;
 use crate::chain::package::PackageSolvingData;
 use crate::chain::package::PackageTemplate;
 use crate::util::logger::Logger;
-use crate::util::ser::{Readable, ReadableArgs, MaybeReadable, Writer, Writeable, VecWriter};
+use crate::util::ser::{Readable, ReadableArgs, MaybeReadable, UpgradableRequired, Writer, Writeable, VecWriter};
 
 use crate::io;
 use crate::prelude::*;
@@ -106,18 +106,14 @@ impl MaybeReadable for OnchainEventEntry {
 		let mut txid = Txid::all_zeros();
 		let mut height = 0;
 		let mut block_hash = None;
-		let mut event = None;
+		let mut event = UpgradableRequired(None);
 		read_tlv_fields!(reader, {
 			(0, txid, required),
 			(1, block_hash, option),
 			(2, height, required),
 			(4, event, upgradable_required),
 		});
-		if let Some(ev) = event {
-			Ok(Some(Self { txid, height, block_hash, event: ev }))
-		} else {
-			Ok(None)
-		}
+		Ok(Some(Self { txid, height, block_hash, event: _init_tlv_based_struct_field!(event, upgradable_required) }))
 	}
 }
 

lightning/src/routing/gossip.rs

@@ -883,9 +883,9 @@ impl Readable for ChannelInfo {
 			(0, features, required),
 			(1, announcement_received_time, (default_value, 0)),
 			(2, node_one, required),
-			(4, one_to_two_wrap, upgradable_required),
+			(4, one_to_two_wrap, upgradable_option),
 			(6, node_two, required),
-			(8, two_to_one_wrap, upgradable_required),
+			(8, two_to_one_wrap, upgradable_option),
 			(10, capacity_sats, required),
 			(12, announcement_message, required),
 		});
@@ -1161,7 +1161,7 @@ impl Readable for NodeInfo {
 
 		read_tlv_fields!(reader, {
 			(0, _lowest_inbound_channel_fees, option),
-			(2, announcement_info_wrap, upgradable_required),
+			(2, announcement_info_wrap, upgradable_option),
 			(4, channels, vec_type),
 		});
 

lightning/src/util/events.rs

@@ -23,7 +23,7 @@ use crate::ln::features::ChannelTypeFeatures;
 use crate::ln::msgs;
 use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret};
 use crate::routing::gossip::NetworkUpdate;
-use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, WithoutLength};
+use crate::util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, RequiredWrapper, UpgradableRequired, WithoutLength};
 use crate::routing::router::{RouteHop, RouteParameters};
 
 use bitcoin::{PackedLockTime, Transaction};
@@ -1199,7 +1199,7 @@ impl MaybeReadable for Event {
 					let mut payment_id = None;
 					read_tlv_fields!(reader, {
 						(0, payment_hash, required),
-						(1, network_update, upgradable_required),
+						(1, network_update, upgradable_option),
 						(2, payment_failed_permanently, required),
 						(5, path, vec_type),
 						(7, short_channel_id, option),
@@ -1276,7 +1276,7 @@ impl MaybeReadable for Event {
 			9u8 => {
 				let f = || {
 					let mut channel_id = [0; 32];
-					let mut reason = None;
+					let mut reason = UpgradableRequired(None);
 					let mut user_channel_id_low_opt: Option<u64> = None;
 					let mut user_channel_id_high_opt: Option<u64> = None;
 					read_tlv_fields!(reader, {
@@ -1285,15 +1285,14 @@ impl MaybeReadable for Event {
 						(2, reason, upgradable_required),
 						(3, user_channel_id_high_opt, option),
 					});
-					if reason.is_none() { return Ok(None); }
 
 					// `user_channel_id` used to be a single u64 value. In order to remain
 					// backwards compatible with versions prior to 0.0.113, the u128 is serialized
 					// as two separate u64 values.
 					let user_channel_id = (user_channel_id_low_opt.unwrap_or(0) as u128) +
 						((user_channel_id_high_opt.unwrap_or(0) as u128) << 64);
 
-					Ok(Some(Event::ChannelClosed { channel_id, user_channel_id, reason: reason.unwrap() }))
+					Ok(Some(Event::ChannelClosed { channel_id, user_channel_id, reason: _init_tlv_based_struct_field!(reason, upgradable_required) }))
 				};
 				f()
 			},
@@ -1349,7 +1348,7 @@ impl MaybeReadable for Event {
 			19u8 => {
 				let f = || {
 					let mut payment_hash = PaymentHash([0; 32]);
-					let mut purpose = None;
+					let mut purpose = UpgradableRequired(None);
 					let mut amount_msat = 0;
 					let mut receiver_node_id = None;
 					read_tlv_fields!(reader, {
@@ -1358,11 +1357,10 @@ impl MaybeReadable for Event {
 						(2, purpose, upgradable_required),
 						(4, amount_msat, required),
 					});
-					if purpose.is_none() { return Ok(None); }
 					Ok(Some(Event::PaymentClaimed {
 						receiver_node_id,
 						payment_hash,
-						purpose: purpose.unwrap(),
+						purpose: _init_tlv_based_struct_field!(purpose, upgradable_required),
 						amount_msat,
 					}))
 				};
@@ -1410,22 +1408,15 @@ impl MaybeReadable for Event {
 			25u8 => {
 				let f = || {
 					let mut prev_channel_id = [0; 32];
-					let mut failed_next_destination_opt = None;
+					let mut failed_next_destination_opt = UpgradableRequired(None);
 					read_tlv_fields!(reader, {
 						(0, prev_channel_id, required),
 						(2, failed_next_destination_opt, upgradable_required),
 					});
-					if let Some(failed_next_destination) = failed_next_destination_opt {
-						Ok(Some(Event::HTLCHandlingFailed {
-							prev_channel_id,
-							failed_next_destination,
-						}))
-					} else {
-						// If we fail to read a `failed_next_destination` assume it's because
-						// `MaybeReadable::read` returned `Ok(None)`, though it's also possible we
-						// were simply missing the field.
-						Ok(None)
-					}
+					Ok(Some(Event::HTLCHandlingFailed {
+						prev_channel_id,
+						failed_next_destination: _init_tlv_based_struct_field!(failed_next_destination_opt, upgradable_required),
+					}))
 				};
 				f()
 			},

lightning/src/util/ser.rs

@@ -303,6 +303,18 @@ impl<T: Readable> From<T> for RequiredWrapper<T> {
 	fn from(t: T) -> RequiredWrapper<T> { RequiredWrapper(Some(t)) }
 }
 
+/// Wrapper to read a required (non-optional) TLV record that may have been upgraded without
+/// backwards compat.
+pub struct UpgradableRequired<T: MaybeReadable>(pub Option<T>);
+impl<T: MaybeReadable> MaybeReadable for UpgradableRequired<T> {
+	#[inline]
+	fn read<R: Read>(reader: &mut R) -> Result<Option<Self>, DecodeError> {
+		let tlv = MaybeReadable::read(reader)?;
+		if let Some(tlv) = tlv { return Ok(Some(Self(Some(tlv)))) }
+		Ok(None)
+	}
+}
+
 pub(crate) struct U48(pub u64);
 impl Writeable for U48 {
 	#[inline]

lightning/src/util/ser_macros.rs

@@ -217,7 +217,7 @@ macro_rules! _check_decoded_tlv_order {
 		// no-op
 	}};
 	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, upgradable_required) => {{
-		// no-op
+		_check_decoded_tlv_order!($last_seen_type, $typ, $type, $field, required)
 	}};
 	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, upgradable_option) => {{
 		// no-op
@@ -259,7 +259,7 @@ macro_rules! _check_missing_tlv {
 		// no-op
 	}};
 	($last_seen_type: expr, $type: expr, $field: ident, upgradable_required) => {{
-		// no-op
+		_check_missing_tlv!($last_seen_type, $type, $field, required)
 	}};
 	($last_seen_type: expr, $type: expr, $field: ident, upgradable_option) => {{
 		// no-op
@@ -293,7 +293,10 @@ macro_rules! _decode_tlv {
 		$field = Some($crate::util::ser::Readable::read(&mut $reader)?);
 	}};
 	($reader: expr, $field: ident, upgradable_required) => {{
-		$field = $crate::util::ser::MaybeReadable::read(&mut $reader)?;
+		$field = match $crate::util::ser::MaybeReadable::read(&mut $reader)? {
+			Some(res) => res,
+			_ => return Ok(None)
+		};
 	}};
 	($reader: expr, $field: ident, upgradable_option) => {{
 		$field = $crate::util::ser::MaybeReadable::read(&mut $reader)?;
@@ -636,7 +639,7 @@ macro_rules! _init_tlv_based_struct_field {
 		$field
 	};
 	($field: ident, upgradable_required) => {
-		if $field.is_none() { return Ok(None); } else { $field.unwrap() }
+		$field.0.unwrap()
 	};
 	($field: ident, upgradable_option) => {
 		$field
@@ -671,7 +674,7 @@ macro_rules! _init_tlv_field_var {
 		let mut $field = None;
 	};
 	($field: ident, upgradable_required) => {
-		let mut $field = None;
+		let mut $field = $crate::util::ser::UpgradableRequired(None);
 	};
 	($field: ident, upgradable_option) => {
 		let mut $field = None;
@@ -1050,6 +1053,47 @@ mod tests {
 			(0xdeadbeef1badbeef, 0x1bad1dea, Some(0x01020304)));
 	}
 
+	#[derive(Debug, PartialEq)]
+	struct TestUpgradable {
+		a: u32,
+		b: u32,
+		c: Option<u32>,
+	}
+
+	fn upgradable_tlv_reader(s: &[u8]) -> Result<Option<TestUpgradable>, DecodeError> {
+		let mut s = Cursor::new(s);
+		let mut a = 0;
+		let mut b = 0;
+		let mut c: Option<u32> = None;
+		decode_tlv_stream!(&mut s, {(2, a, upgradable_required), (3, b, upgradable_required), (4, c, upgradable_option)});
+		Ok(Some(TestUpgradable { a, b, c, }))
+	}
+
+	#[test]
+	fn upgradable_tlv_simple_good_cases() {
+		assert_eq!(upgradable_tlv_reader(&::hex::decode(
+			concat!("0204deadbeef", "03041bad1dea", "0404deadbeef")
+		).unwrap()[..]).unwrap(),
+		Some(TestUpgradable { a: 0xdeadbeef, b: 0x1bad1dea, c: Some(0xdeadbeef) }));
+
+		assert_eq!(upgradable_tlv_reader(&::hex::decode(
+			concat!("0204deadbeef", "03041bad1dea")
+		).unwrap()[..]).unwrap(),
+		Some(TestUpgradable { a: 0xdeadbeef, b: 0x1bad1dea, c: None}));
+	}
+
+	#[test]
+	fn missing_required_upgradable() {
+		if let Err(DecodeError::InvalidValue) = upgradable_tlv_reader(&::hex::decode(
+			concat!("0100", "0204deadbeef")
+			).unwrap()[..]) {
+		} else { panic!(); }
+		if let Err(DecodeError::InvalidValue) = upgradable_tlv_reader(&::hex::decode(
+			concat!("0100", "03041bad1dea")
+		).unwrap()[..]) {
+		} else { panic!(); }
+	}
+
 	// BOLT TLV test cases
 	fn tlv_reader_n1(s: &[u8]) -> Result<(Option<HighZeroBytesDroppedBigSize<u64>>, Option<u64>, Option<(PublicKey, u64, u64)>, Option<u16>), DecodeError> {
 		let mut s = Cursor::new(s);