Merge pull request #1790 from tnull/2022-10-inbound-user-channel-id-randomization

Randomize `user_channel_id` for inbound channels

Author: TheBlueMatt

fuzz/src/full_stack.rs

@@ -403,7 +403,7 @@ pub fn do_test(data: &[u8], logger: &Arc<dyn Logger>) {
 	// Adding new calls to `KeysInterface::get_secure_random_bytes` during startup can change all the
 	// keys subsequently generated in this test. Rather than regenerating all the messages manually,
 	// it's easier to just increment the counter here so the keys don't change.
-	keys_manager.counter.fetch_sub(2, Ordering::AcqRel);
+	keys_manager.counter.fetch_sub(3, Ordering::AcqRel);
 	let our_id = PublicKey::from_secret_key(&Secp256k1::signing_only(), &keys_manager.get_node_secret(Recipient::Node).unwrap());
 	let network_graph = Arc::new(NetworkGraph::new(genesis_block(network).block_hash(), Arc::clone(&logger)));
 	let gossip_sync = Arc::new(P2PGossipSync::new(Arc::clone(&network_graph), None, Arc::clone(&logger)));

lightning/src/ln/channel.rs

@@ -509,7 +509,7 @@ pub(super) struct Channel<Signer: Sign> {
 
 	inbound_handshake_limits_override: Option<ChannelHandshakeLimits>,
 
-	user_id: u64,
+	user_id: u128,
 
 	channel_id: [u8; 32],
 	channel_state: u32,
@@ -902,7 +902,7 @@ impl<Signer: Sign> Channel<Signer> {
 	// Constructors:
 	pub fn new_outbound<K: Deref, F: Deref>(
 		fee_estimator: &LowerBoundedFeeEstimator<F>, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures,
-		channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig, current_chain_height: u32,
+		channel_value_satoshis: u64, push_msat: u64, user_id: u128, config: &UserConfig, current_chain_height: u32,
 		outbound_scid_alias: u64
 	) -> Result<Channel<Signer>, APIError>
 	where K::Target: KeysInterface<Signer = Signer>,
@@ -1102,7 +1102,7 @@ impl<Signer: Sign> Channel<Signer> {
 	/// Assumes chain_hash has already been checked and corresponds with what we expect!
 	pub fn new_from_req<K: Deref, F: Deref, L: Deref>(
 		fee_estimator: &LowerBoundedFeeEstimator<F>, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures,
-		msg: &msgs::OpenChannel, user_id: u64, config: &UserConfig, current_chain_height: u32, logger: &L,
+		msg: &msgs::OpenChannel, user_id: u128, config: &UserConfig, current_chain_height: u32, logger: &L,
 		outbound_scid_alias: u64
 	) -> Result<Channel<Signer>, ChannelError>
 		where K::Target: KeysInterface<Signer = Signer>,
@@ -4482,7 +4482,7 @@ impl<Signer: Sign> Channel<Signer> {
 
 	/// Gets the "user_id" value passed into the construction of this channel. It has no special
 	/// meaning and exists only to allow users to have a persistent identifier of a channel.
-	pub fn get_user_id(&self) -> u64 {
+	pub fn get_user_id(&self) -> u128 {
 		self.user_id
 	}
 
@@ -5178,7 +5178,7 @@ impl<Signer: Sign> Channel<Signer> {
 	/// should be sent back to the counterparty node.
 	///
 	/// [`msgs::AcceptChannel`]: crate::ln::msgs::AcceptChannel
-	pub fn accept_inbound_channel(&mut self, user_id: u64) -> msgs::AcceptChannel {
+	pub fn accept_inbound_channel(&mut self, user_id: u128) -> msgs::AcceptChannel {
 		if self.is_outbound() {
 			panic!("Tried to send accept_channel for an outbound channel?");
 		}
@@ -6007,7 +6007,11 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 
 		write_ver_prefix!(writer, SERIALIZATION_VERSION, MIN_SERIALIZATION_VERSION);
 
-		self.user_id.write(writer)?;
+		// `user_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. We write
+		// the low bytes now and the optional high bytes later.
+		let user_id_low = self.user_id as u64;
+		user_id_low.write(writer)?;
 
 		// Version 1 deserializers expected to read parts of the config object here. Version 2
 		// deserializers (0.0.99) now read config through TLVs, and as we now require them for
@@ -6254,6 +6258,11 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 
 		let channel_ready_event_emitted = Some(self.channel_ready_event_emitted);
 
+		// `user_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. Therefore,
+		// we write the high bytes as an option here.
+		let user_id_high_opt = Some((self.user_id >> 64) as u64);
+
 		write_tlv_fields!(writer, {
 			(0, self.announcement_sigs, option),
 			// minimum_depth and counterparty_selected_channel_reserve_satoshis used to have a
@@ -6277,6 +6286,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
 			(19, self.latest_inbound_scid_alias, option),
 			(21, self.outbound_scid_alias, required),
 			(23, channel_ready_event_emitted, option),
+			(25, user_id_high_opt, option),
 		});
 
 		Ok(())
@@ -6290,7 +6300,10 @@ impl<'a, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<<K::Target as KeysInte
 		let (keys_source, serialized_height) = args;
 		let ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
 
-		let user_id = Readable::read(reader)?;
+		// `user_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. We read
+		// the low bytes now and the high bytes later.
+		let user_id_low: u64 = Readable::read(reader)?;
 
 		let mut config = Some(LegacyChannelConfig::default());
 		if ver == 1 {
@@ -6536,6 +6549,8 @@ impl<'a, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<<K::Target as KeysInte
 		let mut outbound_scid_alias = None;
 		let mut channel_ready_event_emitted = None;
 
+		let mut user_id_high_opt: Option<u64> = None;
+
 		read_tlv_fields!(reader, {
 			(0, announcement_sigs, option),
 			(1, minimum_depth, option),
@@ -6553,6 +6568,7 @@ impl<'a, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<<K::Target as KeysInte
 			(19, latest_inbound_scid_alias, option),
 			(21, outbound_scid_alias, option),
 			(23, channel_ready_event_emitted, option),
+			(25, user_id_high_opt, option),
 		});
 
 		if let Some(preimages) = preimages_opt {
@@ -6589,6 +6605,11 @@ impl<'a, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<<K::Target as KeysInte
 		let mut secp_ctx = Secp256k1::new();
 		secp_ctx.seeded_randomize(&keys_source.get_secure_random_bytes());
 
+		// `user_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_id = user_id_low as u128 + ((user_id_high_opt.unwrap_or(0) as u128) << 64);
+
 		Ok(Channel {
 			user_id,
 

lightning/src/ln/channelmanager.rs

@@ -292,7 +292,7 @@ type ShutdownResult = (Option<(OutPoint, ChannelMonitorUpdate)>, Vec<(HTLCSource
 
 struct MsgHandleErrInternal {
 	err: msgs::LightningError,
-	chan_id: Option<([u8; 32], u64)>, // If Some a channel of ours has been closed
+	chan_id: Option<([u8; 32], u128)>, // If Some a channel of ours has been closed
 	shutdown_finish: Option<(ShutdownResult, Option<msgs::ChannelUpdate>)>,
 }
 impl MsgHandleErrInternal {
@@ -328,7 +328,7 @@ impl MsgHandleErrInternal {
 		Self { err, chan_id: None, shutdown_finish: None }
 	}
 	#[inline]
-	fn from_finish_shutdown(err: String, channel_id: [u8; 32], user_channel_id: u64, shutdown_res: ShutdownResult, channel_update: Option<msgs::ChannelUpdate>) -> Self {
+	fn from_finish_shutdown(err: String, channel_id: [u8; 32], user_channel_id: u128, shutdown_res: ShutdownResult, channel_update: Option<msgs::ChannelUpdate>) -> Self {
 		Self {
 			err: LightningError {
 				err: err.clone(),
@@ -1086,8 +1086,9 @@ pub struct ChannelDetails {
 	///
 	/// [`outbound_capacity_msat`]: ChannelDetails::outbound_capacity_msat
 	pub unspendable_punishment_reserve: Option<u64>,
-	/// The `user_channel_id` passed in to create_channel, or 0 if the channel was inbound.
-	pub user_channel_id: u64,
+	/// The `user_channel_id` passed in to create_channel, or a random value if the channel was
+	/// inbound.
+	pub user_channel_id: u128,
 	/// Our total balance.  This is the amount we would get if we close the channel.
 	/// This value is not exact. Due to various in-flight changes and feerate changes, exactly this
 	/// amount is not likely to be recoverable on close.
@@ -1722,10 +1723,9 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	///
 	/// `user_channel_id` will be provided back as in
 	/// [`Event::FundingGenerationReady::user_channel_id`] to allow tracking of which events
-	/// correspond with which `create_channel` call. Note that the `user_channel_id` defaults to 0
-	/// for inbound channels, so you may wish to avoid using 0 for `user_channel_id` here.
-	/// `user_channel_id` has no meaning inside of LDK, it is simply copied to events and otherwise
-	/// ignored.
+	/// correspond with which `create_channel` call. Note that the `user_channel_id` defaults to a
+	/// randomized value for inbound channels. `user_channel_id` has no meaning inside of LDK, it
+	/// is simply copied to events and otherwise ignored.
 	///
 	/// Raises [`APIError::APIMisuseError`] when `channel_value_satoshis` > 2**24 or `push_msat` is
 	/// greater than `channel_value_satoshis * 1k` or `channel_value_satoshis < 1000`.
@@ -1744,7 +1744,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	/// [`Event::FundingGenerationReady::user_channel_id`]: events::Event::FundingGenerationReady::user_channel_id
 	/// [`Event::FundingGenerationReady::temporary_channel_id`]: events::Event::FundingGenerationReady::temporary_channel_id
 	/// [`Event::ChannelClosed::channel_id`]: events::Event::ChannelClosed::channel_id
-	pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_channel_id: u64, override_config: Option<UserConfig>) -> Result<[u8; 32], APIError> {
+	pub fn create_channel(&self, their_network_key: PublicKey, channel_value_satoshis: u64, push_msat: u64, user_channel_id: u128, override_config: Option<UserConfig>) -> Result<[u8; 32], APIError> {
 		if channel_value_satoshis < 1000 {
 			return Err(APIError::APIMisuseError { err: format!("Channel value must be at least 1000 satoshis. It was {}", channel_value_satoshis) });
 		}
@@ -4536,7 +4536,7 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	///
 	/// [`Event::OpenChannelRequest`]: events::Event::OpenChannelRequest
 	/// [`Event::ChannelClosed::user_channel_id`]: events::Event::ChannelClosed::user_channel_id
-	pub fn accept_inbound_channel(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, user_channel_id: u64) -> Result<(), APIError> {
+	pub fn accept_inbound_channel(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, user_channel_id: u128) -> Result<(), APIError> {
 		self.do_accept_inbound_channel(temporary_channel_id, counterparty_node_id, false, user_channel_id)
 	}
 
@@ -4558,11 +4558,11 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 	///
 	/// [`Event::OpenChannelRequest`]: events::Event::OpenChannelRequest
 	/// [`Event::ChannelClosed::user_channel_id`]: events::Event::ChannelClosed::user_channel_id
-	pub fn accept_inbound_channel_from_trusted_peer_0conf(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, user_channel_id: u64) -> Result<(), APIError> {
+	pub fn accept_inbound_channel_from_trusted_peer_0conf(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, user_channel_id: u128) -> Result<(), APIError> {
 		self.do_accept_inbound_channel(temporary_channel_id, counterparty_node_id, true, user_channel_id)
 	}
 
-	fn do_accept_inbound_channel(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, accept_0conf: bool, user_channel_id: u64) -> Result<(), APIError> {
+	fn do_accept_inbound_channel(&self, temporary_channel_id: &[u8; 32], counterparty_node_id: &PublicKey, accept_0conf: bool, user_channel_id: u128) -> Result<(), APIError> {
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
 
 		let mut channel_state_lock = self.channel_state.lock().unwrap();
@@ -4610,9 +4610,13 @@ impl<M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelManager<M, T, K, F
 			return Err(MsgHandleErrInternal::send_err_msg_no_close("No inbound channels accepted".to_owned(), msg.temporary_channel_id.clone()));
 		}
 
+		let mut random_bytes = [0u8; 16];
+		random_bytes.copy_from_slice(&self.keys_manager.get_secure_random_bytes()[..16]);
+		let user_channel_id = u128::from_be_bytes(random_bytes);
+
 		let outbound_scid_alias = self.create_and_insert_outbound_scid_alias();
 		let mut channel = match Channel::new_from_req(&self.fee_estimator, &self.keys_manager,
-			counterparty_node_id.clone(), &their_features, msg, 0, &self.default_configuration,
+			counterparty_node_id.clone(), &their_features, msg, user_channel_id, &self.default_configuration,
 			self.best_block.read().unwrap().height(), &self.logger, outbound_scid_alias)
 		{
 			Err(e) => {
@@ -6450,33 +6454,108 @@ impl_writeable_tlv_based!(ChannelCounterparty, {
 	(11, outbound_htlc_maximum_msat, option),
 });
 
-impl_writeable_tlv_based!(ChannelDetails, {
-	(1, inbound_scid_alias, option),
-	(2, channel_id, required),
-	(3, channel_type, option),
-	(4, counterparty, required),
-	(5, outbound_scid_alias, option),
-	(6, funding_txo, option),
-	(7, config, option),
-	(8, short_channel_id, option),
-	(10, channel_value_satoshis, required),
-	(12, unspendable_punishment_reserve, option),
-	(14, user_channel_id, required),
-	(16, balance_msat, required),
-	(18, outbound_capacity_msat, required),
-	// Note that by the time we get past the required read above, outbound_capacity_msat will be
-	// filled in, so we can safely unwrap it here.
-	(19, next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap() as u64)),
-	(20, inbound_capacity_msat, required),
-	(22, confirmations_required, option),
-	(24, force_close_spend_delay, option),
-	(26, is_outbound, required),
-	(28, is_channel_ready, required),
-	(30, is_usable, required),
-	(32, is_public, required),
-	(33, inbound_htlc_minimum_msat, option),
-	(35, inbound_htlc_maximum_msat, option),
-});
+impl Writeable for ChannelDetails {
+	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
+		// `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_low = self.user_channel_id as u64;
+		let user_channel_id_high_opt = Some((self.user_channel_id >> 64) as u64);
+		write_tlv_fields!(writer, {
+			(1, self.inbound_scid_alias, option),
+			(2, self.channel_id, required),
+			(3, self.channel_type, option),
+			(4, self.counterparty, required),
+			(5, self.outbound_scid_alias, option),
+			(6, self.funding_txo, option),
+			(7, self.config, option),
+			(8, self.short_channel_id, option),
+			(10, self.channel_value_satoshis, required),
+			(12, self.unspendable_punishment_reserve, option),
+			(14, user_channel_id_low, required),
+			(16, self.balance_msat, required),
+			(18, self.outbound_capacity_msat, required),
+			// Note that by the time we get past the required read above, outbound_capacity_msat will be
+			// filled in, so we can safely unwrap it here.
+			(19, self.next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap() as u64)),
+			(20, self.inbound_capacity_msat, required),
+			(22, self.confirmations_required, option),
+			(24, self.force_close_spend_delay, option),
+			(26, self.is_outbound, required),
+			(28, self.is_channel_ready, required),
+			(30, self.is_usable, required),
+			(32, self.is_public, required),
+			(33, self.inbound_htlc_minimum_msat, option),
+			(35, self.inbound_htlc_maximum_msat, option),
+			(37, user_channel_id_high_opt, option),
+		});
+		Ok(())
+	}
+}
+
+impl Readable for ChannelDetails {
+	fn read<R: Read>(reader: &mut R) -> Result<Self, DecodeError> {
+		init_and_read_tlv_fields!(reader, {
+			(1, inbound_scid_alias, option),
+			(2, channel_id, required),
+			(3, channel_type, option),
+			(4, counterparty, required),
+			(5, outbound_scid_alias, option),
+			(6, funding_txo, option),
+			(7, config, option),
+			(8, short_channel_id, option),
+			(10, channel_value_satoshis, required),
+			(12, unspendable_punishment_reserve, option),
+			(14, user_channel_id_low, required),
+			(16, balance_msat, required),
+			(18, outbound_capacity_msat, required),
+			// Note that by the time we get past the required read above, outbound_capacity_msat will be
+			// filled in, so we can safely unwrap it here.
+			(19, next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap() as u64)),
+			(20, inbound_capacity_msat, required),
+			(22, confirmations_required, option),
+			(24, force_close_spend_delay, option),
+			(26, is_outbound, required),
+			(28, is_channel_ready, required),
+			(30, is_usable, required),
+			(32, is_public, required),
+			(33, inbound_htlc_minimum_msat, option),
+			(35, inbound_htlc_maximum_msat, option),
+			(37, user_channel_id_high_opt, option),
+		});
+
+		// `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_low: u64 = user_channel_id_low.0.unwrap();
+		let user_channel_id = user_channel_id_low as u128 +
+			((user_channel_id_high_opt.unwrap_or(0 as u64) as u128) << 64);
+
+		Ok(Self {
+			inbound_scid_alias,
+			channel_id: channel_id.0.unwrap(),
+			channel_type,
+			counterparty: counterparty.0.unwrap(),
+			outbound_scid_alias,
+			funding_txo,
+			config,
+			short_channel_id,
+			channel_value_satoshis: channel_value_satoshis.0.unwrap(),
+			unspendable_punishment_reserve,
+			user_channel_id,
+			balance_msat: balance_msat.0.unwrap(),
+			outbound_capacity_msat: outbound_capacity_msat.0.unwrap(),
+			next_outbound_htlc_limit_msat: next_outbound_htlc_limit_msat.0.unwrap(),
+			inbound_capacity_msat: inbound_capacity_msat.0.unwrap(),
+			confirmations_required,
+			force_close_spend_delay,
+			is_outbound: is_outbound.0.unwrap(),
+			is_channel_ready: is_channel_ready.0.unwrap(),
+			is_usable: is_usable.0.unwrap(),
+			is_public: is_public.0.unwrap(),
+			inbound_htlc_minimum_msat,
+			inbound_htlc_maximum_msat,
+		})
+	}
+}
 
 impl_writeable_tlv_based!(PhantomRouteHints, {
 	(2, channels, vec_type),

lightning/src/ln/functional_test_utils.rs

@@ -618,7 +618,7 @@ macro_rules! check_added_monitors {
 	}
 }
 
-pub fn create_funding_transaction<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, expected_counterparty_node_id: &PublicKey, expected_chan_value: u64, expected_user_chan_id: u64) -> ([u8; 32], Transaction, OutPoint) {
+pub fn create_funding_transaction<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, expected_counterparty_node_id: &PublicKey, expected_chan_value: u64, expected_user_chan_id: u128) -> ([u8; 32], Transaction, OutPoint) {
 	let chan_id = *node.network_chan_count.borrow();
 
 	let events = node.node.get_and_clear_pending_events();