Add message ordering return value to handling channel_reestablish
[rust-lightning] / src / ln / channel.rs
index 2dc7750e472be7d02a1a85fc2d109cb3a7031a67..c53cf2520d6c79a29e5d4144ebb5124e7776a99d 100644 (file)
@@ -14,7 +14,7 @@ use crypto::digest::Digest;
 use crypto::hkdf::{hkdf_extract,hkdf_expand};
 
 use ln::msgs;
-use ln::msgs::{ErrorAction, HandleError};
+use ln::msgs::{ErrorAction, HandleError, RAACommitmentOrder};
 use ln::channelmonitor::ChannelMonitor;
 use ln::channelmanager::{PendingHTLCStatus, HTLCSource, PendingForwardHTLCInfo, HTLCFailReason, HTLCFailureMsg};
 use ln::chan_utils::{TxCreationKeys,HTLCOutputInCommitment,HTLC_SUCCESS_TX_WEIGHT,HTLC_TIMEOUT_TX_WEIGHT};
@@ -44,6 +44,17 @@ pub struct ChannelKeys {
        pub commitment_seed: [u8; 32],
 }
 
+#[cfg(test)]
+pub struct ChannelValueStat {
+       pub value_to_self_msat: u64,
+       pub channel_value_msat: u64,
+       pub channel_reserve_msat: u64,
+       pub pending_outbound_htlcs_amount_msat: u64,
+       pub pending_inbound_htlcs_amount_msat: u64,
+       pub holding_cell_outbound_amount_msat: u64,
+       pub their_max_htlc_value_in_flight_msat: u64, // outgoing
+}
+
 impl ChannelKeys {
        pub fn new_from_seed(seed: &[u8; 32]) -> Result<ChannelKeys, secp256k1::Error> {
                let mut prk = [0; 32];
@@ -87,22 +98,27 @@ impl ChannelKeys {
        }
 }
 
-#[derive(PartialEq)]
+enum InboundHTLCRemovalReason {
+       FailRelay(msgs::OnionErrorPacket),
+       FailMalformed(([u8; 32], u16)),
+       Fulfill([u8; 32]),
+}
+
 enum InboundHTLCState {
        /// Added by remote, to be included in next local commitment tx.
-       RemoteAnnounced,
+       RemoteAnnounced(PendingHTLCStatus),
        /// Included in a received commitment_signed message (implying we've revoke_and_ack'ed it), but
        /// the remote side hasn't yet revoked their previous state, which we need them to do before we
        /// accept this HTLC. Implies AwaitingRemoteRevoke.
        /// We also have not yet included this HTLC in a commitment_signed message, and are waiting on
        /// a remote revoke_and_ack on a previous state before we can do so.
-       AwaitingRemoteRevokeToAnnounce,
+       AwaitingRemoteRevokeToAnnounce(PendingHTLCStatus),
        /// Included in a received commitment_signed message (implying we've revoke_and_ack'ed it), but
        /// the remote side hasn't yet revoked their previous state, which we need them to do before we
        /// accept this HTLC. Implies AwaitingRemoteRevoke.
        /// We have included this HTLC in our latest commitment_signed and are now just waiting on a
        /// revoke_and_ack.
-       AwaitingAnnouncedRemoteRevoke,
+       AwaitingAnnouncedRemoteRevoke(PendingHTLCStatus),
        Committed,
        /// Removed by us and a new commitment_signed was sent (if we were AwaitingRemoteRevoke when we
        /// created it we would have put it in the holding cell instead). When they next revoke_and_ack
@@ -114,7 +130,7 @@ enum InboundHTLCState {
        /// ChannelMonitor::would_broadcast_at_height) so we actually remove the HTLC from our own
        /// local state before then, once we're sure that the next commitment_signed and
        /// ChannelMonitor::provide_latest_local_commitment_tx_info will not include this HTLC.
-       LocalRemoved,
+       LocalRemoved(InboundHTLCRemovalReason),
 }
 
 struct InboundHTLCOutput {
@@ -123,13 +139,8 @@ struct InboundHTLCOutput {
        cltv_expiry: u32,
        payment_hash: [u8; 32],
        state: InboundHTLCState,
-       /// If we're in LocalRemoved, set to true if we fulfilled the HTLC, and can claim money
-       local_removed_fulfilled: bool,
-       /// state pre-Committed implies pending_forward_state, otherwise it must be None
-       pending_forward_state: Option<PendingHTLCStatus>,
 }
 
-#[derive(PartialEq)]
 enum OutboundHTLCState {
        /// Added by us and included in a commitment_signed (if we were AwaitingRemoteRevoke when we
        /// created it we would have put it in the holding cell instead). When they next revoke_and_ack
@@ -141,7 +152,9 @@ enum OutboundHTLCState {
        ///    allowed to remove it, the "can only be removed once committed on both sides" requirement
        ///    doesn't matter to us and its up to them to enforce it, worst-case they jump ahead but
        ///    we'll never get out of sync).
-       LocalAnnounced,
+       /// Note that we Box the OnionPacket as its rather large and we don't want to blow up
+       /// OutboundHTLCOutput's size just for a temporary bit
+       LocalAnnounced(Box<msgs::OnionPacket>),
        Committed,
        /// Remote removed this (outbound) HTLC. We're waiting on their commitment_signed to finalize
        /// the change (though they'll need to revoke before we fail the payment).
@@ -256,6 +269,7 @@ enum ChannelState {
        ShutdownComplete = 2048,
 }
 const BOTH_SIDES_SHUTDOWN_MASK: u32 = (ChannelState::LocalShutdownSent as u32 | ChannelState::RemoteShutdownSent as u32);
+const MULTI_STATE_FLAGS: u32 = (BOTH_SIDES_SHUTDOWN_MASK | ChannelState::PeerDisconnected as u32);
 
 const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1;
 
@@ -308,6 +322,13 @@ pub(super) struct Channel {
        channel_update_count: u32,
        feerate_per_kw: u64,
 
+       #[cfg(debug_assertions)]
+       /// Max to_local and to_remote outputs in a locally-generated commitment transaction
+       max_commitment_tx_output_local: ::std::sync::Mutex<(u64, u64)>,
+       #[cfg(debug_assertions)]
+       /// Max to_local and to_remote outputs in a remote-generated commitment transaction
+       max_commitment_tx_output_remote: ::std::sync::Mutex<(u64, u64)>,
+
        #[cfg(test)]
        // Used in ChannelManager's tests to send a revoked transaction
        pub last_local_commitment_txn: Vec<Transaction>,
@@ -480,6 +501,11 @@ impl Channel {
                        next_remote_htlc_id: 0,
                        channel_update_count: 1,
 
+                       #[cfg(debug_assertions)]
+                       max_commitment_tx_output_local: ::std::sync::Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
+                       #[cfg(debug_assertions)]
+                       max_commitment_tx_output_remote: ::std::sync::Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
+
                        last_local_commitment_txn: Vec::new(),
 
                        last_sent_closing_fee: None,
@@ -630,6 +656,11 @@ impl Channel {
                        next_remote_htlc_id: 0,
                        channel_update_count: 1,
 
+                       #[cfg(debug_assertions)]
+                       max_commitment_tx_output_local: ::std::sync::Mutex::new((msg.push_msat, msg.funding_satoshis * 1000 - msg.push_msat)),
+                       #[cfg(debug_assertions)]
+                       max_commitment_tx_output_remote: ::std::sync::Mutex::new((msg.push_msat, msg.funding_satoshis * 1000 - msg.push_msat)),
+
                        last_local_commitment_txn: Vec::new(),
 
                        last_sent_closing_fee: None,
@@ -763,21 +794,23 @@ impl Channel {
 
                for ref htlc in self.pending_inbound_htlcs.iter() {
                        let include = match htlc.state {
-                               InboundHTLCState::RemoteAnnounced => !generated_by_local,
-                               InboundHTLCState::AwaitingRemoteRevokeToAnnounce => !generated_by_local,
-                               InboundHTLCState::AwaitingAnnouncedRemoteRevoke => true,
+                               InboundHTLCState::RemoteAnnounced(_) => !generated_by_local,
+                               InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => !generated_by_local,
+                               InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => true,
                                InboundHTLCState::Committed => true,
-                               InboundHTLCState::LocalRemoved => !generated_by_local,
+                               InboundHTLCState::LocalRemoved(_) => !generated_by_local,
                        };
 
                        if include {
                                add_htlc_output!(htlc, false);
                                remote_htlc_total_msat += htlc.amount_msat;
                        } else {
-                               match htlc.state {
-                                       InboundHTLCState::LocalRemoved => {
-                                               if generated_by_local && htlc.local_removed_fulfilled {
-                                                       value_to_self_msat_offset += htlc.amount_msat as i64;
+                               match &htlc.state {
+                                       &InboundHTLCState::LocalRemoved(ref reason) => {
+                                               if generated_by_local {
+                                                       if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
+                                                               value_to_self_msat_offset += htlc.amount_msat as i64;
+                                                       }
                                                }
                                        },
                                        _ => {},
@@ -787,7 +820,7 @@ impl Channel {
 
                for ref htlc in self.pending_outbound_htlcs.iter() {
                        let include = match htlc.state {
-                               OutboundHTLCState::LocalAnnounced => generated_by_local,
+                               OutboundHTLCState::LocalAnnounced(_) => generated_by_local,
                                OutboundHTLCState::Committed => true,
                                OutboundHTLCState::RemoteRemoved => generated_by_local,
                                OutboundHTLCState::AwaitingRemoteRevokeToRemove => generated_by_local,
@@ -815,9 +848,32 @@ impl Channel {
                }
 
 
+               let value_to_self_msat: i64 = (self.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset;
+               let value_to_remote_msat: i64 = (self.channel_value_satoshis * 1000 - self.value_to_self_msat - remote_htlc_total_msat) as i64 - value_to_self_msat_offset;
+
+               #[cfg(debug_assertions)]
+               {
+                       // Make sure that the to_self/to_remote is always either past the appropriate
+                       // channel_reserve *or* it is making progress towards it.
+                       // TODO: This should happen after fee calculation, but we don't handle that correctly
+                       // yet!
+                       let mut max_commitment_tx_output = if generated_by_local {
+                               self.max_commitment_tx_output_local.lock().unwrap()
+                       } else {
+                               self.max_commitment_tx_output_remote.lock().unwrap()
+                       };
+                       debug_assert!(max_commitment_tx_output.0 <= value_to_self_msat as u64 || value_to_self_msat / 1000 >= self.their_channel_reserve_satoshis as i64);
+                       max_commitment_tx_output.0 = cmp::max(max_commitment_tx_output.0, value_to_self_msat as u64);
+                       debug_assert!(max_commitment_tx_output.1 <= value_to_remote_msat as u64 || value_to_remote_msat / 1000 >= Channel::get_our_channel_reserve_satoshis(self.channel_value_satoshis) as i64);
+                       max_commitment_tx_output.1 = cmp::max(max_commitment_tx_output.1, value_to_remote_msat as u64);
+               }
+
                let total_fee: u64 = feerate_per_kw * (COMMITMENT_TX_BASE_WEIGHT + (txouts.len() as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
-               let value_to_self: i64 = ((self.value_to_self_msat - local_htlc_total_msat) as i64 + value_to_self_msat_offset) / 1000 - if self.channel_outbound { total_fee as i64 } else { 0 };
-               let value_to_remote: i64 = (((self.channel_value_satoshis * 1000 - self.value_to_self_msat - remote_htlc_total_msat) as i64 - value_to_self_msat_offset) / 1000) - if self.channel_outbound { 0 } else { total_fee as i64 };
+               let (value_to_self, value_to_remote) = if self.channel_outbound {
+                       (value_to_self_msat / 1000 - total_fee as i64, value_to_remote_msat / 1000)
+               } else {
+                       (value_to_self_msat / 1000, value_to_remote_msat / 1000 - total_fee as i64)
+               };
 
                let value_to_a = if local { value_to_self } else { value_to_remote };
                let value_to_b = if local { value_to_remote } else { value_to_self };
@@ -1080,7 +1136,8 @@ impl Channel {
                for (idx, htlc) in self.pending_inbound_htlcs.iter().enumerate() {
                        if htlc.htlc_id == htlc_id_arg {
                                assert_eq!(htlc.payment_hash, payment_hash_calc);
-                               if htlc.state != InboundHTLCState::Committed {
+                               if let InboundHTLCState::Committed = htlc.state {
+                               } else {
                                        debug_assert!(false, "Have an inbound HTLC we tried to claim before it was fully committed to");
                                        // Don't return in release mode here so that we can update channel_monitor
                                }
@@ -1125,12 +1182,12 @@ impl Channel {
 
                {
                        let htlc = &mut self.pending_inbound_htlcs[pending_idx];
-                       if htlc.state != InboundHTLCState::Committed {
+                       if let InboundHTLCState::Committed = htlc.state {
+                       } else {
                                debug_assert!(false, "Have an inbound HTLC we tried to claim before it was fully committed to");
                                return Ok((None, Some(self.channel_monitor.clone())));
                        }
-                       htlc.state = InboundHTLCState::LocalRemoved;
-                       htlc.local_removed_fulfilled = true;
+                       htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(payment_preimage_arg.clone()));
                }
 
                Ok((Some(msgs::UpdateFulfillHTLC {
@@ -1162,7 +1219,8 @@ impl Channel {
                let mut pending_idx = std::usize::MAX;
                for (idx, htlc) in self.pending_inbound_htlcs.iter().enumerate() {
                        if htlc.htlc_id == htlc_id_arg {
-                               if htlc.state != InboundHTLCState::Committed {
+                               if let InboundHTLCState::Committed = htlc.state {
+                               } else {
                                        debug_assert!(false, "Have an inbound HTLC we tried to fail before it was fully committed to");
                                        return Err(HandleError{err: "Unable to find a pending HTLC which matched the given HTLC ID", action: Some(msgs::ErrorAction::IgnoreError)});
                                }
@@ -1202,8 +1260,7 @@ impl Channel {
 
                {
                        let htlc = &mut self.pending_inbound_htlcs[pending_idx];
-                       htlc.state = InboundHTLCState::LocalRemoved;
-                       htlc.local_removed_fulfilled = false;
+                       htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(err_packet.clone()));
                }
 
                Ok(Some(msgs::UpdateFailHTLC {
@@ -1418,42 +1475,23 @@ impl Channel {
                Ok(())
        }
 
-       /// Returns (inbound_htlc_count, outbound_htlc_count, htlc_outbound_value_msat, htlc_inbound_value_msat)
-       /// If its for a remote update check, we need to be more lax about checking against messages we
-       /// sent but they may not have received/processed before they sent this message. Further, for
-       /// our own sends, we're more conservative and even consider things they've removed against
-       /// totals, though there is little reason to outside of further avoiding any race condition
-       /// issues.
-       fn get_pending_htlc_stats(&self, for_remote_update_check: bool) -> (u32, u32, u64, u64) {
-               //TODO: Can probably split this into inbound/outbound
-               let mut inbound_htlc_count: u32 = 0;
-               let mut outbound_htlc_count: u32 = 0;
-               let mut htlc_outbound_value_msat = 0;
+       /// Returns (inbound_htlc_count, htlc_inbound_value_msat)
+       fn get_inbound_pending_htlc_stats(&self) -> (u32, u64) {
                let mut htlc_inbound_value_msat = 0;
                for ref htlc in self.pending_inbound_htlcs.iter() {
-                       match htlc.state {
-                               InboundHTLCState::RemoteAnnounced => {},
-                               InboundHTLCState::AwaitingRemoteRevokeToAnnounce => {},
-                               InboundHTLCState::AwaitingAnnouncedRemoteRevoke => {},
-                               InboundHTLCState::Committed => {},
-                               InboundHTLCState::LocalRemoved => {},
-                       }
-                       inbound_htlc_count += 1;
                        htlc_inbound_value_msat += htlc.amount_msat;
                }
+               (self.pending_inbound_htlcs.len() as u32, htlc_inbound_value_msat)
+       }
+
+       /// Returns (outbound_htlc_count, htlc_outbound_value_msat)
+       fn get_outbound_pending_htlc_stats(&self) -> (u32, u64) {
+               let mut htlc_outbound_value_msat = 0;
                for ref htlc in self.pending_outbound_htlcs.iter() {
-                       match htlc.state {
-                               OutboundHTLCState::LocalAnnounced => { if for_remote_update_check { continue; } },
-                               OutboundHTLCState::Committed => {},
-                               OutboundHTLCState::RemoteRemoved => { if for_remote_update_check { continue; } },
-                               OutboundHTLCState::AwaitingRemoteRevokeToRemove => { if for_remote_update_check { continue; } },
-                               OutboundHTLCState::AwaitingRemovedRemoteRevoke => { if for_remote_update_check { continue; } },
-                       }
-                       outbound_htlc_count += 1;
                        htlc_outbound_value_msat += htlc.amount_msat;
                }
 
-               (inbound_htlc_count, outbound_htlc_count, htlc_outbound_value_msat, htlc_inbound_value_msat)
+               (self.pending_outbound_htlcs.len() as u32, htlc_outbound_value_msat)
        }
 
        pub fn update_add_htlc(&mut self, msg: &msgs::UpdateAddHTLC, pending_forward_state: PendingHTLCStatus) -> Result<(), HandleError> {
@@ -1470,7 +1508,7 @@ impl Channel {
                        return Err(HandleError{err: "Remote side tried to send less than our minimum HTLC value", action: None});
                }
 
-               let (inbound_htlc_count, _, _, htlc_inbound_value_msat) = self.get_pending_htlc_stats(true);
+               let (inbound_htlc_count, htlc_inbound_value_msat) = self.get_inbound_pending_htlc_stats();
                if inbound_htlc_count + 1 > OUR_MAX_HTLCS as u32 {
                        return Err(HandleError{err: "Remote tried to push more than our max accepted HTLCs", action: None});
                }
@@ -1501,15 +1539,13 @@ impl Channel {
                        amount_msat: msg.amount_msat,
                        payment_hash: msg.payment_hash,
                        cltv_expiry: msg.cltv_expiry,
-                       state: InboundHTLCState::RemoteAnnounced,
-                       local_removed_fulfilled: false,
-                       pending_forward_state: Some(pending_forward_state),
+                       state: InboundHTLCState::RemoteAnnounced(pending_forward_state),
                });
 
                Ok(())
        }
 
-       /// Removes an outbound HTLC which has been commitment_signed by the remote end
+       /// Marks an outbound HTLC which we have received update_fail/fulfill/malformed
        #[inline]
        fn mark_outbound_htlc_removed(&mut self, htlc_id: u64, check_preimage: Option<[u8; 32]>, fail_reason: Option<HTLCFailReason>) -> Result<&HTLCSource, ChannelError> {
                for htlc in self.pending_outbound_htlcs.iter_mut() {
@@ -1522,14 +1558,14 @@ impl Channel {
                                                }
                                };
                                match htlc.state {
-                                       OutboundHTLCState::LocalAnnounced =>
-                                               return Err(ChannelError::Close("Remote tried to fulfill HTLC before it had been committed")),
+                                       OutboundHTLCState::LocalAnnounced(_) =>
+                                               return Err(ChannelError::Close("Remote tried to fulfill/fail HTLC before it had been committed")),
                                        OutboundHTLCState::Committed => {
                                                htlc.state = OutboundHTLCState::RemoteRemoved;
                                                htlc.fail_reason = fail_reason;
                                        },
                                        OutboundHTLCState::AwaitingRemoteRevokeToRemove | OutboundHTLCState::AwaitingRemovedRemoteRevoke | OutboundHTLCState::RemoteRemoved =>
-                                               return Err(ChannelError::Close("Remote tried to fulfill HTLC that they'd already fulfilled")),
+                                               return Err(ChannelError::Close("Remote tried to fulfill/fail HTLC that they'd already fulfilled/failed")),
                                }
                                return Ok(&htlc.source);
                        }
@@ -1643,13 +1679,16 @@ impl Channel {
                self.channel_monitor.provide_latest_local_commitment_tx_info(local_commitment_tx.0, local_keys, self.feerate_per_kw, htlcs_and_sigs);
 
                for htlc in self.pending_inbound_htlcs.iter_mut() {
-                       if htlc.state == InboundHTLCState::RemoteAnnounced {
-                               htlc.state = InboundHTLCState::AwaitingRemoteRevokeToAnnounce;
+                       let new_forward = if let &InboundHTLCState::RemoteAnnounced(ref forward_info) = &htlc.state {
+                               Some(forward_info.clone())
+                       } else { None };
+                       if let Some(forward_info) = new_forward {
+                               htlc.state = InboundHTLCState::AwaitingRemoteRevokeToAnnounce(forward_info);
                                need_our_commitment = true;
                        }
                }
                for htlc in self.pending_outbound_htlcs.iter_mut() {
-                       if htlc.state == OutboundHTLCState::RemoteRemoved {
+                       if let OutboundHTLCState::RemoteRemoved = htlc.state {
                                htlc.state = OutboundHTLCState::AwaitingRemoteRevokeToRemove;
                                need_our_commitment = true;
                        }
@@ -1801,15 +1840,15 @@ impl Channel {
                let mut value_to_self_msat_diff: i64 = 0;
                // We really shouldnt have two passes here, but retain gives a non-mutable ref (Rust bug)
                self.pending_inbound_htlcs.retain(|htlc| {
-                       if htlc.state == InboundHTLCState::LocalRemoved {
-                               if htlc.local_removed_fulfilled {
+                       if let &InboundHTLCState::LocalRemoved(ref reason) = &htlc.state {
+                               if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
                                        value_to_self_msat_diff += htlc.amount_msat as i64;
                                }
                                false
                        } else { true }
                });
                self.pending_outbound_htlcs.retain(|htlc| {
-                       if htlc.state == OutboundHTLCState::AwaitingRemovedRemoteRevoke {
+                       if let OutboundHTLCState::AwaitingRemovedRemoteRevoke = htlc.state {
                                if let Some(reason) = htlc.fail_reason.clone() { // We really want take() here, but, again, non-mut ref :(
                                        revoked_htlcs.push((htlc.source.clone(), htlc.payment_hash, reason));
                                } else {
@@ -1820,30 +1859,45 @@ impl Channel {
                        } else { true }
                });
                for htlc in self.pending_inbound_htlcs.iter_mut() {
-                       if htlc.state == InboundHTLCState::AwaitingRemoteRevokeToAnnounce {
-                               htlc.state = InboundHTLCState::AwaitingAnnouncedRemoteRevoke;
-                               require_commitment = true;
-                       } else if htlc.state == InboundHTLCState::AwaitingAnnouncedRemoteRevoke {
-                               match htlc.pending_forward_state.take().unwrap() {
-                                       PendingHTLCStatus::Fail(fail_msg) => {
-                                               htlc.state = InboundHTLCState::LocalRemoved;
-                                               require_commitment = true;
-                                               match fail_msg {
-                                                       HTLCFailureMsg::Relay(msg) => update_fail_htlcs.push(msg),
-                                                       HTLCFailureMsg::Malformed(msg) => update_fail_malformed_htlcs.push(msg),
+                       let swap = if let &InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) = &htlc.state {
+                               true
+                       } else if let &InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) = &htlc.state {
+                               true
+                       } else { false };
+                       if swap {
+                               let mut state = InboundHTLCState::Committed;
+                               mem::swap(&mut state, &mut htlc.state);
+
+                               if let InboundHTLCState::AwaitingRemoteRevokeToAnnounce(forward_info) = state {
+                                       htlc.state = InboundHTLCState::AwaitingAnnouncedRemoteRevoke(forward_info);
+                                       require_commitment = true;
+                               } else if let InboundHTLCState::AwaitingAnnouncedRemoteRevoke(forward_info) = state {
+                                       match forward_info {
+                                               PendingHTLCStatus::Fail(fail_msg) => {
+                                                       require_commitment = true;
+                                                       match fail_msg {
+                                                               HTLCFailureMsg::Relay(msg) => {
+                                                                       htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailRelay(msg.reason.clone()));
+                                                                       update_fail_htlcs.push(msg)
+                                                               },
+                                                               HTLCFailureMsg::Malformed(msg) => {
+                                                                       htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::FailMalformed((msg.sha256_of_onion, msg.failure_code)));
+                                                                       update_fail_malformed_htlcs.push(msg)
+                                                               },
+                                                       }
+                                               },
+                                               PendingHTLCStatus::Forward(forward_info) => {
+                                                       to_forward_infos.push((forward_info, htlc.htlc_id));
+                                                       htlc.state = InboundHTLCState::Committed;
                                                }
-                                       },
-                                       PendingHTLCStatus::Forward(forward_info) => {
-                                               to_forward_infos.push((forward_info, htlc.htlc_id));
-                                               htlc.state = InboundHTLCState::Committed;
                                        }
                                }
                        }
                }
                for htlc in self.pending_outbound_htlcs.iter_mut() {
-                       if htlc.state == OutboundHTLCState::LocalAnnounced {
+                       if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
                                htlc.state = OutboundHTLCState::Committed;
-                       } else if htlc.state == OutboundHTLCState::AwaitingRemoteRevokeToRemove {
+                       } else if let OutboundHTLCState::AwaitingRemoteRevokeToRemove = htlc.state {
                                htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke;
                                require_commitment = true;
                        }
@@ -1906,10 +1960,12 @@ impl Channel {
                if !self.channel_outbound {
                        panic!("Cannot send fee from inbound channel");
                }
-
                if !self.is_usable() {
                        panic!("Cannot update fee until channel is fully established and we haven't started shutting down");
                }
+               if !self.is_live() {
+                       panic!("Cannot update fee while peer is disconnected (ChannelManager should have caught this)");
+               }
 
                if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
                        self.holding_cell_update_fee = Some(feerate_per_kw);
@@ -1953,22 +2009,21 @@ impl Channel {
                let mut inbound_drop_count = 0;
                self.pending_inbound_htlcs.retain(|htlc| {
                        match htlc.state {
-                               InboundHTLCState::RemoteAnnounced => {
+                               InboundHTLCState::RemoteAnnounced(_) => {
                                        // They sent us an update_add_htlc but we never got the commitment_signed.
                                        // We'll tell them what commitment_signed we're expecting next and they'll drop
                                        // this HTLC accordingly
                                        inbound_drop_count += 1;
                                        false
                                },
-                               InboundHTLCState::AwaitingRemoteRevokeToAnnounce|InboundHTLCState::AwaitingAnnouncedRemoteRevoke => {
-                                       // Same goes for AwaitingRemoteRevokeToRemove and AwaitingRemovedRemoteRevoke
+                               InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_)|InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => {
                                        // We received a commitment_signed updating this HTLC and (at least hopefully)
                                        // sent a revoke_and_ack (which we can re-transmit) and have heard nothing
                                        // in response to it yet, so don't touch it.
                                        true
                                },
                                InboundHTLCState::Committed => true,
-                               InboundHTLCState::LocalRemoved => { // Same goes for LocalAnnounced
+                               InboundHTLCState::LocalRemoved(_) => {
                                        // We (hopefully) sent a commitment_signed updating this HTLC (which we can
                                        // re-transmit if needed) and they may have even sent a revoke_and_ack back
                                        // (that we missed). Keep this around for now and if they tell us they missed
@@ -1979,7 +2034,7 @@ impl Channel {
                });
 
                for htlc in self.pending_outbound_htlcs.iter_mut() {
-                       if htlc.state == OutboundHTLCState::RemoteRemoved {
+                       if let OutboundHTLCState::RemoteRemoved = htlc.state {
                                // They sent us an update to remove this but haven't yet sent the corresponding
                                // commitment_signed, we need to move it back to Committed and they can re-send
                                // the update upon reconnection.
@@ -2017,7 +2072,7 @@ impl Channel {
 
        /// May panic if some calls other than message-handling calls (which will all Err immediately)
        /// have been called between remove_uncommitted_htlcs_and_mark_paused and this call.
-       pub fn channel_reestablish(&mut self, msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitor>), ChannelError> {
+       pub fn channel_reestablish(&mut self, msg: &msgs::ChannelReestablish) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitor>, RAACommitmentOrder), ChannelError> {
                if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 {
                        // While BOLT 2 doesn't indicate explicitly we should error this channel here, it
                        // almost certainly indicates we are going to end up out-of-sync in some way, so we
@@ -2027,40 +2082,51 @@ impl Channel {
 
                if msg.next_local_commitment_number == 0 || msg.next_local_commitment_number >= INITIAL_COMMITMENT_NUMBER ||
                                msg.next_remote_commitment_number == 0 || msg.next_remote_commitment_number >= INITIAL_COMMITMENT_NUMBER {
-                       return Err(ChannelError::Close("Peer send garbage channel_reestablish"));
+                       return Err(ChannelError::Close("Peer sent a garbage channel_reestablish"));
                }
 
                // Go ahead and unmark PeerDisconnected as various calls we may make check for it (and all
                // remaining cases either succeed or ErrorMessage-fail).
                self.channel_state &= !(ChannelState::PeerDisconnected as u32);
 
-               let mut required_revoke = None;
-               if msg.next_remote_commitment_number == INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number {
+               let required_revoke = if msg.next_remote_commitment_number == INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number {
                        // Remote isn't waiting on any RevokeAndACK from us!
                        // Note that if we need to repeat our FundingLocked we'll do that in the next if block.
+                       None
                } else if msg.next_remote_commitment_number == (INITIAL_COMMITMENT_NUMBER - 1) - self.cur_local_commitment_transaction_number {
                        let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &self.build_local_commitment_secret(self.cur_local_commitment_transaction_number));
                        let per_commitment_secret = chan_utils::build_commitment_secret(self.local_keys.commitment_seed, self.cur_local_commitment_transaction_number + 2);
-                       required_revoke = Some(msgs::RevokeAndACK {
+                       Some(msgs::RevokeAndACK {
                                channel_id: self.channel_id,
                                per_commitment_secret,
                                next_per_commitment_point,
-                       });
+                       })
                } else {
                        return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old local commitment transaction"));
-               }
+               };
 
-               if msg.next_local_commitment_number == INITIAL_COMMITMENT_NUMBER - self.cur_remote_commitment_transaction_number {
-                       if msg.next_remote_commitment_number == INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number {
-                               log_debug!(self, "Reconnected channel {} with no lost commitment txn", log_bytes!(self.channel_id()));
-                               if msg.next_local_commitment_number == 1 && msg.next_remote_commitment_number == 1 {
-                                       let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
-                                       let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
-                                       return Ok((Some(msgs::FundingLocked {
-                                               channel_id: self.channel_id(),
-                                               next_per_commitment_point: next_per_commitment_point,
-                                       }), None, None, None));
-                               }
+               // We increment cur_remote_commitment_transaction_number only upon receipt of
+               // revoke_and_ack, not on sending commitment_signed, so we add one if have
+               // AwaitingRemoteRevoke set, which indicates we sent a commitment_signed but haven't gotten
+               // the corresponding revoke_and_ack back yet.
+               let our_next_remote_commitment_number = INITIAL_COMMITMENT_NUMBER - self.cur_remote_commitment_transaction_number + if (self.channel_state & ChannelState::AwaitingRemoteRevoke as u32) != 0 { 1 } else { 0 };
+
+               let resend_funding_locked = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.cur_local_commitment_transaction_number == 1 {
+                       let next_per_commitment_secret = self.build_local_commitment_secret(self.cur_local_commitment_transaction_number);
+                       let next_per_commitment_point = PublicKey::from_secret_key(&self.secp_ctx, &next_per_commitment_secret);
+                       Some(msgs::FundingLocked {
+                               channel_id: self.channel_id(),
+                               next_per_commitment_point: next_per_commitment_point,
+                       })
+               } else { None };
+
+               let order = RAACommitmentOrder::RevokeAndACKFirst;
+
+               if msg.next_local_commitment_number == our_next_remote_commitment_number {
+                       if required_revoke.is_some() {
+                               log_debug!(self, "Reconnected channel {} with only lost outbound RAA", log_bytes!(self.channel_id()));
+                       } else {
+                               log_debug!(self, "Reconnected channel {} with no loss", log_bytes!(self.channel_id()));
                        }
 
                        if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
@@ -2078,22 +2144,71 @@ impl Channel {
                                                        panic!("Got non-channel-failing result from free_holding_cell_htlcs");
                                                }
                                        },
-                                       Ok(Some((commitment_update, channel_monitor))) => return Ok((None, required_revoke, Some(commitment_update), Some(channel_monitor))),
-                                       Ok(None) => return Ok((None, required_revoke, None, None)),
+                                       Ok(Some((commitment_update, channel_monitor))) => return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(channel_monitor), order)),
+                                       Ok(None) => return Ok((resend_funding_locked, required_revoke, None, None, order)),
                                }
                        } else {
-                               return Ok((None, required_revoke, None, None));
+                               return Ok((resend_funding_locked, required_revoke, None, None, order));
+                       }
+               } else if msg.next_local_commitment_number == our_next_remote_commitment_number - 1 {
+                       if required_revoke.is_some() {
+                               log_debug!(self, "Reconnected channel {} with lost outbound RAA and lost remote commitment tx", log_bytes!(self.channel_id()));
+                       } else {
+                               log_debug!(self, "Reconnected channel {} with only lost remote commitment tx", log_bytes!(self.channel_id()));
+                       }
+                       let mut update_add_htlcs = Vec::new();
+                       let mut update_fulfill_htlcs = Vec::new();
+                       let mut update_fail_htlcs = Vec::new();
+                       let mut update_fail_malformed_htlcs = Vec::new();
+
+                       for htlc in self.pending_outbound_htlcs.iter() {
+                               if let &OutboundHTLCState::LocalAnnounced(ref onion_packet) = &htlc.state {
+                                       update_add_htlcs.push(msgs::UpdateAddHTLC {
+                                               channel_id: self.channel_id(),
+                                               htlc_id: htlc.htlc_id,
+                                               amount_msat: htlc.amount_msat,
+                                               payment_hash: htlc.payment_hash,
+                                               cltv_expiry: htlc.cltv_expiry,
+                                               onion_routing_packet: (**onion_packet).clone(),
+                                       });
+                               }
+                       }
+
+                       for htlc in self.pending_inbound_htlcs.iter() {
+                               if let &InboundHTLCState::LocalRemoved(ref reason) = &htlc.state {
+                                       match reason {
+                                               &InboundHTLCRemovalReason::FailRelay(ref err_packet) => {
+                                                       update_fail_htlcs.push(msgs::UpdateFailHTLC {
+                                                               channel_id: self.channel_id(),
+                                                               htlc_id: htlc.htlc_id,
+                                                               reason: err_packet.clone()
+                                                       });
+                                               },
+                                               &InboundHTLCRemovalReason::FailMalformed((ref sha256_of_onion, ref failure_code)) => {
+                                                       update_fail_malformed_htlcs.push(msgs::UpdateFailMalformedHTLC {
+                                                               channel_id: self.channel_id(),
+                                                               htlc_id: htlc.htlc_id,
+                                                               sha256_of_onion: sha256_of_onion.clone(),
+                                                               failure_code: failure_code.clone(),
+                                                       });
+                                               },
+                                               &InboundHTLCRemovalReason::Fulfill(ref payment_preimage) => {
+                                                       update_fulfill_htlcs.push(msgs::UpdateFulfillHTLC {
+                                                               channel_id: self.channel_id(),
+                                                               htlc_id: htlc.htlc_id,
+                                                               payment_preimage: payment_preimage.clone(),
+                                                       });
+                                               },
+                                       }
+                               }
                        }
-               } else if msg.next_local_commitment_number == (INITIAL_COMMITMENT_NUMBER - 1) - self.cur_remote_commitment_transaction_number {
-                       return Ok((None, required_revoke,
+
+                       return Ok((resend_funding_locked, required_revoke,
                                        Some(msgs::CommitmentUpdate {
-                                               update_add_htlcs: Vec::new(),
-                                               update_fulfill_htlcs: Vec::new(),
-                                               update_fail_htlcs: Vec::new(),
-                                               update_fail_malformed_htlcs: Vec::new(),
-                                               update_fee: None,
+                                               update_add_htlcs, update_fulfill_htlcs, update_fail_htlcs, update_fail_malformed_htlcs,
+                                               update_fee: None, //TODO: We need to support re-generating any update_fees in the last commitment_signed!
                                                commitment_signed: self.send_commitment_no_state_update().expect("It looks like we failed to re-generate a commitment_signed we had previously sent?").0,
-                                       }), None));
+                                       }), None, order));
                } else {
                        return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction"));
                }
@@ -2109,7 +2224,7 @@ impl Channel {
                        return Ok((None, None, Vec::new()));
                }
                for htlc in self.pending_inbound_htlcs.iter() {
-                       if htlc.state == InboundHTLCState::RemoteAnnounced {
+                       if let InboundHTLCState::RemoteAnnounced(_) = htlc.state {
                                return Err(HandleError{err: "Got shutdown with remote pending HTLCs", action: None});
                        }
                }
@@ -2174,7 +2289,7 @@ impl Channel {
                        }
                });
                for htlc in self.pending_outbound_htlcs.iter() {
-                       if htlc.state == OutboundHTLCState::LocalAnnounced {
+                       if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
                                return Ok((None, None, dropped_outbound_htlcs));
                        }
                }
@@ -2337,6 +2452,11 @@ impl Channel {
                self.our_htlc_minimum_msat
        }
 
+       /// Allowed in any state (including after shutdown)
+       pub fn get_their_htlc_minimum_msat(&self) -> u64 {
+               self.our_htlc_minimum_msat
+       }
+
        pub fn get_value_satoshis(&self) -> u64 {
                self.channel_value_satoshis
        }
@@ -2352,6 +2472,30 @@ impl Channel {
                &self.local_keys
        }
 
+       #[cfg(test)]
+       pub fn get_value_stat(&self) -> ChannelValueStat {
+               ChannelValueStat {
+                       value_to_self_msat: self.value_to_self_msat,
+                       channel_value_msat: self.channel_value_satoshis * 1000,
+                       channel_reserve_msat: self.their_channel_reserve_satoshis * 1000,
+                       pending_outbound_htlcs_amount_msat: self.pending_outbound_htlcs.iter().map(|ref h| h.amount_msat).sum::<u64>(),
+                       pending_inbound_htlcs_amount_msat: self.pending_inbound_htlcs.iter().map(|ref h| h.amount_msat).sum::<u64>(),
+                       holding_cell_outbound_amount_msat: {
+                               let mut res = 0;
+                               for h in self.holding_cell_htlc_updates.iter() {
+                                       match h {
+                                               &HTLCUpdateAwaitingACK::AddHTLC{amount_msat, .. } => {
+                                                       res += amount_msat;
+                                               }
+                                               _ => {}
+                                       }
+                               }
+                               res
+                       },
+                       their_max_htlc_value_in_flight_msat: self.their_max_htlc_value_in_flight_msat,
+               }
+       }
+
        /// Allowed in any state (including after shutdown)
        pub fn get_channel_update_count(&self) -> u32 {
                self.channel_update_count
@@ -2426,7 +2570,7 @@ impl Channel {
        /// apply - no calls may be made except those explicitly stated to be allowed post-shutdown.
        /// Only returns an ErrorAction of DisconnectPeer, if Err.
        pub fn block_connected(&mut self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[u32]) -> Result<Option<msgs::FundingLocked>, HandleError> {
-               let non_shutdown_state = self.channel_state & (!BOTH_SIDES_SHUTDOWN_MASK);
+               let non_shutdown_state = self.channel_state & (!MULTI_STATE_FLAGS);
                if self.funding_tx_confirmations > 0 {
                        if header.bitcoin_hash() != self.last_block_connected {
                                self.last_block_connected = header.bitcoin_hash();
@@ -2436,10 +2580,10 @@ impl Channel {
                                                self.channel_state |= ChannelState::OurFundingLocked as u32;
                                                true
                                        } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::TheirFundingLocked as u32) {
-                                               self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & BOTH_SIDES_SHUTDOWN_MASK);
+                                               self.channel_state = ChannelState::ChannelFunded as u32 | (self.channel_state & MULTI_STATE_FLAGS);
                                                self.channel_update_count += 1;
                                                true
-                                       } else if self.channel_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) {
+                                       } else if non_shutdown_state == (ChannelState::FundingSent as u32 | ChannelState::OurFundingLocked as u32) {
                                                // We got a reorg but not enough to trigger a force close, just update
                                                // funding_tx_confirmed_in and return.
                                                false
@@ -2725,7 +2869,7 @@ impl Channel {
                        return Err(HandleError{err: "Cannot send an HTLC while disconnected", action: Some(ErrorAction::IgnoreError)});
                }
 
-               let (_, outbound_htlc_count, htlc_outbound_value_msat, _) = self.get_pending_htlc_stats(false);
+               let (outbound_htlc_count, htlc_outbound_value_msat) = self.get_outbound_pending_htlc_stats();
                if outbound_htlc_count + 1 > self.their_max_accepted_htlcs as u32 {
                        return Err(HandleError{err: "Cannot push more than their max accepted HTLCs", action: None});
                }
@@ -2772,7 +2916,7 @@ impl Channel {
                        amount_msat: amount_msat,
                        payment_hash: payment_hash.clone(),
                        cltv_expiry: cltv_expiry,
-                       state: OutboundHTLCState::LocalAnnounced,
+                       state: OutboundHTLCState::LocalAnnounced(Box::new(onion_routing_packet.clone())),
                        source,
                        fail_reason: None,
                });
@@ -2806,7 +2950,7 @@ impl Channel {
                }
                let mut have_updates = self.pending_update_fee.is_some();
                for htlc in self.pending_outbound_htlcs.iter() {
-                       if htlc.state == OutboundHTLCState::LocalAnnounced {
+                       if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
                                have_updates = true;
                        }
                        if have_updates { break; }
@@ -2822,12 +2966,15 @@ impl Channel {
                // fail to generate this, we still are at least at a position where upgrading their status
                // is acceptable.
                for htlc in self.pending_inbound_htlcs.iter_mut() {
-                       if htlc.state == InboundHTLCState::AwaitingRemoteRevokeToAnnounce {
-                               htlc.state = InboundHTLCState::AwaitingAnnouncedRemoteRevoke;
+                       let new_state = if let &InboundHTLCState::AwaitingRemoteRevokeToAnnounce(ref forward_info) = &htlc.state {
+                               Some(InboundHTLCState::AwaitingAnnouncedRemoteRevoke(forward_info.clone()))
+                       } else { None };
+                       if let Some(state) = new_state {
+                               htlc.state = state;
                        }
                }
                for htlc in self.pending_outbound_htlcs.iter_mut() {
-                       if htlc.state == OutboundHTLCState::AwaitingRemoteRevokeToRemove {
+                       if let OutboundHTLCState::AwaitingRemoteRevokeToRemove = htlc.state {
                                htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke;
                        }
                }
@@ -2896,7 +3043,7 @@ impl Channel {
        /// holding cell HTLCs for payment failure.
        pub fn get_shutdown(&mut self) -> Result<(msgs::Shutdown, Vec<(HTLCSource, [u8; 32])>), APIError> {
                for htlc in self.pending_outbound_htlcs.iter() {
-                       if htlc.state == OutboundHTLCState::LocalAnnounced {
+                       if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
                                return Err(APIError::APIMisuseError{err: "Cannot begin shutdown with pending HTLCs. Process pending events first"});
                        }
                }
@@ -3132,8 +3279,6 @@ mod tests {
                                cltv_expiry: 500,
                                payment_hash: [0; 32],
                                state: InboundHTLCState::Committed,
-                               local_removed_fulfilled: false,
-                               pending_forward_state: None,
                        };
                        let mut sha = Sha256::new();
                        sha.input(&hex::decode("0000000000000000000000000000000000000000000000000000000000000000").unwrap());
@@ -3147,8 +3292,6 @@ mod tests {
                                cltv_expiry: 501,
                                payment_hash: [0; 32],
                                state: InboundHTLCState::Committed,
-                               local_removed_fulfilled: false,
-                               pending_forward_state: None,
                        };
                        let mut sha = Sha256::new();
                        sha.input(&hex::decode("0101010101010101010101010101010101010101010101010101010101010101").unwrap());
@@ -3192,8 +3335,6 @@ mod tests {
                                cltv_expiry: 504,
                                payment_hash: [0; 32],
                                state: InboundHTLCState::Committed,
-                               local_removed_fulfilled: false,
-                               pending_forward_state: None,
                        };
                        let mut sha = Sha256::new();
                        sha.input(&hex::decode("0404040404040404040404040404040404040404040404040404040404040404").unwrap());