Clarify policy applied in send htlc error msgs
[rust-lightning] / src / ln / channel.rs
index 42d4caa36a84da209bb59b4cc5807493570ab1b3..07fb3474a5321419db09ca222fd1f10dea6d23df 100644 (file)
@@ -15,7 +15,7 @@ use secp256k1::{Secp256k1,Signature};
 use secp256k1;
 
 use ln::msgs;
-use ln::msgs::DecodeError;
+use ln::msgs::{DecodeError, OptionalField};
 use ln::channelmonitor::ChannelMonitor;
 use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingForwardHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash};
 use ln::chan_utils::{TxCreationKeys,HTLCOutputInCommitment,HTLC_SUCCESS_TX_WEIGHT,HTLC_TIMEOUT_TX_WEIGHT};
@@ -23,9 +23,9 @@ use ln::chan_utils;
 use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
 use chain::transaction::OutPoint;
 use chain::keysinterface::{ChannelKeys, KeysInterface};
-use util::{transaction_utils,rng};
+use util::transaction_utils;
 use util::ser::{Readable, ReadableArgs, Writeable, Writer, WriterWriteAdaptor};
-use util::logger::Logger;
+use util::logger::{Logger, LogHolder};
 use util::errors::APIError;
 use util::config::{UserConfig,ChannelConfig};
 
@@ -93,14 +93,14 @@ 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
        /// we will promote to Committed (note that they may not accept it until the next time we
-       /// revoke, but we dont really care about that:
+       /// revoke, but we don't really care about that:
        ///  * they've revoked, so worst case we can announce an old state and get our (option on)
-       ///    money back (though we wont), and,
+       ///    money back (though we won't), and,
        ///  * we'll send them a revoke when they send a commitment_signed, and since only they're
        ///    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
+       ///    doesn't matter to us and it's up to them to enforce it, worst-case they jump ahead but
        ///    we'll never get out of sync).
-       /// Note that we Box the OnionPacket as its rather large and we don't want to blow up
+       /// Note that we Box the OnionPacket as it's rather large and we don't want to blow up
        /// OutboundHTLCOutput's size just for a temporary bit
        LocalAnnounced(Box<msgs::OnionPacket>),
        Committed,
@@ -292,7 +292,7 @@ pub(super) struct Channel {
        last_sent_closing_fee: Option<(u64, u64)>, // (feerate, fee)
 
        /// The hash of the block in which the funding transaction reached our CONF_TARGET. We use this
-       /// to detect unconfirmation after a serialize-unserialize roudtrip where we may not see a full
+       /// to detect unconfirmation after a serialize-unserialize roundtrip where we may not see a full
        /// series of block_connected/block_disconnected calls. Obviously this is not a guarantee as we
        /// could miss the funding_tx_confirmed_in block as well, but it serves as a useful fallback.
        funding_tx_confirmed_in: Option<Sha256dHash>,
@@ -350,7 +350,10 @@ pub const OUR_MAX_HTLCS: u16 = 50; //TODO
 const UNCONF_THRESHOLD: u32 = 6;
 /// The amount of time we require our counterparty wait to claim their money (ie time between when
 /// we, or our watchtower, must check for them having broadcast a theft transaction).
+#[cfg(not(test))]
 const BREAKDOWN_TIMEOUT: u16 = 6 * 24 * 7; //TODO?
+#[cfg(test)]
+pub const BREAKDOWN_TIMEOUT: u16 = 6 * 24 * 7; //TODO?
 /// The amount of time we're willing to wait to claim money back to us
 const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 6 * 24 * 14;
 /// Exposing these two constants for use in test in ChannelMonitor
@@ -444,7 +447,7 @@ impl Channel {
                        user_id: user_id,
                        config: config.channel_options.clone(),
 
-                       channel_id: rng::rand_u832(),
+                       channel_id: keys_provider.get_channel_id(),
                        channel_state: ChannelState::OurInitSent as u32,
                        channel_outbound: true,
                        secp_ctx: secp_ctx,
@@ -548,7 +551,7 @@ impl Channel {
                        return Err(ChannelError::Close("Bogus; channel reserve is less than dust limit"));
                }
                if msg.htlc_minimum_msat >= (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000 {
-                       return Err(ChannelError::Close("Miminum htlc value is full channel value"));
+                       return Err(ChannelError::Close("Minimum htlc value is full channel value"));
                }
                Channel::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;
 
@@ -780,6 +783,8 @@ impl Channel {
                let mut local_htlc_total_msat = 0;
                let mut value_to_self_msat_offset = 0;
 
+               log_trace!(self, "Building commitment transaction number {} for {}, generated by {} with fee {}...", commitment_number, if local { "us" } else { "remote" }, if generated_by_local { "us" } else { "remote" }, feerate_per_kw);
+
                macro_rules! get_htlc_in_commitment {
                        ($htlc: expr, $offered: expr) => {
                                HTLCOutputInCommitment {
@@ -793,25 +798,29 @@ impl Channel {
                }
 
                macro_rules! add_htlc_output {
-                       ($htlc: expr, $outbound: expr, $source: expr) => {
+                       ($htlc: expr, $outbound: expr, $source: expr, $state_name: expr) => {
                                if $outbound == local { // "offered HTLC output"
                                        let htlc_in_tx = get_htlc_in_commitment!($htlc, true);
                                        if $htlc.amount_msat / 1000 >= dust_limit_satoshis + (feerate_per_kw * HTLC_TIMEOUT_TX_WEIGHT / 1000) {
+                                               log_trace!(self, "   ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, log_bytes!($htlc.payment_hash.0), $htlc.amount_msat);
                                                txouts.push((TxOut {
                                                        script_pubkey: chan_utils::get_htlc_redeemscript(&htlc_in_tx, &keys).to_v0_p2wsh(),
                                                        value: $htlc.amount_msat / 1000
                                                }, Some((htlc_in_tx, $source))));
                                        } else {
+                                               log_trace!(self, "   ...including {} {} dust HTLC {} (hash {}) with value {} due to dust limit", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, log_bytes!($htlc.payment_hash.0), $htlc.amount_msat);
                                                included_dust_htlcs.push((htlc_in_tx, $source));
                                        }
                                } else {
                                        let htlc_in_tx = get_htlc_in_commitment!($htlc, false);
                                        if $htlc.amount_msat / 1000 >= dust_limit_satoshis + (feerate_per_kw * HTLC_SUCCESS_TX_WEIGHT / 1000) {
+                                               log_trace!(self, "   ...including {} {} HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, log_bytes!($htlc.payment_hash.0), $htlc.amount_msat);
                                                txouts.push((TxOut { // "received HTLC output"
                                                        script_pubkey: chan_utils::get_htlc_redeemscript(&htlc_in_tx, &keys).to_v0_p2wsh(),
                                                        value: $htlc.amount_msat / 1000
                                                }, Some((htlc_in_tx, $source))));
                                        } else {
+                                               log_trace!(self, "   ...including {} {} dust HTLC {} (hash {}) with value {}", if $outbound { "outbound" } else { "inbound" }, $state_name, $htlc.htlc_id, log_bytes!($htlc.payment_hash.0), $htlc.amount_msat);
                                                included_dust_htlcs.push((htlc_in_tx, $source));
                                        }
                                }
@@ -819,18 +828,19 @@ 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::Committed => true,
-                               InboundHTLCState::LocalRemoved(_) => !generated_by_local,
+                       let (include, state_name) = match htlc.state {
+                               InboundHTLCState::RemoteAnnounced(_) => (!generated_by_local, "RemoteAnnounced"),
+                               InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) => (!generated_by_local, "AwaitingRemoteRevokeToAnnounce"),
+                               InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) => (true, "AwaitingAnnouncedRemoteRevoke"),
+                               InboundHTLCState::Committed => (true, "Committed"),
+                               InboundHTLCState::LocalRemoved(_) => (!generated_by_local, "LocalRemoved"),
                        };
 
                        if include {
-                               add_htlc_output!(htlc, false, None);
+                               add_htlc_output!(htlc, false, None, state_name);
                                remote_htlc_total_msat += htlc.amount_msat;
                        } else {
+                               log_trace!(self, "   ...not including inbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, log_bytes!(htlc.payment_hash.0), htlc.amount_msat, state_name);
                                match &htlc.state {
                                        &InboundHTLCState::LocalRemoved(ref reason) => {
                                                if generated_by_local {
@@ -845,18 +855,19 @@ impl Channel {
                }
 
                for ref htlc in self.pending_outbound_htlcs.iter() {
-                       let include = match htlc.state {
-                               OutboundHTLCState::LocalAnnounced(_) => generated_by_local,
-                               OutboundHTLCState::Committed => true,
-                               OutboundHTLCState::RemoteRemoved => generated_by_local,
-                               OutboundHTLCState::AwaitingRemoteRevokeToRemove => generated_by_local,
-                               OutboundHTLCState::AwaitingRemovedRemoteRevoke => false,
+                       let (include, state_name) = match htlc.state {
+                               OutboundHTLCState::LocalAnnounced(_) => (generated_by_local, "LocalAnnounced"),
+                               OutboundHTLCState::Committed => (true, "Committed"),
+                               OutboundHTLCState::RemoteRemoved => (generated_by_local, "RemoteRemoved"),
+                               OutboundHTLCState::AwaitingRemoteRevokeToRemove => (generated_by_local, "AwaitingRemoteRevokeToRemove"),
+                               OutboundHTLCState::AwaitingRemovedRemoteRevoke => (false, "AwaitingRemovedRemoteRevoke"),
                        };
 
                        if include {
-                               add_htlc_output!(htlc, true, Some(&htlc.source));
+                               add_htlc_output!(htlc, true, Some(&htlc.source), state_name);
                                local_htlc_total_msat += htlc.amount_msat;
                        } else {
+                               log_trace!(self, "   ...not including outbound HTLC {} (hash {}) with value {} due to state ({})", htlc.htlc_id, log_bytes!(htlc.payment_hash.0), htlc.amount_msat, state_name);
                                match htlc.state {
                                        OutboundHTLCState::AwaitingRemoteRevokeToRemove|OutboundHTLCState::AwaitingRemovedRemoteRevoke => {
                                                if htlc.fail_reason.is_none() {
@@ -1110,7 +1121,7 @@ impl Channel {
        }
 
        /// Signs a transaction created by build_htlc_transaction. If the transaction is an
-       /// HTLC-Success transaction (ie htlc.offered is false), preimate must be set!
+       /// HTLC-Success transaction (ie htlc.offered is false), preimage must be set!
        fn sign_htlc_transaction(&self, tx: &mut Transaction, their_sig: &Signature, preimage: &Option<PaymentPreimage>, htlc: &HTLCOutputInCommitment, keys: &TxCreationKeys) -> Result<Signature, ChannelError> {
                if tx.input.len() != 1 {
                        panic!("Tried to sign HTLC transaction that had input count != 1!");
@@ -1148,7 +1159,7 @@ impl Channel {
        /// In such cases we debug_assert!(false) and return an IgnoreError. Thus, will always return
        /// Ok(_) if debug assertions are turned on and preconditions are met.
        fn get_update_fulfill_htlc(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage) -> Result<(Option<msgs::UpdateFulfillHTLC>, Option<ChannelMonitor>), ChannelError> {
-               // Either ChannelFunded got set (which means it wont bet unset) or there is no way any
+               // Either ChannelFunded got set (which means it won't be unset) or there is no way any
                // caller thought we could have something claimed (cause we wouldn't have accepted in an
                // incoming HTLC anyway). If we got to ShutdownComplete, callers aren't allowed to call us,
                // either.
@@ -1214,6 +1225,7 @@ impl Channel {
                                        _ => {}
                                }
                        }
+                       log_trace!(self, "Adding HTLC claim to holding_cell! Current state: {}", self.channel_state);
                        self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::ClaimHTLC {
                                payment_preimage: payment_preimage_arg, htlc_id: htlc_id_arg,
                        });
@@ -1227,6 +1239,7 @@ impl Channel {
                                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())));
                        }
+                       log_trace!(self, "Upgrading HTLC {} to LocalRemoved with a Fulfill!", log_bytes!(htlc.payment_hash.0));
                        htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(payment_preimage_arg.clone()));
                }
 
@@ -1271,7 +1284,7 @@ impl Channel {
                                        },
                                        _ => {
                                                debug_assert!(false, "Have an inbound HTLC we tried to claim before it was fully committed to");
-                                               return Err(ChannelError::Ignore("Unable to find a pending HTLC which matchd the given HTLC ID"));
+                                               return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID"));
                                        }
                                }
                                pending_idx = idx;
@@ -1349,10 +1362,10 @@ impl Channel {
                        return Err(ChannelError::Close("They wanted our payments to be delayed by a needlessly long period"));
                }
                if msg.max_accepted_htlcs < 1 {
-                       return Err(ChannelError::Close("0 max_accpted_htlcs makes for a useless channel"));
+                       return Err(ChannelError::Close("0 max_accepted_htlcs makes for a useless channel"));
                }
                if msg.max_accepted_htlcs > 483 {
-                       return Err(ChannelError::Close("max_accpted_htlcs > 483"));
+                       return Err(ChannelError::Close("max_accepted_htlcs > 483"));
                }
 
                // Now check against optional parameters as set by config...
@@ -1430,7 +1443,7 @@ impl Channel {
                }
                if self.channel_state != (ChannelState::OurInitSent as u32 | ChannelState::TheirInitSent as u32) {
                        // BOLT 2 says that if we disconnect before we send funding_signed we SHOULD NOT
-                       // remember the channel, so its safe to just send an error_message here and drop the
+                       // remember the channel, so it's safe to just send an error_message here and drop the
                        // channel.
                        return Err(ChannelError::Close("Received funding_created after we got the channel!"));
                }
@@ -1540,14 +1553,23 @@ impl Channel {
                (self.pending_inbound_htlcs.len() as u32, htlc_inbound_value_msat)
        }
 
-       /// Returns (outbound_htlc_count, htlc_outbound_value_msat)
+       /// Returns (outbound_htlc_count, htlc_outbound_value_msat) *including* pending adds in our
+       /// holding cell.
        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() {
                        htlc_outbound_value_msat += htlc.amount_msat;
                }
 
-               (self.pending_outbound_htlcs.len() as u32, htlc_outbound_value_msat)
+               let mut htlc_outbound_count = self.pending_outbound_htlcs.len();
+               for update in self.holding_cell_htlc_updates.iter() {
+                       if let &HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, .. } = update {
+                               htlc_outbound_count += 1;
+                               htlc_outbound_value_msat += amount_msat;
+                       }
+               }
+
+               (htlc_outbound_count as u32, htlc_outbound_value_msat)
        }
 
        pub fn update_add_htlc(&mut self, msg: &msgs::UpdateAddHTLC, pending_forward_state: PendingHTLCStatus) -> Result<(), ChannelError> {
@@ -1568,10 +1590,9 @@ impl Channel {
                if inbound_htlc_count + 1 > OUR_MAX_HTLCS as u32 {
                        return Err(ChannelError::Close("Remote tried to push more than our max accepted HTLCs"));
                }
-               //TODO: Spec is unclear if this is per-direction or in total (I assume per direction):
                // Check our_max_htlc_value_in_flight_msat
                if htlc_inbound_value_msat + msg.amount_msat > Channel::get_our_max_htlc_value_in_flight_msat(self.channel_value_satoshis) {
-                       return Err(ChannelError::Close("Remote HTLC add would put them over their max HTLC value in flight"));
+                       return Err(ChannelError::Close("Remote HTLC add would put them over our max HTLC value"));
                }
                // Check our_channel_reserve_satoshis (we're getting paid, so they have to at least meet
                // the reserve_satoshis we told them to always have as direct payment so that they lose
@@ -1700,6 +1721,7 @@ impl Channel {
                };
                let local_commitment_txid = local_commitment_tx.0.txid();
                let local_sighash = hash_to_message!(&bip143::SighashComponents::new(&local_commitment_tx.0).sighash_all(&local_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]);
+               log_trace!(self, "Checking commitment tx signature {} by key {} against tx {} with redeemscript {}", log_bytes!(msg.signature.serialize_compact()[..]), log_bytes!(self.their_funding_pubkey.unwrap().serialize()), encode::serialize_hex(&local_commitment_tx.0), encode::serialize_hex(&funding_script));
                secp_check!(self.secp_ctx.verify(&local_sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid commitment tx signature from peer");
 
                //If channel fee was updated by funder confirm funder can afford the new fee rate when applied to the current local commitment transaction
@@ -1725,6 +1747,7 @@ impl Channel {
                        if let Some(_) = htlc.transaction_output_index {
                                let mut htlc_tx = self.build_htlc_transaction(&local_commitment_txid, &htlc, true, &local_keys, feerate_per_kw);
                                let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &local_keys);
+                               log_trace!(self, "Checking HTLC tx signature {} by key {} against tx {} with redeemscript {}", log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(local_keys.b_htlc_key.serialize()), encode::serialize_hex(&htlc_tx), encode::serialize_hex(&htlc_redeemscript));
                                let htlc_sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]);
                                secp_check!(self.secp_ctx.verify(&htlc_sighash, &msg.htlc_signatures[idx], &local_keys.b_htlc_key), "Invalid HTLC tx signature from peer");
                                let htlc_sig = if htlc.offered {
@@ -1787,8 +1810,24 @@ impl Channel {
                self.received_commitment_while_awaiting_raa = (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) != 0;
 
                if (self.channel_state & ChannelState::MonitorUpdateFailed as u32) != 0 {
+                       // In case we initially failed monitor updating without requiring a response, we need
+                       // to make sure the RAA gets sent first.
+                       if !self.monitor_pending_commitment_signed {
+                               self.monitor_pending_order = Some(RAACommitmentOrder::RevokeAndACKFirst);
+                       }
                        self.monitor_pending_revoke_and_ack = true;
-                       self.monitor_pending_commitment_signed |= need_our_commitment;
+                       if need_our_commitment && (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == 0 {
+                               // If we were going to send a commitment_signed after the RAA, go ahead and do all
+                               // the corresponding HTLC status updates so that get_last_commitment_update
+                               // includes the right HTLCs.
+                               // Note that this generates a monitor update that we ignore! This is OK since we
+                               // won't actually send the commitment_signed that generated the update to the other
+                               // side until the latest monitor has been pulled from us and stored.
+                               self.monitor_pending_commitment_signed = true;
+                               self.send_commitment_no_status_check()?;
+                       }
+                       // TODO: Call maybe_propose_first_closing_signed on restoration (or call it here and
+                       // re-send the message on restoration)
                        return Err(ChannelError::Ignore("Previous monitor update failure prevented generation of RAA"));
                }
 
@@ -1823,8 +1862,8 @@ impl Channel {
                        for htlc_update in htlc_updates.drain(..) {
                                // Note that this *can* fail, though it should be due to rather-rare conditions on
                                // fee races with adding too many outputs which push our total payments just over
-                               // the limit. In case its less rare than I anticipate, we may want to revisit
-                               // handling this case better and maybe fufilling some of the HTLCs while attempting
+                               // the limit. In case it's less rare than I anticipate, we may want to revisit
+                               // handling this case better and maybe fulfilling some of the HTLCs while attempting
                                // to rebalance channels.
                                if err.is_some() { // We're back to AwaitingRemoteRevoke (or are about to fail the channel)
                                        self.holding_cell_htlc_updates.push(htlc_update);
@@ -1834,6 +1873,14 @@ impl Channel {
                                                        match self.send_htlc(amount_msat, *payment_hash, cltv_expiry, source.clone(), onion_routing_packet.clone()) {
                                                                Ok(update_add_msg_option) => update_add_htlcs.push(update_add_msg_option.unwrap()),
                                                                Err(e) => {
+                                                                       match e {
+                                                                               ChannelError::Ignore(ref msg) => {
+                                                                                       log_info!(self, "Failed to send HTLC with payment_hash {} due to {}", log_bytes!(payment_hash.0), msg);
+                                                                               },
+                                                                               _ => {
+                                                                                       log_info!(self, "Failed to send HTLC with payment_hash {} resulting in a channel closure during holding_cell freeing", log_bytes!(payment_hash.0));
+                                                                               },
+                                                                       }
                                                                        err = Some(e);
                                                                }
                                                        }
@@ -1863,11 +1910,16 @@ impl Channel {
                                        }
                                        if err.is_some() {
                                                self.holding_cell_htlc_updates.push(htlc_update);
+                                               if let Some(ChannelError::Ignore(_)) = err {
+                                                       // If we failed to add the HTLC, but got an Ignore error, we should
+                                                       // still send the new commitment_signed, so reset the err to None.
+                                                       err = None;
+                                               }
                                        }
                                }
                        }
-                       //TODO: Need to examine the type of err - if its a fee issue or similar we may want to
-                       //fail it back the route, if its a temporary issue we can ignore it...
+                       //TODO: Need to examine the type of err - if it's a fee issue or similar we may want to
+                       //fail it back the route, if it's a temporary issue we can ignore it...
                        match err {
                                None => {
                                        if update_add_htlcs.is_empty() && update_fulfill_htlcs.is_empty() && update_fail_htlcs.is_empty() && self.holding_cell_update_fee.is_none() {
@@ -1941,74 +1993,89 @@ impl Channel {
                        self.monitor_pending_order = None;
                }
 
+               log_trace!(self, "Updating HTLCs on receipt of RAA...");
                let mut to_forward_infos = Vec::new();
                let mut revoked_htlcs = Vec::new();
                let mut update_fail_htlcs = Vec::new();
                let mut update_fail_malformed_htlcs = Vec::new();
                let mut require_commitment = false;
                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 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 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 {
-                                       // They fulfilled, so we sent them money
-                                       value_to_self_msat_diff -= htlc.amount_msat as i64;
-                               }
-                               false
-                       } else { true }
-               });
-               for htlc in self.pending_inbound_htlcs.iter_mut() {
-                       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)
-                                                               },
+
+               {
+                       // Take references explicitly so that we can hold multiple references to self.
+                       let pending_inbound_htlcs: &mut Vec<_> = &mut self.pending_inbound_htlcs;
+                       let pending_outbound_htlcs: &mut Vec<_> = &mut self.pending_outbound_htlcs;
+                       let logger = LogHolder { logger: &self.logger };
+
+                       // We really shouldnt have two passes here, but retain gives a non-mutable ref (Rust bug)
+                       pending_inbound_htlcs.retain(|htlc| {
+                               if let &InboundHTLCState::LocalRemoved(ref reason) = &htlc.state {
+                                       log_trace!(logger, " ...removing inbound LocalRemoved {}", log_bytes!(htlc.payment_hash.0));
+                                       if let &InboundHTLCRemovalReason::Fulfill(_) = reason {
+                                               value_to_self_msat_diff += htlc.amount_msat as i64;
+                                       }
+                                       false
+                               } else { true }
+                       });
+                       pending_outbound_htlcs.retain(|htlc| {
+                               if let OutboundHTLCState::AwaitingRemovedRemoteRevoke = htlc.state {
+                                       log_trace!(logger, " ...removing outbound AwaitingRemovedRemoteRevoke {}", log_bytes!(htlc.payment_hash.0));
+                                       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 {
+                                               // They fulfilled, so we sent them money
+                                               value_to_self_msat_diff -= htlc.amount_msat as i64;
+                                       }
+                                       false
+                               } else { true }
+                       });
+                       for htlc in pending_inbound_htlcs.iter_mut() {
+                               let swap = if let &InboundHTLCState::AwaitingRemoteRevokeToAnnounce(_) = &htlc.state {
+                                       log_trace!(logger, " ...promoting inbound AwaitingRemoteRevokeToAnnounce {} to Committed", log_bytes!(htlc.payment_hash.0));
+                                       true
+                               } else if let &InboundHTLCState::AwaitingAnnouncedRemoteRevoke(_) = &htlc.state {
+                                       log_trace!(logger, " ...promoting inbound AwaitingAnnouncedRemoteRevoke {} to Committed", log_bytes!(htlc.payment_hash.0));
+                                       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 let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
-                               htlc.state = OutboundHTLCState::Committed;
-                       } else if let OutboundHTLCState::AwaitingRemoteRevokeToRemove = htlc.state {
-                               htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke;
-                               require_commitment = true;
+                       for htlc in pending_outbound_htlcs.iter_mut() {
+                               if let OutboundHTLCState::LocalAnnounced(_) = htlc.state {
+                                       log_trace!(logger, " ...promoting outbound LocalAnnounced {} to Committed", log_bytes!(htlc.payment_hash.0));
+                                       htlc.state = OutboundHTLCState::Committed;
+                               } else if let OutboundHTLCState::AwaitingRemoteRevokeToRemove = htlc.state {
+                                       log_trace!(logger, " ...promoting outbound AwaitingRemoteRevokeToRemove {} to AwaitingRemovedRemoteRevoke", log_bytes!(htlc.payment_hash.0));
+                                       htlc.state = OutboundHTLCState::AwaitingRemovedRemoteRevoke;
+                                       require_commitment = true;
+                               }
                        }
                }
                self.value_to_self_msat = (self.value_to_self_msat as i64 + value_to_self_msat_diff) as u64;
@@ -2019,7 +2086,7 @@ impl Channel {
                        }
                } else {
                        if let Some(feerate) = self.pending_update_fee {
-                               // Because a node cannot send two commitment_signed's in a row without getting a
+                               // Because a node cannot send two commitment_signeds in a row without getting a
                                // revoke_and_ack from us (as it would otherwise not know the per_commitment_point
                                // it should use to create keys with) and because a node can't send a
                                // commitment_signed without changes, checking if the feerate is equal to the
@@ -2036,6 +2103,10 @@ impl Channel {
                        // cells) while we can't update the monitor, so we just return what we have.
                        if require_commitment {
                                self.monitor_pending_commitment_signed = true;
+                               // When the monitor updating is restored we'll call get_last_commitment_update(),
+                               // which does not update state, but we're definitely now awaiting a remote revoke
+                               // before we can step forward any more, so set it here.
+                               self.channel_state |= ChannelState::AwaitingRemoteRevoke as u32;
                        }
                        self.monitor_pending_forwards.append(&mut to_forward_infos);
                        self.monitor_pending_failures.append(&mut revoked_htlcs);
@@ -2187,23 +2258,15 @@ impl Channel {
        /// commitment update or a revoke_and_ack generation). The messages which were generated from
        /// that original call 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, order: RAACommitmentOrder, mut pending_forwards: Vec<(PendingForwardHTLCInfo, u64)>, mut pending_fails: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, raa_first_dropped_cs: bool) {
+       pub fn monitor_update_failed(&mut self, order: RAACommitmentOrder, resend_raa: bool, resend_commitment: bool, mut pending_forwards: Vec<(PendingForwardHTLCInfo, u64)>, mut pending_fails: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>) {
                assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, 0);
-               match order {
-                       RAACommitmentOrder::CommitmentFirst => {
-                               self.monitor_pending_revoke_and_ack = false;
-                               self.monitor_pending_commitment_signed = true;
-                       },
-                       RAACommitmentOrder::RevokeAndACKFirst => {
-                               self.monitor_pending_revoke_and_ack = true;
-                               self.monitor_pending_commitment_signed = raa_first_dropped_cs;
-                       },
-               }
+               self.monitor_pending_revoke_and_ack = resend_raa;
+               self.monitor_pending_commitment_signed = resend_commitment;
+               self.monitor_pending_order = Some(order);
                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_order = Some(order);
                self.channel_state |= ChannelState::MonitorUpdateFailed as u32;
        }
 
@@ -2309,6 +2372,8 @@ impl Channel {
                        }
                }
 
+               log_trace!(self, "Regenerated latest commitment update with {} update_adds, {} update_fulfills, {} update_fails, and {} update_fail_malformeds",
+                               update_add_htlcs.len(), update_fulfill_htlcs.len(), update_fail_htlcs.len(), update_fail_malformed_htlcs.len());
                msgs::CommitmentUpdate {
                        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!
@@ -2426,9 +2491,6 @@ impl Channel {
                                log_debug!(self, "Reconnected channel {} with only lost remote commitment tx", log_bytes!(self.channel_id()));
                        }
 
-                       // If monitor_pending_order is set, it must be CommitmentSigned if we have no RAA
-                       debug_assert!(self.monitor_pending_order != Some(RAACommitmentOrder::RevokeAndACKFirst) || required_revoke.is_some());
-
                        if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 {
                                self.monitor_pending_commitment_signed = true;
                                return Ok((resend_funding_locked, None, None, None, order, shutdown_msg));
@@ -2484,7 +2546,7 @@ impl Channel {
                assert_eq!(self.channel_state & ChannelState::ShutdownComplete as u32, 0);
 
                // BOLT 2 says we must only send a scriptpubkey of certain standard forms, which are up to
-               // 34 bytes in length, so dont let the remote peer feed us some super fee-heavy script.
+               // 34 bytes in length, so don't let the remote peer feed us some super fee-heavy script.
                if self.channel_outbound && msg.scriptpubkey.len() > 34 {
                        return Err(ChannelError::Close("Got shutdown_scriptpubkey of absurd length from remote peer"));
                }
@@ -2951,7 +3013,7 @@ impl Channel {
                        htlc_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.htlc_base_key),
                        first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret),
                        channel_flags: if self.config.announced_channel {1} else {0},
-                       shutdown_scriptpubkey: None,
+                       shutdown_scriptpubkey: OptionalField::Absent
                }
        }
 
@@ -2983,7 +3045,7 @@ impl Channel {
                        delayed_payment_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.delayed_payment_base_key),
                        htlc_basepoint: PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.htlc_base_key),
                        first_per_commitment_point: PublicKey::from_secret_key(&self.secp_ctx, &local_commitment_secret),
-                       shutdown_scriptpubkey: None,
+                       shutdown_scriptpubkey: OptionalField::Absent
                }
        }
 
@@ -3111,7 +3173,7 @@ impl Channel {
                        // dropped this channel on disconnect as it hasn't yet reached FundingSent so we can't
                        // overflow here.
                        next_remote_commitment_number: INITIAL_COMMITMENT_NUMBER - self.cur_remote_commitment_transaction_number - 1,
-                       data_loss_protect: None,
+                       data_loss_protect: OptionalField::Absent,
                }
        }
 
@@ -3124,7 +3186,7 @@ impl Channel {
        /// waiting on the remote peer to send us a revoke_and_ack during which time we cannot add new
        /// HTLCs on the wire or we wouldn't be able to determine what they actually ACK'ed.
        /// You MUST call send_commitment prior to any other calls on this Channel
-       /// If an Err is returned, its a ChannelError::Ignore!
+       /// If an Err is returned, it's a ChannelError::Ignore!
        pub fn send_htlc(&mut self, amount_msat: u64, payment_hash: PaymentHash, cltv_expiry: u32, source: HTLCSource, onion_routing_packet: msgs::OnionPacket) -> Result<Option<msgs::UpdateAddHTLC>, ChannelError> {
                if (self.channel_state & (ChannelState::ChannelFunded as u32 | BOTH_SIDES_SHUTDOWN_MASK)) != (ChannelState::ChannelFunded as u32) {
                        return Err(ChannelError::Ignore("Cannot send HTLC until channel is fully established and we haven't started shutting down"));
@@ -3151,33 +3213,21 @@ impl Channel {
                if outbound_htlc_count + 1 > self.their_max_accepted_htlcs as u32 {
                        return Err(ChannelError::Ignore("Cannot push more than their max accepted HTLCs"));
                }
-               //TODO: Spec is unclear if this is per-direction or in total (I assume per direction):
                // Check their_max_htlc_value_in_flight_msat
                if htlc_outbound_value_msat + amount_msat > self.their_max_htlc_value_in_flight_msat {
-                       return Err(ChannelError::Ignore("Cannot send value that would put us over our max HTLC value in flight"));
-               }
-
-               let mut holding_cell_outbound_amount_msat = 0;
-               for holding_htlc in self.holding_cell_htlc_updates.iter() {
-                       match holding_htlc {
-                               &HTLCUpdateAwaitingACK::AddHTLC { ref amount_msat, .. } => {
-                                       holding_cell_outbound_amount_msat += *amount_msat;
-                               }
-                               _ => {}
-                       }
+                       return Err(ChannelError::Ignore("Cannot send value that would put us over the max HTLC value in flight our peer will accept"));
                }
 
                // Check self.their_channel_reserve_satoshis (the amount we must keep as
                // reserve for them to have something to claim if we misbehave)
-               if self.value_to_self_msat < self.their_channel_reserve_satoshis * 1000 + amount_msat + holding_cell_outbound_amount_msat + htlc_outbound_value_msat {
-                       return Err(ChannelError::Ignore("Cannot send value that would put us over our reserve value"));
+               if self.value_to_self_msat < self.their_channel_reserve_satoshis * 1000 + amount_msat + htlc_outbound_value_msat {
+                       return Err(ChannelError::Ignore("Cannot send value that would put us over their reserve value"));
                }
 
                //TODO: Check cltv_expiry? Do this in channel manager?
 
                // Now update local state:
                if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
-                       //TODO: Check the limits *including* other pending holding cell HTLCs!
                        self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
                                amount_msat: amount_msat,
                                payment_hash: payment_hash,
@@ -3297,6 +3347,7 @@ impl Channel {
                let remote_commitment_txid = remote_commitment_tx.0.txid();
                let remote_sighash = hash_to_message!(&bip143::SighashComponents::new(&remote_commitment_tx.0).sighash_all(&remote_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]);
                let our_sig = self.secp_ctx.sign(&remote_sighash, &self.local_keys.funding_key);
+               log_trace!(self, "Signing remote commitment tx {} with redeemscript {} with pubkey {} -> {}", encode::serialize_hex(&remote_commitment_tx.0), encode::serialize_hex(&funding_script), log_bytes!(PublicKey::from_secret_key(&self.secp_ctx, &self.local_keys.funding_key).serialize()), log_bytes!(our_sig.serialize_compact()[..]));
 
                let mut htlc_sigs = Vec::with_capacity(remote_commitment_tx.1);
                for &(ref htlc, _) in remote_commitment_tx.2.iter() {
@@ -3306,6 +3357,7 @@ impl Channel {
                                let htlc_sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]);
                                let our_htlc_key = secp_check!(chan_utils::derive_private_key(&self.secp_ctx, &remote_keys.per_commitment_point, &self.local_keys.htlc_base_key), "Derived invalid key, peer is maliciously selecting parameters");
                                htlc_sigs.push(self.secp_ctx.sign(&htlc_sighash, &our_htlc_key));
+                               log_trace!(self, "Signing remote HTLC tx {} with redeemscript {} with pubkey {} -> {}", encode::serialize_hex(&htlc_tx), encode::serialize_hex(&htlc_redeemscript), log_bytes!(PublicKey::from_secret_key(&self.secp_ctx, &our_htlc_key).serialize()), log_bytes!(htlc_sigs.last().unwrap().serialize_compact()[..]));
                        }
                }
 
@@ -3381,7 +3433,7 @@ impl Channel {
                }, dropped_outbound_htlcs))
        }
 
-       /// Gets the latest commitment transaction and any dependant transactions for relay (forcing
+       /// Gets the latest commitment transaction and any dependent transactions for relay (forcing
        /// shutdown of this channel - no more calls into this Channel may be made afterwards except
        /// those explicitly stated to be allowed after shutdown completes, eg some simple getters).
        /// Also returns the list of payment_hashes for channels which we can safely fail backwards
@@ -3696,14 +3748,6 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
                        });
                }
 
-               macro_rules! read_option { () => {
-                       match <u8 as Readable<R>>::read(reader)? {
-                               0 => None,
-                               1 => Some(Readable::read(reader)?),
-                               _ => return Err(DecodeError::InvalidValue),
-                       }
-               } }
-
                let pending_outbound_htlc_count: u64 = Readable::read(reader)?;
                let mut pending_outbound_htlcs = Vec::with_capacity(cmp::min(pending_outbound_htlc_count as usize, OUR_MAX_HTLCS as usize));
                for _ in 0..pending_outbound_htlc_count {
@@ -3713,7 +3757,7 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
                                cltv_expiry: Readable::read(reader)?,
                                payment_hash: Readable::read(reader)?,
                                source: Readable::read(reader)?,
-                               fail_reason: read_option!(),
+                               fail_reason: Readable::read(reader)?,
                                state: match <u8 as Readable<R>>::read(reader)? {
                                        0 => OutboundHTLCState::LocalAnnounced(Box::new(Readable::read(reader)?)),
                                        1 => OutboundHTLCState::Committed,
@@ -3771,8 +3815,8 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
                        monitor_pending_failures.push((Readable::read(reader)?, Readable::read(reader)?, Readable::read(reader)?));
                }
 
-               let pending_update_fee = read_option!();
-               let holding_cell_update_fee = read_option!();
+               let pending_update_fee = Readable::read(reader)?;
+               let holding_cell_update_fee = Readable::read(reader)?;
 
                let next_local_htlc_id = Readable::read(reader)?;
                let next_remote_htlc_id = Readable::read(reader)?;
@@ -3794,8 +3838,8 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
                        _ => return Err(DecodeError::InvalidValue),
                };
 
-               let funding_tx_confirmed_in = read_option!();
-               let short_channel_id = read_option!();
+               let funding_tx_confirmed_in = Readable::read(reader)?;
+               let short_channel_id = Readable::read(reader)?;
 
                let last_block_connected = Readable::read(reader)?;
                let funding_tx_confirmations = Readable::read(reader)?;
@@ -3810,17 +3854,17 @@ impl<R : ::std::io::Read> ReadableArgs<R, Arc<Logger>> for Channel {
                let their_max_accepted_htlcs = Readable::read(reader)?;
                let minimum_depth = Readable::read(reader)?;
 
-               let their_funding_pubkey = read_option!();
-               let their_revocation_basepoint = read_option!();
-               let their_payment_basepoint = read_option!();
-               let their_delayed_payment_basepoint = read_option!();
-               let their_htlc_basepoint = read_option!();
-               let their_cur_commitment_point = read_option!();
+               let their_funding_pubkey = Readable::read(reader)?;
+               let their_revocation_basepoint = Readable::read(reader)?;
+               let their_payment_basepoint = Readable::read(reader)?;
+               let their_delayed_payment_basepoint = Readable::read(reader)?;
+               let their_htlc_basepoint = Readable::read(reader)?;
+               let their_cur_commitment_point = Readable::read(reader)?;
 
-               let their_prev_commitment_point = read_option!();
+               let their_prev_commitment_point = Readable::read(reader)?;
                let their_node_id = Readable::read(reader)?;
 
-               let their_shutdown_scriptpubkey = read_option!();
+               let their_shutdown_scriptpubkey = Readable::read(reader)?;
                let (monitor_last_block, channel_monitor) = ReadableArgs::read(reader, logger.clone())?;
                // We drop the ChannelMonitor's last block connected hash cause we don't actually bother
                // doing full block connection operations on the internal CHannelMonitor copies
@@ -3943,7 +3987,7 @@ mod tests {
        #[test]
        fn test_max_funding_satoshis() {
                assert!(MAX_FUNDING_SATOSHIS <= 21_000_000 * 100_000_000,
-                       "MAX_FUNDING_SATOSHIS is greater than all satoshis on existence");
+                       "MAX_FUNDING_SATOSHIS is greater than all satoshis in existence");
        }
 
        struct Keys {
@@ -3966,6 +4010,7 @@ mod tests {
 
                fn get_channel_keys(&self, _inbound: bool) -> ChannelKeys { self.chan_keys.clone() }
                fn get_session_key(&self) -> SecretKey { panic!(); }
+               fn get_channel_id(&self) -> [u8; 32] { [0; 32] }
        }
 
        #[test]