Merge pull request #1047 from TheBlueMatt/2021-08-985-followups
[rust-lightning] / lightning / src / ln / channel.rs
index 0ada482f3679563b98de11a861ee9a9d9caf6080..58384f3855e6ceddd7a5a4da646a5e1454e0c45c 100644 (file)
@@ -255,8 +255,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.
@@ -424,22 +422,18 @@ pub(super) struct Channel<Signer: Sign> {
        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<u32>,
        next_holder_htlc_id: u64,
        next_counterparty_htlc_id: u64,
@@ -454,7 +448,6 @@ pub(super) struct Channel<Signer: Sign> {
        counterparty_max_commitment_tx_output: Mutex<(u64, u64)>,
 
        last_sent_closing_fee: Option<(u64, Signature)>, // (fee, holder_sig)
-       closing_fee_limits: Option<(u64, u64)>,
        target_closing_feerate_sats_per_kw: Option<u32>,
 
        /// If our counterparty sent us a closing_signed while we were waiting for a `ChannelMonitor`
@@ -462,6 +455,13 @@ pub(super) struct Channel<Signer: Sign> {
        /// closing_signed message and handling it in `maybe_propose_closing_signed`.
        pending_counterparty_closing_signed: Option<msgs::ClosingSigned>,
 
+       /// The minimum and maximum absolute fee 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<BlockHash>,
        funding_tx_confirmation_height: u32,
@@ -503,6 +503,13 @@ pub(super) struct Channel<Signer: Sign> {
        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.
@@ -556,7 +563,9 @@ 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;
 
@@ -740,6 +749,7 @@ impl<Signer: Sign> Channel<Signer> {
                        commitment_secrets: CounterpartyCommitmentSecrets::new(),
 
                        channel_update_status: ChannelUpdateStatus::Enabled,
+                       closing_signed_in_flight: false,
 
                        announcement_sigs: None,
 
@@ -1006,6 +1016,7 @@ impl<Signer: Sign> Channel<Signer> {
                        commitment_secrets: CounterpartyCommitmentSecrets::new(),
 
                        channel_update_status: ChannelUpdateStatus::Enabled,
+                       closing_signed_in_flight: false,
 
                        announcement_sigs: None,
 
@@ -1193,6 +1204,11 @@ impl<Signer: Sign> Channel<Signer> {
 
                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);
@@ -1214,6 +1230,9 @@ impl<Signer: Sign> Channel<Signer> {
                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,
@@ -1778,6 +1797,9 @@ impl<Signer: Sign> Channel<Signer> {
                        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();
@@ -1852,6 +1874,9 @@ impl<Signer: Sign> Channel<Signer> {
                        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();
@@ -2489,6 +2514,8 @@ impl<Signer: Sign> Channel<Signer> {
                );
 
                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...
@@ -2725,8 +2752,10 @@ impl<Signer: Sign> Channel<Signer> {
                        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()));
                        }
                }
@@ -2748,6 +2777,11 @@ impl<Signer: Sign> Channel<Signer> {
                        *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;
@@ -3413,16 +3447,38 @@ impl<Signer: Sign> Channel<Signer> {
                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<F: Deref, L: Deref>(&mut self, fee_estimator: &F, logger: &L)
                -> Result<(Option<msgs::ClosingSigned>, Option<Transaction>), ChannelError>
                where F::Target: FeeEstimator, L::Target: Logger
        {
-               if !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.last_sent_closing_fee.is_some() || self.pending_update_fee.is_some() {
+               if self.last_sent_closing_fee.is_some() || !self.closing_negotiation_ready() {
                        return Ok((None, None));
                }
 
@@ -3600,7 +3656,7 @@ impl<Signer: Sign> Channel<Signer> {
                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)[..]);
 
@@ -3816,6 +3872,8 @@ impl<Signer: Sign> Channel<Signer> {
                // 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);
                }
@@ -5064,9 +5122,10 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
                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::<u32>.write(writer)?;
                }
                self.holding_cell_update_fee.write(writer)?;
@@ -5463,6 +5522,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
                        commitment_secrets,
 
                        channel_update_status,
+                       closing_signed_in_flight: false,
 
                        announcement_sigs,