X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=blobdiff_plain;f=lightning%2Fsrc%2Fln%2Fchannel.rs;h=4a6e4eb656b6f6fa2f9a5076814853d8ac77f9ea;hb=843d25d750c3408d3f8f917764b8a58019a9dd81;hp=d6ebbf1cff20c769f6911f9e0d0243fc55b3230c;hpb=bd14069f049066007fd159be85f92c217be50f55;p=rust-lightning diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index d6ebbf1c..4a6e4eb6 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -8,7 +8,7 @@ // licenses. use bitcoin::blockdata::script::{Script,Builder}; -use bitcoin::blockdata::transaction::{TxIn, TxOut, Transaction, SigHashType}; +use bitcoin::blockdata::transaction::{Transaction, SigHashType}; use bitcoin::util::bip143; use bitcoin::consensus::encode; @@ -26,16 +26,15 @@ use ln::{PaymentPreimage, PaymentHash}; use ln::features::{ChannelFeatures, InitFeatures}; use ln::msgs; use ln::msgs::{DecodeError, OptionalField, DataLossProtect}; -use ln::script::ShutdownScript; -use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT}; -use ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor}; +use ln::script::{self, ShutdownScript}; +use ln::channelmanager::{CounterpartyForwardingInfo, PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT}; +use ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor, ClosingTransaction}; use ln::chan_utils; use chain::BestBlock; use chain::chaininterface::{FeeEstimator,ConfirmationTarget}; use chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, HTLC_FAIL_BACK_BUFFER}; use chain::transaction::{OutPoint, TransactionData}; use chain::keysinterface::{Sign, KeysInterface}; -use util::transaction_utils; use util::ser::{Readable, ReadableArgs, Writeable, Writer, VecWriter}; use util::logger::Logger; use util::errors::APIError; @@ -45,7 +44,6 @@ use util::scid_utils::scid_from_parts; use io; use prelude::*; use core::{cmp,mem,fmt}; -use core::convert::TryFrom; use core::ops::Deref; #[cfg(any(test, feature = "fuzztarget", debug_assertions))] use sync::Mutex; @@ -255,8 +253,6 @@ enum ChannelState { RemoteShutdownSent = 1 << 10, /// Flag which is set on ChannelFunded or FundingSent after sending a shutdown message. At this /// point, we may not add any new HTLCs to the channel. - /// TODO: Investigate some kind of timeout mechanism by which point the remote end must provide - /// us their shutdown. LocalShutdownSent = 1 << 11, /// We've successfully negotiated a closing_signed dance. At this point ChannelManager is about /// to drop us, but we store this anyway. @@ -313,19 +309,6 @@ impl HTLCCandidate { } } -/// Information needed for constructing an invoice route hint for this channel. -#[derive(Clone, Debug, PartialEq)] -pub struct CounterpartyForwardingInfo { - /// Base routing fee in millisatoshis. - pub fee_base_msat: u32, - /// Amount in millionths of a satoshi the channel will charge per transferred satoshi. - pub fee_proportional_millionths: u32, - /// The minimum difference in cltv_expiry between an ingoing HTLC and its outgoing counterpart, - /// such that the outgoing HTLC is forwardable to this counterparty. See `msgs::ChannelUpdate`'s - /// `cltv_expiry_delta` for more details. - pub cltv_expiry_delta: u16, -} - /// A return value enum for get_update_fulfill_htlc. See UpdateFulfillCommitFetch variants for /// description enum UpdateFulfillFetch { @@ -424,22 +407,18 @@ pub(super) struct Channel { monitor_pending_forwards: Vec<(PendingHTLCInfo, u64)>, monitor_pending_failures: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, - // pending_update_fee is filled when sending and receiving update_fee - // For outbound channel, feerate_per_kw is updated with the value from - // pending_update_fee when revoke_and_ack is received + // pending_update_fee is filled when sending and receiving update_fee. // - // For inbound channel, feerate_per_kw is updated when it receives - // commitment_signed and revoke_and_ack is generated - // The pending value is kept when another pair of update_fee and commitment_signed - // is received during AwaitingRemoteRevoke and relieved when the expected - // revoke_and_ack is received and new commitment_signed is generated to be - // sent to the funder. Otherwise, the pending value is removed when receiving - // commitment_signed. + // Because it follows the same commitment flow as HTLCs, `FeeUpdateState` is either `Outbound` + // or matches a subset of the `InboundHTLCOutput` variants. It is then updated/used when + // generating new commitment transactions with exactly the same criteria as inbound/outbound + // HTLCs with similar state. pending_update_fee: Option<(u32, FeeUpdateState)>, - // update_fee() during ChannelState::AwaitingRemoteRevoke is hold in - // holdina_cell_update_fee then moved to pending_udpate_fee when revoke_and_ack - // is received. holding_cell_update_fee is updated when there are additional - // update_fee() during ChannelState::AwaitingRemoteRevoke. + // If a `send_update_fee()` call is made with ChannelState::AwaitingRemoteRevoke set, we place + // it here instead of `pending_update_fee` in the same way as we place outbound HTLC updates in + // `holding_cell_htlc_updates` instead of `pending_outbound_htlcs`. It is released into + // `pending_update_fee` with the same criteria as outbound HTLC updates but can be updated by + // further `send_update_fee` calls, dropping the previous holding cell update entirely. holding_cell_update_fee: Option, next_holder_htlc_id: u64, next_counterparty_htlc_id: u64, @@ -453,7 +432,20 @@ pub(super) struct Channel { /// Max to_local and to_remote outputs in a remote-generated commitment transaction counterparty_max_commitment_tx_output: Mutex<(u64, u64)>, - last_sent_closing_fee: Option<(u32, u64, Signature)>, // (feerate, fee, holder_sig) + last_sent_closing_fee: Option<(u64, Signature)>, // (fee, holder_sig) + target_closing_feerate_sats_per_kw: Option, + + /// If our counterparty sent us a closing_signed while we were waiting for a `ChannelMonitor` + /// update, we need to delay processing it until later. We do that here by simply storing the + /// closing_signed message and handling it in `maybe_propose_closing_signed`. + pending_counterparty_closing_signed: Option, + + /// The minimum and maximum absolute fee, in satoshis, we are willing to place on the closing + /// transaction. These are set once we reach `closing_negotiation_ready`. + #[cfg(test)] + pub(crate) closing_fee_limits: Option<(u64, u64)>, + #[cfg(not(test))] + closing_fee_limits: Option<(u64, u64)>, /// The hash of the block in which the funding transaction was included. funding_tx_confirmed_in: Option, @@ -496,6 +488,13 @@ pub(super) struct Channel { commitment_secrets: CounterpartyCommitmentSecrets, channel_update_status: ChannelUpdateStatus, + /// Once we reach `closing_negotiation_ready`, we set this, indicating if closing_signed does + /// not complete within a single timer tick (one minute), we should force-close the channel. + /// This prevents us from keeping unusable channels around forever if our counterparty wishes + /// to DoS us. + /// Note that this field is reset to false on deserialization to give us a chance to connect to + /// our peer and start the closing_signed negotiation fresh. + closing_signed_in_flight: bool, /// Our counterparty's channel_announcement signatures provided in announcement_signatures. /// This can be used to rebroadcast the channel_announcement message later. @@ -549,25 +548,30 @@ const COMMITMENT_TX_WEIGHT_PER_HTLC: u64 = 172; #[cfg(test)] pub const COMMITMENT_TX_WEIGHT_PER_HTLC: u64 = 172; -/// Maximmum `funding_satoshis` value, according to the BOLT #2 specification +pub const ANCHOR_OUTPUT_VALUE_SATOSHI: u64 = 330; + +/// Maximum `funding_satoshis` value, according to the BOLT #2 specification /// it's 2^24. pub const MAX_FUNDING_SATOSHIS: u64 = 1 << 24; -/// Maximum counterparty `dust_limit_satoshis` allowed. 2 * standard dust threshold on p2wsh output -/// Scales up on Bitcoin Core's proceeding policy with dust outputs. A typical p2wsh output is 43 -/// bytes to which Core's `GetDustThreshold()` sums up a minimal spend of 67 bytes (even if -/// a p2wsh witnessScript might be *effectively* smaller), `dustRelayFee` is set to 3000sat/kb, thus -/// 110 * 3000 / 1000 = 330. Per-protocol rules, all time-sensitive outputs are p2wsh, a value of -/// 330 sats is the lower bound desired to ensure good propagation of transactions. We give a bit -/// of margin to our counterparty and pick up 660 satoshis as an accepted `dust_limit_satoshis` -/// upper bound to avoid negotiation conflicts with other implementations. -pub const MAX_DUST_LIMIT_SATOSHIS: u64 = 2 * 330; - -/// A typical p2wsh output is 43 bytes to which Core's `GetDustThreshold()` sums up a minimal -/// spend of 67 bytes (even if a p2wsh witnessScript might be *effectively* smaller), `dustRelayFee` -/// is set to 3000sat/kb, thus 110 * 3000 / 1000 = 330. Per-protocol rules, all time-sensitive outputs -/// are p2wsh, a value of 330 sats is the lower bound desired to ensure good propagation of transactions. -pub const MIN_DUST_LIMIT_SATOSHIS: u64 = 330; +/// The maximum network dust limit for standard script formats. This currently represents the +/// minimum output value for a P2SH output before Bitcoin Core 22 considers the entire +/// transaction non-standard and thus refuses to relay it. +/// We also use this as the maximum counterparty `dust_limit_satoshis` allowed, given many +/// implementations use this value for their dust limit today. +pub const MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS: u64 = 546; + +/// The maximum channel dust limit we will accept from our counterparty. +pub const MAX_CHAN_DUST_LIMIT_SATOSHIS: u64 = MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS; + +/// The dust limit is used for both the commitment transaction outputs as well as the closing +/// transactions. For cooperative closing transactions, we require segwit outputs, though accept +/// *any* segwit scripts, which are allowed to be up to 42 bytes in length. +/// In order to avoid having to concern ourselves with standardness during the closing process, we +/// simply require our counterparty to use a dust limit which will leave any segwit output +/// standard. +/// See https://github.com/lightningnetwork/lightning-rfc/issues/905 for more details. +pub const MIN_CHAN_DUST_LIMIT_SATOSHIS: u64 = 354; /// Used to return a simple Error back to ChannelManager. Will get converted to a /// msgs::ErrorAction::SendErrorMessage or msgs::ErrorAction::IgnoreError as appropriate with our @@ -634,7 +638,7 @@ impl Channel { return Err(APIError::APIMisuseError {err: format!("Configured with an unreasonable our_to_self_delay ({}) putting user funds at risks", holder_selected_contest_delay)}); } let holder_selected_channel_reserve_satoshis = Channel::::get_holder_selected_channel_reserve_satoshis(channel_value_satoshis); - if holder_selected_channel_reserve_satoshis < MIN_DUST_LIMIT_SATOSHIS { + if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { return Err(APIError::APIMisuseError { err: format!("Holder selected channel reserve below implemention limit dust_limit_satoshis {}", holder_selected_channel_reserve_satoshis) }); } @@ -695,6 +699,9 @@ impl Channel { counterparty_max_commitment_tx_output: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)), last_sent_closing_fee: None, + pending_counterparty_closing_signed: None, + closing_fee_limits: None, + target_closing_feerate_sats_per_kw: None, funding_tx_confirmed_in: None, funding_tx_confirmation_height: 0, @@ -702,7 +709,7 @@ impl Channel { feerate_per_kw: feerate, counterparty_dust_limit_satoshis: 0, - holder_dust_limit_satoshis: MIN_DUST_LIMIT_SATOSHIS, + holder_dust_limit_satoshis: MIN_CHAN_DUST_LIMIT_SATOSHIS, counterparty_max_htlc_value_in_flight_msat: 0, counterparty_selected_channel_reserve_satoshis: None, // Filled in in accept_channel counterparty_htlc_minimum_msat: 0, @@ -730,6 +737,7 @@ impl Channel { commitment_secrets: CounterpartyCommitmentSecrets::new(), channel_update_status: ChannelUpdateStatus::Enabled, + closing_signed_in_flight: false, announcement_sigs: None, @@ -835,11 +843,11 @@ impl Channel { if msg.max_accepted_htlcs < config.peer_channel_config_limits.min_max_accepted_htlcs { return Err(ChannelError::Close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", msg.max_accepted_htlcs, config.peer_channel_config_limits.min_max_accepted_htlcs))); } - if msg.dust_limit_satoshis < MIN_DUST_LIMIT_SATOSHIS { - return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_DUST_LIMIT_SATOSHIS))); + if msg.dust_limit_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { + return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); } - if msg.dust_limit_satoshis > MAX_DUST_LIMIT_SATOSHIS { - return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.dust_limit_satoshis, MAX_DUST_LIMIT_SATOSHIS))); + if msg.dust_limit_satoshis > MAX_CHAN_DUST_LIMIT_SATOSHIS { + return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.dust_limit_satoshis, MAX_CHAN_DUST_LIMIT_SATOSHIS))); } // Convert things into internal flags and prep our state: @@ -856,11 +864,11 @@ impl Channel { let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background); let holder_selected_channel_reserve_satoshis = Channel::::get_holder_selected_channel_reserve_satoshis(msg.funding_satoshis); - if holder_selected_channel_reserve_satoshis < MIN_DUST_LIMIT_SATOSHIS { - return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, MIN_DUST_LIMIT_SATOSHIS))); + if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { + return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); } - if msg.channel_reserve_satoshis < MIN_DUST_LIMIT_SATOSHIS { - return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is smaller than our dust limit ({})", msg.channel_reserve_satoshis, MIN_DUST_LIMIT_SATOSHIS))); + if msg.channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { + return Err(ChannelError::Close(format!("channel_reserve_satoshis ({}) is smaller than our dust limit ({})", msg.channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); } if holder_selected_channel_reserve_satoshis < msg.dust_limit_satoshis { return Err(ChannelError::Close(format!("Dust limit ({}) too high for the channel reserve we require the remote to keep ({})", msg.dust_limit_satoshis, holder_selected_channel_reserve_satoshis))); @@ -887,10 +895,10 @@ impl Channel { if script.len() == 0 { None } else { - match ShutdownScript::try_from((script.clone(), their_features)) { - Ok(shutdown_script) => Some(shutdown_script.into_inner()), - Err(_) => return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: {}", script))), + if !script::is_bolt2_compliant(&script, their_features) { + return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: {}", script))) } + Some(script.clone()) } }, // Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel @@ -954,6 +962,9 @@ impl Channel { counterparty_max_commitment_tx_output: Mutex::new((msg.push_msat, msg.funding_satoshis * 1000 - msg.push_msat)), last_sent_closing_fee: None, + pending_counterparty_closing_signed: None, + closing_fee_limits: None, + target_closing_feerate_sats_per_kw: None, funding_tx_confirmed_in: None, funding_tx_confirmation_height: 0, @@ -962,7 +973,7 @@ impl Channel { feerate_per_kw: msg.feerate_per_kw, channel_value_satoshis: msg.funding_satoshis, counterparty_dust_limit_satoshis: msg.dust_limit_satoshis, - holder_dust_limit_satoshis: MIN_DUST_LIMIT_SATOSHIS, + holder_dust_limit_satoshis: MIN_CHAN_DUST_LIMIT_SATOSHIS, counterparty_max_htlc_value_in_flight_msat: cmp::min(msg.max_htlc_value_in_flight_msat, msg.funding_satoshis * 1000), counterparty_selected_channel_reserve_satoshis: Some(msg.channel_reserve_satoshis), counterparty_htlc_minimum_msat: msg.htlc_minimum_msat, @@ -993,6 +1004,7 @@ impl Channel { commitment_secrets: CounterpartyCommitmentSecrets::new(), channel_update_status: ChannelUpdateStatus::Enabled, + closing_signed_in_flight: false, announcement_sigs: None, @@ -1180,6 +1192,11 @@ impl Channel { let mut value_to_a = if local { value_to_self } else { value_to_remote }; let mut value_to_b = if local { value_to_remote } else { value_to_self }; + let (funding_pubkey_a, funding_pubkey_b) = if local { + (self.get_holder_pubkeys().funding_pubkey, self.get_counterparty_pubkeys().funding_pubkey) + } else { + (self.get_counterparty_pubkeys().funding_pubkey, self.get_holder_pubkeys().funding_pubkey) + }; if value_to_a >= (broadcaster_dust_limit_satoshis as i64) { log_trace!(logger, " ...including {} output with value {}", if local { "to_local" } else { "to_remote" }, value_to_a); @@ -1201,6 +1218,9 @@ impl Channel { let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number, value_to_a as u64, value_to_b as u64, + false, + funding_pubkey_a, + funding_pubkey_b, keys.clone(), feerate_per_kw, &mut included_non_dust_htlcs, @@ -1250,63 +1270,38 @@ impl Channel { } #[inline] - fn build_closing_transaction(&self, proposed_total_fee_satoshis: u64, skip_remote_output: bool) -> (Transaction, u64) { - let txins = { - let mut ins: Vec = Vec::new(); - ins.push(TxIn { - previous_output: self.funding_outpoint().into_bitcoin_outpoint(), - script_sig: Script::new(), - sequence: 0xffffffff, - witness: Vec::new(), - }); - ins - }; - + fn build_closing_transaction(&self, proposed_total_fee_satoshis: u64, skip_remote_output: bool) -> (ClosingTransaction, u64) { assert!(self.pending_inbound_htlcs.is_empty()); assert!(self.pending_outbound_htlcs.is_empty()); assert!(self.pending_update_fee.is_none()); - let mut txouts: Vec<(TxOut, ())> = Vec::new(); let mut total_fee_satoshis = proposed_total_fee_satoshis; - let value_to_self: i64 = (self.value_to_self_msat as i64) / 1000 - if self.is_outbound() { total_fee_satoshis as i64 } else { 0 }; - let value_to_remote: i64 = ((self.channel_value_satoshis * 1000 - self.value_to_self_msat) as i64 / 1000) - if self.is_outbound() { 0 } else { total_fee_satoshis as i64 }; + let mut value_to_holder: i64 = (self.value_to_self_msat as i64) / 1000 - if self.is_outbound() { total_fee_satoshis as i64 } else { 0 }; + let mut value_to_counterparty: i64 = ((self.channel_value_satoshis * 1000 - self.value_to_self_msat) as i64 / 1000) - if self.is_outbound() { 0 } else { total_fee_satoshis as i64 }; - if value_to_self < 0 { + if value_to_holder < 0 { assert!(self.is_outbound()); - total_fee_satoshis += (-value_to_self) as u64; - } else if value_to_remote < 0 { + total_fee_satoshis += (-value_to_holder) as u64; + } else if value_to_counterparty < 0 { assert!(!self.is_outbound()); - total_fee_satoshis += (-value_to_remote) as u64; + total_fee_satoshis += (-value_to_counterparty) as u64; } - if !skip_remote_output && value_to_remote as u64 > self.holder_dust_limit_satoshis { - txouts.push((TxOut { - script_pubkey: self.counterparty_shutdown_scriptpubkey.clone().unwrap(), - value: value_to_remote as u64 - }, ())); + if skip_remote_output || value_to_counterparty as u64 <= self.holder_dust_limit_satoshis { + value_to_counterparty = 0; } - assert!(self.shutdown_scriptpubkey.is_some()); - if value_to_self as u64 > self.holder_dust_limit_satoshis { - txouts.push((TxOut { - script_pubkey: self.get_closing_scriptpubkey(), - value: value_to_self as u64 - }, ())); + if value_to_holder as u64 <= self.holder_dust_limit_satoshis { + value_to_holder = 0; } - transaction_utils::sort_outputs(&mut txouts, |_, _| { cmp::Ordering::Equal }); // Ordering doesnt matter if they used our pubkey... - - let mut outputs: Vec = Vec::new(); - for out in txouts.drain(..) { - outputs.push(out.0); - } + assert!(self.shutdown_scriptpubkey.is_some()); + let holder_shutdown_script = self.get_closing_scriptpubkey(); + let counterparty_shutdown_script = self.counterparty_shutdown_scriptpubkey.clone().unwrap(); + let funding_outpoint = self.funding_outpoint().into_bitcoin_outpoint(); - (Transaction { - version: 2, - lock_time: 0, - input: txins, - output: outputs, - }, total_fee_satoshis) + let closing_transaction = ClosingTransaction::new(value_to_holder as u64, value_to_counterparty as u64, holder_shutdown_script, counterparty_shutdown_script, funding_outpoint); + (closing_transaction, total_fee_satoshis) } fn funding_outpoint(&self) -> OutPoint { @@ -1622,11 +1617,11 @@ impl Channel { if msg.max_accepted_htlcs < config.peer_channel_config_limits.min_max_accepted_htlcs { return Err(ChannelError::Close(format!("max_accepted_htlcs ({}) is less than the user specified limit ({})", msg.max_accepted_htlcs, config.peer_channel_config_limits.min_max_accepted_htlcs))); } - if msg.dust_limit_satoshis < MIN_DUST_LIMIT_SATOSHIS { - return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_DUST_LIMIT_SATOSHIS))); + if msg.dust_limit_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS { + return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is less than the implementation limit ({})", msg.dust_limit_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS))); } - if msg.dust_limit_satoshis > MAX_DUST_LIMIT_SATOSHIS { - return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.dust_limit_satoshis, MAX_DUST_LIMIT_SATOSHIS))); + if msg.dust_limit_satoshis > MAX_CHAN_DUST_LIMIT_SATOSHIS { + return Err(ChannelError::Close(format!("dust_limit_satoshis ({}) is greater than the implementation limit ({})", msg.dust_limit_satoshis, MAX_CHAN_DUST_LIMIT_SATOSHIS))); } if msg.minimum_depth > config.peer_channel_config_limits.max_minimum_depth { return Err(ChannelError::Close(format!("We consider the minimum depth to be unreasonably large. Expected minimum: ({}). Actual: ({})", config.peer_channel_config_limits.max_minimum_depth, msg.minimum_depth))); @@ -1645,10 +1640,10 @@ impl Channel { if script.len() == 0 { None } else { - match ShutdownScript::try_from((script.clone(), their_features)) { - Ok(shutdown_script) => Some(shutdown_script.into_inner()), - Err(_) => return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: {}", script))), + if !script::is_bolt2_compliant(&script, their_features) { + return Err(ChannelError::Close(format!("Peer is signaling upfront_shutdown but has provided an unacceptable scriptpubkey format: {}", script))); } + Some(script.clone()) } }, // Peer is signaling upfront shutdown but don't opt-out with correct mechanism (a.k.a 0-length script). Peer looks buggy, we fail the channel @@ -1765,6 +1760,9 @@ impl Channel { self.counterparty_funding_pubkey() ); + self.holder_signer.validate_holder_commitment(&holder_commitment_tx) + .map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?; + // Now that we're past error-generating stuff, update our local state: let funding_redeemscript = self.get_funding_redeemscript(); @@ -1839,6 +1837,9 @@ impl Channel { self.counterparty_funding_pubkey() ); + self.holder_signer.validate_holder_commitment(&holder_commitment_tx) + .map_err(|_| ChannelError::Close("Failed to validate our commitment".to_owned()))?; + let funding_redeemscript = self.get_funding_redeemscript(); let funding_txo = self.get_funding_txo().unwrap(); @@ -2166,7 +2167,7 @@ impl Channel { // We can't accept HTLCs sent after we've sent a shutdown. let local_sent_shutdown = (self.channel_state & (ChannelState::ChannelFunded as u32 | ChannelState::LocalShutdownSent as u32)) != (ChannelState::ChannelFunded as u32); if local_sent_shutdown { - pending_forward_status = create_pending_htlc_status(self, pending_forward_status, 0x1000|20); + pending_forward_status = create_pending_htlc_status(self, pending_forward_status, 0x4000|8); } // If the remote has sent a shutdown prior to adding this HTLC, then they are in violation of the spec. let remote_sent_shutdown = (self.channel_state & (ChannelState::ChannelFunded as u32 | ChannelState::RemoteShutdownSent as u32)) != (ChannelState::ChannelFunded as u32); @@ -2373,9 +2374,8 @@ impl Channel { Ok(()) } - pub fn commitment_signed(&mut self, msg: &msgs::CommitmentSigned, fee_estimator: &F, logger: &L) -> Result<(msgs::RevokeAndACK, Option, Option, ChannelMonitorUpdate), (Option, ChannelError)> - where F::Target: FeeEstimator, - L::Target: Logger + pub fn commitment_signed(&mut self, msg: &msgs::CommitmentSigned, logger: &L) -> Result<(msgs::RevokeAndACK, Option, ChannelMonitorUpdate), (Option, ChannelError)> + where L::Target: Logger { if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) { return Err((None, ChannelError::Close("Got commitment signed message when channel was not in an operational state".to_owned()))); @@ -2477,6 +2477,8 @@ impl Channel { ); let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number - 1, &self.secp_ctx); + self.holder_signer.validate_holder_commitment(&holder_commitment_tx) + .map_err(|_| (None, ChannelError::Close("Failed to validate our commitment".to_owned())))?; let per_commitment_secret = self.holder_signer.release_commitment_secret(self.cur_holder_commitment_transaction_number + 1); // Update state now that we've passed all the can-fail calls... @@ -2541,12 +2543,10 @@ impl Channel { } log_debug!(logger, "Received valid commitment_signed from peer in channel {}, updated HTLC state but awaiting a monitor update resolution to reply.", log_bytes!(self.channel_id)); - // TODO: Call maybe_propose_first_closing_signed on restoration (or call it here and - // re-send the message on restoration) return Err((Some(monitor_update), ChannelError::Ignore("Previous monitor update failure prevented generation of RAA".to_owned()))); } - let (commitment_signed, closing_signed) = if need_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 { + let commitment_signed = if need_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 { // If we're AwaitingRemoteRevoke we can't send a new commitment here, but that's ok - // we'll send one right away when we get the revoke_and_ack when we // free_holding_cell_htlcs(). @@ -2555,10 +2555,8 @@ impl Channel { // strictly increasing by one, so decrement it here. self.latest_monitor_update_id = monitor_update.update_id; monitor_update.updates.append(&mut additional_update.updates); - (Some(msg), None) - } else if !need_commitment { - (None, self.maybe_propose_first_closing_signed(fee_estimator)) - } else { (None, None) }; + Some(msg) + } else { None }; log_debug!(logger, "Received valid commitment_signed from peer in channel {}, updating HTLC state and responding with{} a revoke_and_ack.", log_bytes!(self.channel_id()), if commitment_signed.is_some() { " our own commitment_signed and" } else { "" }); @@ -2567,7 +2565,7 @@ impl Channel { channel_id: self.channel_id, per_commitment_secret, next_per_commitment_point, - }, commitment_signed, closing_signed, monitor_update)) + }, commitment_signed, monitor_update)) } /// Public version of the below, checking relevant preconditions first. @@ -2704,9 +2702,8 @@ impl Channel { /// waiting on this revoke_and_ack. The generation of this new commitment_signed may also fail, /// generating an appropriate error *after* the channel state has been updated based on the /// revoke_and_ack message. - pub fn revoke_and_ack(&mut self, msg: &msgs::RevokeAndACK, fee_estimator: &F, logger: &L) -> Result<(Option, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, Option, ChannelMonitorUpdate, Vec<(HTLCSource, PaymentHash)>), ChannelError> - where F::Target: FeeEstimator, - L::Target: Logger, + pub fn revoke_and_ack(&mut self, msg: &msgs::RevokeAndACK, logger: &L) -> Result<(Option, Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, ChannelMonitorUpdate, Vec<(HTLCSource, PaymentHash)>), ChannelError> + where L::Target: Logger, { if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) { return Err(ChannelError::Close("Got revoke/ACK message when channel was not in an operational state".to_owned())); @@ -2718,8 +2715,10 @@ impl Channel { return Err(ChannelError::Close("Peer sent revoke_and_ack after we'd started exchanging closing_signeds".to_owned())); } + let secret = secp_check!(SecretKey::from_slice(&msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret".to_owned()); + if let Some(counterparty_prev_commitment_point) = self.counterparty_prev_commitment_point { - if PublicKey::from_secret_key(&self.secp_ctx, &secp_check!(SecretKey::from_slice(&msg.per_commitment_secret), "Peer provided an invalid per_commitment_secret".to_owned())) != counterparty_prev_commitment_point { + if PublicKey::from_secret_key(&self.secp_ctx, &secret) != counterparty_prev_commitment_point { return Err(ChannelError::Close("Got a revoke commitment secret which didn't correspond to their current pubkey".to_owned())); } } @@ -2741,6 +2740,11 @@ impl Channel { *self.next_remote_commitment_tx_fee_info_cached.lock().unwrap() = None; } + self.holder_signer.validate_counterparty_revocation( + self.cur_counterparty_commitment_transaction_number + 1, + &secret + ).map_err(|_| ChannelError::Close("Failed to validate revocation from peer".to_owned()))?; + self.commitment_secrets.provide_secret(self.cur_counterparty_commitment_transaction_number + 1, msg.per_commitment_secret) .map_err(|_| ChannelError::Close("Previous secrets did not match new one".to_owned()))?; self.latest_monitor_update_id += 1; @@ -2887,7 +2891,7 @@ impl Channel { self.monitor_pending_forwards.append(&mut to_forward_infos); self.monitor_pending_failures.append(&mut revoked_htlcs); log_debug!(logger, "Received a valid revoke_and_ack for channel {} but awaiting a monitor update resolution to reply.", log_bytes!(self.channel_id())); - return Ok((None, Vec::new(), Vec::new(), None, monitor_update, Vec::new())) + return Ok((None, Vec::new(), Vec::new(), monitor_update, Vec::new())) } match self.free_holding_cell_htlcs(logger)? { @@ -2906,7 +2910,7 @@ impl Channel { self.latest_monitor_update_id = monitor_update.update_id; monitor_update.updates.append(&mut additional_update.updates); - Ok((Some(commitment_update), to_forward_infos, revoked_htlcs, None, monitor_update, htlcs_to_fail)) + Ok((Some(commitment_update), to_forward_infos, revoked_htlcs, monitor_update, htlcs_to_fail)) }, (None, htlcs_to_fail) => { if require_commitment { @@ -2926,14 +2930,13 @@ impl Channel { update_fail_malformed_htlcs, update_fee: None, commitment_signed - }), to_forward_infos, revoked_htlcs, None, monitor_update, htlcs_to_fail)) + }), to_forward_infos, revoked_htlcs, monitor_update, htlcs_to_fail)) } else { log_debug!(logger, "Received a valid revoke_and_ack for channel {} with no reply necessary.", log_bytes!(self.channel_id())); - Ok((None, to_forward_infos, revoked_htlcs, self.maybe_propose_first_closing_signed(fee_estimator), monitor_update, htlcs_to_fail)) + Ok((None, to_forward_infos, revoked_htlcs, monitor_update, htlcs_to_fail)) } } } - } /// Adds a pending update to this channel. See the doc for send_htlc for @@ -2988,6 +2991,8 @@ impl Channel { // Upon reconnect we have to start the closing_signed dance over, but shutdown messages // will be retransmitted. self.last_sent_closing_fee = None; + self.pending_counterparty_closing_signed = None; + self.closing_fee_limits = None; let mut inbound_drop_count = 0; self.pending_inbound_htlcs.retain(|htlc| { @@ -3044,13 +3049,10 @@ impl Channel { /// monitor update failure must *not* have been sent to the remote end, and must instead /// have been dropped. They will be regenerated when monitor_updating_restored is called. pub fn monitor_update_failed(&mut self, resend_raa: bool, resend_commitment: bool, mut pending_forwards: Vec<(PendingHTLCInfo, u64)>, mut pending_fails: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>) { - assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, 0); - self.monitor_pending_revoke_and_ack = resend_raa; - self.monitor_pending_commitment_signed = resend_commitment; - assert!(self.monitor_pending_forwards.is_empty()); - mem::swap(&mut pending_forwards, &mut self.monitor_pending_forwards); - assert!(self.monitor_pending_failures.is_empty()); - mem::swap(&mut pending_fails, &mut self.monitor_pending_failures); + self.monitor_pending_revoke_and_ack |= resend_raa; + self.monitor_pending_commitment_signed |= resend_commitment; + self.monitor_pending_forwards.append(&mut pending_forwards); + self.monitor_pending_failures.append(&mut pending_fails); self.channel_state |= ChannelState::MonitorUpdateFailed as u32; } @@ -3355,44 +3357,125 @@ impl Channel { } } - fn maybe_propose_first_closing_signed(&mut self, fee_estimator: &F) -> Option + /// Calculates and returns our minimum and maximum closing transaction fee amounts, in whole + /// satoshis. The amounts remain consistent unless a peer disconnects/reconnects or we restart, + /// at which point they will be recalculated. + fn calculate_closing_fee_limits(&mut self, fee_estimator: &F) -> (u64, u64) where F::Target: FeeEstimator { - if !self.is_outbound() || !self.pending_inbound_htlcs.is_empty() || !self.pending_outbound_htlcs.is_empty() || - self.channel_state & (BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32) != BOTH_SIDES_SHUTDOWN_MASK || - self.last_sent_closing_fee.is_some() || self.pending_update_fee.is_some() { - return None; - } + if let Some((min, max)) = self.closing_fee_limits { return (min, max); } + // Propose a range from our current Background feerate to our Normal feerate plus our + // force_close_avoidance_max_fee_satoshis. + // If we fail to come to consensus, we'll have to force-close. let mut proposed_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background); - if self.feerate_per_kw > proposed_feerate { - proposed_feerate = self.feerate_per_kw; - } - assert!(self.shutdown_scriptpubkey.is_some()); + let normal_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal); + let mut proposed_max_feerate = if self.is_outbound() { normal_feerate } else { u32::max_value() }; + + // The spec requires that (when the channel does not have anchors) we only send absolute + // channel fees no greater than the absolute channel fee on the current commitment + // transaction. It's unclear *which* commitment transaction this refers to, and there isn't + // very good reason to apply such a limit in any case. We don't bother doing so, risking + // some force-closure by old nodes, but we wanted to close the channel anyway. + + if let Some(target_feerate) = self.target_closing_feerate_sats_per_kw { + let min_feerate = if self.is_outbound() { target_feerate } else { cmp::min(self.feerate_per_kw, target_feerate) }; + proposed_feerate = cmp::max(proposed_feerate, min_feerate); + proposed_max_feerate = cmp::max(proposed_max_feerate, min_feerate); + } + + // Note that technically we could end up with a lower minimum fee if one sides' balance is + // below our dust limit, causing the output to disappear. We don't bother handling this + // case, however, as this should only happen if a channel is closed before any (material) + // payments have been made on it. This may cause slight fee overpayment and/or failure to + // come to consensus with our counterparty on appropriate fees, however it should be a + // relatively rare case. We can revisit this later, though note that in order to determine + // if the funders' output is dust we have to know the absolute fee we're going to use. let tx_weight = self.get_closing_transaction_weight(Some(&self.get_closing_scriptpubkey()), Some(self.counterparty_shutdown_scriptpubkey.as_ref().unwrap())); let proposed_total_fee_satoshis = proposed_feerate as u64 * tx_weight / 1000; + let proposed_max_total_fee_satoshis = if self.is_outbound() { + // We always add force_close_avoidance_max_fee_satoshis to our normal + // feerate-calculated fee, but allow the max to be overridden if we're using a + // target feerate-calculated fee. + cmp::max(normal_feerate as u64 * tx_weight / 1000 + self.config.force_close_avoidance_max_fee_satoshis, + proposed_max_feerate as u64 * tx_weight / 1000) + } else { + self.channel_value_satoshis - (self.value_to_self_msat + 999) / 1000 + }; + + self.closing_fee_limits = Some((proposed_total_fee_satoshis, proposed_max_total_fee_satoshis)); + self.closing_fee_limits.clone().unwrap() + } + + /// Returns true if we're ready to commence the closing_signed negotiation phase. This is true + /// after both sides have exchanged a `shutdown` message and all HTLCs have been drained. At + /// this point if we're the funder we should send the initial closing_signed, and in any case + /// shutdown should complete within a reasonable timeframe. + fn closing_negotiation_ready(&self) -> bool { + self.pending_inbound_htlcs.is_empty() && self.pending_outbound_htlcs.is_empty() && + self.channel_state & + (BOTH_SIDES_SHUTDOWN_MASK | ChannelState::AwaitingRemoteRevoke as u32 | + ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateFailed as u32) + == BOTH_SIDES_SHUTDOWN_MASK && + self.pending_update_fee.is_none() + } + + /// Checks if the closing_signed negotiation is making appropriate progress, possibly returning + /// an Err if no progress is being made and the channel should be force-closed instead. + /// Should be called on a one-minute timer. + pub fn timer_check_closing_negotiation_progress(&mut self) -> Result<(), ChannelError> { + if self.closing_negotiation_ready() { + if self.closing_signed_in_flight { + return Err(ChannelError::Close("closing_signed negotiation failed to finish within two timer ticks".to_owned())); + } else { + self.closing_signed_in_flight = true; + } + } + Ok(()) + } + + pub fn maybe_propose_closing_signed(&mut self, fee_estimator: &F, logger: &L) + -> Result<(Option, Option), ChannelError> + where F::Target: FeeEstimator, L::Target: Logger + { + if self.last_sent_closing_fee.is_some() || !self.closing_negotiation_ready() { + return Ok((None, None)); + } + + if !self.is_outbound() { + if let Some(msg) = &self.pending_counterparty_closing_signed.take() { + return self.closing_signed(fee_estimator, &msg); + } + return Ok((None, None)); + } + + let (our_min_fee, our_max_fee) = self.calculate_closing_fee_limits(fee_estimator); + + assert!(self.shutdown_scriptpubkey.is_some()); + let (closing_tx, total_fee_satoshis) = self.build_closing_transaction(our_min_fee, false); + log_trace!(logger, "Proposing initial closing_signed for our counterparty with a fee range of {}-{} sat (with initial proposal {} sats)", + our_min_fee, our_max_fee, total_fee_satoshis); - let (closing_tx, total_fee_satoshis) = self.build_closing_transaction(proposed_total_fee_satoshis, false); let sig = self.holder_signer .sign_closing_transaction(&closing_tx, &self.secp_ctx) - .ok(); - assert!(closing_tx.get_weight() as u64 <= tx_weight); - if sig.is_none() { return None; } + .map_err(|()| ChannelError::Close("Failed to get signature for closing transaction.".to_owned()))?; - self.last_sent_closing_fee = Some((proposed_feerate, total_fee_satoshis, sig.clone().unwrap())); - Some(msgs::ClosingSigned { + self.last_sent_closing_fee = Some((total_fee_satoshis, sig.clone())); + Ok((Some(msgs::ClosingSigned { channel_id: self.channel_id, fee_satoshis: total_fee_satoshis, - signature: sig.unwrap(), - }) + signature: sig, + fee_range: Some(msgs::ClosingSignedFeeRange { + min_fee_satoshis: our_min_fee, + max_fee_satoshis: our_max_fee, + }), + }), None)) } - pub fn shutdown( - &mut self, fee_estimator: &F, keys_provider: &K, their_features: &InitFeatures, msg: &msgs::Shutdown - ) -> Result<(Option, Option, Option, Vec<(HTLCSource, PaymentHash)>), ChannelError> - where - F::Target: FeeEstimator, - K::Target: KeysInterface + pub fn shutdown( + &mut self, keys_provider: &K, their_features: &InitFeatures, msg: &msgs::Shutdown + ) -> Result<(Option, Option, Vec<(HTLCSource, PaymentHash)>), ChannelError> + where K::Target: KeysInterface { if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 { return Err(ChannelError::Close("Peer sent shutdown when we needed a channel_reestablish".to_owned())); @@ -3410,17 +3493,16 @@ impl Channel { } assert_eq!(self.channel_state & ChannelState::ShutdownComplete as u32, 0); - let shutdown_scriptpubkey = match ShutdownScript::try_from((msg.scriptpubkey.clone(), their_features)) { - Ok(script) => script.into_inner(), - Err(_) => return Err(ChannelError::Close(format!("Got a nonstandard scriptpubkey ({}) from remote peer", msg.scriptpubkey.to_bytes().to_hex()))), - }; + if !script::is_bolt2_compliant(&msg.scriptpubkey, their_features) { + return Err(ChannelError::Close(format!("Got a nonstandard scriptpubkey ({}) from remote peer", msg.scriptpubkey.to_bytes().to_hex()))); + } if self.counterparty_shutdown_scriptpubkey.is_some() { - if Some(&shutdown_scriptpubkey) != self.counterparty_shutdown_scriptpubkey.as_ref() { - return Err(ChannelError::Close(format!("Got shutdown request with a scriptpubkey ({}) which did not match their previous scriptpubkey.", shutdown_scriptpubkey.to_bytes().to_hex()))); + if Some(&msg.scriptpubkey) != self.counterparty_shutdown_scriptpubkey.as_ref() { + return Err(ChannelError::Close(format!("Got shutdown request with a scriptpubkey ({}) which did not match their previous scriptpubkey.", msg.scriptpubkey.to_bytes().to_hex()))); } } else { - self.counterparty_shutdown_scriptpubkey = Some(shutdown_scriptpubkey); + self.counterparty_shutdown_scriptpubkey = Some(msg.scriptpubkey.clone()); } // If we have any LocalAnnounced updates we'll probably just get back an update_fail_htlc @@ -3480,13 +3562,11 @@ impl Channel { self.channel_state |= ChannelState::LocalShutdownSent as u32; self.update_time_counter += 1; - Ok((shutdown, self.maybe_propose_first_closing_signed(fee_estimator), monitor_update, dropped_outbound_htlcs)) + Ok((shutdown, monitor_update, dropped_outbound_htlcs)) } - fn build_signed_closing_transaction(&self, tx: &mut Transaction, counterparty_sig: &Signature, sig: &Signature) { - if tx.input.len() != 1 { panic!("Tried to sign closing transaction that had input count != 1!"); } - if tx.input[0].witness.len() != 0 { panic!("Tried to re-sign closing transaction"); } - if tx.output.len() > 2 { panic!("Tried to sign bogus closing transaction"); } + fn build_signed_closing_transaction(&self, closing_tx: &ClosingTransaction, counterparty_sig: &Signature, sig: &Signature) -> Transaction { + let mut tx = closing_tx.trust().built_transaction().clone(); tx.input[0].witness.push(Vec::new()); // First is the multisig dummy @@ -3503,6 +3583,7 @@ impl Channel { tx.input[0].witness[2].push(SigHashType::All as u8); tx.input[0].witness.push(self.get_funding_redeemscript().into_bytes()); + tx } pub fn closing_signed(&mut self, fee_estimator: &F, msg: &msgs::ClosingSigned) -> Result<(Option, Option), ChannelError> @@ -3521,12 +3602,21 @@ impl Channel { return Err(ChannelError::Close("Remote tried to send us a closing tx with > 21 million BTC fee".to_owned())); } + if self.is_outbound() && self.last_sent_closing_fee.is_none() { + return Err(ChannelError::Close("Remote tried to send a closing_signed when we were supposed to propose the first one".to_owned())); + } + + if self.channel_state & ChannelState::MonitorUpdateFailed as u32 != 0 { + self.pending_counterparty_closing_signed = Some(msg.clone()); + return Ok((None, None)); + } + let funding_redeemscript = self.get_funding_redeemscript(); let (mut closing_tx, used_total_fee) = self.build_closing_transaction(msg.fee_satoshis, false); if used_total_fee != msg.fee_satoshis { - return Err(ChannelError::Close(format!("Remote sent us a closing_signed with a fee greater than the value they can claim. Fee in message: {}", msg.fee_satoshis))); + return Err(ChannelError::Close(format!("Remote sent us a closing_signed with a fee other than the value they can claim. Fee in message: {}. Actual closing tx fee: {}", msg.fee_satoshis, used_total_fee))); } - let mut sighash = hash_to_message!(&bip143::SigHashCache::new(&closing_tx).signature_hash(0, &funding_redeemscript, self.channel_value_satoshis, SigHashType::All)[..]); + let sighash = closing_tx.trust().get_sighash_all(&funding_redeemscript, self.channel_value_satoshis); match self.secp_ctx.verify(&sighash, &msg.signature, &self.get_counterparty_pubkeys().funding_pubkey) { Ok(_) => {}, @@ -3534,81 +3624,116 @@ impl Channel { // The remote end may have decided to revoke their output due to inconsistent dust // limits, so check for that case by re-checking the signature here. closing_tx = self.build_closing_transaction(msg.fee_satoshis, true).0; - sighash = hash_to_message!(&bip143::SigHashCache::new(&closing_tx).signature_hash(0, &funding_redeemscript, self.channel_value_satoshis, SigHashType::All)[..]); + let sighash = closing_tx.trust().get_sighash_all(&funding_redeemscript, self.channel_value_satoshis); secp_check!(self.secp_ctx.verify(&sighash, &msg.signature, self.counterparty_funding_pubkey()), "Invalid closing tx signature from peer".to_owned()); }, }; - let closing_tx_max_weight = self.get_closing_transaction_weight( - if let Some(oup) = closing_tx.output.get(0) { Some(&oup.script_pubkey) } else { None }, - if let Some(oup) = closing_tx.output.get(1) { Some(&oup.script_pubkey) } else { None }); - if let Some((_, last_fee, sig)) = self.last_sent_closing_fee { + for outp in closing_tx.trust().built_transaction().output.iter() { + if !outp.script_pubkey.is_witness_program() && outp.value < MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS { + return Err(ChannelError::Close("Remote sent us a closing_signed with a dust output. Always use segwit closing scripts!".to_owned())); + } + } + + assert!(self.shutdown_scriptpubkey.is_some()); + if let Some((last_fee, sig)) = self.last_sent_closing_fee { if last_fee == msg.fee_satoshis { - self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig); - assert!(closing_tx.get_weight() as u64 <= closing_tx_max_weight); - debug_assert!(closing_tx.get_weight() as u64 >= closing_tx_max_weight - 2); + let tx = self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig); self.channel_state = ChannelState::ShutdownComplete as u32; self.update_time_counter += 1; - return Ok((None, Some(closing_tx))); + return Ok((None, Some(tx))); } } - macro_rules! propose_new_feerate { - ($new_feerate: expr) => { - assert!(self.shutdown_scriptpubkey.is_some()); - let tx_weight = self.get_closing_transaction_weight(Some(&self.get_closing_scriptpubkey()), Some(self.counterparty_shutdown_scriptpubkey.as_ref().unwrap())); - let (closing_tx, used_total_fee) = self.build_closing_transaction($new_feerate as u64 * tx_weight / 1000, false); + let (our_min_fee, our_max_fee) = self.calculate_closing_fee_limits(fee_estimator); + + macro_rules! propose_fee { + ($new_fee: expr) => { + let (closing_tx, used_fee) = if $new_fee == msg.fee_satoshis { + (closing_tx, $new_fee) + } else { + self.build_closing_transaction($new_fee, false) + }; + let sig = self.holder_signer .sign_closing_transaction(&closing_tx, &self.secp_ctx) .map_err(|_| ChannelError::Close("External signer refused to sign closing transaction".to_owned()))?; - assert!(closing_tx.get_weight() as u64 <= tx_weight); - self.last_sent_closing_fee = Some(($new_feerate, used_total_fee, sig.clone())); + + let signed_tx = if $new_fee == msg.fee_satoshis { + self.channel_state = ChannelState::ShutdownComplete as u32; + self.update_time_counter += 1; + let tx = self.build_signed_closing_transaction(&closing_tx, &msg.signature, &sig); + Some(tx) + } else { None }; + + self.last_sent_closing_fee = Some((used_fee, sig.clone())); return Ok((Some(msgs::ClosingSigned { channel_id: self.channel_id, - fee_satoshis: used_total_fee, + fee_satoshis: used_fee, signature: sig, - }), None)) + fee_range: Some(msgs::ClosingSignedFeeRange { + min_fee_satoshis: our_min_fee, + max_fee_satoshis: our_max_fee, + }), + }), signed_tx)) } } - let mut min_feerate = 253; - if self.is_outbound() { - let max_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal); - if (msg.fee_satoshis as u64) > max_feerate as u64 * closing_tx_max_weight / 1000 { - if let Some((last_feerate, _, _)) = self.last_sent_closing_fee { - if max_feerate <= last_feerate { - return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wanted something higher ({}) than our Normal feerate ({})", last_feerate, max_feerate))); - } + if let Some(msgs::ClosingSignedFeeRange { min_fee_satoshis, max_fee_satoshis }) = msg.fee_range { + if msg.fee_satoshis < min_fee_satoshis || msg.fee_satoshis > max_fee_satoshis { + return Err(ChannelError::Close(format!("Peer sent a bogus closing_signed - suggested fee of {} sat was not in their desired range of {} sat - {} sat", msg.fee_satoshis, min_fee_satoshis, max_fee_satoshis))); + } + if max_fee_satoshis < our_min_fee { + return Err(ChannelError::Warn(format!("Unable to come to consensus about closing feerate, remote's max fee ({} sat) was smaller than our min fee ({} sat)", max_fee_satoshis, our_min_fee))); + } + if min_fee_satoshis > our_max_fee { + return Err(ChannelError::Warn(format!("Unable to come to consensus about closing feerate, remote's min fee ({} sat) was greater than our max fee ({} sat)", min_fee_satoshis, our_max_fee))); + } + + if !self.is_outbound() { + // They have to pay, so pick the highest fee in the overlapping range. + // We should never set an upper bound aside from their full balance + debug_assert_eq!(our_max_fee, self.channel_value_satoshis - (self.value_to_self_msat + 999) / 1000); + propose_fee!(cmp::min(max_fee_satoshis, our_max_fee)); + } else { + if msg.fee_satoshis < our_min_fee || msg.fee_satoshis > our_max_fee { + return Err(ChannelError::Close(format!("Peer sent a bogus closing_signed - suggested fee of {} sat was not in our desired range of {} sat - {} sat after we informed them of our range.", + msg.fee_satoshis, our_min_fee, our_max_fee))); } - propose_new_feerate!(max_feerate); + // The proposed fee is in our acceptable range, accept it and broadcast! + propose_fee!(msg.fee_satoshis); } } else { - min_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background); - } - if (msg.fee_satoshis as u64) < min_feerate as u64 * closing_tx_max_weight / 1000 { - if let Some((last_feerate, _, _)) = self.last_sent_closing_fee { - if min_feerate >= last_feerate { - return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wanted something lower ({}) than our Background feerate ({}).", last_feerate, min_feerate))); + // Old fee style negotiation. We don't bother to enforce whether they are complying + // with the "making progress" requirements, we just comply and hope for the best. + if let Some((last_fee, _)) = self.last_sent_closing_fee { + if msg.fee_satoshis > last_fee { + if msg.fee_satoshis < our_max_fee { + propose_fee!(msg.fee_satoshis); + } else if last_fee < our_max_fee { + propose_fee!(our_max_fee); + } else { + return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wants something ({} sat) higher than our max fee ({} sat)", msg.fee_satoshis, our_max_fee))); + } + } else { + if msg.fee_satoshis > our_min_fee { + propose_fee!(msg.fee_satoshis); + } else if last_fee > our_min_fee { + propose_fee!(our_min_fee); + } else { + return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wants something ({} sat) lower than our min fee ({} sat)", msg.fee_satoshis, our_min_fee))); + } + } + } else { + if msg.fee_satoshis < our_min_fee { + propose_fee!(our_min_fee); + } else if msg.fee_satoshis > our_max_fee { + propose_fee!(our_max_fee); + } else { + propose_fee!(msg.fee_satoshis); } } - propose_new_feerate!(min_feerate); } - - let sig = self.holder_signer - .sign_closing_transaction(&closing_tx, &self.secp_ctx) - .map_err(|_| ChannelError::Close("External signer refused to sign closing transaction".to_owned()))?; - self.build_signed_closing_transaction(&mut closing_tx, &msg.signature, &sig); - assert!(closing_tx.get_weight() as u64 <= closing_tx_max_weight); - debug_assert!(closing_tx.get_weight() as u64 >= closing_tx_max_weight - 2); - - self.channel_state = ChannelState::ShutdownComplete as u32; - self.update_time_counter += 1; - - Ok((Some(msgs::ClosingSigned { - channel_id: self.channel_id, - fee_satoshis: msg.fee_satoshis, - signature: sig, - }), Some(closing_tx))) } // Public utilities: @@ -3712,6 +3837,8 @@ impl Channel { // more dust balance if the feerate increases when we have several HTLCs pending // which are near the dust limit. let mut feerate_per_kw = self.feerate_per_kw; + // If there's a pending update fee, use it to ensure we aren't under-estimating + // potential feerate updates coming soon. if let Some((feerate, _)) = self.pending_update_fee { feerate_per_kw = cmp::max(feerate_per_kw, feerate); } @@ -3813,6 +3940,16 @@ impl Channel { self.channel_state >= ChannelState::FundingSent as u32 } + /// Returns true if our peer has either initiated or agreed to shut down the channel. + pub fn received_shutdown(&self) -> bool { + (self.channel_state & ChannelState::RemoteShutdownSent as u32) != 0 + } + + /// Returns true if we either initiated or agreed to shut down the channel. + pub fn sent_shutdown(&self) -> bool { + (self.channel_state & ChannelState::LocalShutdownSent as u32) != 0 + } + /// Returns true if this channel is fully shut down. True here implies that no further actions /// may/will be taken on this channel, and thus this object should be freed. Any future changes /// will be handled appropriately by the chain monitor. @@ -4641,7 +4778,8 @@ impl Channel { /// Begins the shutdown process, getting a message for the remote peer and returning all /// holding cell HTLCs for payment failure. - pub fn get_shutdown(&mut self, keys_provider: &K, their_features: &InitFeatures) -> Result<(msgs::Shutdown, Option, Vec<(HTLCSource, PaymentHash)>), APIError> + pub fn get_shutdown(&mut self, keys_provider: &K, their_features: &InitFeatures, target_feerate_sats_per_kw: Option) + -> Result<(msgs::Shutdown, Option, Vec<(HTLCSource, PaymentHash)>), APIError> where K::Target: KeysInterface { for htlc in self.pending_outbound_htlcs.iter() { if let OutboundHTLCState::LocalAnnounced(_) = htlc.state { @@ -4674,6 +4812,7 @@ impl Channel { }; // From here on out, we may not fail! + self.target_closing_feerate_sats_per_kw = target_feerate_sats_per_kw; if self.channel_state < ChannelState::FundingSent as u32 { self.channel_state = ChannelState::ShutdownComplete as u32; } else { @@ -4948,9 +5087,10 @@ impl Writeable for Channel { if self.is_outbound() { self.pending_update_fee.map(|(a, _)| a).write(writer)?; } else if let Some((feerate, FeeUpdateState::AwaitingRemoteRevokeToAnnounce)) = self.pending_update_fee { - // As for inbound HTLCs, if the update was only announced and never committed, drop it. Some(feerate).write(writer)?; } else { + // As for inbound HTLCs, if the update was only announced and never committed in a + // commitment_signed, drop it. None::.write(writer)?; } self.holding_cell_update_fee.write(writer)?; @@ -4960,15 +5100,11 @@ impl Writeable for Channel { self.update_time_counter.write(writer)?; self.feerate_per_kw.write(writer)?; - match self.last_sent_closing_fee { - Some((feerate, fee, sig)) => { - 1u8.write(writer)?; - feerate.write(writer)?; - fee.write(writer)?; - sig.write(writer)?; - }, - None => 0u8.write(writer)?, - } + // Versions prior to 0.0.100 expected to read the fields of `last_sent_closing_fee` here, + // however we are supposed to restart shutdown fee negotiation on reconnect (and wipe + // `last_send_closing_fee` in `remove_uncommitted_htlcs_and_mark_paused`) so we should never + // consider the stale state on reload. + 0u8.write(writer)?; self.funding_tx_confirmed_in.write(writer)?; self.funding_tx_confirmation_height.write(writer)?; @@ -5030,6 +5166,7 @@ impl Writeable for Channel { (3, self.counterparty_selected_channel_reserve_satoshis, option), (5, self.config, required), (7, self.shutdown_scriptpubkey, option), + (9, self.target_closing_feerate_sats_per_kw, option), }); Ok(()) @@ -5176,11 +5313,19 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel let update_time_counter = Readable::read(reader)?; let feerate_per_kw = Readable::read(reader)?; - let last_sent_closing_fee = match ::read(reader)? { - 0 => None, - 1 => Some((Readable::read(reader)?, Readable::read(reader)?, Readable::read(reader)?)), + // Versions prior to 0.0.100 expected to read the fields of `last_sent_closing_fee` here, + // however we are supposed to restart shutdown fee negotiation on reconnect (and wipe + // `last_send_closing_fee` in `remove_uncommitted_htlcs_and_mark_paused`) so we should never + // consider the stale state on reload. + match ::read(reader)? { + 0 => {}, + 1 => { + let _: u32 = Readable::read(reader)?; + let _: u64 = Readable::read(reader)?; + let _: Signature = Readable::read(reader)?; + }, _ => return Err(DecodeError::InvalidValue), - }; + } let funding_tx_confirmed_in = Readable::read(reader)?; let funding_tx_confirmation_height = Readable::read(reader)?; @@ -5254,12 +5399,14 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel }; let mut announcement_sigs = None; + let mut target_closing_feerate_sats_per_kw = None; read_tlv_fields!(reader, { (0, announcement_sigs, option), (1, minimum_depth, option), (3, counterparty_selected_channel_reserve_satoshis, option), (5, config, option), // Note that if none is provided we will *not* overwrite the existing one. (7, shutdown_scriptpubkey, option), + (9, target_closing_feerate_sats_per_kw, option), }); let mut secp_ctx = Secp256k1::new(); @@ -5308,7 +5455,10 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel #[cfg(debug_assertions)] counterparty_max_commitment_tx_output: Mutex::new((0, 0)), - last_sent_closing_fee, + last_sent_closing_fee: None, + pending_counterparty_closing_signed: None, + closing_fee_limits: None, + target_closing_feerate_sats_per_kw, funding_tx_confirmed_in, funding_tx_confirmation_height, @@ -5337,6 +5487,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel commitment_secrets, channel_update_status, + closing_signed_in_flight: false, announcement_sigs, @@ -5365,7 +5516,7 @@ mod tests { use bitcoin::hashes::hex::FromHex; use hex; use ln::{PaymentPreimage, PaymentHash}; - use ln::channelmanager::HTLCSource; + use ln::channelmanager::{HTLCSource, PaymentId}; use ln::channel::{Channel,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,HTLCCandidate,HTLCInitiator,TxCreationKeys}; use ln::channel::MAX_FUNDING_SATOSHIS; use ln::features::InitFeatures; @@ -5539,6 +5690,7 @@ mod tests { path: Vec::new(), session_priv: SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap(), first_hop_htlc_msat: 548, + payment_id: PaymentId([42; 32]), } });