Add fee spike buffer + incl commit tx fee in chan reserve calculation
authorValentine Wallace <vwallace@protonmail.com>
Wed, 6 May 2020 23:18:23 +0000 (19:18 -0400)
committerValentine Wallace <vwallace@protonmail.com>
Mon, 15 Jun 2020 19:51:09 +0000 (15:51 -0400)
When we receive an inbound HTLC from a peer on an inbound channel,
make sure the funder can still cover the additional on-chain cost
of the HTLC while maintaining their channel reserve.

When we're sending an outbound HTLC, make sure the funder can still
cover the additional on-chain cost of the HTLC while maintaining
their channel reserve.

+ implement fee spike buffer for channel initiators sending payments.
Also add an additional spec-deviating fee spike buffer on the
receiving side (but don't close the channel if this reserve is
violated, just fail the HTLC).
From lightning-rfc PR #740.

Co-authored-by: Matt Corallo <git@bluematt.me>
Co-authored-by: Valentine Wallace <vwallace@protonmail.com>
lightning/src/ln/channel.rs
lightning/src/ln/functional_tests.rs

index f792a0a5e3963f255947eb8c894ac2eef3b10e28..f2695a8ff77d115ddec45429dbefac994fa24c50 100644 (file)
@@ -44,6 +44,7 @@ pub struct ChannelValueStat {
        pub pending_inbound_htlcs_amount_msat: u64,
        pub holding_cell_outbound_amount_msat: u64,
        pub their_max_htlc_value_in_flight_msat: u64, // outgoing
+       pub their_dust_limit_msat: u64,
 }
 
 enum InboundHTLCRemovalReason {
@@ -1663,6 +1664,73 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                cmp::min(self.value_to_self_msat as i64 - self.get_outbound_pending_htlc_stats().1 as i64, 0) as u64)
        }
 
+       // Get the fee cost of a commitment tx with a given number of HTLC outputs.
+       // Note that num_htlcs should not include dust HTLCs.
+       fn commit_tx_fee_msat(&self, num_htlcs: usize) -> u64 {
+               // Note that we need to divide before multiplying to round properly,
+               // since the lowest denomination of bitcoin on-chain is the satoshi.
+               (COMMITMENT_TX_BASE_WEIGHT + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * self.feerate_per_kw / 1000 * 1000
+       }
+
+       // Get the commitment tx fee for the local (i.e our) next commitment transaction
+       // based on the number of pending HTLCs that are on track to be in our next
+       // commitment tx. `addl_htcs` is an optional parameter allowing the caller
+       // to add a number of additional HTLCs to the calculation. Note that dust
+       // HTLCs are excluded.
+       fn next_local_commit_tx_fee_msat(&self, addl_htlcs: usize) -> u64 {
+               assert!(self.channel_outbound);
+
+               let mut their_acked_htlcs = self.pending_inbound_htlcs.len();
+               for ref htlc in self.pending_outbound_htlcs.iter() {
+                       if htlc.amount_msat / 1000 <= self.our_dust_limit_satoshis {
+                               continue
+                       }
+                       match htlc.state {
+                               OutboundHTLCState::Committed => their_acked_htlcs += 1,
+                               OutboundHTLCState::RemoteRemoved {..} => their_acked_htlcs += 1,
+                               OutboundHTLCState::LocalAnnounced {..} => their_acked_htlcs += 1,
+                               _ => {},
+                       }
+               }
+
+               for htlc in self.holding_cell_htlc_updates.iter() {
+                       match htlc {
+                               &HTLCUpdateAwaitingACK::AddHTLC { .. } => their_acked_htlcs += 1,
+                               _ => {},
+                       }
+               }
+
+               self.commit_tx_fee_msat(their_acked_htlcs + addl_htlcs)
+       }
+
+       // Get the commitment tx fee for the remote's next commitment transaction
+       // based on the number of pending HTLCs that are on track to be in their
+       // next commitment tx. `addl_htcs` is an optional parameter allowing the caller
+       // to add a number of additional HTLCs to the calculation. Note that dust HTLCs
+       // are excluded.
+       fn next_remote_commit_tx_fee_msat(&self, addl_htlcs: usize) -> u64 {
+               assert!(!self.channel_outbound);
+
+               // When calculating the set of HTLCs which will be included in their next
+               // commitment_signed, all inbound HTLCs are included (as all states imply it will be
+               // included) and only committed outbound HTLCs, see below.
+               let mut their_acked_htlcs = self.pending_inbound_htlcs.len();
+               for ref htlc in self.pending_outbound_htlcs.iter() {
+                       if htlc.amount_msat / 1000 <= self.their_dust_limit_satoshis {
+                               continue
+                       }
+                       // We only include outbound HTLCs if it will not be included in their next
+                       // commitment_signed, i.e. if they've responded to us with an RAA after announcement.
+                       match htlc.state {
+                               OutboundHTLCState::Committed => their_acked_htlcs += 1,
+                               OutboundHTLCState::RemoteRemoved {..} => their_acked_htlcs += 1,
+                               _ => {},
+                       }
+               }
+
+               self.commit_tx_fee_msat(their_acked_htlcs + addl_htlcs)
+       }
+
        pub fn update_add_htlc<F>(&mut self, msg: &msgs::UpdateAddHTLC, mut pending_forward_status: PendingHTLCStatus, create_pending_htlc_status: F) -> Result<(), ChannelError>
        where F: for<'a> Fn(&'a Self, PendingHTLCStatus, u16) -> PendingHTLCStatus {
                // We can't accept HTLCs sent after we've sent a shutdown.
@@ -1716,9 +1784,55 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                                removed_outbound_total_msat += htlc.amount_msat;
                        }
                }
-               if htlc_inbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 + removed_outbound_total_msat {
-                       return Err(ChannelError::Close("Remote HTLC add would put them under their reserve value"));
+
+               let pending_value_to_self_msat =
+                       self.value_to_self_msat + htlc_inbound_value_msat - removed_outbound_total_msat;
+               let pending_remote_value_msat =
+                       self.channel_value_satoshis * 1000 - pending_value_to_self_msat;
+               if pending_remote_value_msat < msg.amount_msat {
+                       return Err(ChannelError::Close("Remote HTLC add would overdraw remaining funds"));
+               }
+
+               // Check that the remote can afford to pay for this HTLC on-chain at the current
+               // feerate_per_kw, while maintaining their channel reserve (as required by the spec).
+               let remote_commit_tx_fee_msat = if self.channel_outbound { 0 } else {
+                       // +1 for this HTLC.
+                       self.next_remote_commit_tx_fee_msat(1)
+               };
+               if pending_remote_value_msat - msg.amount_msat < remote_commit_tx_fee_msat {
+                       return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees"));
+               };
+
+               let chan_reserve_msat =
+                       Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis) * 1000;
+               if pending_remote_value_msat - msg.amount_msat - remote_commit_tx_fee_msat < chan_reserve_msat {
+                       return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value"));
+               }
+
+               if !self.channel_outbound {
+                       // `+1` for this HTLC, `2 *` and `+1` fee spike buffer we keep for the remote. This deviates from the
+                       // spec because in the spec, the fee spike buffer requirement doesn't exist on the receiver's side,
+                       // only on the sender's.
+                       // Note that when we eventually remove support for fee updates and switch to anchor output fees,
+                       // we will drop the `2 *`, since we no longer be as sensitive to fee spikes. But, keep the extra +1
+                       // as we should still be able to afford adding this HTLC plus one more future HTLC, regardless of
+                       // being sensitive to fee spikes.
+                       let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.next_remote_commit_tx_fee_msat(1 + 1);
+                       if pending_remote_value_msat - msg.amount_msat - chan_reserve_msat < remote_fee_cost_incl_stuck_buffer_msat {
+                               // Note that if the pending_forward_status is not updated here, then it's because we're already failing
+                               // the HTLC, i.e. its status is already set to failing.
+                               pending_forward_status = create_pending_htlc_status(self, pending_forward_status, 0x1000|7);
+                       }
+               } else {
+                       // Check that they won't violate our local required channel reserve by adding this HTLC.
+
+                       // +1 for this HTLC.
+                       let local_commit_tx_fee_msat = self.next_local_commit_tx_fee_msat(1);
+                       if self.value_to_self_msat < self.local_channel_reserve_satoshis * 1000 + local_commit_tx_fee_msat {
+                               return Err(ChannelError::Close("Cannot receive value that would put us under local channel reserve value"));
+                       }
                }
+
                if self.next_remote_htlc_id != msg.htlc_id {
                        return Err(ChannelError::Close("Remote skipped HTLC ID"));
                }
@@ -3044,6 +3158,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                                res
                        },
                        their_max_htlc_value_in_flight_msat: self.their_max_htlc_value_in_flight_msat,
+                       their_dust_limit_msat: self.their_dust_limit_satoshis * 1000,
                }
        }
 
@@ -3554,29 +3669,56 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                        return Err(ChannelError::Ignore("Cannot send value that would put us over the max HTLC value in flight our peer will accept"));
                }
 
+               if !self.channel_outbound {
+                       // Check that we won't violate the remote channel reserve by adding this HTLC.
+
+                       let remote_balance_msat = self.channel_value_satoshis * 1000 - self.value_to_self_msat;
+                       let remote_chan_reserve_msat = Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis);
+                       // 1 additional HTLC corresponding to this HTLC.
+                       let remote_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(1);
+                       if remote_balance_msat < remote_chan_reserve_msat + remote_commit_tx_fee_msat {
+                               return Err(ChannelError::Ignore("Cannot send value that would put them under remote channel reserve value"));
+                       }
+               }
+
+               let pending_value_to_self_msat = self.value_to_self_msat - htlc_outbound_value_msat;
+               if pending_value_to_self_msat < amount_msat {
+                       return Err(ChannelError::Ignore("Cannot send value that would overdraw remaining funds"));
+               }
+
+               // The `+1` is for the HTLC currently being added to the commitment tx and
+               // the `2 *` and `+1` are for the fee spike buffer.
+               let local_commit_tx_fee_msat = if self.channel_outbound {
+                       2 * self.next_local_commit_tx_fee_msat(1 + 1)
+               } else { 0 };
+               if pending_value_to_self_msat - amount_msat < local_commit_tx_fee_msat {
+                       return Err(ChannelError::Ignore("Cannot send value that would not leave enough to pay for fees"));
+               }
+
                // Check self.local_channel_reserve_satoshis (the amount we must keep as
                // reserve for the remote to have something to claim if we misbehave)
-               if self.value_to_self_msat < self.local_channel_reserve_satoshis * 1000 + amount_msat + htlc_outbound_value_msat {
+               let chan_reserve_msat = self.local_channel_reserve_satoshis * 1000;
+               if pending_value_to_self_msat - amount_msat - local_commit_tx_fee_msat < chan_reserve_msat {
                        return Err(ChannelError::Ignore("Cannot send value that would put us under local channel reserve value"));
                }
 
                // Now update local state:
                if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
                        self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
-                               amount_msat: amount_msat,
-                               payment_hash: payment_hash,
-                               cltv_expiry: cltv_expiry,
+                               amount_msat,
+                               payment_hash,
+                               cltv_expiry,
                                source,
-                               onion_routing_packet: onion_routing_packet,
+                               onion_routing_packet,
                        });
                        return Ok(None);
                }
 
                self.pending_outbound_htlcs.push(OutboundHTLCOutput {
                        htlc_id: self.next_local_htlc_id,
-                       amount_msat: amount_msat,
+                       amount_msat,
                        payment_hash: payment_hash.clone(),
-                       cltv_expiry: cltv_expiry,
+                       cltv_expiry,
                        state: OutboundHTLCState::LocalAnnounced(Box::new(onion_routing_packet.clone())),
                        source,
                });
@@ -3584,10 +3726,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                let res = msgs::UpdateAddHTLC {
                        channel_id: self.channel_id,
                        htlc_id: self.next_local_htlc_id,
-                       amount_msat: amount_msat,
-                       payment_hash: payment_hash,
-                       cltv_expiry: cltv_expiry,
-                       onion_routing_packet: onion_routing_packet,
+                       amount_msat,
+                       payment_hash,
+                       cltv_expiry,
+                       onion_routing_packet,
                };
                self.next_local_htlc_id += 1;
 
index bd1b1d4f029ee1799ed0d97cfe9e2ca51ff02f3d..6111720177c4a25583e1ec51680da41b19c964cf 100644 (file)
@@ -1520,14 +1520,220 @@ fn test_duplicate_htlc_different_direction_onchain() {
        }
 }
 
-fn do_channel_reserve_test(test_recv: bool) {
+#[test]
+fn test_basic_channel_reserve() {
+       let chanmon_cfgs = create_chanmon_cfgs(2);
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+       let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
+       let logger = test_utils::TestLogger::new();
+
+       let chan_stat = get_channel_value_stat!(nodes[0], chan.2);
+       let channel_reserve = chan_stat.channel_reserve_msat;
+
+       // The 2* and +1 are for the fee spike reserve.
+       let (_, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
+       let commit_tx_fee = 2 * commit_tx_fee_msat(get_feerate!(nodes[0], chan.2), 1 + 1);
+       let max_can_send = 5000000 - channel_reserve - commit_tx_fee;
+       let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
+       let route = get_route(&nodes[0].node.get_our_node_id(), net_graph_msg_handler, &nodes.last().unwrap().node.get_our_node_id(), None, &Vec::new(), max_can_send + 1, TEST_FINAL_CLTV, &logger).unwrap();
+       let err = nodes[0].node.send_payment(&route, our_payment_hash, &None).err().unwrap();
+       match err {
+               PaymentSendFailure::AllFailedRetrySafe(ref fails) => {
+                       match fails[0] {
+                               APIError::ChannelUnavailable{err} =>
+                                       assert_eq!(err, "Cannot send value that would put us under local channel reserve value"),
+                               _ => panic!("Unexpected error variant"),
+                       }
+               },
+               _ => panic!("Unexpected error variant"),
+       }
+       assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+       nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us under local channel reserve value".to_string(), 1);
+
+       send_payment(&nodes[0], &vec![&nodes[1]], max_can_send, max_can_send);
+}
+
+#[test]
+fn test_chan_reserve_violation_outbound_htlc_inbound_chan() {
+       let mut chanmon_cfgs = create_chanmon_cfgs(2);
+       // Set the fee rate for the channel very high, to the point where the fundee
+       // sending any amount would result in a channel reserve violation. In this test
+       // we check that we would be prevented from sending an HTLC in this situation.
+       chanmon_cfgs[0].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 6000 };
+       chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 6000 };
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+       let _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
+       let logger = test_utils::TestLogger::new();
+
+       macro_rules! get_route_and_payment_hash {
+               ($recv_value: expr) => {{
+                       let (payment_preimage, payment_hash) = get_payment_preimage_hash!(nodes[1]);
+                       let net_graph_msg_handler = &nodes[1].net_graph_msg_handler;
+                       let route = get_route(&nodes[1].node.get_our_node_id(), net_graph_msg_handler, &nodes.first().unwrap().node.get_our_node_id(), None, &Vec::new(), $recv_value, TEST_FINAL_CLTV, &logger).unwrap();
+                       (route, payment_hash, payment_preimage)
+               }}
+       };
+
+       let (route, our_payment_hash, _) = get_route_and_payment_hash!(1000);
+       unwrap_send_err!(nodes[1].node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { err },
+               assert_eq!(err, "Cannot send value that would put them under remote channel reserve value"));
+       assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
+       nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put them under remote channel reserve value".to_string(), 1);
+}
+
+#[test]
+fn test_chan_reserve_violation_inbound_htlc_outbound_channel() {
+       let mut chanmon_cfgs = create_chanmon_cfgs(2);
+       // Set the fee rate for the channel very high, to the point where the funder
+       // receiving 1 update_add_htlc would result in them closing the channel due
+       // to channel reserve violation. This close could also happen if the fee went
+       // up a more realistic amount, but many HTLCs were outstanding at the time of
+       // the update_add_htlc.
+       chanmon_cfgs[0].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 6000 };
+       chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 6000 };
+       let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+       let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
+       let logger = test_utils::TestLogger::new();
+
+       macro_rules! get_route_and_payment_hash {
+               ($recv_value: expr) => {{
+                       let (payment_preimage, payment_hash) = get_payment_preimage_hash!(nodes[1]);
+                       let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
+                       let route = get_route(&nodes[1].node.get_our_node_id(), net_graph_msg_handler, &nodes.first().unwrap().node.get_our_node_id(), None, &Vec::new(), $recv_value, TEST_FINAL_CLTV, &logger).unwrap();
+                       (route, payment_hash, payment_preimage)
+               }}
+       };
+
+       let (route, payment_hash, _) = get_route_and_payment_hash!(1000);
+       // Need to manually create the update_add_htlc message to go around the channel reserve check in send_htlc()
+       let secp_ctx = Secp256k1::new();
+       let session_priv = SecretKey::from_slice(&{
+               let mut session_key = [0; 32];
+               let mut rng = thread_rng();
+               rng.fill_bytes(&mut session_key);
+               session_key
+       }).expect("RNG is bad!");
+
+       let cur_height = nodes[1].node.latest_block_height.load(Ordering::Acquire) as u32 + 1;
+
+       let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route.paths[0], &session_priv).unwrap();
+       let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route.paths[0], 1000, &None, cur_height).unwrap();
+       let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash);
+       let msg = msgs::UpdateAddHTLC {
+               channel_id: chan.2,
+               htlc_id: 1,
+               amount_msat: htlc_msat + 1,
+               payment_hash: payment_hash,
+               cltv_expiry: htlc_cltv,
+               onion_routing_packet: onion_packet,
+       };
+
+       nodes[0].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &msg);
+       // Check that the payment failed and the channel is closed in response to the malicious UpdateAdd.
+       nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot receive value that would put us under local channel reserve value".to_string(), 1);
+       assert_eq!(nodes[0].node.list_channels().len(), 0);
+       let err_msg = check_closed_broadcast!(nodes[0], true).unwrap();
+       assert_eq!(err_msg.data, "Cannot receive value that would put us under local channel reserve value");
+       check_added_monitors!(nodes[0], 1);
+}
+
+#[test]
+fn test_chan_reserve_violation_inbound_htlc_inbound_chan() {
+       let chanmon_cfgs = create_chanmon_cfgs(3);
+       let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+       let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+       let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
+       let _ = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
+       let logger = test_utils::TestLogger::new();
+
+       macro_rules! get_route_and_payment_hash {
+               ($recv_value: expr) => {{
+                       let (payment_preimage, payment_hash) = get_payment_preimage_hash!(nodes[0]);
+                       let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
+                       let route = get_route(&nodes[0].node.get_our_node_id(), net_graph_msg_handler, &nodes.last().unwrap().node.get_our_node_id(), None, &Vec::new(), $recv_value, TEST_FINAL_CLTV, &logger).unwrap();
+                       (route, payment_hash, payment_preimage)
+               }}
+       };
+
+       let feemsat = 239;
+       let total_routing_fee_msat = (nodes.len() - 2) as u64 * feemsat;
+       let chan_stat = get_channel_value_stat!(nodes[0], chan.2);
+       let feerate = get_feerate!(nodes[0], chan.2);
+
+       // Add a 2* and +1 for the fee spike reserve.
+       let commit_tx_fee_2_htlc = 2*commit_tx_fee_msat(feerate, 2 + 1);
+       let recv_value_1 = (chan_stat.value_to_self_msat - chan_stat.channel_reserve_msat - total_routing_fee_msat - commit_tx_fee_2_htlc)/2;
+       let amt_msat_1 = recv_value_1 + total_routing_fee_msat;
+
+       // Add a pending HTLC.
+       let (route_1, our_payment_hash_1, _) = get_route_and_payment_hash!(amt_msat_1);
+       let payment_event_1 = {
+               nodes[0].node.send_payment(&route_1, our_payment_hash_1, &None).unwrap();
+               check_added_monitors!(nodes[0], 1);
+
+               let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+               assert_eq!(events.len(), 1);
+               SendEvent::from_event(events.remove(0))
+       };
+       nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event_1.msgs[0]);
 
+       // Attempt to trigger a channel reserve violation --> payment failure.
+       let commit_tx_fee_2_htlcs = commit_tx_fee_msat(feerate, 2);
+       let recv_value_2 = chan_stat.value_to_self_msat - amt_msat_1 - chan_stat.channel_reserve_msat - total_routing_fee_msat - commit_tx_fee_2_htlcs + 1;
+       let amt_msat_2 = recv_value_2 + total_routing_fee_msat;
+       let (route_2, _, _) = get_route_and_payment_hash!(amt_msat_2);
+
+       // Need to manually create the update_add_htlc message to go around the channel reserve check in send_htlc()
+       let secp_ctx = Secp256k1::new();
+       let session_priv = SecretKey::from_slice(&{
+               let mut session_key = [0; 32];
+               let mut rng = thread_rng();
+               rng.fill_bytes(&mut session_key);
+               session_key
+       }).expect("RNG is bad!");
+
+       let cur_height = nodes[0].node.latest_block_height.load(Ordering::Acquire) as u32 + 1;
+
+       let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route_2.paths[0], &session_priv).unwrap();
+       let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route_2.paths[0], recv_value_2, &None, cur_height).unwrap();
+       let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash_1);
+       let msg = msgs::UpdateAddHTLC {
+               channel_id: chan.2,
+               htlc_id: 1,
+               amount_msat: htlc_msat + 1,
+               payment_hash: our_payment_hash_1,
+               cltv_expiry: htlc_cltv,
+               onion_routing_packet: onion_packet,
+       };
+
+       nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &msg);
+       // Check that the payment failed and the channel is closed in response to the malicious UpdateAdd.
+       nodes[1].logger.assert_log("lightning::ln::channelmanager".to_string(), "Remote HTLC add would put them under remote reserve value".to_string(), 1);
+       assert_eq!(nodes[1].node.list_channels().len(), 1);
+       let err_msg = check_closed_broadcast!(nodes[1], true).unwrap();
+       assert_eq!(err_msg.data, "Remote HTLC add would put them under remote reserve value");
+       check_added_monitors!(nodes[1], 1);
+}
+
+fn commit_tx_fee_msat(feerate: u64, num_htlcs: u64) -> u64 {
+       (COMMITMENT_TX_BASE_WEIGHT + num_htlcs * COMMITMENT_TX_WEIGHT_PER_HTLC) * feerate / 1000 * 1000
+}
+
+#[test]
+fn test_channel_reserve_holding_cell_htlcs() {
        let chanmon_cfgs = create_chanmon_cfgs(3);
        let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
        let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
        let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
-       let chan_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1900, 1001, InitFeatures::known(), InitFeatures::known());
-       let chan_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1900, 1001, InitFeatures::known(), InitFeatures::known());
+       let chan_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 190000, 1001, InitFeatures::known(), InitFeatures::known());
+       let chan_2 = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 190000, 1001, InitFeatures::known(), InitFeatures::known());
        let logger = test_utils::TestLogger::new();
 
        let mut stat01 = get_channel_value_stat!(nodes[0], chan_1.2);
@@ -1556,7 +1762,8 @@ fn do_channel_reserve_test(test_recv: bool) {
        }
 
        let feemsat = 239; // somehow we know?
-       let total_fee_msat = (nodes.len() - 2) as u64 * 239;
+       let total_fee_msat = (nodes.len() - 2) as u64 * feemsat;
+       let feerate = get_feerate!(nodes[0], chan_1.2);
 
        let recv_value_0 = stat01.their_max_htlc_value_in_flight_msat - total_fee_msat;
 
@@ -1570,16 +1777,19 @@ fn do_channel_reserve_test(test_recv: bool) {
                nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us over the max HTLC value in flight our peer will accept".to_string(), 1);
        }
 
-       let mut htlc_id = 0;
        // channel reserve is bigger than their_max_htlc_value_in_flight_msat so loop to deplete
        // nodes[0]'s wealth
        loop {
                let amt_msat = recv_value_0 + total_fee_msat;
-               if stat01.value_to_self_msat - amt_msat < stat01.channel_reserve_msat {
+               // 3 for the 3 HTLCs that will be sent, 2* and +1 for the fee spike reserve.
+               // Also, ensure that each payment has enough to be over the dust limit to
+               // ensure it'll be included in each commit tx fee calculation.
+               let commit_tx_fee_all_htlcs = 2*commit_tx_fee_msat(feerate, 3 + 1);
+               let ensure_htlc_amounts_above_dust_buffer = 3 * (stat01.their_dust_limit_msat + 1000);
+               if stat01.value_to_self_msat < stat01.channel_reserve_msat + commit_tx_fee_all_htlcs + ensure_htlc_amounts_above_dust_buffer + amt_msat {
                        break;
                }
                send_payment(&nodes[0], &vec![&nodes[1], &nodes[2]][..], recv_value_0, recv_value_0);
-               htlc_id += 1;
 
                let (stat01_, stat11_, stat12_, stat22_) = (
                        get_channel_value_stat!(nodes[0], chan_1.2),
@@ -1595,18 +1805,19 @@ fn do_channel_reserve_test(test_recv: bool) {
                stat01 = stat01_; stat11 = stat11_; stat12 = stat12_; stat22 = stat22_;
        }
 
-       {
-               let recv_value = stat01.value_to_self_msat - stat01.channel_reserve_msat - total_fee_msat;
-               // attempt to get channel_reserve violation
-               let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value + 1);
-               unwrap_send_err!(nodes[0].node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { err },
-                       assert_eq!(err, "Cannot send value that would put us under local channel reserve value"));
-               assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-               nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us under local channel reserve value".to_string(), 1);
-       }
-
-       // adding pending output
-       let recv_value_1 = (stat01.value_to_self_msat - stat01.channel_reserve_msat - total_fee_msat)/2;
+       // adding pending output.
+       // 2* and +1 HTLCs on the commit tx fee for the fee spike reserve.
+       // The reason we're dividing by two here is as follows: the dividend is the total outbound liquidity
+       // after fees, the channel reserve, and the fee spike buffer are removed. We eventually want to
+       // divide this quantity into 3 portions, that will each be sent in an HTLC. This allows us
+       // to test channel channel reserve policy at the edges of what amount is sendable, i.e.
+       // cases where 1 msat over X amount will cause a payment failure, but anything less than
+       // that can be sent successfully. So, dividing by two is a somewhat arbitrary way of getting
+       // the amount of the first of these aforementioned 3 payments. The reason we split into 3 payments
+       // is to test the behavior of the holding cell with respect to channel reserve and commit tx fee
+       // policy.
+       let commit_tx_fee_2_htlcs = 2*commit_tx_fee_msat(feerate, 2 + 1);
+       let recv_value_1 = (stat01.value_to_self_msat - stat01.channel_reserve_msat - total_fee_msat - commit_tx_fee_2_htlcs)/2;
        let amt_msat_1 = recv_value_1 + total_fee_msat;
 
        let (route_1, our_payment_hash_1, our_payment_preimage_1) = get_route_and_payment_hash!(recv_value_1);
@@ -1621,59 +1832,22 @@ fn do_channel_reserve_test(test_recv: bool) {
        nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event_1.msgs[0]);
 
        // channel reserve test with htlc pending output > 0
-       let recv_value_2 = stat01.value_to_self_msat - amt_msat_1 - stat01.channel_reserve_msat - total_fee_msat;
+       let recv_value_2 = stat01.value_to_self_msat - amt_msat_1 - stat01.channel_reserve_msat - total_fee_msat - commit_tx_fee_2_htlcs;
        {
                let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value_2 + 1);
                unwrap_send_err!(nodes[0].node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { err },
                        assert_eq!(err, "Cannot send value that would put us under local channel reserve value"));
                assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-               nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us under local channel reserve value".to_string(), 2);
-       }
-
-       {
-               // test channel_reserve test on nodes[1] side
-               let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value_2 + 1);
-
-               // Need to manually create update_add_htlc message to go around the channel reserve check in send_htlc()
-               let secp_ctx = Secp256k1::new();
-               let session_priv = SecretKey::from_slice(&{
-                       let mut session_key = [0; 32];
-                       let mut rng = thread_rng();
-                       rng.fill_bytes(&mut session_key);
-                       session_key
-               }).expect("RNG is bad!");
-
-               let cur_height = nodes[0].node.latest_block_height.load(Ordering::Acquire) as u32 + 1;
-               let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route.paths[0], &session_priv).unwrap();
-               let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route.paths[0], recv_value_2 + 1, &None, cur_height).unwrap();
-               let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash);
-               let msg = msgs::UpdateAddHTLC {
-                       channel_id: chan_1.2,
-                       htlc_id,
-                       amount_msat: htlc_msat,
-                       payment_hash: our_payment_hash,
-                       cltv_expiry: htlc_cltv,
-                       onion_routing_packet: onion_packet,
-               };
-
-               if test_recv {
-                       nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &msg);
-                       // If we send a garbage message, the channel should get closed, making the rest of this test case fail.
-                       assert_eq!(nodes[1].node.list_channels().len(), 1);
-                       assert_eq!(nodes[1].node.list_channels().len(), 1);
-                       let err_msg = check_closed_broadcast!(nodes[1], true).unwrap();
-                       assert_eq!(err_msg.data, "Remote HTLC add would put them under their reserve value");
-                       check_added_monitors!(nodes[1], 1);
-                       return;
-               }
        }
 
        // split the rest to test holding cell
-       let recv_value_21 = recv_value_2/2;
-       let recv_value_22 = recv_value_2 - recv_value_21 - total_fee_msat;
+       let commit_tx_fee_3_htlcs = 2*commit_tx_fee_msat(feerate, 3 + 1);
+       let additional_htlc_cost_msat = commit_tx_fee_3_htlcs - commit_tx_fee_2_htlcs;
+       let recv_value_21 = recv_value_2/2 - additional_htlc_cost_msat/2;
+       let recv_value_22 = recv_value_2 - recv_value_21 - total_fee_msat - additional_htlc_cost_msat;
        {
                let stat = get_channel_value_stat!(nodes[0], chan_1.2);
-               assert_eq!(stat.value_to_self_msat - (stat.pending_outbound_htlcs_amount_msat + recv_value_21 + recv_value_22 + total_fee_msat + total_fee_msat), stat.channel_reserve_msat);
+               assert_eq!(stat.value_to_self_msat - (stat.pending_outbound_htlcs_amount_msat + recv_value_21 + recv_value_22 + total_fee_msat + total_fee_msat + commit_tx_fee_3_htlcs), stat.channel_reserve_msat);
        }
 
        // now see if they go through on both sides
@@ -1690,7 +1864,7 @@ fn do_channel_reserve_test(test_recv: bool) {
                unwrap_send_err!(nodes[0].node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { err },
                        assert_eq!(err, "Cannot send value that would put us under local channel reserve value"));
                assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
-               nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us under local channel reserve value".to_string(), 3);
+               nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us under local channel reserve value".to_string(), 2);
        }
 
        let (route_22, our_payment_hash_22, our_payment_preimage_22) = get_route_and_payment_hash!(recv_value_22);
@@ -1705,6 +1879,7 @@ fn do_channel_reserve_test(test_recv: bool) {
        let (as_revoke_and_ack, as_commitment_signed) = get_revoke_commit_msgs!(nodes[1], nodes[0].node.get_our_node_id());
        check_added_monitors!(nodes[1], 1);
 
+       // the pending htlc should be promoted to committed
        nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_revoke_and_ack);
        check_added_monitors!(nodes[0], 1);
        let commitment_update_2 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
@@ -1765,19 +1940,35 @@ fn do_channel_reserve_test(test_recv: bool) {
        claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[2]), our_payment_preimage_21, recv_value_21);
        claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[2]), our_payment_preimage_22, recv_value_22);
 
-       let expected_value_to_self = stat01.value_to_self_msat - (recv_value_1 + total_fee_msat) - (recv_value_21 + total_fee_msat) - (recv_value_22 + total_fee_msat);
+       let commit_tx_fee_0_htlcs = 2*commit_tx_fee_msat(feerate, 1);
+       let recv_value_3 = commit_tx_fee_2_htlcs - commit_tx_fee_0_htlcs - total_fee_msat;
+       {
+               let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value_3 + 1);
+               let err = nodes[0].node.send_payment(&route, our_payment_hash, &None).err().unwrap();
+               match err {
+                       PaymentSendFailure::AllFailedRetrySafe(ref fails) => {
+                               match fails[0] {
+                                       APIError::ChannelUnavailable{err} =>
+                                               assert_eq!(err, "Cannot send value that would put us under local channel reserve value"),
+                                       _ => panic!("Unexpected error variant"),
+                               }
+                       },
+                       _ => panic!("Unexpected error variant"),
+               }
+               assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
+               nodes[0].logger.assert_log("lightning::ln::channelmanager".to_string(), "Cannot send value that would put us under local channel reserve value".to_string(), 3);
+       }
+
+       send_payment(&nodes[0], &vec![&nodes[1], &nodes[2]][..], recv_value_3, recv_value_3);
+
+       let commit_tx_fee_1_htlc = 2*commit_tx_fee_msat(feerate, 1 + 1);
+       let expected_value_to_self = stat01.value_to_self_msat - (recv_value_1 + total_fee_msat) - (recv_value_21 + total_fee_msat) - (recv_value_22 + total_fee_msat) - (recv_value_3 + total_fee_msat);
        let stat0 = get_channel_value_stat!(nodes[0], chan_1.2);
        assert_eq!(stat0.value_to_self_msat, expected_value_to_self);
-       assert_eq!(stat0.value_to_self_msat, stat0.channel_reserve_msat);
+       assert_eq!(stat0.value_to_self_msat, stat0.channel_reserve_msat + commit_tx_fee_1_htlc);
 
        let stat2 = get_channel_value_stat!(nodes[2], chan_2.2);
-       assert_eq!(stat2.value_to_self_msat, stat22.value_to_self_msat + recv_value_1 + recv_value_21 + recv_value_22);
-}
-
-#[test]
-fn channel_reserve_test() {
-       do_channel_reserve_test(false);
-       do_channel_reserve_test(true);
+       assert_eq!(stat2.value_to_self_msat, stat22.value_to_self_msat + recv_value_1 + recv_value_21 + recv_value_22 + recv_value_3);
 }
 
 #[test]
@@ -6244,23 +6435,31 @@ fn test_update_add_htlc_bolt2_receiver_sender_can_afford_amount_sent() {
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
        let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
        let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
+       let logger = test_utils::TestLogger::new();
 
-       let their_channel_reserve = get_channel_value_stat!(nodes[0], chan.2).channel_reserve_msat;
+       let chan_stat = get_channel_value_stat!(nodes[0], chan.2);
+       let channel_reserve = chan_stat.channel_reserve_msat;
+       let feerate = get_feerate!(nodes[0], chan.2);
+       // The 2* and +1 are for the fee spike reserve.
+       let commit_tx_fee_outbound = 2 * commit_tx_fee_msat(feerate, 1 + 1);
 
+       let max_can_send = 5000000 - channel_reserve - commit_tx_fee_outbound;
        let (_, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
        let net_graph_msg_handler = &nodes[0].net_graph_msg_handler;
-       let logger = test_utils::TestLogger::new();
-       let route = get_route(&nodes[0].node.get_our_node_id(), net_graph_msg_handler, &nodes[1].node.get_our_node_id(), None, &[], 5000000-their_channel_reserve, TEST_FINAL_CLTV, &logger).unwrap();
+       let route = get_route(&nodes[0].node.get_our_node_id(), net_graph_msg_handler, &nodes[1].node.get_our_node_id(), None, &[], max_can_send, TEST_FINAL_CLTV, &logger).unwrap();
        nodes[0].node.send_payment(&route, our_payment_hash, &None).unwrap();
        check_added_monitors!(nodes[0], 1);
        let mut updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
 
-       updates.update_add_htlcs[0].amount_msat = 5000000-their_channel_reserve+1;
+       // Even though channel-initiator senders are required to respect the fee_spike_reserve,
+       // at this time channel-initiatee receivers are not required to enforce that senders
+       // respect the fee_spike_reserve.
+       updates.update_add_htlcs[0].amount_msat = max_can_send + commit_tx_fee_outbound + 1;
        nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]);
 
        assert!(nodes[1].node.list_channels().is_empty());
        let err_msg = check_closed_broadcast!(nodes[1], true).unwrap();
-       assert_eq!(err_msg.data, "Remote HTLC add would put them under their reserve value");
+       assert_eq!(err_msg.data, "Remote HTLC add would put them under remote reserve value");
        check_added_monitors!(nodes[1], 1);
 }