Merge pull request #1519 from tnull/2022-06-require-htlc-max

Make `htlc_maximum_msat` a required field.

Author: TheBlueMatt

.github/workflows/build.yml

@@ -232,14 +232,14 @@ jobs:
           EXPECTED_ROUTING_GRAPH_SNAPSHOT_SHASUM: 05a5361278f68ee2afd086cc04a1f927a63924be451f3221d380533acfacc303
       - name: Fetch rapid graph sync reference input
         run: |
-          curl --verbose -L -o lightning-rapid-gossip-sync/res/full_graph.lngossip https://bitcoin.ninja/ldk-compressed_graph-bc08df7542-2022-05-05.bin
+          curl --verbose -L -o lightning-rapid-gossip-sync/res/full_graph.lngossip https://bitcoin.ninja/ldk-compressed_graph-285cb27df79-2022-07-21.bin
           echo "Sha sum: $(sha256sum lightning-rapid-gossip-sync/res/full_graph.lngossip | awk '{ print $1 }')"
           if [ "$(sha256sum lightning-rapid-gossip-sync/res/full_graph.lngossip | awk '{ print $1 }')" != "${EXPECTED_RAPID_GOSSIP_SHASUM}" ]; then
             echo "Bad hash"
             exit 1
           fi
         env:
-          EXPECTED_RAPID_GOSSIP_SHASUM: 9637b91cea9d64320cf48fc0787c70fe69fc062f90d3512e207044110cadfd7b
+          EXPECTED_RAPID_GOSSIP_SHASUM: e0f5d11641c11896d7af3a2246d3d6c3f1720b7d2d17aab321ecce82e6b7deb8
       - name: Test with Network Graph on Rust ${{ matrix.toolchain }}
         run: |
           cd lightning

lightning-rapid-gossip-sync/src/lib.rs

@@ -240,7 +240,7 @@ mod tests {
 		let sync_result = rapid_sync
 			.sync_network_graph_with_file_path("./res/full_graph.lngossip");
 		if let Err(crate::error::GraphSyncError::DecodeError(DecodeError::Io(io_error))) = &sync_result {
-			let error_string = format!("Input file lightning-rapid-gossip-sync/res/full_graph.lngossip is missing! Download it from https://bitcoin.ninja/ldk-compressed_graph-bc08df7542-2022-05-05.bin\n\n{:?}", io_error);
+			let error_string = format!("Input file lightning-rapid-gossip-sync/res/full_graph.lngossip is missing! Download it from https://bitcoin.ninja/ldk-compressed_graph-285cb27df79-2022-07-21.bin\n\n{:?}", io_error);
 			#[cfg(not(require_route_graph_test))]
 			{
 				println!("{}", error_string);

lightning-rapid-gossip-sync/src/processing.rs

@@ -8,7 +8,7 @@ use bitcoin::BlockHash;
 use bitcoin::secp256k1::PublicKey;
 
 use lightning::ln::msgs::{
-	DecodeError, ErrorAction, LightningError, OptionalField, UnsignedChannelUpdate,
+	DecodeError, ErrorAction, LightningError, UnsignedChannelUpdate,
 };
 use lightning::routing::gossip::NetworkGraph;
 use lightning::util::logger::Logger;
@@ -119,12 +119,7 @@ impl<NG: Deref<Target=NetworkGraph<L>>, L: Deref> RapidGossipSync<NG, L> where L
 		let default_htlc_minimum_msat: u64 = Readable::read(&mut read_cursor)?;
 		let default_fee_base_msat: u32 = Readable::read(&mut read_cursor)?;
 		let default_fee_proportional_millionths: u32 = Readable::read(&mut read_cursor)?;
-		let tentative_default_htlc_maximum_msat: u64 = Readable::read(&mut read_cursor)?;
-		let default_htlc_maximum_msat = if tentative_default_htlc_maximum_msat == u64::max_value() {
-			OptionalField::Absent
-		} else {
-			OptionalField::Present(tentative_default_htlc_maximum_msat)
-		};
+		let default_htlc_maximum_msat: u64 = Readable::read(&mut read_cursor)?;
 
 		for _ in 0..update_count {
 			let scid_delta: BigSize = Readable::read(read_cursor)?;
@@ -147,7 +142,7 @@ impl<NG: Deref<Target=NetworkGraph<L>>, L: Deref> RapidGossipSync<NG, L> where L
 					flags: standard_channel_flags,
 					cltv_expiry_delta: default_cltv_expiry_delta,
 					htlc_minimum_msat: default_htlc_minimum_msat,
-					htlc_maximum_msat: default_htlc_maximum_msat.clone(),
+					htlc_maximum_msat: default_htlc_maximum_msat,
 					fee_base_msat: default_fee_base_msat,
 					fee_proportional_millionths: default_fee_proportional_millionths,
 					excess_data: vec![],
@@ -170,21 +165,14 @@ impl<NG: Deref<Target=NetworkGraph<L>>, L: Deref> RapidGossipSync<NG, L> where L
 						action: ErrorAction::IgnoreError,
 					})?;
 
-				let htlc_maximum_msat =
-					if let Some(htlc_maximum_msat) = directional_info.htlc_maximum_msat {
-						OptionalField::Present(htlc_maximum_msat)
-					} else {
-						OptionalField::Absent
-					};
-
 				UnsignedChannelUpdate {
 					chain_hash,
 					short_channel_id,
 					timestamp: backdated_timestamp,
 					flags: standard_channel_flags,
 					cltv_expiry_delta: directional_info.cltv_expiry_delta,
 					htlc_minimum_msat: directional_info.htlc_minimum_msat,
-					htlc_maximum_msat,
+					htlc_maximum_msat: directional_info.htlc_maximum_msat,
 					fee_base_msat: directional_info.fees.base_msat,
 					fee_proportional_millionths: directional_info.fees.proportional_millionths,
 					excess_data: vec![],
@@ -212,13 +200,8 @@ impl<NG: Deref<Target=NetworkGraph<L>>, L: Deref> RapidGossipSync<NG, L> where L
 			}
 
 			if channel_flags & 0b_0000_0100 > 0 {
-				let tentative_htlc_maximum_msat: u64 = Readable::read(read_cursor)?;
-				synthetic_update.htlc_maximum_msat = if tentative_htlc_maximum_msat == u64::max_value()
-				{
-					OptionalField::Absent
-				} else {
-					OptionalField::Present(tentative_htlc_maximum_msat)
-				};
+				let htlc_maximum_msat: u64 = Readable::read(read_cursor)?;
+				synthetic_update.htlc_maximum_msat = htlc_maximum_msat;
 			}
 
 			network_graph.update_channel_unsigned(&synthetic_update)?;

lightning/src/ln/channel.rs

@@ -6572,7 +6572,7 @@ mod tests {
 	use ln::channel::{Channel, InboundHTLCOutput, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator};
 	use ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS};
 	use ln::features::{InitFeatures, ChannelTypeFeatures};
-	use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate};
+	use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate, MAX_VALUE_MSAT};
 	use ln::script::ShutdownScript;
 	use ln::chan_utils;
 	use ln::chan_utils::{htlc_success_tx_weight, htlc_timeout_tx_weight};
@@ -6988,7 +6988,7 @@ mod tests {
 				flags: 0,
 				cltv_expiry_delta: 100,
 				htlc_minimum_msat: 5,
-				htlc_maximum_msat: OptionalField::Absent,
+				htlc_maximum_msat: MAX_VALUE_MSAT,
 				fee_base_msat: 110,
 				fee_proportional_millionths: 11,
 				excess_data: Vec::new(),

lightning/src/ln/channelmanager.rs

@@ -48,7 +48,7 @@ use routing::router::{PaymentParameters, Route, RouteHop, RoutePath, RouteParame
 use ln::msgs;
 use ln::msgs::NetAddress;
 use ln::onion_utils;
-use ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT, OptionalField};
+use ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, MAX_VALUE_MSAT};
 use ln::wire::Encode;
 use chain::keysinterface::{Sign, KeysInterface, KeysManager, InMemorySigner, Recipient};
 use util::config::{UserConfig, ChannelConfig};
@@ -2397,7 +2397,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			flags: (!were_node_one) as u8 | ((!chan.is_live() as u8) << 1),
 			cltv_expiry_delta: chan.get_cltv_expiry_delta(),
 			htlc_minimum_msat: chan.get_counterparty_htlc_minimum_msat(),
-			htlc_maximum_msat: OptionalField::Present(chan.get_announced_htlc_max_msat()),
+			htlc_maximum_msat: chan.get_announced_htlc_max_msat(),
 			fee_base_msat: chan.get_outbound_forwarding_fee_base_msat(),
 			fee_proportional_millionths: chan.get_fee_proportional_millionths(),
 			excess_data: Vec::new(),

lightning/src/ln/functional_tests.rs

@@ -28,7 +28,7 @@ use routing::gossip::NetworkGraph;
 use routing::router::{PaymentParameters, Route, RouteHop, RouteParameters, find_route, get_route};
 use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures};
 use ln::msgs;
-use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, OptionalField, ErrorAction};
+use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ErrorAction};
 use util::enforcing_trait_impls::EnforcingSigner;
 use util::{byte_utils, test_utils};
 use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose, ClosureReason};
@@ -8312,19 +8312,19 @@ fn test_channel_update_has_correct_htlc_maximum_msat() {
 
 	// Assert that `node[0]`'s `ChannelUpdate` is capped at 50 percent of the `channel_value`, as
 	// that's the value of `node[1]`'s `holder_max_htlc_value_in_flight_msat`.
-	assert_eq!(node_0_chan_update.contents.htlc_maximum_msat, OptionalField::Present(channel_value_50_percent_msat));
+	assert_eq!(node_0_chan_update.contents.htlc_maximum_msat, channel_value_50_percent_msat);
 	// Assert that `node[1]`'s `ChannelUpdate` is capped at 30 percent of the `channel_value`, as
 	// that's the value of `node[0]`'s `holder_max_htlc_value_in_flight_msat`.
-	assert_eq!(node_1_chan_update.contents.htlc_maximum_msat, OptionalField::Present(channel_value_30_percent_msat));
+	assert_eq!(node_1_chan_update.contents.htlc_maximum_msat, channel_value_30_percent_msat);
 
 	// Assert that `node[2]`'s `ChannelUpdate` is capped at 90 percent of the `channel_value`, as
 	// the value of `node[3]`'s `holder_max_htlc_value_in_flight_msat` (100%), exceeds 90% of the
 	// `channel_value`.
-	assert_eq!(node_2_chan_update.contents.htlc_maximum_msat, OptionalField::Present(channel_value_90_percent_msat));
+	assert_eq!(node_2_chan_update.contents.htlc_maximum_msat, channel_value_90_percent_msat);
 	// Assert that `node[3]`'s `ChannelUpdate` is capped at 90 percent of the `channel_value`, as
 	// the value of `node[2]`'s `holder_max_htlc_value_in_flight_msat` (95%), exceeds 90% of the
 	// `channel_value`.
-	assert_eq!(node_3_chan_update.contents.htlc_maximum_msat, OptionalField::Present(channel_value_90_percent_msat));
+	assert_eq!(node_3_chan_update.contents.htlc_maximum_msat, channel_value_90_percent_msat);
 }
 
 #[test]

lightning/src/ln/msgs.rs

@@ -647,8 +647,8 @@ pub struct UnsignedChannelUpdate {
 	pub cltv_expiry_delta: u16,
 	/// The minimum HTLC size incoming to sender, in milli-satoshi
 	pub htlc_minimum_msat: u64,
-	/// Optionally, the maximum HTLC value incoming to sender, in milli-satoshi
-	pub htlc_maximum_msat: OptionalField<u64>,
+	/// The maximum HTLC value incoming to sender, in milli-satoshi. Used to be optional.
+	pub htlc_maximum_msat: u64,
 	/// The base HTLC fee charged by sender, in milli-satoshi
 	pub fee_base_msat: u32,
 	/// The amount to fee multiplier, in micro-satoshi
@@ -1514,14 +1514,12 @@ impl_writeable!(ChannelAnnouncement, {
 
 impl Writeable for UnsignedChannelUpdate {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
-		let mut message_flags: u8 = 0;
-		if let OptionalField::Present(_) = self.htlc_maximum_msat {
-			message_flags = 1;
-		}
+		// `message_flags` used to indicate presence of `htlc_maximum_msat`, but was deprecated in the spec.
+		const MESSAGE_FLAGS: u8 = 1;
 		self.chain_hash.write(w)?;
 		self.short_channel_id.write(w)?;
 		self.timestamp.write(w)?;
-		let all_flags = self.flags as u16 | ((message_flags as u16) << 8);
+		let all_flags = self.flags as u16 | ((MESSAGE_FLAGS as u16) << 8);
 		all_flags.write(w)?;
 		self.cltv_expiry_delta.write(w)?;
 		self.htlc_minimum_msat.write(w)?;
@@ -1535,22 +1533,20 @@ impl Writeable for UnsignedChannelUpdate {
 
 impl Readable for UnsignedChannelUpdate {
 	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-		let has_htlc_maximum_msat;
 		Ok(Self {
 			chain_hash: Readable::read(r)?,
 			short_channel_id: Readable::read(r)?,
 			timestamp: Readable::read(r)?,
 			flags: {
 				let flags: u16 = Readable::read(r)?;
-				let message_flags = flags >> 8;
-				has_htlc_maximum_msat = (message_flags as i32 & 1) == 1;
+				// Note: we ignore the `message_flags` for now, since it was deprecated by the spec.
 				flags as u8
 			},
 			cltv_expiry_delta: Readable::read(r)?,
 			htlc_minimum_msat: Readable::read(r)?,
 			fee_base_msat: Readable::read(r)?,
 			fee_proportional_millionths: Readable::read(r)?,
-			htlc_maximum_msat: if has_htlc_maximum_msat { Readable::read(r)? } else { OptionalField::Absent },
+			htlc_maximum_msat: Readable::read(r)?,
 			excess_data: read_to_end(r)?,
 		})
 	}
@@ -2103,7 +2099,7 @@ mod tests {
 		do_encoding_node_announcement(false, false, true, false, true, false, false, false);
 	}
 
-	fn do_encoding_channel_update(direction: bool, disable: bool, htlc_maximum_msat: bool, excess_data: bool) {
+	fn do_encoding_channel_update(direction: bool, disable: bool, excess_data: bool) {
 		let secp_ctx = Secp256k1::new();
 		let (privkey_1, _) = get_keys_from!("0101010101010101010101010101010101010101010101010101010101010101", secp_ctx);
 		let sig_1 = get_sig_on!(privkey_1, secp_ctx, String::from("01010101010101010101010101010101"));
@@ -2114,7 +2110,7 @@ mod tests {
 			flags: if direction { 1 } else { 0 } | if disable { 1 << 1 } else { 0 },
 			cltv_expiry_delta: 144,
 			htlc_minimum_msat: 1000000,
-			htlc_maximum_msat: if htlc_maximum_msat { OptionalField::Present(131355275467161) } else { OptionalField::Absent },
+			htlc_maximum_msat: 131355275467161,
 			fee_base_msat: 10000,
 			fee_proportional_millionths: 20,
 			excess_data: if excess_data { vec![0, 0, 0, 0, 59, 154, 202, 0] } else { Vec::new() }
@@ -2127,11 +2123,7 @@ mod tests {
 		let mut target_value = hex::decode("d977cb9b53d93a6ff64bb5f1e158b4094b66e798fb12911168a3ccdf80a83096340a6a95da0ae8d9f776528eecdbb747eb6b545495a4319ed5378e35b21e073a").unwrap();
 		target_value.append(&mut hex::decode("000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f").unwrap());
 		target_value.append(&mut hex::decode("00083a840000034d013413a7").unwrap());
-		if htlc_maximum_msat {
-			target_value.append(&mut hex::decode("01").unwrap());
-		} else {
-			target_value.append(&mut hex::decode("00").unwrap());
-		}
+		target_value.append(&mut hex::decode("01").unwrap());
 		target_value.append(&mut hex::decode("00").unwrap());
 		if direction {
 			let flag = target_value.last_mut().unwrap();
@@ -2142,9 +2134,7 @@ mod tests {
 			*flag = *flag | 1 << 1;
 		}
 		target_value.append(&mut hex::decode("009000000000000f42400000271000000014").unwrap());
-		if htlc_maximum_msat {
-			target_value.append(&mut hex::decode("0000777788889999").unwrap());
-		}
+		target_value.append(&mut hex::decode("0000777788889999").unwrap());
 		if excess_data {
 			target_value.append(&mut hex::decode("000000003b9aca00").unwrap());
 		}
@@ -2153,16 +2143,14 @@ mod tests {
 
 	#[test]
 	fn encoding_channel_update() {
-		do_encoding_channel_update(false, false, false, false);
-		do_encoding_channel_update(false, false, false, true);
-		do_encoding_channel_update(true, false, false, false);
-		do_encoding_channel_update(true, false, false, true);
-		do_encoding_channel_update(false, true, false, false);
-		do_encoding_channel_update(false, true, false, true);
-		do_encoding_channel_update(false, false, true, false);
-		do_encoding_channel_update(false, false, true, true);
-		do_encoding_channel_update(true, true, true, false);
-		do_encoding_channel_update(true, true, true, true);
+		do_encoding_channel_update(false, false, false);
+		do_encoding_channel_update(false, false, true);
+		do_encoding_channel_update(true, false, false);
+		do_encoding_channel_update(true, false, true);
+		do_encoding_channel_update(false, true, false);
+		do_encoding_channel_update(false, true, true);
+		do_encoding_channel_update(true, true, false);
+		do_encoding_channel_update(true, true, true);
 	}
 
 	fn do_encoding_open_channel(random_bit: bool, shutdown: bool, incl_chan_type: bool) {

lightning/src/ln/onion_route_tests.rs

@@ -21,7 +21,7 @@ use routing::gossip::{NetworkUpdate, RoutingFees, NodeId};
 use routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop};
 use ln::features::{InitFeatures, InvoiceFeatures, NodeFeatures};
 use ln::msgs;
-use ln::msgs::{ChannelMessageHandler, ChannelUpdate, OptionalField};
+use ln::msgs::{ChannelMessageHandler, ChannelUpdate};
 use ln::wire::Encode;
 use util::events::{Event, MessageSendEvent, MessageSendEventsProvider};
 use util::ser::{ReadableArgs, Writeable, Writer};
@@ -227,7 +227,7 @@ impl msgs::ChannelUpdate {
 				flags: 0,
 				cltv_expiry_delta: 0,
 				htlc_minimum_msat: 0,
-				htlc_maximum_msat: OptionalField::Absent,
+				htlc_maximum_msat: msgs::MAX_VALUE_MSAT,
 				fee_base_msat: 0,
 				fee_proportional_millionths: 0,
 				excess_data: vec![],

lightning/src/ln/priv_short_conf_tests.rs

@@ -19,7 +19,7 @@ use routing::gossip::RoutingFees;
 use routing::router::{PaymentParameters, RouteHint, RouteHintHop};
 use ln::features::{InitFeatures, InvoiceFeatures, ChannelTypeFeatures};
 use ln::msgs;
-use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, OptionalField, ChannelUpdate, ErrorAction};
+use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, ChannelUpdate, ErrorAction};
 use ln::wire::Encode;
 use util::enforcing_trait_impls::EnforcingSigner;
 use util::events::{ClosureReason, Event, MessageSendEvent, MessageSendEventsProvider};
@@ -523,7 +523,7 @@ fn test_scid_alias_returned() {
 		flags: 1,
 		cltv_expiry_delta: accept_forward_cfg.channel_config.cltv_expiry_delta,
 		htlc_minimum_msat: 1_000,
-		htlc_maximum_msat: OptionalField::Present(1_000_000), // Defaults to 10% of the channel value
+		htlc_maximum_msat: 1_000_000, // Defaults to 10% of the channel value
 		fee_base_msat: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().fee_base_msat,
 		fee_proportional_millionths: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().fee_proportional_millionths,
 		excess_data: Vec::new(),