Correct default expiry.
[rust-lightning] / lightning / src / ln / channelmanager.rs
index 0c7fa3e95a3f8d24867b13db514c3c0ac5377473..a0168bdcef5bb86ae54f68b93d537f075e4fda9b 100644 (file)
@@ -43,6 +43,7 @@ use chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitor
 use chain::transaction::{OutPoint, TransactionData};
 // Since this struct is returned in `list_channels` methods, expose it here in case users want to
 // construct one themselves.
+use ln::{PaymentHash, PaymentPreimage, PaymentSecret};
 pub use ln::channel::CounterpartyForwardingInfo;
 use ln::channel::{Channel, ChannelError};
 use ln::features::{InitFeatures, NodeFeatures};
@@ -68,7 +69,6 @@ use std::sync::atomic::{AtomicUsize, Ordering};
 use std::time::Duration;
 #[cfg(any(test, feature = "allow_wallclock_use"))]
 use std::time::Instant;
-use std::marker::{Sync, Send};
 use std::ops::Deref;
 use bitcoin::hashes::hex::ToHex;
 
@@ -96,7 +96,7 @@ enum PendingHTLCRouting {
                short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
        },
        Receive {
-               payment_data: Option<msgs::FinalOnionHopData>,
+               payment_data: msgs::FinalOnionHopData,
                incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
        },
 }
@@ -156,11 +156,10 @@ pub(crate) struct HTLCPreviousHopData {
 struct ClaimableHTLC {
        prev_hop: HTLCPreviousHopData,
        value: u64,
-       /// Filled in when the HTLC was received with a payment_secret packet, which contains a
-       /// total_msat (which may differ from value if this is a Multi-Path Payment) and a
+       /// Contains a total_msat (which may differ from value if this is a Multi-Path Payment) and a
        /// payment_secret which prevents path-probing attacks and can associate different HTLCs which
        /// are part of the same payment.
-       payment_data: Option<msgs::FinalOnionHopData>,
+       payment_data: msgs::FinalOnionHopData,
        cltv_expiry: u32,
 }
 
@@ -198,19 +197,6 @@ pub(super) enum HTLCFailReason {
        }
 }
 
-/// payment_hash type, use to cross-lock hop
-/// (C-not exported) as we just use [u8; 32] directly
-#[derive(Hash, Copy, Clone, PartialEq, Eq, Debug)]
-pub struct PaymentHash(pub [u8;32]);
-/// payment_preimage type, use to route payment between hop
-/// (C-not exported) as we just use [u8; 32] directly
-#[derive(Hash, Copy, Clone, PartialEq, Eq, Debug)]
-pub struct PaymentPreimage(pub [u8;32]);
-/// payment_secret type, use to authenticate sender to the receiver and tie MPP HTLCs together
-/// (C-not exported) as we just use [u8; 32] directly
-#[derive(Hash, Copy, Clone, PartialEq, Eq, Debug)]
-pub struct PaymentSecret(pub [u8;32]);
-
 type ShutdownResult = (Option<(OutPoint, ChannelMonitorUpdate)>, Vec<(HTLCSource, PaymentHash)>);
 
 /// Error type returned across the channel_state mutex boundary. When an Err is generated for a
@@ -327,12 +313,11 @@ pub(super) struct ChannelHolder<Signer: Sign> {
        /// guarantees are made about the existence of a channel with the short id here, nor the short
        /// ids in the PendingHTLCInfo!
        pub(super) forward_htlcs: HashMap<u64, Vec<HTLCForwardInfo>>,
-       /// (payment_hash, payment_secret) -> Vec<HTLCs> for tracking HTLCs that
-       /// were to us and can be failed/claimed by the user
+       /// Map from payment hash to any HTLCs which are to us and can be failed/claimed by the user.
        /// Note that while this is held in the same mutex as the channels themselves, no consistency
        /// guarantees are made about the channels given here actually existing anymore by the time you
        /// go to read them!
-       claimable_htlcs: HashMap<(PaymentHash, Option<PaymentSecret>), Vec<ClaimableHTLC>>,
+       claimable_htlcs: HashMap<PaymentHash, Vec<ClaimableHTLC>>,
        /// Messages to send to peers - pushed to in the same lock that they are generated in (except
        /// for broadcast messages, where ordering isn't as strict).
        pub(super) pending_msg_events: Vec<MessageSendEvent>,
@@ -364,6 +349,8 @@ struct PendingInboundPayment {
        /// Time at which this HTLC expires - blocks with a header time above this value will result in
        /// this payment being removed.
        expiry_time: u64,
+       /// Arbitrary identifier the user specifies (or not)
+       user_payment_id: u64,
        // Other required attributes of the payment, optionally enforced:
        payment_preimage: Option<PaymentPreimage>,
        min_value_msat: Option<u64>,
@@ -451,7 +438,7 @@ pub struct ChannelManager<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref,
        /// Storage for PaymentSecrets and any requirements on future inbound payments before we will
        /// expose them to users via a PaymentReceived event. HTLCs which do not meet the requirements
        /// here are failed when we process them as pending-forwardable-HTLCs, and entries are removed
-       /// after we generate a PaymentReceived upon receipt of all MPP parts.
+       /// after we generate a PaymentReceived upon receipt of all MPP parts or when they time out.
        /// Locked *after* channel_state.
        pending_inbound_payments: Mutex<HashMap<PaymentHash, PendingInboundPayment>>,
 
@@ -576,7 +563,7 @@ pub const BREAKDOWN_TIMEOUT: u16 = 6 * 24;
 pub(crate) const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 2 * 6 * 24 * 7;
 
 /// The minimum number of blocks between an inbound HTLC's CLTV and the corresponding outbound
-/// HTLC's CLTV. The current default represents roughly six hours of blocks at six blocks/hour.
+/// HTLC's CLTV. The current default represents roughly seven hours of blocks at six blocks/hour.
 ///
 /// This can be increased (but not decreased) through [`ChannelConfig::cltv_expiry_delta`]
 ///
@@ -585,9 +572,17 @@ pub(crate) const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 2 * 6 * 24 * 7;
 // i.e. the node we forwarded the payment on to should always have enough room to reliably time out
 // the HTLC via a full update_fail_htlc/commitment_signed dance before we hit the
 // CLTV_CLAIM_BUFFER point (we static assert that it's at least 3 blocks more).
-pub const MIN_CLTV_EXPIRY_DELTA: u16 = 6 * 6;
+pub const MIN_CLTV_EXPIRY_DELTA: u16 = 6*7;
 pub(super) const CLTV_FAR_FAR_AWAY: u32 = 6 * 24 * 7; //TODO?
 
+/// Minimum CLTV difference between the current block height and received inbound payments.
+/// Invoices generated for payment to us must set their `min_final_cltv_expiry` field to at least
+/// this value.
+// Note that we fail if exactly HTLC_FAIL_BACK_BUFFER + 1 was used, so we need to add one for
+// any payments to succeed. Further, we don't want payments to fail if a block was found while
+// a payment was being routed, so we add an extra block to be safe.
+pub const MIN_FINAL_CLTV_EXPIRY: u32 = HTLC_FAIL_BACK_BUFFER + 3;
+
 // Check that our CLTV_EXPIRY is at least CLTV_CLAIM_BUFFER + ANTI_REORG_DELAY + LATENCY_GRACE_PERIOD_BLOCKS,
 // ie that if the next-hop peer fails the HTLC within
 // LATENCY_GRACE_PERIOD_BLOCKS then we'll still have CLTV_CLAIM_BUFFER left to timeout it onchain,
@@ -598,7 +593,7 @@ pub(super) const CLTV_FAR_FAR_AWAY: u32 = 6 * 24 * 7; //TODO?
 #[allow(dead_code)]
 const CHECK_CLTV_EXPIRY_SANITY: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - CLTV_CLAIM_BUFFER - ANTI_REORG_DELAY - LATENCY_GRACE_PERIOD_BLOCKS;
 
-// Check for ability of an attacker to make us fail on-chain by delaying inbound claim. See
+// Check for ability of an attacker to make us fail on-chain by delaying an HTLC claim. See
 // ChannelMontior::would_broadcast_at_height for a description of why this is needed.
 #[deny(const_err)]
 #[allow(dead_code)]
@@ -612,6 +607,12 @@ pub struct ChannelDetails {
        /// Note that this means this value is *not* persistent - it can change once during the
        /// lifetime of the channel.
        pub channel_id: [u8; 32],
+       /// The Channel's funding transaction output, if we've negotiated the funding transaction with
+       /// our counterparty already.
+       ///
+       /// Note that, if this has been set, `channel_id` will be equivalent to
+       /// `funding_txo.unwrap().to_channel_id()`.
+       pub funding_txo: Option<OutPoint>,
        /// The position of the funding transaction in the chain. None if the funding transaction has
        /// not yet been confirmed and the channel fully opened.
        pub short_channel_id: Option<u64>,
@@ -636,10 +637,21 @@ pub struct ChannelDetails {
        /// Note that there are some corner cases not fully handled here, so the actual available
        /// inbound capacity may be slightly higher than this.
        pub inbound_capacity_msat: u64,
+       /// True if the channel was initiated (and thus funded) by us.
+       pub is_outbound: bool,
+       /// True if the channel is confirmed, funding_locked messages have been exchanged, and the
+       /// channel is not currently being shut down. `funding_locked` message exchange implies the
+       /// required confirmation count has been reached (and we were connected to the peer at some
+       /// point after the funding transaction received enough confirmations).
+       pub is_funding_locked: bool,
        /// True if the channel is (a) confirmed and funding_locked messages have been exchanged, (b)
-       /// the peer is connected, and (c) no monitor update failure is pending resolution.
-       pub is_live: bool,
-
+       /// the peer is connected, (c) no monitor update failure is pending resolution, and (d) the
+       /// channel is not currently negotiating a shutdown.
+       ///
+       /// This is a strict superset of `is_funding_locked`.
+       pub is_usable: bool,
+       /// True if this channel is (or will be) publicly-announced.
+       pub is_public: bool,
        /// Information on the fees and requirements that the counterparty requires when forwarding
        /// payments to us through this channel.
        pub counterparty_forwarding_info: Option<CounterpartyForwardingInfo>,
@@ -962,6 +974,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                let (inbound_capacity_msat, outbound_capacity_msat) = channel.get_inbound_outbound_available_balance_msat();
                                res.push(ChannelDetails {
                                        channel_id: (*channel_id).clone(),
+                                       funding_txo: channel.get_funding_txo(),
                                        short_channel_id: channel.get_short_channel_id(),
                                        remote_network_id: channel.get_counterparty_node_id(),
                                        counterparty_features: InitFeatures::empty(),
@@ -969,7 +982,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                        inbound_capacity_msat,
                                        outbound_capacity_msat,
                                        user_id: channel.get_user_id(),
-                                       is_live: channel.is_live(),
+                                       is_outbound: channel.is_outbound(),
+                                       is_funding_locked: channel.is_usable(),
+                                       is_usable: channel.is_live(),
+                                       is_public: channel.should_announce(),
                                        counterparty_forwarding_info: channel.counterparty_forwarding_info(),
                                });
                        }
@@ -992,8 +1008,9 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// Gets the list of usable channels, in random order. Useful as an argument to
        /// get_route to ensure non-announced channels are used.
        ///
-       /// These are guaranteed to have their is_live value set to true, see the documentation for
-       /// ChannelDetails::is_live for more info on exactly what the criteria are.
+       /// These are guaranteed to have their [`ChannelDetails::is_usable`] value set to true, see the
+       /// documentation for [`ChannelDetails::is_usable`] for more info on exactly what the criteria
+       /// are.
        pub fn list_usable_channels(&self) -> Vec<ChannelDetails> {
                // Note we use is_live here instead of usable which leads to somewhat confused
                // internal/external nomenclature, but that's ok cause that's probably what the user
@@ -1247,6 +1264,10 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                        msgs::OnionHopDataFormat::FinalNode { payment_data } => payment_data,
                                };
 
+                               if payment_data.is_none() {
+                                       return_err!("We require payment_secrets", 0x4000|0x2000|3, &[0;0]);
+                               }
+
                                // Note that we could obviously respond immediately with an update_fulfill_htlc
                                // message, however that would leak that we are the recipient of this payment, so
                                // instead we stay symmetric with the forwarding case, only responding (after a
@@ -1254,7 +1275,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 
                                PendingHTLCStatus::Forward(PendingHTLCInfo {
                                        routing: PendingHTLCRouting::Receive {
-                                               payment_data,
+                                               payment_data: payment_data.unwrap(),
                                                incoming_cltv_expiry: msg.cltv_expiry,
                                        },
                                        payment_hash: msg.payment_hash.clone(),
@@ -1747,13 +1768,17 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// only Tor Onion addresses.
        ///
        /// Panics if addresses is absurdly large (more than 500).
-       pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], addresses: Vec<NetAddress>) {
+       pub fn broadcast_node_announcement(&self, rgb: [u8; 3], alias: [u8; 32], mut addresses: Vec<NetAddress>) {
                let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 
                if addresses.len() > 500 {
                        panic!("More than half the message size was taken up by public addresses!");
                }
 
+               // While all existing nodes handle unsorted addresses just fine, the spec requires that
+               // addresses be sorted for future compatibility.
+               addresses.sort_by_key(|addr| addr.get_id());
+
                let announcement = msgs::UnsignedNodeAnnouncement {
                        features: NodeFeatures::known(),
                        timestamp: self.last_node_announcement_serial.fetch_add(1, Ordering::AcqRel) as u32,
@@ -1948,61 +1973,95 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                                        routing: PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry },
                                                                        incoming_shared_secret, payment_hash, amt_to_forward, .. },
                                                                        prev_funding_outpoint } => {
-                                                               let prev_hop = HTLCPreviousHopData {
-                                                                       short_channel_id: prev_short_channel_id,
-                                                                       outpoint: prev_funding_outpoint,
-                                                                       htlc_id: prev_htlc_id,
-                                                                       incoming_packet_shared_secret: incoming_shared_secret,
-                                                               };
-
-                                                               let mut total_value = 0;
-                                                               let payment_secret_opt =
-                                                                       if let &Some(ref data) = &payment_data { Some(data.payment_secret.clone()) } else { None };
-                                                               let htlcs = channel_state.claimable_htlcs.entry((payment_hash, payment_secret_opt))
-                                                                       .or_insert(Vec::new());
-                                                               htlcs.push(ClaimableHTLC {
-                                                                       prev_hop,
+                                                               let claimable_htlc = ClaimableHTLC {
+                                                                       prev_hop: HTLCPreviousHopData {
+                                                                               short_channel_id: prev_short_channel_id,
+                                                                               outpoint: prev_funding_outpoint,
+                                                                               htlc_id: prev_htlc_id,
+                                                                               incoming_packet_shared_secret: incoming_shared_secret,
+                                                                       },
                                                                        value: amt_to_forward,
                                                                        payment_data: payment_data.clone(),
                                                                        cltv_expiry: incoming_cltv_expiry,
-                                                               });
-                                                               if let &Some(ref data) = &payment_data {
-                                                                       for htlc in htlcs.iter() {
-                                                                               total_value += htlc.value;
-                                                                               if htlc.payment_data.as_ref().unwrap().total_msat != data.total_msat {
-                                                                                       total_value = msgs::MAX_VALUE_MSAT;
-                                                                               }
-                                                                               if total_value >= msgs::MAX_VALUE_MSAT { break; }
-                                                                       }
-                                                                       if total_value >= msgs::MAX_VALUE_MSAT || total_value > data.total_msat  {
-                                                                               for htlc in htlcs.iter() {
-                                                                                       let mut htlc_msat_height_data = byte_utils::be64_to_array(htlc.value).to_vec();
-                                                                                       htlc_msat_height_data.extend_from_slice(
-                                                                                               &byte_utils::be32_to_array(self.best_block.read().unwrap().height()),
-                                                                                       );
-                                                                                       failed_forwards.push((HTLCSource::PreviousHopData(HTLCPreviousHopData {
-                                                                                                       short_channel_id: htlc.prev_hop.short_channel_id,
-                                                                                                       outpoint: prev_funding_outpoint,
-                                                                                                       htlc_id: htlc.prev_hop.htlc_id,
-                                                                                                       incoming_packet_shared_secret: htlc.prev_hop.incoming_packet_shared_secret,
-                                                                                               }), payment_hash,
-                                                                                               HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data }
-                                                                                       ));
-                                                                               }
-                                                                       } else if total_value == data.total_msat {
-                                                                               new_events.push(events::Event::PaymentReceived {
-                                                                                       payment_hash,
-                                                                                       payment_secret: Some(data.payment_secret),
-                                                                                       amt: total_value,
-                                                                               });
+                                                               };
+
+                                                               macro_rules! fail_htlc {
+                                                                       ($htlc: expr) => {
+                                                                               let mut htlc_msat_height_data = byte_utils::be64_to_array($htlc.value).to_vec();
+                                                                               htlc_msat_height_data.extend_from_slice(
+                                                                                       &byte_utils::be32_to_array(self.best_block.read().unwrap().height()),
+                                                                               );
+                                                                               failed_forwards.push((HTLCSource::PreviousHopData(HTLCPreviousHopData {
+                                                                                               short_channel_id: $htlc.prev_hop.short_channel_id,
+                                                                                               outpoint: prev_funding_outpoint,
+                                                                                               htlc_id: $htlc.prev_hop.htlc_id,
+                                                                                               incoming_packet_shared_secret: $htlc.prev_hop.incoming_packet_shared_secret,
+                                                                                       }), payment_hash,
+                                                                                       HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data }
+                                                                               ));
                                                                        }
-                                                               } else {
-                                                                       new_events.push(events::Event::PaymentReceived {
-                                                                               payment_hash,
-                                                                               payment_secret: None,
-                                                                               amt: amt_to_forward,
-                                                                       });
                                                                }
+
+                                                               // Check that the payment hash and secret are known. Note that we
+                                                               // MUST take care to handle the "unknown payment hash" and
+                                                               // "incorrect payment secret" cases here identically or we'd expose
+                                                               // that we are the ultimate recipient of the given payment hash.
+                                                               // Further, we must not expose whether we have any other HTLCs
+                                                               // associated with the same payment_hash pending or not.
+                                                               let mut payment_secrets = self.pending_inbound_payments.lock().unwrap();
+                                                               match payment_secrets.entry(payment_hash) {
+                                                                       hash_map::Entry::Vacant(_) => {
+                                                                               log_trace!(self.logger, "Failing new HTLC with payment_hash {} as we didn't have a corresponding inbound payment.", log_bytes!(payment_hash.0));
+                                                                               fail_htlc!(claimable_htlc);
+                                                                       },
+                                                                       hash_map::Entry::Occupied(inbound_payment) => {
+                                                                               if inbound_payment.get().payment_secret != payment_data.payment_secret {
+                                                                                       log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our expected payment secret.", log_bytes!(payment_hash.0));
+                                                                                       fail_htlc!(claimable_htlc);
+                                                                               } else if inbound_payment.get().min_value_msat.is_some() && payment_data.total_msat < inbound_payment.get().min_value_msat.unwrap() {
+                                                                                       log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our minimum value (had {}, needed {}).",
+                                                                                               log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap());
+                                                                                       fail_htlc!(claimable_htlc);
+                                                                               } else {
+                                                                                       let mut total_value = 0;
+                                                                                       let htlcs = channel_state.claimable_htlcs.entry(payment_hash)
+                                                                                               .or_insert(Vec::new());
+                                                                                       htlcs.push(claimable_htlc);
+                                                                                       for htlc in htlcs.iter() {
+                                                                                               total_value += htlc.value;
+                                                                                               if htlc.payment_data.total_msat != payment_data.total_msat {
+                                                                                                       log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the HTLCs had inconsistent total values (eg {} and {})",
+                                                                                                               log_bytes!(payment_hash.0), payment_data.total_msat, htlc.payment_data.total_msat);
+                                                                                                       total_value = msgs::MAX_VALUE_MSAT;
+                                                                                               }
+                                                                                               if total_value >= msgs::MAX_VALUE_MSAT { break; }
+                                                                                       }
+                                                                                       if total_value >= msgs::MAX_VALUE_MSAT || total_value > payment_data.total_msat {
+                                                                                               log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)",
+                                                                                                       log_bytes!(payment_hash.0), total_value, payment_data.total_msat);
+                                                                                               for htlc in htlcs.iter() {
+                                                                                                       fail_htlc!(htlc);
+                                                                                               }
+                                                                                       } else if total_value == payment_data.total_msat {
+                                                                                               new_events.push(events::Event::PaymentReceived {
+                                                                                                       payment_hash,
+                                                                                                       payment_preimage: inbound_payment.get().payment_preimage,
+                                                                                                       payment_secret: payment_data.payment_secret,
+                                                                                                       amt: total_value,
+                                                                                                       user_payment_id: inbound_payment.get().user_payment_id,
+                                                                                               });
+                                                                                               // Only ever generate at most one PaymentReceived
+                                                                                               // per registered payment_hash, even if it isn't
+                                                                                               // claimed.
+                                                                                               inbound_payment.remove_entry();
+                                                                                       } else {
+                                                                                               // Nothing to do - we haven't reached the total
+                                                                                               // payment value yet, wait until we receive more
+                                                                                               // MPP parts.
+                                                                                       }
+                                                                               }
+                                                                       },
+                                                               };
                                                        },
                                                        HTLCForwardInfo::AddHTLC { .. } => {
                                                                panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive");
@@ -2088,11 +2147,11 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// along the path (including in our own channel on which we received it).
        /// Returns false if no payment was found to fail backwards, true if the process of failing the
        /// HTLC backwards has been started.
-       pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash, payment_secret: &Option<PaymentSecret>) -> bool {
+       pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash) -> bool {
                let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 
                let mut channel_state = Some(self.channel_state.lock().unwrap());
-               let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&(*payment_hash, *payment_secret));
+               let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(payment_hash);
                if let Some(mut sources) = removed_source {
                        for htlc in sources.drain(..) {
                                if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
@@ -2257,24 +2316,22 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// generating message events for the net layer to claim the payment, if possible. Thus, you
        /// should probably kick the net layer to go send messages if this returns true!
        ///
-       /// You must specify the expected amounts for this HTLC, and we will only claim HTLCs
-       /// available within a few percent of the expected amount. This is critical for several
-       /// reasons : a) it avoids providing senders with `proof-of-payment` (in the form of the
-       /// payment_preimage without having provided the full value and b) it avoids certain
-       /// privacy-breaking recipient-probing attacks which may reveal payment activity to
-       /// motivated attackers.
-       ///
-       /// Note that the privacy concerns in (b) are not relevant in payments with a payment_secret
-       /// set. Thus, for such payments we will claim any payments which do not under-pay.
+       /// Note that if you did not set an `amount_msat` when calling [`create_inbound_payment`] or
+       /// [`create_inbound_payment_for_hash`] you must check that the amount in the `PaymentReceived`
+       /// event matches your expectation. If you fail to do so and call this method, you may provide
+       /// the sender "proof-of-payment" when they did not fulfill the full expected payment.
        ///
        /// May panic if called except in response to a PaymentReceived event.
-       pub fn claim_funds(&self, payment_preimage: PaymentPreimage, payment_secret: &Option<PaymentSecret>, expected_amount: u64) -> bool {
+       ///
+       /// [`create_inbound_payment`]: Self::create_inbound_payment
+       /// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash
+       pub fn claim_funds(&self, payment_preimage: PaymentPreimage) -> bool {
                let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
 
                let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 
                let mut channel_state = Some(self.channel_state.lock().unwrap());
-               let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&(payment_hash, *payment_secret));
+               let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&payment_hash);
                if let Some(mut sources) = removed_source {
                        assert!(!sources.is_empty());
 
@@ -2289,27 +2346,19 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        // we got all the HTLCs and then a channel closed while we were waiting for the user to
                        // provide the preimage, so worrying too much about the optimal handling isn't worth
                        // it.
-
-                       let (is_mpp, mut valid_mpp) = if let &Some(ref data) = &sources[0].payment_data {
-                               assert!(payment_secret.is_some());
-                               (true, data.total_msat >= expected_amount)
-                       } else {
-                               assert!(payment_secret.is_none());
-                               (false, false)
-                       };
-
+                       let mut valid_mpp = true;
                        for htlc in sources.iter() {
-                               if !is_mpp || !valid_mpp { break; }
                                if let None = channel_state.as_ref().unwrap().short_to_id.get(&htlc.prev_hop.short_channel_id) {
                                        valid_mpp = false;
+                                       break;
                                }
                        }
 
                        let mut errs = Vec::new();
                        let mut claimed_any_htlcs = false;
                        for htlc in sources.drain(..) {
-                               if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
-                               if (is_mpp && !valid_mpp) || (!is_mpp && (htlc.value < expected_amount || htlc.value > expected_amount * 2)) {
+                               if !valid_mpp {
+                                       if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
                                        let mut htlc_msat_height_data = byte_utils::be64_to_array(htlc.value).to_vec();
                                        htlc_msat_height_data.extend_from_slice(&byte_utils::be32_to_array(
                                                        self.best_block.read().unwrap().height()));
@@ -2326,10 +2375,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                                                claimed_any_htlcs = true;
                                                        } else { errs.push(e); }
                                                },
-                                               Err(None) if is_mpp => unreachable!("We already checked for channel existence, we can't fail here!"),
-                                               Err(None) => {
-                                                       log_warn!(self.logger, "Channel we expected to claim an HTLC from was closed.");
-                                               },
+                                               Err(None) => unreachable!("We already checked for channel existence, we can't fail here!"),
                                                Ok(()) => claimed_any_htlcs = true,
                                        }
                                }
@@ -2519,6 +2565,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                },
                        }
                        if let Some(tx) = funding_broadcastable {
+                               log_info!(self.logger, "Broadcasting funding transaction with txid {}", tx.txid());
                                self.tx_broadcaster.broadcast_transaction(&tx);
                        }
                        if let Some(msg) = funding_locked {
@@ -2674,6 +2721,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                                hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
                        }
                };
+               log_info!(self.logger, "Broadcasting funding transaction with txid {}", funding_tx.txid());
                self.tx_broadcaster.broadcast_transaction(&funding_tx);
                Ok(())
        }
@@ -2788,7 +2836,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
                        }
                };
                if let Some(broadcast_tx) = tx {
-                       log_trace!(self.logger, "Broadcast onchain {}", log_tx!(broadcast_tx));
+                       log_info!(self.logger, "Broadcasting {}", log_tx!(broadcast_tx));
                        self.tx_broadcaster.broadcast_transaction(&broadcast_tx);
                }
                if let Some(chan) = chan_option {
@@ -3354,7 +3402,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) -> 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, user_payment_id: u64) -> 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());
@@ -3364,7 +3412,7 @@ 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, payment_preimage,
+                                       payment_secret, min_value_msat, user_payment_id, payment_preimage,
                                        // 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.
@@ -3386,15 +3434,22 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// This differs from [`create_inbound_payment_for_hash`] only in that it generates the
        /// [`PaymentHash`] and [`PaymentPreimage`] for you, returning the first and storing the second.
        ///
+       /// The [`PaymentPreimage`] will ultimately be returned to you in the [`PaymentReceived`], which
+       /// will have the [`PaymentReceived::payment_preimage`] field filled in. That should then be
+       /// passed directly to [`claim_funds`].
+       ///
        /// See [`create_inbound_payment_for_hash`] for detailed documentation on behavior and requirements.
        ///
+       /// [`claim_funds`]: Self::claim_funds
+       /// [`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) -> (PaymentHash, PaymentSecret) {
+       pub fn create_inbound_payment(&self, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32, user_payment_id: u64) -> (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)
+                       self.set_payment_hash_secret_map(payment_hash, Some(payment_preimage), min_value_msat, invoice_expiry_delta_secs, user_payment_id)
                                .expect("RNG Generated Duplicate PaymentHash"))
        }
 
@@ -3408,6 +3463,12 @@ 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 [`PaymentReceived::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
@@ -3416,7 +3477,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// `invoice_expiry_delta_secs` describes the number of seconds that the invoice is valid for
        /// in excess of the current time. This should roughly match the expiry time set in the invoice.
        /// After this many seconds, we will remove the inbound payment, resulting in any attempts to
-       /// pay the invoice failing. The BOLT spec suggests 7,200 secs as a default validity time for
+       /// pay the invoice failing. The BOLT spec suggests 3,600 secs as a default validity time for
        /// invoices when no timeout is set.
        ///
        /// Note that we use block header time to time-out pending inbound payments (with some margin
@@ -3425,12 +3486,20 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
        /// If you need exact expiry semantics, you should enforce them upon receipt of
        /// [`PaymentReceived`].
        ///
+       /// Pending inbound payments are stored in memory and in serialized versions of this
+       /// [`ChannelManager`]. If potentially unbounded numbers of inbound payments may exist and
+       /// space is limited, you may wish to rate-limit inbound payment creation.
+       ///
        /// May panic if `invoice_expiry_delta_secs` is greater than one year.
        ///
+       /// Note that invoices generated for inbound payments should have their `min_final_cltv_expiry`
+       /// set to at least [`MIN_FINAL_CLTV_EXPIRY`].
+       ///
        /// [`create_inbound_payment`]: Self::create_inbound_payment
        /// [`PaymentReceived`]: events::Event::PaymentReceived
-       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)
+       /// [`PaymentReceived::user_payment_id`]: events::Event::PaymentReceived::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)
        }
 }
 
@@ -3562,6 +3631,10 @@ where
                }
                max_time!(self.last_node_announcement_serial);
                max_time!(self.highest_seen_timestamp);
+               let mut payment_secrets = self.pending_inbound_payments.lock().unwrap();
+               payment_secrets.retain(|_, inbound_payment| {
+                       inbound_payment.expiry_time > header.time as u64
+               });
        }
 
        fn get_relevant_txids(&self) -> Vec<Txid> {
@@ -3659,7 +3732,7 @@ where
                        });
 
                        if let Some(height) = height_opt {
-                               channel_state.claimable_htlcs.retain(|&(ref payment_hash, _), htlcs| {
+                               channel_state.claimable_htlcs.retain(|payment_hash, htlcs| {
                                        htlcs.retain(|htlc| {
                                                // If height is approaching the number of blocks we think it takes us to get
                                                // our commitment transaction confirmed before the HTLC expires, plus the
@@ -3713,7 +3786,7 @@ where
        }
 }
 
-impl<Signer: Sign, M: Deref + Sync + Send, T: Deref + Sync + Send, K: Deref + Sync + Send, F: Deref + Sync + Send, L: Deref + Sync + Send>
+impl<Signer: Sign, M: Deref , T: Deref , K: Deref , F: Deref , L: Deref >
        ChannelMessageHandler for ChannelManager<Signer, M, T, K, F, L>
        where M::Target: chain::Watch<Signer>,
         T::Target: BroadcasterInterface,
@@ -4033,7 +4106,8 @@ impl Writeable for PendingHTLCInfo {
                        },
                        &PendingHTLCRouting::Receive { ref payment_data, ref incoming_cltv_expiry } => {
                                1u8.write(writer)?;
-                               payment_data.write(writer)?;
+                               payment_data.payment_secret.write(writer)?;
+                               payment_data.total_msat.write(writer)?;
                                incoming_cltv_expiry.write(writer)?;
                        },
                }
@@ -4054,7 +4128,10 @@ impl Readable for PendingHTLCInfo {
                                        short_channel_id: Readable::read(reader)?,
                                },
                                1u8 => PendingHTLCRouting::Receive {
-                                       payment_data: Readable::read(reader)?,
+                                       payment_data: msgs::FinalOnionHopData {
+                                               payment_secret: Readable::read(reader)?,
+                                               total_msat: Readable::read(reader)?,
+                                       },
                                        incoming_cltv_expiry: Readable::read(reader)?,
                                },
                                _ => return Err(DecodeError::InvalidValue),
@@ -4126,12 +4203,29 @@ impl_writeable!(HTLCPreviousHopData, 0, {
        incoming_packet_shared_secret
 });
 
-impl_writeable!(ClaimableHTLC, 0, {
-       prev_hop,
-       value,
-       payment_data,
-       cltv_expiry
-});
+impl Writeable for ClaimableHTLC {
+       fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
+               self.prev_hop.write(writer)?;
+               self.value.write(writer)?;
+               self.payment_data.payment_secret.write(writer)?;
+               self.payment_data.total_msat.write(writer)?;
+               self.cltv_expiry.write(writer)
+       }
+}
+
+impl Readable for ClaimableHTLC {
+       fn read<R: ::std::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+               Ok(ClaimableHTLC {
+                       prev_hop: Readable::read(reader)?,
+                       value: Readable::read(reader)?,
+                       payment_data: msgs::FinalOnionHopData {
+                               payment_secret: Readable::read(reader)?,
+                               total_msat: Readable::read(reader)?,
+                       },
+                       cltv_expiry: Readable::read(reader)?,
+               })
+       }
+}
 
 impl Writeable for HTLCSource {
        fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
@@ -4236,6 +4330,7 @@ impl Readable for HTLCForwardInfo {
 impl_writeable!(PendingInboundPayment, 0, {
        payment_secret,
        expiry_time,
+       user_payment_id,
        payment_preimage,
        min_value_msat
 });
@@ -4769,16 +4864,20 @@ pub mod bench {
 
                let dummy_graph = NetworkGraph::new(genesis_hash);
 
+               let mut payment_count: u64 = 0;
                macro_rules! send_payment {
                        ($node_a: expr, $node_b: expr) => {
                                let usable_channels = $node_a.list_usable_channels();
                                let route = get_route(&$node_a.get_our_node_id(), &dummy_graph, &$node_b.get_our_node_id(), Some(InvoiceFeatures::known()),
                                        Some(&usable_channels.iter().map(|r| r).collect::<Vec<_>>()), &[], 10_000, TEST_FINAL_CLTV, &logger_a).unwrap();
 
-                               let payment_preimage = PaymentPreimage([0; 32]);
+                               let mut payment_preimage = PaymentPreimage([0; 32]);
+                               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();
 
-                               $node_a.send_payment(&route, payment_hash, &None).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());
                                $node_b.handle_update_add_htlc(&$node_a.get_our_node_id(), &payment_event.msgs[0]);
                                $node_b.handle_commitment_signed(&$node_a.get_our_node_id(), &payment_event.commitment_msg);
@@ -4788,8 +4887,8 @@ pub mod bench {
                                $node_b.handle_revoke_and_ack(&$node_a.get_our_node_id(), &get_event_msg!(NodeHolder { node: &$node_a }, MessageSendEvent::SendRevokeAndACK, $node_b.get_our_node_id()));
 
                                expect_pending_htlcs_forwardable!(NodeHolder { node: &$node_b });
-                               expect_payment_received!(NodeHolder { node: &$node_b }, payment_hash, 10_000);
-                               assert!($node_b.claim_funds(payment_preimage, &None, 10_000));
+                               expect_payment_received!(NodeHolder { node: &$node_b }, payment_hash, payment_secret, 10_000);
+                               assert!($node_b.claim_funds(payment_preimage));
 
                                match $node_b.get_and_clear_pending_msg_events().pop().unwrap() {
                                        MessageSendEvent::UpdateHTLCs { node_id, updates } => {