Support negotiating anchors throughout channel open

Author: wpaulino

lightning/src/ln/channel.rs

@@ -881,11 +881,20 @@ impl<Signer: Sign> Channel<Signer> {
 		// The default channel type (ie the first one we try) depends on whether the channel is
 		// public - if it is, we just go with `only_static_remotekey` as it's the only option
 		// available. If it's private, we first try `scid_privacy` as it provides better privacy
-		// with no other changes, and fall back to `only_static_remotekey`
+		// with no other changes, and fall back to `only_static_remotekey`.
 		let mut ret = ChannelTypeFeatures::only_static_remote_key();
 		if !config.channel_handshake_config.announced_channel && config.channel_handshake_config.negotiate_scid_privacy {
 			ret.set_scid_privacy_required();
 		}
+
+		// Optionally, if the user would like to negotiate the `anchors_zero_fee_htlc_tx` option, we
+		// set it now. If they don't understand it, we'll fall back to our default of
+		// `only_static_remotekey`.
+		#[cfg(anchors)]
+		if config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx {
+			ret.set_anchors_zero_fee_htlc_tx_required();
+		}
+
 		ret
 	}
 
@@ -898,7 +907,17 @@ impl<Signer: Sign> Channel<Signer> {
 			// We've exhausted our options
 			return Err(());
 		}
-		self.channel_type = ChannelTypeFeatures::only_static_remote_key(); // We only currently support two types
+		// We support opening a few different types of channels. Try removing our additional
+		// features one by one until we've either arrived at our default or the counterparty has
+		// accepted one.
+		if self.channel_type.supports_anchors_zero_fee_htlc_tx() {
+			self.channel_type.clear_anchors_zero_fee_htlc_tx();
+			self.channel_transaction_parameters.opt_anchors = None;
+		} else if self.channel_type.supports_scid_privacy() {
+			self.channel_type.clear_scid_privacy();
+		} else {
+			self.channel_type = ChannelTypeFeatures::only_static_remote_key();
+		}
 		Ok(self.get_open_channel(chain_hash))
 	}
 
@@ -911,7 +930,11 @@ impl<Signer: Sign> Channel<Signer> {
 	where K::Target: KeysInterface<Signer = Signer>,
 	      F::Target: FeeEstimator,
 	{
-		let opt_anchors = false; // TODO - should be based on features
+		#[cfg(anchors)]
+		let opt_anchors = config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx &&
+			their_features.supports_anchors_zero_fee_htlc_tx();
+		#[cfg(not(anchors))]
+		let opt_anchors = false;
 
 		let holder_selected_contest_delay = config.channel_handshake_config.our_to_self_delay;
 		let channel_keys_id = keys_provider.generate_channel_keys_id(false, channel_value_satoshis, user_id);
@@ -1124,7 +1147,6 @@ impl<Signer: Sign> Channel<Signer> {
 		      F::Target: FeeEstimator,
 		      L::Target: Logger,
 	{
-		let opt_anchors = false; // TODO - should be based on features
 		let announced_channel = if (msg.channel_flags & 1) == 1 { true } else { false };
 
 		// First check the channel type is known, failing before we do anything else if we don't
@@ -1138,13 +1160,24 @@ impl<Signer: Sign> Channel<Signer> {
 				return Err(ChannelError::Close("Channel Type field contains unknown bits".to_owned()));
 			}
 
-			// We currently only allow four channel types, so write it all out here - we allow
-			// `only_static_remote_key` or `static_remote_key | zero_conf` in all contexts, and
-			// further allow `static_remote_key | scid_privacy` or
-			// `static_remote_key | scid_privacy | zero_conf`, if the channel is not
-			// publicly announced.
+			// We currently allow 8 channel types, and all must support `static_remote_key`, so
+			// write it all out here - we allow
+			// - `only_static_remote_key`
+			// - `static_remote_key | zero_conf`
+			// - `static_remote_key | scid_privacy`
+			// - `static_remote_key | scid_privacy | zero_conf`
+			// - `static_remote_key | anchors_zero_fee_htlc_tx`
+			// - `static_remote_key | anchors_zero_fee_htlc_tx | zero_conf`
+			// - `static_remote_key | anchors_zero_fee_htlc_tx | scid_privacy`
+			// - `static_remote_key | anchors_zero_fee_htlc_tx | scid_privacy | zero_conf`
+			if !channel_type.requires_static_remote_key() {
+				return Err(ChannelError::Close("Channel Type was not understood - we require static remote key".to_owned()));
+			}
 			if *channel_type != ChannelTypeFeatures::only_static_remote_key() {
-				if !channel_type.requires_scid_privacy() && !channel_type.requires_zero_conf() {
+				// If we have more features than just `static_remote_key`, make sure we only allow
+				// those we support.
+				if !channel_type.requires_scid_privacy() && !channel_type.requires_zero_conf() &&
+					!channel_type.requires_anchors_zero_fee_htlc_tx() {
 					return Err(ChannelError::Close("Channel Type was not understood".to_owned()));
 				}
 
@@ -1154,11 +1187,16 @@ impl<Signer: Sign> Channel<Signer> {
 			}
 			channel_type.clone()
 		} else {
-			ChannelTypeFeatures::from_counterparty_init(&their_features)
+			let channel_type = ChannelTypeFeatures::from_counterparty_init(&their_features);
+			if channel_type != ChannelTypeFeatures::only_static_remote_key() {
+				return Err(ChannelError::Close("Only static_remote_key is supported for non-negotiated channel types".to_owned()));
+			}
+			channel_type
 		};
-		if !channel_type.supports_static_remote_key() {
-			return Err(ChannelError::Close("Channel Type was not understood - we require static remote key".to_owned()));
-		}
+		#[cfg(anchors)]
+		let opt_anchors = channel_type.supports_anchors_zero_fee_htlc_tx();
+		#[cfg(not(anchors))]
+		let opt_anchors = false;
 
 		let channel_keys_id = keys_provider.generate_channel_keys_id(true, msg.funding_satoshis, user_id);
 		let holder_signer = keys_provider.derive_channel_signer(msg.funding_satoshis, channel_keys_id);
@@ -2128,7 +2166,16 @@ impl<Signer: Sign> Channel<Signer> {
 		} else if their_features.supports_channel_type() {
 			// Assume they've accepted the channel type as they said they understand it.
 		} else {
-			self.channel_type = ChannelTypeFeatures::from_counterparty_init(&their_features)
+			let channel_type = ChannelTypeFeatures::from_counterparty_init(&their_features);
+			if channel_type.requires_unknown_bits() {
+				return Err(ChannelError::Close("Peer's features contain unknown ChannelType features".to_owned()));
+			}
+			self.channel_type = channel_type;
+			self.channel_transaction_parameters.opt_anchors = if self.channel_type.supports_anchors_zero_fee_htlc_tx() {
+				Some(())
+			} else {
+				None
+			};
 		}
 
 		let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
@@ -6654,11 +6701,6 @@ impl<'a, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<<K::Target as KeysInte
 			return Err(DecodeError::UnknownRequiredFeature);
 		}
 
-		if channel_parameters.opt_anchors.is_some() {
-			// Relax this check when ChannelTypeFeatures supports anchors.
-			return Err(DecodeError::InvalidValue);
-		}
-
 		let mut secp_ctx = Secp256k1::new();
 		secp_ctx.seeded_randomize(&keys_source.get_secure_random_bytes());
 
@@ -6793,6 +6835,8 @@ mod tests {
 	use hex;
 	use crate::ln::PaymentHash;
 	use crate::ln::channelmanager::{self, HTLCSource, PaymentId};
+	#[cfg(anchors)]
+	use crate::ln::channel::InitFeatures;
 	use crate::ln::channel::{Channel, InboundHTLCOutput, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator};
 	use crate::ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS, MIN_THEIR_CHAN_RESERVE_SATOSHIS};
 	use crate::ln::features::ChannelTypeFeatures;
@@ -6807,6 +6851,8 @@ mod tests {
 	use crate::util::config::UserConfig;
 	use crate::util::enforcing_trait_impls::EnforcingSigner;
 	use crate::util::errors::APIError;
+	#[cfg(anchors)]
+	use crate::util::ser::Writeable;
 	use crate::util::test_utils;
 	use crate::util::test_utils::OnGetShutdownScriptpubkey;
 	use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature, Scalar};
@@ -8069,4 +8115,152 @@ mod tests {
 			node_b_node_id, &channelmanager::provided_init_features(), &open_channel_msg, 7, &config, 0, &&logger, 42);
 		assert!(res.is_ok());
 	}
+
+	#[cfg(anchors)]
+	#[test]
+	fn test_supports_anchors_zero_htlc_tx_fee() {
+		// Tests that if both sides support and negotiate `anchors_zero_fee_htlc_tx`, it is the
+		// resulting `channel_type`.
+		let secp_ctx = Secp256k1::new();
+		let fee_estimator = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
+		let network = Network::Testnet;
+		let keys_provider = test_utils::TestKeysInterface::new(&[42; 32], network);
+		let logger = test_utils::TestLogger::new();
+
+		let node_id_a = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[1; 32]).unwrap());
+		let node_id_b = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[2; 32]).unwrap());
+
+		let mut config = UserConfig::default();
+		config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
+
+		let mut expected_channel_type = ChannelTypeFeatures::empty();
+		expected_channel_type.set_static_remote_key_required();
+		expected_channel_type.set_anchors_zero_fee_htlc_tx_required();
+
+		let channel_a = Channel::<EnforcingSigner>::new_outbound(
+			&fee_estimator, &&keys_provider, node_id_b, &channelmanager::provided_init_features(),
+			10000000, 100000, 42, &config, 0, 42
+		).unwrap();
+
+		let open_channel_msg = channel_a.get_open_channel(genesis_block(network).header.block_hash());
+		let channel_b = Channel::<EnforcingSigner>::new_from_req(
+			&fee_estimator, &&keys_provider, node_id_a, &channelmanager::provided_init_features(),
+			&open_channel_msg, 7, &config, 0, &&logger, 42
+		).unwrap();
+
+		assert_eq!(channel_a.channel_type, expected_channel_type);
+		assert_eq!(channel_b.channel_type, expected_channel_type);
+	}
+
+	#[cfg(anchors)]
+	#[test]
+	fn test_rejects_implicit_simple_anchors() {
+		// Tests that if `option_anchors` is being negotiated implicitly through the intersection of
+		// each side's `InitFeatures`, it is rejected.
+		let secp_ctx = Secp256k1::new();
+		let fee_estimator = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
+		let network = Network::Testnet;
+		let keys_provider = test_utils::TestKeysInterface::new(&[42; 32], network);
+		let logger = test_utils::TestLogger::new();
+
+		let node_id_a = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[1; 32]).unwrap());
+		let node_id_b = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[2; 32]).unwrap());
+
+		let config = UserConfig::default();
+
+		// See feature bit assignments: https://github.com/lightning/bolts/blob/master/09-features.md
+		let static_remote_key_required: u64 = 1 << 12;
+		let simple_anchors_required: u64 = 1 << 20;
+		let raw_init_features = static_remote_key_required | simple_anchors_required;
+		let init_features_with_simple_anchors = InitFeatures::from_le_bytes(raw_init_features.to_le_bytes().to_vec());
+
+		let channel_a = Channel::<EnforcingSigner>::new_outbound(
+			&fee_estimator, &&keys_provider, node_id_b, &channelmanager::provided_init_features(),
+			10000000, 100000, 42, &config, 0, 42
+		).unwrap();
+
+		// Set `channel_type` to `None` to force the implicit feature negotiation.
+		let mut open_channel_msg = channel_a.get_open_channel(genesis_block(network).header.block_hash());
+		open_channel_msg.channel_type = None;
+
+		// Since A supports both `static_remote_key` and `option_anchors`, but B only supports
+		// `static_remote_key`, the resulting channel will succeed with only `static_remote_key`.
+		let channel_b = Channel::<EnforcingSigner>::new_from_req(
+			&fee_estimator, &&keys_provider, node_id_a, &init_features_with_simple_anchors,
+			&open_channel_msg, 7, &config, 0, &&logger, 42
+		).unwrap();
+
+		// Verify the resulting `channel_type` is indeed only `static_remote_key`.
+		let mut encoded_channel_type = [0; 8];
+		encoded_channel_type[1..8].copy_from_slice(channel_b.channel_type.encode().as_slice());
+		let mut encoded_only_static_remote_key = [0; 8];
+		encoded_only_static_remote_key[6..8].copy_from_slice(ChannelTypeFeatures::only_static_remote_key().encode().as_slice());
+		assert_eq!(u64::from_be_bytes(encoded_channel_type), u64::from_be_bytes(encoded_only_static_remote_key));
+		assert_eq!(u64::from_be_bytes(encoded_channel_type), u64::from_be_bytes(encoded_only_static_remote_key));
+	}
+
+	#[cfg(anchors)]
+	#[test]
+	fn test_rejects_simple_anchors_channel_type() {
+		// Tests that if `option_anchors` is being negotiated through the `channel_type` feature,
+		// it is rejected.
+		let secp_ctx = Secp256k1::new();
+		let fee_estimator = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
+		let network = Network::Testnet;
+		let keys_provider = test_utils::TestKeysInterface::new(&[42; 32], network);
+		let logger = test_utils::TestLogger::new();
+
+		let node_id_a = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[1; 32]).unwrap());
+		let node_id_b = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[2; 32]).unwrap());
+
+		let config = UserConfig::default();
+
+		// See feature bit assignments: https://github.com/lightning/bolts/blob/master/09-features.md
+		let static_remote_key_required: u64 = 1 << 12;
+		let simple_anchors_required: u64 = 1 << 20;
+		let simple_anchors_raw_features = static_remote_key_required | simple_anchors_required;
+		let simple_anchors_features = InitFeatures::from_le_bytes(simple_anchors_raw_features.to_le_bytes().to_vec());
+		let simple_anchors_channel_type = ChannelTypeFeatures::from_counterparty_init(&simple_anchors_features);
+
+		// First, we'll try to open a channel between A and B where A requests a channel type for
+		// the original `option_anchors` feature (non zero fee htlc tx). This should be rejected by
+		// B as it's not supported by LDK.
+		let channel_a = Channel::<EnforcingSigner>::new_outbound(
+			&fee_estimator, &&keys_provider, node_id_b, &channelmanager::provided_init_features(),
+			10000000, 100000, 42, &config, 0, 42
+		).unwrap();
+
+		let mut open_channel_msg = channel_a.get_open_channel(genesis_block(network).header.block_hash());
+		open_channel_msg.channel_type = Some(simple_anchors_channel_type.clone());
+
+		let res = Channel::<EnforcingSigner>::new_from_req(
+			&fee_estimator, &&keys_provider, node_id_a, &simple_anchors_features, &open_channel_msg,
+			7, &config, 0, &&logger, 42
+		);
+		assert!(res.is_err());
+
+		// Then, we'll try to open another channel where A requests a channel type for
+		// `anchors_zero_fee_htlc_tx`. B is malicious and tries to downgrade the channel type to the
+		// original `option_anchors` feature, which should be rejected by A as it's not supported by
+		// LDK.
+		let mut channel_a = Channel::<EnforcingSigner>::new_outbound(
+			&fee_estimator, &&keys_provider, node_id_b, &simple_anchors_features,
+			10000000, 100000, 42, &config, 0, 42
+		).unwrap();
+
+		let open_channel_msg = channel_a.get_open_channel(genesis_block(network).header.block_hash());
+
+		let channel_b = Channel::<EnforcingSigner>::new_from_req(
+			&fee_estimator, &&keys_provider, node_id_a, &channelmanager::provided_init_features(),
+			&open_channel_msg, 7, &config, 0, &&logger, 42
+		).unwrap();
+
+		let mut accept_channel_msg = channel_b.get_accept_channel_message();
+		accept_channel_msg.channel_type = Some(simple_anchors_channel_type.clone());
+
+		let res = channel_a.accept_channel(
+			&accept_channel_msg, &config.channel_handshake_limits, &simple_anchors_features
+		);
+		assert!(res.is_err());
+	}
 }

lightning/src/ln/channelmanager.rs

@@ -6559,6 +6559,7 @@ pub fn provided_init_features() -> InitFeatures {
 	features.set_channel_type_optional();
 	features.set_scid_privacy_optional();
 	features.set_zero_conf_optional();
+	#[cfg(anchors)]
 	features.set_anchors_zero_fee_htlc_tx_optional();
 	features
 }

lightning/src/ln/features.rs

@@ -698,6 +698,18 @@ impl<T: sealed::Wumbo> Features<T> {
 	}
 }
 
+impl<T: sealed::SCIDPrivacy> Features<T> {
+	pub(crate) fn clear_scid_privacy(&mut self) {
+		<T as sealed::SCIDPrivacy>::clear_bits(&mut self.flags);
+	}
+}
+
+impl<T: sealed::AnchorsZeroFeeHtlcTx> Features<T> {
+	pub(crate) fn clear_anchors_zero_fee_htlc_tx(&mut self) {
+		<T as sealed::AnchorsZeroFeeHtlcTx>::clear_bits(&mut self.flags);
+	}
+}
+
 #[cfg(test)]
 impl<T: sealed::UnknownFeature> Features<T> {
 	pub(crate) fn unknown() -> Self {
@@ -787,6 +799,7 @@ mod tests {
 		init_features.set_channel_type_optional();
 		init_features.set_scid_privacy_optional();
 		init_features.set_zero_conf_optional();
+		init_features.set_anchors_zero_fee_htlc_tx_optional();
 
 		assert!(init_features.initial_routing_sync());
 		assert!(!init_features.supports_upfront_shutdown_script());

lightning/src/util/config.rs

@@ -126,7 +126,6 @@ pub struct ChannelHandshakeConfig {
 	///
 	/// [`KeysInterface::get_shutdown_scriptpubkey`]: crate::chain::keysinterface::KeysInterface::get_shutdown_scriptpubkey
 	pub commit_upfront_shutdown_pubkey: bool,
-
 	/// The Proportion of the channel value to configure as counterparty's channel reserve,
 	/// i.e., `their_channel_reserve_satoshis` for both outbound and inbound channels.
 	///
@@ -149,7 +148,28 @@ pub struct ChannelHandshakeConfig {
 	///                as 1000 sats instead, which is a safe implementation-specific lower bound.
 	/// Maximum value: 1,000,000, any values larger than 1 Million will be treated as 1 Million (or 100%)
 	///                instead, although channel negotiations will fail in that case.
-	pub their_channel_reserve_proportional_millionths: u32
+	pub their_channel_reserve_proportional_millionths: u32,
+	#[cfg(anchors)]
+	/// If set, we attempt to negotiate the `anchors_zero_fee_htlc_tx`option for outbound channels.
+	///
+	/// If this option is set, channels may be created that will not be readable by LDK versions
+	/// prior to 0.0.114, causing [`ChannelManager`]'s read method to return a
+	/// [`DecodeError::InvalidValue`].
+	///
+	/// Note that setting this to true does *not* prevent us from opening channels with
+	/// counterparties that do not support the `anchors_zero_fee_htlc_tx` option; we will simply
+	/// fall back to a `static_remote_key` channel.
+	///
+	/// LDK will not support the legacy `option_anchors` commitment version due to a discovered
+	/// vulnerability after its deployment. For more context, see the [`SIGHASH_SINGLE + update_fee
+	/// Considered Harmful`] mailing list post.
+	///
+	/// Default value: false. This value is likely to change to true in the future.
+	///
+	/// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager
+	/// [`DecodeError::InvalidValue`]: crate::ln::msgs::DecodeError::InvalidValue
+	/// [`SIGHASH_SINGLE + update_fee Considered Harmful`]: https://lists.linuxfoundation.org/pipermail/lightning-dev/2020-September/002796.html
+	pub negotiate_anchors_zero_fee_htlc_tx: bool,
 }
 
 impl Default for ChannelHandshakeConfig {
@@ -163,6 +183,8 @@ impl Default for ChannelHandshakeConfig {
 			announced_channel: false,
 			commit_upfront_shutdown_pubkey: true,
 			their_channel_reserve_proportional_millionths: 10_000,
+			#[cfg(anchors)]
+			negotiate_anchors_zero_fee_htlc_tx: false,
 		}
 	}
 }