Don't include below-dust inbound HTLCs in commit tx fee calculation
authorValentine Wallace <vwallace@protonmail.com>
Fri, 4 Dec 2020 21:05:10 +0000 (16:05 -0500)
committerValentine Wallace <vwallace@protonmail.com>
Thu, 18 Feb 2021 18:09:17 +0000 (13:09 -0500)
Also remove part of the holding cell channel reserve test that's newly failing but
a bit of a redundant test anyway.

lightning/src/ln/channel.rs
lightning/src/ln/functional_tests.rs

index e700b1f3bd52e728768946d21acfaeae116cbb44..316c6721b7cd7e5102bc50b17ac7eed63802d0d1 100644 (file)
@@ -258,6 +258,27 @@ enum UpdateStatus {
        DisabledStaged,
 }
 
+/// An enum indicating whether the local or remote side offered a given HTLC.
+enum HTLCInitiator {
+       LocalOffered,
+       RemoteOffered,
+}
+
+/// Used when calculating whether we or the remote can afford an additional HTLC.
+struct HTLCCandidate {
+       amount_msat: u64,
+       origin: HTLCInitiator,
+}
+
+impl HTLCCandidate {
+       fn new(amount_msat: u64, origin: HTLCInitiator) -> Self {
+               Self {
+                       amount_msat,
+                       origin,
+               }
+       }
+}
+
 // TODO: We should refactor this to be an Inbound/OutboundChannel until initial setup handshaking
 // has been completed, and then turn into a Channel to get compiler-time enforcement of things like
 // calling channel_id() before we're set up or things like get_outbound_funding_signed on an
@@ -1684,63 +1705,122 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                (COMMITMENT_TX_BASE_WEIGHT + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * self.feerate_per_kw as u64 / 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 {
+       // Get the commitment tx fee for the local's (i.e. our) next commitment transaction based on the
+       // number of pending HTLCs that are on track to be in our next commitment tx, plus an additional
+       // HTLC if `fee_spike_buffer_htlc` is Some, plus a new HTLC given by `new_htlc_amount`. Dust HTLCs
+       // are excluded.
+       fn next_local_commit_tx_fee_msat(&self, htlc: HTLCCandidate, fee_spike_buffer_htlc: Option<()>) -> u64 {
                assert!(self.is_outbound());
 
-               let mut their_acked_htlcs = self.pending_inbound_htlcs.len();
+               let real_dust_limit_success_sat = (self.feerate_per_kw as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
+               let real_dust_limit_timeout_sat = (self.feerate_per_kw as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.holder_dust_limit_satoshis;
+
+               let mut addl_htlcs = 0;
+               if fee_spike_buffer_htlc.is_some() { addl_htlcs += 1; }
+               match htlc.origin {
+                       HTLCInitiator::LocalOffered => {
+                               if htlc.amount_msat / 1000 >= real_dust_limit_timeout_sat {
+                                       addl_htlcs += 1;
+                               }
+                       },
+                       HTLCInitiator::RemoteOffered => {
+                               if htlc.amount_msat / 1000 >= real_dust_limit_success_sat {
+                                       addl_htlcs += 1;
+                               }
+                       }
+               }
+
+               let mut included_htlcs = 0;
+               for ref htlc in self.pending_inbound_htlcs.iter() {
+                       if htlc.amount_msat / 1000 < real_dust_limit_success_sat {
+                               continue
+                       }
+                       // We include LocalRemoved HTLCs here because we may still need to broadcast a commitment
+                       // transaction including this HTLC if it times out before they RAA.
+                       included_htlcs += 1;
+               }
+
                for ref htlc in self.pending_outbound_htlcs.iter() {
-                       if htlc.amount_msat / 1000 <= self.holder_dust_limit_satoshis {
+                       if htlc.amount_msat / 1000 < real_dust_limit_timeout_sat {
                                continue
                        }
                        match htlc.state {
-                               OutboundHTLCState::Committed => their_acked_htlcs += 1,
-                               OutboundHTLCState::RemoteRemoved {..} => their_acked_htlcs += 1,
-                               OutboundHTLCState::LocalAnnounced {..} => their_acked_htlcs += 1,
+                               OutboundHTLCState::LocalAnnounced {..} => included_htlcs += 1,
+                               OutboundHTLCState::Committed => included_htlcs += 1,
+                               OutboundHTLCState::RemoteRemoved {..} => included_htlcs += 1,
+                               // We don't include AwaitingRemoteRevokeToRemove HTLCs because our next commitment
+                               // transaction won't be generated until they send us their next RAA, which will mean
+                               // dropping any HTLCs in this state.
                                _ => {},
                        }
                }
 
                for htlc in self.holding_cell_htlc_updates.iter() {
                        match htlc {
-                               &HTLCUpdateAwaitingACK::AddHTLC { .. } => their_acked_htlcs += 1,
-                               _ => {},
+                               &HTLCUpdateAwaitingACK::AddHTLC { amount_msat, .. } => {
+                                       if amount_msat / 1000 < real_dust_limit_timeout_sat {
+                                               continue
+                                       }
+                                       included_htlcs += 1
+                               },
+                               _ => {}, // Don't include claims/fails that are awaiting ack, because once we get the
+                                        // ack we're guaranteed to never include them in commitment txs anymore.
                        }
                }
 
-               self.commit_tx_fee_msat(their_acked_htlcs + addl_htlcs)
+               self.commit_tx_fee_msat(included_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 {
+       // 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, plus an additional HTLC if
+       // `fee_spike_buffer_htlc` is Some, plus a new HTLC given by `new_htlc_amount`. Dust HTLCs are
+       // excluded.
+       fn next_remote_commit_tx_fee_msat(&self, htlc: HTLCCandidate, fee_spike_buffer_htlc: Option<()>) -> u64 {
                assert!(!self.is_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();
+               let real_dust_limit_success_sat = (self.feerate_per_kw as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
+               let real_dust_limit_timeout_sat = (self.feerate_per_kw as u64 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + self.counterparty_dust_limit_satoshis;
+
+               let mut addl_htlcs = 0;
+               if fee_spike_buffer_htlc.is_some() { addl_htlcs += 1; }
+               match htlc.origin {
+                       HTLCInitiator::LocalOffered => {
+                               if htlc.amount_msat / 1000 >= real_dust_limit_success_sat {
+                                       addl_htlcs += 1;
+                               }
+                       },
+                       HTLCInitiator::RemoteOffered => {
+                               if htlc.amount_msat / 1000 >= real_dust_limit_timeout_sat {
+                                       addl_htlcs += 1;
+                               }
+                       }
+               }
+
+               // When calculating the set of HTLCs which will be included in their next commitment_signed, all
+               // non-dust inbound HTLCs are included (as all states imply it will be included) and only
+               // committed outbound HTLCs, see below.
+               let mut included_htlcs = 0;
+               for ref htlc in self.pending_inbound_htlcs.iter() {
+                       if htlc.amount_msat / 1000 <= real_dust_limit_timeout_sat {
+                               continue
+                       }
+                       included_htlcs += 1;
+               }
+
                for ref htlc in self.pending_outbound_htlcs.iter() {
-                       if htlc.amount_msat / 1000 <= self.counterparty_dust_limit_satoshis {
+                       if htlc.amount_msat / 1000 <= real_dust_limit_success_sat {
                                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.
+                       // 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,
+                               OutboundHTLCState::Committed => included_htlcs += 1,
+                               OutboundHTLCState::RemoteRemoved {..} => included_htlcs += 1,
                                _ => {},
                        }
                }
 
-               self.commit_tx_fee_msat(their_acked_htlcs + addl_htlcs)
+               self.commit_tx_fee_msat(included_htlcs + addl_htlcs)
        }
 
        pub fn update_add_htlc<F, L: Deref>(&mut self, msg: &msgs::UpdateAddHTLC, mut pending_forward_status: PendingHTLCStatus, create_pending_htlc_status: F, logger: &L) -> Result<(), ChannelError>
@@ -1808,8 +1888,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                // 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.is_outbound() { 0 } else {
-                       // +1 for this HTLC.
-                       self.next_remote_commit_tx_fee_msat(1)
+                       let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
+                       self.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations
                };
                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".to_owned()));
@@ -1822,14 +1902,16 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                }
 
                if !self.is_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);
+                       // `2 *` and `Some(())` is for the 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 htlc when calculating the next remote commitment transaction fee as we should
+                       // still be able to afford adding this HTLC plus one more future HTLC, regardless of being
+                       // sensitive to fee spikes.
+                       let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
+                       let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.next_remote_commit_tx_fee_msat(htlc_candidate, Some(()));
                        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.
@@ -1838,9 +1920,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                        }
                } 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);
+                       let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
+                       let local_commit_tx_fee_msat = self.next_local_commit_tx_fee_msat(htlc_candidate, None);
                        if self.value_to_self_msat < self.counterparty_selected_channel_reserve_satoshis * 1000 + local_commit_tx_fee_msat {
                                return Err(ChannelError::Close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned()));
                        }
@@ -3720,11 +3801,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 
                if !self.is_outbound() {
                        // Check that we won't violate the remote channel reserve by adding this HTLC.
-
                        let counterparty_balance_msat = self.channel_value_satoshis * 1000 - self.value_to_self_msat;
                        let holder_selected_chan_reserve_msat = Channel::<ChanSigner>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis);
-                       // 1 additional HTLC corresponding to this HTLC.
-                       let counterparty_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(1);
+                       let htlc_candidate = HTLCCandidate::new(amount_msat, HTLCInitiator::LocalOffered);
+                       let counterparty_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(htlc_candidate, None);
                        if counterparty_balance_msat < holder_selected_chan_reserve_msat + counterparty_commit_tx_fee_msat {
                                return Err(ChannelError::Ignore("Cannot send value that would put counterparty balance under holder-announced channel reserve value".to_owned()));
                        }
@@ -3735,10 +3815,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
                        return Err(ChannelError::Ignore(format!("Cannot send value that would overdraw remaining funds. Amount: {}, pending value to self {}", amount_msat, pending_value_to_self_msat)));
                }
 
-               // The `+1` is for the HTLC currently being added to the commitment tx and
-               // the `2 *` and `+1` are for the fee spike buffer.
+               // `2 *` and extra HTLC are for the fee spike buffer.
                let commit_tx_fee_msat = if self.is_outbound() {
-                       2 * self.next_local_commit_tx_fee_msat(1 + 1)
+                       let htlc_candidate = HTLCCandidate::new(amount_msat, HTLCInitiator::LocalOffered);
+                       2 * self.next_local_commit_tx_fee_msat(htlc_candidate, Some(()))
                } else { 0 };
                if pending_value_to_self_msat - amount_msat < commit_tx_fee_msat {
                        return Err(ChannelError::Ignore(format!("Cannot send value that would not leave enough to pay for fees. Pending value to self: {}. local_commit_tx_fee {}", pending_value_to_self_msat, commit_tx_fee_msat)));
@@ -4487,12 +4567,12 @@ mod tests {
        use bitcoin::hashes::hex::FromHex;
        use hex;
        use ln::channelmanager::{HTLCSource, PaymentPreimage, PaymentHash};
-       use ln::channel::{Channel,ChannelKeys,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,TxCreationKeys};
+       use ln::channel::{Channel,ChannelKeys,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,HTLCCandidate,HTLCInitiator,TxCreationKeys};
        use ln::channel::MAX_FUNDING_SATOSHIS;
        use ln::features::InitFeatures;
        use ln::msgs::{OptionalField, DataLossProtect, DecodeError};
        use ln::chan_utils;
-       use ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters};
+       use ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT};
        use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
        use chain::keysinterface::{InMemoryChannelKeys, KeysInterface};
        use chain::transaction::OutPoint;
@@ -4575,6 +4655,122 @@ mod tests {
                assert_eq!(open_channel_msg.feerate_per_kw, original_fee);
        }
 
+       #[test]
+       fn test_holder_vs_counterparty_dust_limit() {
+               // Test that when calculating the local and remote commitment transaction fees, the correct
+               // dust limits are used.
+               let feeest = TestFeeEstimator{fee_est: 15000};
+               let secp_ctx = Secp256k1::new();
+               let seed = [42; 32];
+               let network = Network::Testnet;
+               let keys_provider = test_utils::TestKeysInterface::new(&seed, network);
+
+               // Go through the flow of opening a channel between two nodes, making sure
+               // they have different dust limits.
+
+               // Create Node A's channel pointing to Node B's pubkey
+               let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
+               let config = UserConfig::default();
+               let mut node_a_chan = Channel::<EnforcingChannelKeys>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, 10000000, 100000, 42, &config).unwrap();
+
+               // Create Node B's channel by receiving Node A's open_channel message
+               // Make sure A's dust limit is as we expect.
+               let open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
+               assert_eq!(open_channel_msg.dust_limit_satoshis, 1560);
+               let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
+               let node_b_chan = Channel::<EnforcingChannelKeys>::new_from_req(&&feeest, &&keys_provider, node_b_node_id, InitFeatures::known(), &open_channel_msg, 7, &config).unwrap();
+
+               // Node B --> Node A: accept channel, explicitly setting B's dust limit.
+               let mut accept_channel_msg = node_b_chan.get_accept_channel();
+               accept_channel_msg.dust_limit_satoshis = 546;
+               node_a_chan.accept_channel(&accept_channel_msg, &config, InitFeatures::known()).unwrap();
+
+               // Put some inbound and outbound HTLCs in A's channel.
+               let htlc_amount_msat = 11_092_000; // put an amount below A's effective dust limit but above B's.
+               node_a_chan.pending_inbound_htlcs.push(InboundHTLCOutput {
+                       htlc_id: 0,
+                       amount_msat: htlc_amount_msat,
+                       payment_hash: PaymentHash(Sha256::hash(&[42; 32]).into_inner()),
+                       cltv_expiry: 300000000,
+                       state: InboundHTLCState::Committed,
+               });
+
+               node_a_chan.pending_outbound_htlcs.push(OutboundHTLCOutput {
+                       htlc_id: 1,
+                       amount_msat: htlc_amount_msat, // put an amount below A's dust amount but above B's.
+                       payment_hash: PaymentHash(Sha256::hash(&[43; 32]).into_inner()),
+                       cltv_expiry: 200000000,
+                       state: OutboundHTLCState::Committed,
+                       source: HTLCSource::OutboundRoute {
+                               path: Vec::new(),
+                               session_priv: SecretKey::from_slice(&hex::decode("0fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap()[..]).unwrap(),
+                               first_hop_htlc_msat: 548,
+                       }
+               });
+
+               // Make sure when Node A calculates their local commitment transaction, none of the HTLCs pass
+               // the dust limit check.
+               let htlc_candidate = HTLCCandidate::new(htlc_amount_msat, HTLCInitiator::LocalOffered);
+               let local_commit_tx_fee = node_a_chan.next_local_commit_tx_fee_msat(htlc_candidate, None);
+               let local_commit_fee_0_htlcs = node_a_chan.commit_tx_fee_msat(0);
+               assert_eq!(local_commit_tx_fee, local_commit_fee_0_htlcs);
+
+               // Finally, make sure that when Node A calculates the remote's commitment transaction fees, all
+               // of the HTLCs are seen to be above the dust limit.
+               node_a_chan.channel_transaction_parameters.is_outbound_from_holder = false;
+               let remote_commit_fee_3_htlcs = node_a_chan.commit_tx_fee_msat(3);
+               let htlc_candidate = HTLCCandidate::new(htlc_amount_msat, HTLCInitiator::LocalOffered);
+               let remote_commit_tx_fee = node_a_chan.next_remote_commit_tx_fee_msat(htlc_candidate, None);
+               assert_eq!(remote_commit_tx_fee, remote_commit_fee_3_htlcs);
+       }
+
+       #[test]
+       fn test_timeout_vs_success_htlc_dust_limit() {
+               // Make sure that when `next_remote_commit_tx_fee_msat` and `next_local_commit_tx_fee_msat`
+               // calculate the real dust limits for HTLCs (i.e. the dust limit given by the counterparty
+               // *plus* the fees paid for the HTLC) they don't swap `HTLC_SUCCESS_TX_WEIGHT` for
+               // `HTLC_TIMEOUT_TX_WEIGHT`, and vice versa.
+               let fee_est = TestFeeEstimator{fee_est: 253 };
+               let secp_ctx = Secp256k1::new();
+               let seed = [42; 32];
+               let network = Network::Testnet;
+               let keys_provider = test_utils::TestKeysInterface::new(&seed, network);
+
+               let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
+               let config = UserConfig::default();
+               let mut chan = Channel::<EnforcingChannelKeys>::new_outbound(&&fee_est, &&keys_provider, node_id, 10000000, 100000, 42, &config).unwrap();
+
+               let commitment_tx_fee_0_htlcs = chan.commit_tx_fee_msat(0);
+               let commitment_tx_fee_1_htlc = chan.commit_tx_fee_msat(1);
+
+               // If HTLC_SUCCESS_TX_WEIGHT and HTLC_TIMEOUT_TX_WEIGHT were swapped: then this HTLC would be
+               // counted as dust when it shouldn't be.
+               let htlc_amt_above_timeout = ((253 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + chan.holder_dust_limit_satoshis + 1) * 1000;
+               let htlc_candidate = HTLCCandidate::new(htlc_amt_above_timeout, HTLCInitiator::LocalOffered);
+               let commitment_tx_fee = chan.next_local_commit_tx_fee_msat(htlc_candidate, None);
+               assert_eq!(commitment_tx_fee, commitment_tx_fee_1_htlc);
+
+               // If swapped: this HTLC would be counted as non-dust when it shouldn't be.
+               let dust_htlc_amt_below_success = ((253 * HTLC_SUCCESS_TX_WEIGHT / 1000) + chan.holder_dust_limit_satoshis - 1) * 1000;
+               let htlc_candidate = HTLCCandidate::new(dust_htlc_amt_below_success, HTLCInitiator::RemoteOffered);
+               let commitment_tx_fee = chan.next_local_commit_tx_fee_msat(htlc_candidate, None);
+               assert_eq!(commitment_tx_fee, commitment_tx_fee_0_htlcs);
+
+               chan.channel_transaction_parameters.is_outbound_from_holder = false;
+
+               // If swapped: this HTLC would be counted as non-dust when it shouldn't be.
+               let dust_htlc_amt_above_timeout = ((253 * HTLC_TIMEOUT_TX_WEIGHT / 1000) + chan.counterparty_dust_limit_satoshis + 1) * 1000;
+               let htlc_candidate = HTLCCandidate::new(dust_htlc_amt_above_timeout, HTLCInitiator::LocalOffered);
+               let commitment_tx_fee = chan.next_remote_commit_tx_fee_msat(htlc_candidate, None);
+               assert_eq!(commitment_tx_fee, commitment_tx_fee_0_htlcs);
+
+               // If swapped: this HTLC would be counted as dust when it shouldn't be.
+               let htlc_amt_below_success = ((253 * HTLC_SUCCESS_TX_WEIGHT / 1000) + chan.counterparty_dust_limit_satoshis - 1) * 1000;
+               let htlc_candidate = HTLCCandidate::new(htlc_amt_below_success, HTLCInitiator::RemoteOffered);
+               let commitment_tx_fee = chan.next_remote_commit_tx_fee_msat(htlc_candidate, None);
+               assert_eq!(commitment_tx_fee, commitment_tx_fee_1_htlc);
+       }
+
        #[test]
        fn channel_reestablish_no_updates() {
                let feeest = TestFeeEstimator{fee_est: 15000};
index 16212f9272f98201dd40bf0406dd4af7fc959f30..6273604c63850e3fc63bf5bb33f31fcd9db7b45d 100644 (file)
@@ -1701,8 +1701,9 @@ fn test_fee_spike_violation_fails_htlc() {
 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.
+       // sending any above-dust 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);
@@ -1720,7 +1721,7 @@ fn test_chan_reserve_violation_outbound_htlc_inbound_chan() {
                }}
        }
 
-       let (route, our_payment_hash, _) = get_route_and_payment_hash!(1000);
+       let (route, our_payment_hash, _) = get_route_and_payment_hash!(4843000);
        unwrap_send_err!(nodes[1].node.send_payment(&route, our_payment_hash, &None), true, APIError::ChannelUnavailable { ref err },
                assert_eq!(err, "Cannot send value that would put counterparty balance under holder-announced channel reserve value"));
        assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
@@ -1778,6 +1779,57 @@ fn test_chan_reserve_violation_inbound_htlc_outbound_channel() {
        check_added_monitors!(nodes[0], 1);
 }
 
+#[test]
+fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() {
+       // Test that if we receive many dust HTLCs over an outbound channel, they don't count when
+       // calculating our commitment transaction fee (this was previously broken).
+       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, None]);
+       let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+       // Set nodes[0]'s balance such that they will consider any above-dust received HTLC to be a
+       // channel reserve violation (so their balance is channel reserve (1000 sats) + commitment
+       // transaction fee with 0 HTLCs (183 sats)).
+       create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 98817000, InitFeatures::known(), InitFeatures::known());
+
+       let dust_amt = 546000; // Dust amount
+       // In the previous code, routing this dust payment would cause nodes[0] to perceive a channel
+       // reserve violation even though it's a dust HTLC and therefore shouldn't count towards the
+       // commitment transaction fee.
+       let (_, _) = route_payment(&nodes[1], &[&nodes[0]], dust_amt);
+}
+
+#[test]
+fn test_chan_reserve_dust_inbound_htlcs_inbound_chan() {
+       // Test that if we receive many dust HTLCs over an inbound channel, they don't count when
+       // calculating our counterparty's commitment transaction fee (this was previously broken).
+       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, None]);
+       let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+       create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 98000000, InitFeatures::known(), InitFeatures::known());
+
+       let payment_amt = 46000; // Dust amount
+       // In the previous code, these first four payments would succeed.
+       let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+       let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+       let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+       let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+
+       // Then these next 5 would be interpreted by nodes[1] as violating the fee spike buffer.
+       let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+       let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+       let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+       let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+       let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+
+       // And this last payment previously resulted in nodes[1] closing on its inbound-channel
+       // counterparty, because it counted all the previous dust HTLCs against nodes[0]'s commitment
+       // transaction fee and therefore perceived this next payment as a channel reserve violation.
+       let (_, _) = route_payment(&nodes[0], &[&nodes[1]], payment_amt);
+}
+
 #[test]
 fn test_chan_reserve_violation_inbound_htlc_inbound_chan() {
        let chanmon_cfgs = create_chanmon_cfgs(3);
@@ -2089,23 +2141,6 @@ fn test_channel_reserve_holding_cell_htlcs() {
 
        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{ref err} =>
-                                               assert!(regex::Regex::new(r"Cannot send value that would put our balance under counterparty-announced channel reserve value \(\d+\)").unwrap().is_match(err)),
-                                       _ => 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_contains("lightning::ln::channelmanager".to_string(), "Cannot send value that would put our balance under counterparty-announced 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);