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

@@ -462,11 +462,7 @@ impl MaybeReadable for OnchainEventEntry {
 			(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

@@ -113,11 +113,7 @@ impl MaybeReadable for OnchainEventEntry {
 			(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

@@ -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),
@@ -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()
 			},
@@ -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,
 					}))
 				};
@@ -1415,17 +1413,10 @@ impl MaybeReadable for Event {
 						(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_macros.rs

@@ -217,7 +217,11 @@ macro_rules! _check_decoded_tlv_order {
 		// no-op
 	}};
 	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, upgradable_required) => {{
-		// no-op
+		#[allow(unused_comparisons)] // Note that $type may be 0 making the second comparison always false
+		let invalid_order = ($last_seen_type.is_none() || $last_seen_type.unwrap() < $type) && $typ.0 > $type;
+		if invalid_order {
+			return Ok(None);
+		}
 	}};
 	($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, upgradable_option) => {{
 		// no-op