From 7ebc0a9365be496adc74a5a0118a7bfc76b6a2df Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Fri, 19 Jul 2019 19:23:10 -0400 Subject: [PATCH] Add user configurable csv delay encumbering channel refund output, within reasonable lower or upper bound Add our_to_self_delay in Channel, to cache user config field at channel construction. --- src/ln/channel.rs | 30 +++++++++++++++++++++--------- src/util/config.rs | 23 +++++++++++++++++++++++ 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 4afcbefe..e838305a 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -317,7 +317,7 @@ pub(super) struct Channel { their_htlc_minimum_msat: u64, our_htlc_minimum_msat: u64, their_to_self_delay: u16, - //implied by BREAKDOWN_TIMEOUT: our_to_self_delay: u16, + our_to_self_delay: u16, #[cfg(test)] pub their_max_accepted_htlcs: u16, #[cfg(not(test))] @@ -413,6 +413,9 @@ impl Channel { if push_msat > channel_value_satoshis * 1000 { return Err(APIError::APIMisuseError{err: "push value > channel value"}); } + if config.own_channel_config.our_to_self_delay < BREAKDOWN_TIMEOUT { + return Err(APIError::APIMisuseError{err: "Configured with an unreasonable our_to_self_delay putting user funds at risks"}); + } let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background); @@ -424,7 +427,7 @@ impl Channel { let secp_ctx = Secp256k1::new(); let channel_monitor = ChannelMonitor::new(&chan_keys.revocation_base_key, &chan_keys.delayed_payment_base_key, - &chan_keys.htlc_base_key, &chan_keys.payment_base_key, &keys_provider.get_shutdown_pubkey(), BREAKDOWN_TIMEOUT, + &chan_keys.htlc_base_key, &chan_keys.payment_base_key, &keys_provider.get_shutdown_pubkey(), config.own_channel_config.our_to_self_delay, keys_provider.get_destination_script(), logger.clone()); Ok(Channel { @@ -481,6 +484,7 @@ impl Channel { their_htlc_minimum_msat: 0, our_htlc_minimum_msat: Channel::derive_our_htlc_minimum_msat(feerate), their_to_self_delay: 0, + our_to_self_delay: config.own_channel_config.our_to_self_delay, their_max_accepted_htlcs: 0, minimum_depth: 0, // Filled in in accept_channel @@ -518,6 +522,10 @@ impl Channel { let chan_keys = keys_provider.get_channel_keys(true); let mut local_config = (*config).channel_options.clone(); + if config.own_channel_config.our_to_self_delay < BREAKDOWN_TIMEOUT { + return Err(ChannelError::Close("Configured with an unreasonable our_to_self_delay putting user funds at risks")); + } + // Check sanity of message fields: if msg.funding_satoshis >= MAX_FUNDING_SATOSHIS { return Err(ChannelError::Close("funding value > 2^24")); @@ -539,7 +547,7 @@ impl Channel { } Channel::check_remote_fee(fee_estimator, msg.feerate_per_kw)?; - if msg.to_self_delay > MAX_LOCAL_BREAKDOWN_TIMEOUT { + if msg.to_self_delay > config.peer_channel_config_limits.their_to_self_delay || msg.to_self_delay > MAX_LOCAL_BREAKDOWN_TIMEOUT { return Err(ChannelError::Close("They wanted our payments to be delayed by a needlessly long period")); } if msg.max_accepted_htlcs < 1 { @@ -612,7 +620,7 @@ impl Channel { let secp_ctx = Secp256k1::new(); let mut channel_monitor = ChannelMonitor::new(&chan_keys.revocation_base_key, &chan_keys.delayed_payment_base_key, - &chan_keys.htlc_base_key, &chan_keys.payment_base_key, &keys_provider.get_shutdown_pubkey(), BREAKDOWN_TIMEOUT, + &chan_keys.htlc_base_key, &chan_keys.payment_base_key, &keys_provider.get_shutdown_pubkey(), config.own_channel_config.our_to_self_delay, keys_provider.get_destination_script(), logger.clone()); channel_monitor.set_their_base_keys(&msg.htlc_basepoint, &msg.delayed_payment_basepoint); channel_monitor.set_their_to_self_delay(msg.to_self_delay); @@ -692,6 +700,7 @@ impl Channel { their_htlc_minimum_msat: msg.htlc_minimum_msat, our_htlc_minimum_msat: Channel::derive_our_htlc_minimum_msat(msg.feerate_per_kw as u64), their_to_self_delay: msg.to_self_delay, + our_to_self_delay: config.own_channel_config.our_to_self_delay, their_max_accepted_htlcs: msg.max_accepted_htlcs, minimum_depth: config.own_channel_config.minimum_depth, @@ -927,7 +936,7 @@ impl Channel { log_trace!(self, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a); txouts.push((TxOut { script_pubkey: chan_utils::get_revokeable_redeemscript(&keys.revocation_key, - if local { self.their_to_self_delay } else { BREAKDOWN_TIMEOUT }, + if local { self.their_to_self_delay } else { self.our_to_self_delay }, &keys.a_delayed_payment_key).to_v0_p2wsh(), value: value_to_a as u64 }, None)); @@ -1126,7 +1135,7 @@ impl Channel { /// @local is used only to convert relevant internal structures which refer to remote vs local /// to decide value of outputs and direction of HTLCs. fn build_htlc_transaction(&self, prev_hash: &Sha256dHash, htlc: &HTLCOutputInCommitment, local: bool, keys: &TxCreationKeys, feerate_per_kw: u64) -> Transaction { - chan_utils::build_htlc_transaction(prev_hash, feerate_per_kw, if local { self.their_to_self_delay } else { BREAKDOWN_TIMEOUT }, htlc, &keys.a_delayed_payment_key, &keys.revocation_key) + chan_utils::build_htlc_transaction(prev_hash, feerate_per_kw, if local { self.their_to_self_delay } else { self.our_to_self_delay }, htlc, &keys.a_delayed_payment_key, &keys.revocation_key) } fn create_htlc_tx_signature(&self, tx: &Transaction, htlc: &HTLCOutputInCommitment, keys: &TxCreationKeys) -> Result<(Script, Signature, bool), ChannelError> { @@ -1380,7 +1389,7 @@ impl Channel { if msg.htlc_minimum_msat >= (self.channel_value_satoshis - msg.channel_reserve_satoshis) * 1000 { return Err(ChannelError::Close("Minimum htlc value is full channel value")); } - if msg.to_self_delay > MAX_LOCAL_BREAKDOWN_TIMEOUT { + if msg.to_self_delay > config.peer_channel_config_limits.their_to_self_delay || msg.to_self_delay > MAX_LOCAL_BREAKDOWN_TIMEOUT { return Err(ChannelError::Close("They wanted our payments to be delayed by a needlessly long period")); } if msg.max_accepted_htlcs < 1 { @@ -3064,7 +3073,7 @@ impl Channel { channel_reserve_satoshis: Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis), htlc_minimum_msat: self.our_htlc_minimum_msat, feerate_per_kw: fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background) as u32, - to_self_delay: BREAKDOWN_TIMEOUT, + to_self_delay: self.our_to_self_delay, max_accepted_htlcs: OUR_MAX_HTLCS, funding_pubkey: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.funding_key), revocation_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.revocation_base_key), @@ -3097,7 +3106,7 @@ impl Channel { channel_reserve_satoshis: Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis), htlc_minimum_msat: self.our_htlc_minimum_msat, minimum_depth: self.minimum_depth, - to_self_delay: BREAKDOWN_TIMEOUT, + to_self_delay: self.our_to_self_delay, max_accepted_htlcs: OUR_MAX_HTLCS, funding_pubkey: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.funding_key), revocation_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.revocation_base_key), @@ -3746,6 +3755,7 @@ impl Writeable for Channel { self.their_htlc_minimum_msat.write(writer)?; self.our_htlc_minimum_msat.write(writer)?; self.their_to_self_delay.write(writer)?; + self.our_to_self_delay.write(writer)?; self.their_max_accepted_htlcs.write(writer)?; self.minimum_depth.write(writer)?; @@ -3907,6 +3917,7 @@ impl ReadableArgs> for Channel { let their_htlc_minimum_msat = Readable::read(reader)?; let our_htlc_minimum_msat = Readable::read(reader)?; let their_to_self_delay = Readable::read(reader)?; + let our_to_self_delay = Readable::read(reader)?; let their_max_accepted_htlcs = Readable::read(reader)?; let minimum_depth = Readable::read(reader)?; @@ -3984,6 +3995,7 @@ impl ReadableArgs> for Channel { their_htlc_minimum_msat, our_htlc_minimum_msat, their_to_self_delay, + our_to_self_delay, their_max_accepted_htlcs, minimum_depth, diff --git a/src/util/config.rs b/src/util/config.rs index e1a7e597..5b805d45 100644 --- a/src/util/config.rs +++ b/src/util/config.rs @@ -1,6 +1,8 @@ //! Various user-configurable channel limits and settings which ChannelManager //! applies for you. +use ln::channelmanager::{BREAKDOWN_TIMEOUT, MAX_LOCAL_BREAKDOWN_TIMEOUT}; + /// Top-level config which holds ChannelHandshakeLimits and ChannelConfig. #[derive(Clone, Debug)] pub struct UserConfig { @@ -30,6 +32,18 @@ pub struct ChannelHandshakeConfig { /// Applied only for inbound channels (see ChannelHandshakeLimits::max_minimum_depth for the /// equivalent limit applied to outbound channels). pub minimum_depth: u32, + /// Set to the amount of time we require our counterparty to wait to claim their money. + /// + /// It's one of the main parameter of our security model. We (or one of our watchtowers) MUST + /// be online to check for peer having broadcast a revoked transaction to steal our funds + /// at least once every our_to_self_delay blocks. + /// Default is BREAKDOWN_TIMEOUT, we enforce it as a minimum at channel opening so you can + /// tweak config to ask for more security, not less. + /// + /// Meanwhile, asking for a too high delay, we bother peer to freeze funds for nothing in + /// case of an honest unilateral channel close, which implicitly decrease the economic value of + /// our channel. + pub our_to_self_delay: u16, } impl ChannelHandshakeConfig { @@ -37,6 +51,7 @@ impl ChannelHandshakeConfig { pub fn new() -> ChannelHandshakeConfig { ChannelHandshakeConfig { minimum_depth: 6, + our_to_self_delay: BREAKDOWN_TIMEOUT, } } } @@ -88,6 +103,13 @@ pub struct ChannelHandshakeLimits { /// Defaults to true to make the default that no announced channels are possible (which is /// appropriate for any nodes which are not online very reliably). pub force_announced_channel_preference: bool, + /// Set to the amount of time we're willing to wait to claim money back to us. + /// + /// Not checking this value would be a security issue, as our peer would be able to set it to + /// max relative lock-time (a year) and we would "lose" money as it would be locked for a long time. + /// Default is MAX_LOCAL_BREAKDOWN_TIMEOUT, which we also enforce as a maximum value + /// so you can tweak config to reduce the loss of having useless locked funds (if your peer accepts) + pub their_to_self_delay: u16 } impl ChannelHandshakeLimits { @@ -107,6 +129,7 @@ impl ChannelHandshakeLimits { max_dust_limit_satoshis: ::max_value(), max_minimum_depth: 144, force_announced_channel_preference: true, + their_to_self_delay: MAX_LOCAL_BREAKDOWN_TIMEOUT, } } } -- 2.30.2