Explicitly support counterparty setting 0 channel reserve
[rust-lightning] / lightning / src / ln / channelmanager.rs
index 17a287c74c0859180292dfd21c9a85c6d5b10fda..0eaf244d0e47653cc04b2cd0c508f519820f5c78 100644 (file)
@@ -413,6 +413,9 @@ struct PeerState {
 ///
 /// For users who don't want to bother doing their own payment preimage storage, we also store that
 /// here.
+///
+/// Note that this struct will be removed entirely soon, in favor of storing no inbound payment data
+/// and instead encoding it in the payment secret.
 struct PendingInboundPayment {
        /// The payment secret that the sender must use for us to accept this payment
        payment_secret: PaymentSecret,
@@ -449,6 +452,7 @@ pub(crate) enum PendingOutboundPayment {
        /// and add a pending payment that was already fulfilled.
        Fulfilled {
                session_privs: HashSet<[u8; 32]>,
+               payment_hash: Option<PaymentHash>,
        },
 }
 
@@ -472,15 +476,24 @@ impl PendingOutboundPayment {
                }
        }
 
+       fn payment_hash(&self) -> Option<PaymentHash> {
+               match self {
+                       PendingOutboundPayment::Legacy { .. } => None,
+                       PendingOutboundPayment::Retryable { payment_hash, .. } => Some(*payment_hash),
+                       PendingOutboundPayment::Fulfilled { payment_hash, .. } => *payment_hash,
+               }
+       }
+
        fn mark_fulfilled(&mut self) {
                let mut session_privs = HashSet::new();
                core::mem::swap(&mut session_privs, match self {
                        PendingOutboundPayment::Legacy { session_privs } |
                        PendingOutboundPayment::Retryable { session_privs, .. } |
-                       PendingOutboundPayment::Fulfilled { session_privs }
+                       PendingOutboundPayment::Fulfilled { session_privs, .. }
                                => session_privs
                });
-               *self = PendingOutboundPayment::Fulfilled { session_privs };
+               let payment_hash = self.payment_hash();
+               *self = PendingOutboundPayment::Fulfilled { session_privs, payment_hash };
        }
 
        /// panics if path is None and !self.is_fulfilled
@@ -488,7 +501,7 @@ impl PendingOutboundPayment {
                let remove_res = match self {
                        PendingOutboundPayment::Legacy { session_privs } |
                        PendingOutboundPayment::Retryable { session_privs, .. } |
-                       PendingOutboundPayment::Fulfilled { session_privs } => {
+                       PendingOutboundPayment::Fulfilled { session_privs, .. } => {
                                session_privs.remove(session_priv)
                        }
                };
@@ -529,7 +542,7 @@ impl PendingOutboundPayment {
                match self {
                        PendingOutboundPayment::Legacy { session_privs } |
                        PendingOutboundPayment::Retryable { session_privs, .. } |
-                       PendingOutboundPayment::Fulfilled { session_privs } => {
+                       PendingOutboundPayment::Fulfilled { session_privs, .. } => {
                                session_privs.len()
                        }
                }
@@ -1141,7 +1154,7 @@ macro_rules! handle_monitor_err {
                res
        } };
        ($self: ident, $err: expr, $channel_state: expr, $entry: expr, $action_type: path, $resend_raa: expr, $resend_commitment: expr, $failed_forwards: expr, $failed_fails: expr) => {
-               handle_monitor_err!($self, $err, $channel_state, $entry, $action_type, $resend_raa, $resend_commitment, $failed_forwards, $failed_fails, Vec::new());
+               handle_monitor_err!($self, $err, $channel_state, $entry, $action_type, $resend_raa, $resend_commitment, $failed_forwards, $failed_fails, Vec::new())
        }
 }
 
@@ -1398,7 +1411,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                        let peer_state = peer_state.lock().unwrap();
                                        let their_features = &peer_state.latest_features;
                                        let config = if override_config.is_some() { override_config.as_ref().unwrap() } else { &self.default_configuration };
-                                       Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, their_features, channel_value_satoshis, push_msat, user_channel_id, config)?
+                                       Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, their_features,
+                                               channel_value_satoshis, push_msat, user_channel_id, config, self.best_block.read().unwrap().height())?
                                },
                                None => return Err(APIError::ChannelUnavailable { err: format!("Not connected to node: {}", their_network_key) }),
                        }
@@ -1959,17 +1973,24 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
                                        let cur_height = self.best_block.read().unwrap().height() + 1;
-                                       // Theoretically, channel counterparty shouldn't send us a HTLC expiring now, but we want to be robust wrt to counterparty
-                                       // packet sanitization (see HTLC_FAIL_BACK_BUFFER rational)
+                                       // Theoretically, channel counterparty shouldn't send us a HTLC expiring now,
+                                       // but we want to be robust wrt to counterparty packet sanitization (see
+                                       // HTLC_FAIL_BACK_BUFFER rationale).
                                        if msg.cltv_expiry <= cur_height + HTLC_FAIL_BACK_BUFFER as u32 { // expiry_too_soon
                                                break Some(("CLTV expiry is too close", 0x1000 | 14, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
                                        if msg.cltv_expiry > cur_height + CLTV_FAR_FAR_AWAY as u32 { // expiry_too_far
                                                break Some(("CLTV expiry is too far in the future", 21, None));
                                        }
-                                       // In theory, we would be safe against unintentional channel-closure, if we only required a margin of LATENCY_GRACE_PERIOD_BLOCKS.
-                                       // But, to be safe against policy reception, we use a longer delay.
-                                       if (*outgoing_cltv_value) as u64 <= (cur_height + HTLC_FAIL_BACK_BUFFER) as u64 {
+                                       // If the HTLC expires ~now, don't bother trying to forward it to our
+                                       // counterparty. They should fail it anyway, but we don't want to bother with
+                                       // the round-trips or risk them deciding they definitely want the HTLC and
+                                       // force-closing to ensure they get it if we're offline.
+                                       // We previously had a much more aggressive check here which tried to ensure
+                                       // our counterparty receives an HTLC which has *our* risk threshold met on it,
+                                       // but there is no need to do that, and since we're a bit conservative with our
+                                       // risk threshold it just results in failing to forward payments.
+                                       if (*outgoing_cltv_value) as u64 <= (cur_height + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
                                                break Some(("Outgoing CLTV value is too soon", 0x1000 | 14, Some(self.get_channel_update_for_unicast(chan).unwrap())));
                                        }
 
@@ -2085,6 +2106,21 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                Some(id) => id.clone(),
                        };
 
+                       macro_rules! insert_outbound_payment {
+                               () => {
+                                       let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
+                                               session_privs: HashSet::new(),
+                                               pending_amt_msat: 0,
+                                               pending_fee_msat: Some(0),
+                                               payment_hash: *payment_hash,
+                                               payment_secret: *payment_secret,
+                                               starting_block_height: self.best_block.read().unwrap().height(),
+                                               total_msat: total_value,
+                                       });
+                                       assert!(payment.insert(session_priv_bytes, path));
+                               }
+                       }
+
                        let channel_state = &mut *channel_lock;
                        if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(id) {
                                match {
@@ -2094,7 +2130,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                        if !chan.get().is_live() {
                                                return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected/pending monitor update!".to_owned()});
                                        }
-                                       let send_res = break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(
+                                       break_chan_entry!(self, chan.get_mut().send_htlc_and_commit(
                                                htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute {
                                                        path: path.clone(),
                                                        session_priv: session_priv.clone(),
@@ -2103,20 +2139,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        payment_secret: payment_secret.clone(),
                                                        payee: payee.clone(),
                                                }, onion_packet, &self.logger),
-                                       channel_state, chan);
-
-                                       let payment = payment_entry.or_insert_with(|| PendingOutboundPayment::Retryable {
-                                               session_privs: HashSet::new(),
-                                               pending_amt_msat: 0,
-                                               pending_fee_msat: Some(0),
-                                               payment_hash: *payment_hash,
-                                               payment_secret: *payment_secret,
-                                               starting_block_height: self.best_block.read().unwrap().height(),
-                                               total_msat: total_value,
-                                       });
-                                       assert!(payment.insert(session_priv_bytes, path));
-
-                                       send_res
+                                       channel_state, chan)
                                } {
                                        Some((update_add, commitment_signed, monitor_update)) => {
                                                if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
@@ -2126,8 +2149,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        // is restored. Therefore, we must return an error indicating that
                                                        // it is unsafe to retry the payment wholesale, which we do in the
                                                        // send_payment check for MonitorUpdateFailed, below.
+                                                       insert_outbound_payment!(); // Only do this after possibly break'ing on Perm failure above.
                                                        return Err(APIError::MonitorUpdateFailed);
                                                }
+                                               insert_outbound_payment!();
 
                                                log_debug!(self.logger, "Sending payment along path resulted in a commitment_signed for channel {}", log_bytes!(chan.get().channel_id()));
                                                channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
@@ -2142,7 +2167,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                        },
                                                });
                                        },
-                                       None => {},
+                                       None => { insert_outbound_payment!(); },
                                }
                        } else { unreachable!(); }
                        return Ok(());
@@ -2275,6 +2300,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                } else { None },
                        })
                } else if has_err {
+                       // If we failed to send any paths, we shouldn't have inserted the new PaymentId into
+                       // our `pending_outbound_payments` map at all.
+                       debug_assert!(self.pending_outbound_payments.lock().unwrap().get(&payment_id).is_none());
                        Err(PaymentSendFailure::AllFailedRetrySafe(results.drain(..).map(|r| r.unwrap_err()).collect()))
                } else {
                        Ok(payment_id)
@@ -2633,7 +2661,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                                                htlc_id: prev_htlc_id,
                                                                                incoming_packet_shared_secret: incoming_shared_secret,
                                                                        });
-                                                                       match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) {
+                                                                       match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet, &self.logger) {
                                                                                Err(e) => {
                                                                                        if let ChannelError::Ignore(msg) = e {
                                                                                                log_trace!(self.logger, "Failed to forward HTLC with payment_hash {}: {}", log_bytes!(payment_hash.0), msg);
@@ -2872,7 +2900,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                                                                        purpose: events::PaymentPurpose::InvoicePayment {
                                                                                                                payment_preimage: inbound_payment.get().payment_preimage,
                                                                                                                payment_secret: payment_data.payment_secret,
-                                                                                                               user_payment_id: inbound_payment.get().user_payment_id,
                                                                                                        },
                                                                                                        amt: total_value,
                                                                                                });
@@ -3476,14 +3503,23 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        }
 
        fn finalize_claims(&self, mut sources: Vec<HTLCSource>) {
+               let mut pending_events = self.pending_events.lock().unwrap();
                for source in sources.drain(..) {
-                       if let HTLCSource::OutboundRoute { session_priv, payment_id, .. } = source {
+                       if let HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } = source {
                                let mut session_priv_bytes = [0; 32];
                                session_priv_bytes.copy_from_slice(&session_priv[..]);
                                let mut outbounds = self.pending_outbound_payments.lock().unwrap();
                                if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
                                        assert!(payment.get().is_fulfilled());
-                                       payment.get_mut().remove(&session_priv_bytes, None);
+                                       if payment.get_mut().remove(&session_priv_bytes, None) {
+                                               pending_events.push(
+                                                       events::Event::PaymentPathSuccessful {
+                                                               payment_id,
+                                                               payment_hash: payment.get().payment_hash(),
+                                                               path,
+                                                       }
+                                               );
+                                       }
                                        if payment.get().remaining_parts() == 0 {
                                                payment.remove();
                                        }
@@ -3500,9 +3536,21 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                session_priv_bytes.copy_from_slice(&session_priv[..]);
                                let mut outbounds = self.pending_outbound_payments.lock().unwrap();
                                if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
-                                       let found_payment = !payment.get().is_fulfilled();
-                                       let fee_paid_msat = payment.get().get_pending_fee_msat();
-                                       payment.get_mut().mark_fulfilled();
+                                       let mut pending_events = self.pending_events.lock().unwrap();
+                                       if !payment.get().is_fulfilled() {
+                                               let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
+                                               let fee_paid_msat = payment.get().get_pending_fee_msat();
+                                               pending_events.push(
+                                                       events::Event::PaymentSent {
+                                                               payment_id: Some(payment_id),
+                                                               payment_preimage,
+                                                               payment_hash,
+                                                               fee_paid_msat,
+                                                       }
+                                               );
+                                               payment.get_mut().mark_fulfilled();
+                                       }
+
                                        if from_onchain {
                                                // We currently immediately remove HTLCs which were fulfilled on-chain.
                                                // This could potentially lead to removing a pending payment too early,
@@ -3510,22 +3558,21 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                // restart.
                                                // TODO: We should have a second monitor event that informs us of payments
                                                // irrevocably fulfilled.
-                                               payment.get_mut().remove(&session_priv_bytes, Some(&path));
+                                               if payment.get_mut().remove(&session_priv_bytes, Some(&path)) {
+                                                       let payment_hash = Some(PaymentHash(Sha256::hash(&payment_preimage.0).into_inner()));
+                                                       pending_events.push(
+                                                               events::Event::PaymentPathSuccessful {
+                                                                       payment_id,
+                                                                       payment_hash,
+                                                                       path,
+                                                               }
+                                                       );
+                                               }
+
                                                if payment.get().remaining_parts() == 0 {
                                                        payment.remove();
                                                }
                                        }
-                                       if found_payment {
-                                               let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
-                                               self.pending_events.lock().unwrap().push(
-                                                       events::Event::PaymentSent {
-                                                               payment_id: Some(payment_id),
-                                                               payment_preimage,
-                                                               payment_hash: payment_hash,
-                                                               fee_paid_msat,
-                                                       }
-                                               );
-                                       }
                                } else {
                                        log_trace!(self.logger, "Received duplicative fulfill for HTLC with payment_preimage {}", log_bytes!(payment_preimage.0));
                                }
@@ -3631,7 +3678,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        return Err(MsgHandleErrInternal::send_err_msg_no_close("Unknown genesis block hash".to_owned(), msg.temporary_channel_id.clone()));
                }
 
-               let channel = Channel::new_from_req(&self.fee_estimator, &self.keys_manager, counterparty_node_id.clone(), &their_features, msg, 0, &self.default_configuration)
+               if !self.default_configuration.accept_inbound_channels {
+                       return Err(MsgHandleErrInternal::send_err_msg_no_close("No inbound channels accepted".to_owned(), msg.temporary_channel_id.clone()));
+               }
+
+               let channel = Channel::new_from_req(&self.fee_estimator, &self.keys_manager, counterparty_node_id.clone(),
+                               &their_features, msg, 0, &self.default_configuration, self.best_block.read().unwrap().height(), &self.logger)
                        .map_err(|e| MsgHandleErrInternal::from_chan_no_close(e, msg.temporary_channel_id))?;
                let mut channel_state_lock = self.channel_state.lock().unwrap();
                let channel_state = &mut *channel_state_lock;
@@ -4503,7 +4555,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                }
        }
 
-       fn set_payment_hash_secret_map(&self, payment_hash: PaymentHash, payment_preimage: Option<PaymentPreimage>, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> Result<PaymentSecret, APIError> {
+       fn set_payment_hash_secret_map(&self, payment_hash: PaymentHash, payment_preimage: Option<PaymentPreimage>, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> Result<PaymentSecret, APIError> {
                assert!(invoice_expiry_delta_secs <= 60*60*24*365); // Sadly bitcoin timestamps are u32s, so panic before 2106
 
                let payment_secret = PaymentSecret(self.keys_manager.get_secure_random_bytes());
@@ -4513,7 +4565,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                match payment_secrets.entry(payment_hash) {
                        hash_map::Entry::Vacant(e) => {
                                e.insert(PendingInboundPayment {
-                                       payment_secret, min_value_msat, user_payment_id, payment_preimage,
+                                       payment_secret, min_value_msat, payment_preimage,
+                                       user_payment_id: 0, // For compatibility with version 0.0.103 and earlier
                                        // We assume that highest_seen_timestamp is pretty close to the current time -
                                        // its updated when we receive a new block with the maximum time we've seen in
                                        // a header. It should never be more than two hours in the future.
@@ -4545,12 +4598,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// [`PaymentReceived`]: events::Event::PaymentReceived
        /// [`PaymentReceived::payment_preimage`]: events::Event::PaymentReceived::payment_preimage
        /// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash
-       pub fn create_inbound_payment(&self, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> (PaymentHash, PaymentSecret) {
+       pub fn create_inbound_payment(&self, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> (PaymentHash, PaymentSecret) {
                let payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes());
                let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
 
                (payment_hash,
-                       self.set_payment_hash_secret_map(payment_hash, Some(payment_preimage), min_value_msat, invoice_expiry_delta_secs, user_payment_id)
+                       self.set_payment_hash_secret_map(payment_hash, Some(payment_preimage), min_value_msat, invoice_expiry_delta_secs)
                                .expect("RNG Generated Duplicate PaymentHash"))
        }
 
@@ -4564,12 +4617,6 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// The [`PaymentHash`] (and corresponding [`PaymentPreimage`]) must be globally unique. This
        /// method may return an Err if another payment with the same payment_hash is still pending.
        ///
-       /// `user_payment_id` will be provided back in [`PaymentPurpose::InvoicePayment::user_payment_id`] events to
-       /// allow tracking of which events correspond with which calls to this and
-       /// [`create_inbound_payment`]. `user_payment_id` has no meaning inside of LDK, it is simply
-       /// copied to events and otherwise ignored. It may be used to correlate PaymentReceived events
-       /// with invoice metadata stored elsewhere.
-       ///
        /// `min_value_msat` should be set if the invoice being generated contains a value. Any payment
        /// received for the returned [`PaymentHash`] will be required to be at least `min_value_msat`
        /// before a [`PaymentReceived`] event will be generated, ensuring that we do not provide the
@@ -4598,9 +4645,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        ///
        /// [`create_inbound_payment`]: Self::create_inbound_payment
        /// [`PaymentReceived`]: events::Event::PaymentReceived
-       /// [`PaymentPurpose::InvoicePayment::user_payment_id`]: events::PaymentPurpose::InvoicePayment::user_payment_id
-       pub fn create_inbound_payment_for_hash(&self, payment_hash: PaymentHash, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> Result<PaymentSecret, APIError> {
-               self.set_payment_hash_secret_map(payment_hash, None, min_value_msat, invoice_expiry_delta_secs, user_payment_id)
+       pub fn create_inbound_payment_for_hash(&self, payment_hash: PaymentHash, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> Result<PaymentSecret, APIError> {
+               self.set_payment_hash_secret_map(payment_hash, None, min_value_msat, invoice_expiry_delta_secs)
        }
 
        #[cfg(any(test, feature = "fuzztarget", feature = "_test_utils"))]
@@ -4615,6 +4661,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        pub fn has_pending_payments(&self) -> bool {
                !self.pending_outbound_payments.lock().unwrap().is_empty()
        }
+
+       #[cfg(test)]
+       pub fn clear_pending_payments(&self) {
+               self.pending_outbound_payments.lock().unwrap().clear()
+       }
 }
 
 impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> MessageSendEventsProvider for ChannelManager<Signer, M, T, K, F, L>
@@ -4836,7 +4887,7 @@ where
        /// Calls a function which handles an on-chain event (blocks dis/connected, transactions
        /// un/confirmed, etc) on each channel, handling any resulting errors or messages generated by
        /// the function.
-       fn do_chain_event<FN: Fn(&mut Channel<Signer>) -> Result<(Option<msgs::FundingLocked>, Vec<(HTLCSource, PaymentHash)>), msgs::ErrorMessage>>
+       fn do_chain_event<FN: Fn(&mut Channel<Signer>) -> Result<(Option<msgs::FundingLocked>, Vec<(HTLCSource, PaymentHash)>), ClosureReason>>
                        (&self, height_opt: Option<u32>, f: FN) {
                // Note that we MUST NOT end up calling methods on self.chain_monitor here - we're called
                // during initialization prior to the chain_monitor being fully configured in some cases.
@@ -4881,7 +4932,7 @@ where
                                                }
                                                short_to_id.insert(channel.get_short_channel_id().unwrap(), channel.channel_id());
                                        }
-                               } else if let Err(e) = res {
+                               } else if let Err(reason) = res {
                                        if let Some(short_id) = channel.get_short_channel_id() {
                                                short_to_id.remove(&short_id);
                                        }
@@ -4893,10 +4944,14 @@ where
                                                        msg: update
                                                });
                                        }
-                                       self.issue_channel_close_events(channel, ClosureReason::CommitmentTxConfirmed);
+                                       let reason_message = format!("{}", reason);
+                                       self.issue_channel_close_events(channel, reason);
                                        pending_msg_events.push(events::MessageSendEvent::HandleError {
                                                node_id: channel.get_counterparty_node_id(),
-                                               action: msgs::ErrorAction::SendErrorMessage { msg: e },
+                                               action: msgs::ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage {
+                                                       channel_id: channel.channel_id(),
+                                                       data: reason_message,
+                                               } },
                                        });
                                        return false;
                                }
@@ -5535,6 +5590,7 @@ impl_writeable_tlv_based_enum_upgradable!(PendingOutboundPayment,
        },
        (1, Fulfilled) => {
                (0, session_privs, required),
+               (1, payment_hash, option),
        },
        (2, Retryable) => {
                (0, session_privs, required),
@@ -5815,7 +5871,7 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
                let mut short_to_id = HashMap::with_capacity(cmp::min(channel_count as usize, 128));
                let mut channel_closures = Vec::new();
                for _ in 0..channel_count {
-                       let mut channel: Channel<Signer> = Channel::read(reader, &args.keys_manager)?;
+                       let mut channel: Channel<Signer> = Channel::read(reader, (&args.keys_manager, best_block_height))?;
                        let funding_txo = channel.get_funding_txo().ok_or(DecodeError::InvalidValue)?;
                        funding_txo_set.insert(funding_txo.clone());
                        if let Some(ref mut monitor) = args.channel_monitors.get_mut(&funding_txo) {
@@ -5850,6 +5906,7 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
                                                reason: ClosureReason::OutdatedChannelManager
                                        });
                                } else {
+                                       log_info!(args.logger, "Successfully loaded channel {}", log_bytes!(channel.channel_id()));
                                        if let Some(short_channel_id) = channel.get_short_channel_id() {
                                                short_to_id.insert(short_channel_id, channel.channel_id());
                                        }
@@ -5867,6 +5924,7 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 
                for (ref funding_txo, ref mut monitor) in args.channel_monitors.iter_mut() {
                        if !funding_txo_set.contains(funding_txo) {
+                               log_info!(args.logger, "Broadcasting latest holder commitment transaction for closed channel {}", log_bytes!(funding_txo.to_channel_id()));
                                monitor.broadcast_latest_holder_commitment_txn(&args.tx_broadcaster, &args.logger);
                        }
                }
@@ -6301,9 +6359,10 @@ mod tests {
                nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_third_raa);
                check_added_monitors!(nodes[0], 1);
 
-               // Note that successful MPP payments will generate 1 event upon the first path's success. No
-               // further events will be generated for subsequence path successes.
+               // Note that successful MPP payments will generate a single PaymentSent event upon the first
+               // path's success and a PaymentPathSuccessful event for each path's success.
                let events = nodes[0].node.get_and_clear_pending_events();
+               assert_eq!(events.len(), 3);
                match events[0] {
                        Event::PaymentSent { payment_id: ref id, payment_preimage: ref preimage, payment_hash: ref hash, .. } => {
                                assert_eq!(Some(payment_id), *id);
@@ -6312,6 +6371,22 @@ mod tests {
                        },
                        _ => panic!("Unexpected event"),
                }
+               match events[1] {
+                       Event::PaymentPathSuccessful { payment_id: ref actual_payment_id, ref payment_hash, ref path } => {
+                               assert_eq!(payment_id, *actual_payment_id);
+                               assert_eq!(our_payment_hash, *payment_hash.as_ref().unwrap());
+                               assert_eq!(route.paths[0], *path);
+                       },
+                       _ => panic!("Unexpected event"),
+               }
+               match events[2] {
+                       Event::PaymentPathSuccessful { payment_id: ref actual_payment_id, ref payment_hash, ref path } => {
+                               assert_eq!(payment_id, *actual_payment_id);
+                               assert_eq!(our_payment_hash, *payment_hash.as_ref().unwrap());
+                               assert_eq!(route.paths[0], *path);
+                       },
+                       _ => panic!("Unexpected event"),
+               }
        }
 
        #[test]
@@ -6538,7 +6613,7 @@ pub mod bench {
        use ln::msgs::{ChannelMessageHandler, Init};
        use routing::network_graph::NetworkGraph;
        use routing::router::{Payee, get_route};
-       use routing::scorer::Scorer;
+       use routing::scoring::Scorer;
        use util::test_utils;
        use util::config::UserConfig;
        use util::events::{Event, MessageSendEvent, MessageSendEventsProvider, PaymentPurpose};
@@ -6646,7 +6721,7 @@ pub mod bench {
                macro_rules! send_payment {
                        ($node_a: expr, $node_b: expr) => {
                                let usable_channels = $node_a.list_usable_channels();
-                               let payee = Payee::new($node_b.get_our_node_id())
+                               let payee = Payee::from_node_id($node_b.get_our_node_id())
                                        .with_features(InvoiceFeatures::known());
                                let scorer = Scorer::with_fixed_penalty(0);
                                let route = get_route(&$node_a.get_our_node_id(), &payee, &dummy_graph,
@@ -6656,7 +6731,7 @@ pub mod bench {
                                payment_preimage.0[0..8].copy_from_slice(&payment_count.to_le_bytes());
                                payment_count += 1;
                                let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).into_inner());
-                               let payment_secret = $node_b.create_inbound_payment_for_hash(payment_hash, None, 7200, 0).unwrap();
+                               let payment_secret = $node_b.create_inbound_payment_for_hash(payment_hash, None, 7200).unwrap();
 
                                $node_a.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap();
                                let payment_event = SendEvent::from_event($node_a.get_and_clear_pending_msg_events().pop().unwrap());