Merge pull request #1162 from TheBlueMatt/2021-11-fix-accept-chan-checks
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Tue, 23 Nov 2021 20:46:38 +0000 (20:46 +0000)
committerGitHub <noreply@github.com>
Tue, 23 Nov 2021 20:46:38 +0000 (20:46 +0000)
Correct initial commitment tx fee affordability checks on open

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

index c8c54f0b091a0fc72d99419c56f43b3dbc9c987b,94b3cb576b4915f1bbee1b354e60ff07cfb9a56c..d76840236a1b04945de7703eae62906a6ddd4469
@@@ -406,7 -406,12 +406,12 @@@ pub(crate) const FUNDING_CONF_DEADLINE_
  /// size 2. However, if the number of concurrent update_add_htlc is higher, this still
  /// leads to a channel force-close. Ultimately, this is an issue coming from the
  /// design of LN state machines, allowing asynchronous updates.
- const CONCURRENT_INBOUND_HTLC_FEE_BUFFER: u32 = 2;
+ pub(crate) const CONCURRENT_INBOUND_HTLC_FEE_BUFFER: u32 = 2;
+ /// When a channel is opened, we check that the funding amount is enough to pay for relevant
+ /// commitment transaction fees, with at least this many HTLCs present on the commitment
+ /// transaction (not counting the value of the HTLCs themselves).
+ pub(crate) const MIN_AFFORDABLE_HTLC_COUNT: usize = 4;
  
  // 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
@@@ -679,10 -684,6 +684,10 @@@ impl<Signer: Sign> Channel<Signer> 
                cmp::min(channel_value_satoshis, cmp::max(q, 1000)) //TODO
        }
  
 +      pub(crate) fn opt_anchors(&self) -> bool {
 +              self.channel_transaction_parameters.opt_anchors.is_some()
 +      }
 +
        // Constructors:
        pub fn new_outbound<K: Deref, F: Deref>(
                fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures,
  
                let feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
  
+               let value_to_self_msat = channel_value_satoshis * 1000 - push_msat;
+               let commitment_tx_fee = Self::commit_tx_fee_msat(feerate, MIN_AFFORDABLE_HTLC_COUNT);
+               if value_to_self_msat < commitment_tx_fee {
+                       return Err(APIError::APIMisuseError{ err: format!("Funding amount ({}) can't even pay fee for initial commitment transaction fee of {}.", value_to_self_msat / 1000, commitment_tx_fee / 1000) });
+               }
                let mut secp_ctx = Secp256k1::new();
                secp_ctx.seeded_randomize(&keys_provider.get_secure_random_bytes());
  
  
                        cur_holder_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
                        cur_counterparty_commitment_transaction_number: INITIAL_COMMITMENT_NUMBER,
-                       value_to_self_msat: channel_value_satoshis * 1000 - push_msat,
+                       value_to_self_msat,
  
                        pending_inbound_htlcs: Vec::new(),
                        pending_outbound_htlcs: Vec::new(),
                                holder_selected_contest_delay: config.own_channel_config.our_to_self_delay,
                                is_outbound_from_holder: true,
                                counterparty_parameters: None,
 -                              funding_outpoint: None
 +                              funding_outpoint: None,
 +                              opt_anchors: None,
                        },
                        funding_transaction: None,
  
                // we either accept their preference or the preferences match
                local_config.announced_channel = announce;
  
-               let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
                let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(msg.funding_satoshis);
                if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
                        return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). dust_limit_satoshis is ({}).", holder_selected_channel_reserve_satoshis, MIN_CHAN_DUST_LIMIT_SATOSHIS)));
                }
  
                // check if the funder's amount for the initial commitment tx is sufficient
-               // for full fee payment
+               // for full fee payment plus a few HTLCs to ensure the channel will be useful.
                let funders_amount_msat = msg.funding_satoshis * 1000 - msg.push_msat;
-               let lower_limit = background_feerate as u64 * COMMITMENT_TX_BASE_WEIGHT;
-               if funders_amount_msat < lower_limit {
-                       return Err(ChannelError::Close(format!("Insufficient funding amount ({}) for initial commitment. Must be at least {}", funders_amount_msat, lower_limit)));
+               let commitment_tx_fee = Self::commit_tx_fee_msat(msg.feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT) / 1000;
+               if funders_amount_msat / 1000 < commitment_tx_fee {
+                       return Err(ChannelError::Close(format!("Funding amount ({} sats) can't even pay fee for initial commitment transaction fee of {} sats.", funders_amount_msat / 1000, commitment_tx_fee)));
                }
  
-               let to_local_msat = msg.push_msat;
-               let to_remote_msat = funders_amount_msat - background_feerate as u64 * COMMITMENT_TX_BASE_WEIGHT;
-               if to_local_msat <= msg.channel_reserve_satoshis * 1000 && to_remote_msat <= holder_selected_channel_reserve_satoshis * 1000 {
-                       return Err(ChannelError::Close("Insufficient funding amount for initial commitment".to_owned()));
+               let to_remote_satoshis = funders_amount_msat / 1000 - commitment_tx_fee;
+               // While it's reasonable for us to not meet the channel reserve initially (if they don't
+               // want to push much to us), our counterparty should always have more than our reserve.
+               if to_remote_satoshis < holder_selected_channel_reserve_satoshis {
+                       return Err(ChannelError::Close("Insufficient funding amount for initial reserve".to_owned()));
                }
  
                let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
                                        selected_contest_delay: msg.to_self_delay,
                                        pubkeys: counterparty_pubkeys,
                                }),
 -                              funding_outpoint: None
 +                              funding_outpoint: None,
 +                              opt_anchors: None
                        },
                        funding_transaction: None,
  
                let tx = CommitmentTransaction::new_with_auxiliary_htlc_data(commitment_number,
                                                                             value_to_a as u64,
                                                                             value_to_b as u64,
 -                                                                           false,
 +                                                                           self.channel_transaction_parameters.opt_anchors.is_some(),
                                                                             funding_pubkey_a,
                                                                             funding_pubkey_b,
                                                                             keys.clone(),
  
        // Get the fee cost in MSATS 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 {
+       fn commit_tx_fee_msat(feerate_per_kw: u32, 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 as u64 / 1000 * 1000
+               (COMMITMENT_TX_BASE_WEIGHT + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * feerate_per_kw as u64 / 1000 * 1000
        }
  
        // Get the fee cost in SATS of a commitment tx with a given number of HTLC outputs.
                }
  
                let num_htlcs = included_htlcs + addl_htlcs;
-               let res = self.commit_tx_fee_msat(num_htlcs);
+               let res = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs);
                #[cfg(any(test, feature = "fuzztarget"))]
                {
                        let mut fee = res;
                        if fee_spike_buffer_htlc.is_some() {
-                               fee = self.commit_tx_fee_msat(num_htlcs - 1);
+                               fee = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs - 1);
                        }
                        let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len()
                                + self.holding_cell_htlc_updates.len();
                }
  
                let num_htlcs = included_htlcs + addl_htlcs;
-               let res = self.commit_tx_fee_msat(num_htlcs);
+               let res = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs);
                #[cfg(any(test, feature = "fuzztarget"))]
                {
                        let mut fee = res;
                        if fee_spike_buffer_htlc.is_some() {
-                               fee = self.commit_tx_fee_msat(num_htlcs - 1);
+                               fee = Self::commit_tx_fee_msat(self.feerate_per_kw, num_htlcs - 1);
                        }
                        let total_pending_htlcs = self.pending_inbound_htlcs.len() + self.pending_outbound_htlcs.len();
                        let commitment_tx_info = CommitmentTxInfoCached {
                for (idx, (htlc, source)) in htlcs_cloned.drain(..).enumerate() {
                        if let Some(_) = htlc.transaction_output_index {
                                let htlc_tx = chan_utils::build_htlc_transaction(&commitment_txid, commitment_stats.feerate_per_kw,
 -                                      self.get_counterparty_selected_contest_delay().unwrap(), &htlc,
 +                                      self.get_counterparty_selected_contest_delay().unwrap(), &htlc, self.opt_anchors(),
                                        &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
  
 -                              let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &keys);
 +                              let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, self.opt_anchors(), &keys);
                                let htlc_sighash = hash_to_message!(&bip143::SigHashCache::new(&htlc_tx).signature_hash(0, &htlc_redeemscript, htlc.amount_msat / 1000, SigHashType::All)[..]);
                                log_trace!(logger, "Checking HTLC tx signature {} by key {} against tx {} (sighash {}) with redeemscript {} in channel {}.",
                                        log_bytes!(msg.htlc_signatures[idx].serialize_compact()[..]), log_bytes!(keys.countersignatory_htlc_key.serialize()),
                                                && info.next_holder_htlc_id == self.next_holder_htlc_id
                                                && info.next_counterparty_htlc_id == self.next_counterparty_htlc_id
                                                && info.feerate == self.feerate_per_kw {
-                                                       let actual_fee = self.commit_tx_fee_msat(commitment_stats.num_nondust_htlcs);
+                                                       let actual_fee = Self::commit_tx_fee_msat(self.feerate_per_kw, commitment_stats.num_nondust_htlcs);
                                                        assert_eq!(actual_fee, info.fee);
                                                }
                                }
  
                        for (ref htlc_sig, ref htlc) in htlc_signatures.iter().zip(htlcs) {
                                log_trace!(logger, "Signed remote HTLC tx {} with redeemscript {} with pubkey {} -> {} in channel {}",
 -                                      encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.feerate_per_kw, self.get_holder_selected_contest_delay(), htlc, &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
 -                                      encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, &counterparty_keys)),
 +                                      encode::serialize_hex(&chan_utils::build_htlc_transaction(&counterparty_commitment_txid, commitment_stats.feerate_per_kw, self.get_holder_selected_contest_delay(), htlc, self.opt_anchors(), &counterparty_keys.broadcaster_delayed_payment_key, &counterparty_keys.revocation_key)),
 +                                      encode::serialize_hex(&chan_utils::get_htlc_redeemscript(&htlc, self.opt_anchors(), &counterparty_keys)),
                                        log_bytes!(counterparty_keys.broadcaster_htlc_key.serialize()),
                                        log_bytes!(htlc_sig.serialize_compact()[..]), log_bytes!(self.channel_id()));
                        }
@@@ -5632,11 -5636,6 +5642,11 @@@ impl<'a, Signer: Sign, K: Deref> Readab
                        return Err(DecodeError::UnknownRequiredFeature);
                }
  
 +              if channel_parameters.opt_anchors.is_some() {
 +                      // Relax this check when ChannelTypeFeatures supports anchors.
 +                      return Err(DecodeError::InvalidValue);
 +              }
 +
                let mut secp_ctx = Secp256k1::new();
                secp_ctx.seeded_randomize(&keys_source.get_secure_random_bytes());
  
@@@ -5932,13 -5931,13 +5942,13 @@@ mod tests 
                // 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);
+               let local_commit_fee_0_htlcs = Channel::<EnforcingSigner>::commit_tx_fee_msat(node_a_chan.feerate_per_kw, 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 remote_commit_fee_3_htlcs = Channel::<EnforcingSigner>::commit_tx_fee_msat(node_a_chan.feerate_per_kw, 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);
                let config = UserConfig::default();
                let mut chan = Channel::<EnforcingSigner>::new_outbound(&&fee_est, &&keys_provider, node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0).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);
+               let commitment_tx_fee_0_htlcs = Channel::<EnforcingSigner>::commit_tx_fee_msat(chan.feerate_per_kw, 0);
+               let commitment_tx_fee_1_htlc = Channel::<EnforcingSigner>::commit_tx_fee_msat(chan.feerate_per_kw, 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 remote_signature = Signature::from_der(&hex::decode($counterparty_htlc_sig_hex).unwrap()[..]).unwrap();
  
                                        let ref htlc = htlcs[$htlc_idx];
 +                                      let opt_anchors = false;
                                        let htlc_tx = chan_utils::build_htlc_transaction(&unsigned_tx.txid, chan.feerate_per_kw,
                                                chan.get_counterparty_selected_contest_delay().unwrap(),
 -                                              &htlc, &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
 -                                      let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &keys);
 +                                              &htlc, opt_anchors, &keys.broadcaster_delayed_payment_key, &keys.revocation_key);
 +                                      let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, opt_anchors, &keys);
                                        let htlc_sighash = Message::from_slice(&bip143::SigHashCache::new(&htlc_tx).signature_hash(0, &htlc_redeemscript, htlc.amount_msat / 1000, SigHashType::All)[..]).unwrap();
                                        secp_ctx.verify(&htlc_sighash, &remote_signature, &keys.countersignatory_htlc_key).unwrap();
  
index bf3f8c1d56ab0cce2290e467cd08fef85a2a0ce8,520a86f4caa7fc02bf0ee8e9a5bc4ead794aa971..f144dbd9ad61faab79d73e82def57ef91cfd7106
@@@ -18,7 -18,7 +18,7 @@@ use chain::channelmonitor::{ChannelMoni
  use chain::transaction::OutPoint;
  use chain::keysinterface::BaseSign;
  use ln::{PaymentPreimage, PaymentSecret, PaymentHash};
- use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
+ use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC, CONCURRENT_INBOUND_HTLC_FEE_BUFFER, MIN_AFFORDABLE_HTLC_COUNT};
  use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
  use ln::channel::{Channel, ChannelError};
  use ln::{chan_utils, onion_utils};
@@@ -584,12 -584,19 +584,19 @@@ fn test_update_fee_that_funder_cannot_a
        let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
-       let channel_value = 1977;
-       let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, channel_value, 700000, InitFeatures::known(), InitFeatures::known());
+       let channel_value = 5000;
+       let push_sats = 700;
+       let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, channel_value, push_sats * 1000, InitFeatures::known(), InitFeatures::known());
        let channel_id = chan.2;
        let secp_ctx = Secp256k1::new();
-       let feerate = 260;
+       let bs_channel_reserve_sats = Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(channel_value);
+       // Calculate the maximum feerate that A can afford. Note that we don't send an update_fee
+       // CONCURRENT_INBOUND_HTLC_FEE_BUFFER HTLCs before actually running out of local balance, so we
+       // calculate two different feerates here - the expected local limit as well as the expected
+       // remote limit.
+       let feerate = ((channel_value - bs_channel_reserve_sats - push_sats) * 1000 / (COMMITMENT_TX_BASE_WEIGHT + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC)) as u32;
+       let non_buffer_feerate = ((channel_value - bs_channel_reserve_sats - push_sats) * 1000 / COMMITMENT_TX_BASE_WEIGHT) as u32;
        {
                let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
                *feerate_lock = feerate;
  
        commitment_signed_dance!(nodes[1], nodes[0], update_msg.commitment_signed, false);
  
-       //Confirm that the new fee based on the last local commitment txn is what we expected based on the feerate of 260 set above.
-       //This value results in a fee that is exactly what the funder can afford (277 sat + 1000 sat channel reserve)
+       // Confirm that the new fee based on the last local commitment txn is what we expected based on the feerate set above.
        {
                let commitment_tx = get_local_commitment_txn!(nodes[1], channel_id)[0].clone();
  
-               //We made sure neither party's funds are below the dust limit so -2 non-HTLC txns from number of outputs
-               let num_htlcs = commitment_tx.output.len() - 2;
-               let total_fee: u64 = feerate as u64 * (COMMITMENT_TX_BASE_WEIGHT + (num_htlcs as u64) * COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000;
+               //We made sure neither party's funds are below the dust limit and there are no HTLCs here
+               assert_eq!(commitment_tx.output.len(), 2);
+               let total_fee: u64 = commit_tx_fee_msat(feerate, 0) / 1000;
                let mut actual_fee = commitment_tx.output.iter().fold(0, |acc, output| acc + output.value);
                actual_fee = channel_value - actual_fee;
                assert_eq!(total_fee, actual_fee);
        }
  
-       //Add 2 to the previous fee rate to the final fee increases by 1 (with no HTLCs the fee is essentially
-       //fee_rate*(724/1000) so the increment of 1*0.724 is rounded back down)
        {
+               // Increment the feerate by a small constant, accounting for rounding errors
                let mut feerate_lock = chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
-               *feerate_lock = feerate + 2;
+               *feerate_lock += 4;
        }
        nodes[0].node.timer_tick_occurred();
-       nodes[0].logger.assert_log("lightning::ln::channel".to_string(), format!("Cannot afford to send new feerate at {}", feerate + 2), 1);
+       nodes[0].logger.assert_log("lightning::ln::channel".to_string(), format!("Cannot afford to send new feerate at {}", feerate + 4), 1);
        check_added_monitors!(nodes[0], 0);
  
        const INITIAL_COMMITMENT_NUMBER: u64 = 281474976710654;
                let mut htlcs: Vec<(HTLCOutputInCommitment, ())> = vec![];
                let commitment_tx = CommitmentTransaction::new_with_auxiliary_htlc_data(
                        INITIAL_COMMITMENT_NUMBER - 1,
-                       700,
-                       999,
+                       push_sats,
+                       channel_value - push_sats - commit_tx_fee_msat(non_buffer_feerate + 4, 0) / 1000,
                        false, local_funding, remote_funding,
                        commit_tx_keys.clone(),
-                       feerate + 124,
+                       non_buffer_feerate + 4,
                        &mut htlcs,
                        &local_chan.channel_transaction_parameters.as_counterparty_broadcastable()
                );
  
        let update_fee = msgs::UpdateFee {
                channel_id: chan.2,
-               feerate_per_kw: feerate + 124,
+               feerate_per_kw: non_buffer_feerate + 4,
        };
  
        nodes[1].node.handle_update_fee(&nodes[0].node.get_our_node_id(), &update_fee);
@@@ -1446,21 -1451,21 +1451,21 @@@ fn test_chan_reserve_violation_outbound
        // 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.
-       let feerate_per_kw = 253;
-       chanmon_cfgs[0].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(feerate_per_kw) };
-       chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(feerate_per_kw) };
+       let feerate_per_kw = *chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
        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 mut push_amt = 100_000_000;
-       push_amt -= feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT + COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000 * 1000;
+       push_amt -= commit_tx_fee_msat(feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT as u64);
        push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
  
        let _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, push_amt, InitFeatures::known(), InitFeatures::known());
  
        // Sending exactly enough to hit the reserve amount should be accepted
-       let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], 1_000_000);
+       for _ in 0..MIN_AFFORDABLE_HTLC_COUNT {
+               let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], 1_000_000);
+       }
  
        // However one more HTLC should be significantly over the reserve amount and fail.
        let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 1_000_000);
  #[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: Mutex::new(6000) };
-       chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(6000) };
+       let feerate_per_kw = *chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
        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 (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 1000);
+       // 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)).
+       let mut push_amt = 100_000_000;
+       push_amt -= commit_tx_fee_msat(feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT as u64);
+       push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
+       let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, push_amt, InitFeatures::known(), InitFeatures::known());
+       // Send four HTLCs to cover the initial push_msat buffer we're required to include
+       for _ in 0..MIN_AFFORDABLE_HTLC_COUNT {
+               let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], 1_000_000);
+       }
+       let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 700_000);
        // 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(&[42; 32]).unwrap();
        let cur_height = nodes[1].node.best_block.read().unwrap().height() + 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, &Some(payment_secret), cur_height, &None).unwrap();
+       let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route.paths[0], 700_000, &Some(payment_secret), cur_height, &None).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,
+               htlc_id: MIN_AFFORDABLE_HTLC_COUNT as u64,
+               amount_msat: htlc_msat,
                payment_hash: payment_hash,
                cltv_expiry: htlc_cltv,
                onion_routing_packet: onion_packet,
@@@ -1517,9 -1528,7 +1528,7 @@@ fn test_chan_reserve_dust_inbound_htlcs
        // 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 mut chanmon_cfgs = create_chanmon_cfgs(2);
-       let feerate_per_kw = 253;
-       chanmon_cfgs[0].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(feerate_per_kw) };
-       chanmon_cfgs[1].fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: Mutex::new(feerate_per_kw) };
+       let feerate_per_kw = *chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
  
        let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None, None]);
        // channel reserve violation (so their balance is channel reserve (1000 sats) + commitment
        // transaction fee with 0 HTLCs (183 sats)).
        let mut push_amt = 100_000_000;
-       push_amt -= feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT) / 1000 * 1000;
+       push_amt -= commit_tx_fee_msat(feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT as u64);
        push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
        create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, push_amt, InitFeatures::known(), InitFeatures::known());
  
        // commitment transaction fee.
        let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], dust_amt);
  
+       // Send four HTLCs to cover the initial push_msat buffer we're required to include
+       for _ in 0..MIN_AFFORDABLE_HTLC_COUNT {
+               let (_, _, _) = route_payment(&nodes[1], &[&nodes[0]], 1_000_000);
+       }
        // One more than the dust amt should fail, however.
        let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], dust_amt + 1);
        unwrap_send_err!(nodes[1].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret)), true, APIError::ChannelUnavailable { ref err },
                assert_eq!(err, "Cannot send value that would put counterparty balance under holder-announced channel reserve value"));
  }
  
+ #[test]
+ fn test_chan_init_feerate_unaffordability() {
+       // Test that we will reject channel opens which do not leave enough to pay for any HTLCs due to
+       // channel reserve and feerate requirements.
+       let mut chanmon_cfgs = create_chanmon_cfgs(2);
+       let feerate_per_kw = *chanmon_cfgs[0].fee_estimator.sat_per_kw.lock().unwrap();
+       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);
+       // Set the push_msat amount such that nodes[0] will not be able to afford to add even a single
+       // HTLC.
+       let mut push_amt = 100_000_000;
+       push_amt -= commit_tx_fee_msat(feerate_per_kw, MIN_AFFORDABLE_HTLC_COUNT as u64);
+       assert_eq!(nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, push_amt + 1, 42, None).unwrap_err(),
+               APIError::APIMisuseError { err: "Funding amount (356) can't even pay fee for initial commitment transaction fee of 357.".to_string() });
+       // During open, we don't have a "counterparty channel reserve" to check against, so that
+       // requirement only comes into play on the open_channel handling side.
+       push_amt -= Channel::<EnforcingSigner>::get_holder_selected_channel_reserve_satoshis(100_000) * 1000;
+       nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, push_amt, 42, None).unwrap();
+       let mut open_channel_msg = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+       open_channel_msg.push_msat += 1;
+       nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &open_channel_msg);
+       let msg_events = nodes[1].node.get_and_clear_pending_msg_events();
+       assert_eq!(msg_events.len(), 1);
+       match msg_events[0] {
+               MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, node_id: _ } => {
+                       assert_eq!(msg.data, "Insufficient funding amount for initial reserve");
+               },
+               _ => panic!("Unexpected event"),
+       }
+ }
  #[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
@@@ -1959,7 -2008,7 +2008,7 @@@ fn channel_reserve_in_flight_removes() 
        nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_removes.commitment_signed);
        check_added_monitors!(nodes[0], 1);
        let as_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());
 -      expect_payment_sent!(nodes[0], payment_preimage_1);
 +      expect_payment_sent_without_paths!(nodes[0], payment_preimage_1);
  
        nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &send_1.msgs[0]);
        nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &send_1.commitment_msg);
        nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_cs.commitment_signed);
        check_added_monitors!(nodes[0], 1);
        let as_raa = get_event_msg!(nodes[0], MessageSendEvent::SendRevokeAndACK, nodes[1].node.get_our_node_id());
 -      expect_payment_sent!(nodes[0], payment_preimage_2);
 +      expect_payment_sent_without_paths!(nodes[0], payment_preimage_2);
  
        nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &as_raa);
        check_added_monitors!(nodes[1], 1);
        // resolve the second HTLC from A's point of view.
        nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa);
        check_added_monitors!(nodes[0], 1);
 +      expect_payment_path_successful!(nodes[0]);
        let as_cs = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
  
        // Now that B doesn't have the second RAA anymore, but A still does, send a payment from B back
  
        nodes[0].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &bs_raa);
        check_added_monitors!(nodes[0], 1);
 +      expect_payment_path_successful!(nodes[0]);
        let as_cs = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id());
  
        nodes[1].node.handle_commitment_signed(&nodes[0].node.get_our_node_id(), &as_cs.commitment_signed);
@@@ -2711,7 -2758,7 +2760,7 @@@ fn test_htlc_on_chain_success() 
        check_closed_broadcast!(nodes[0], true);
        check_added_monitors!(nodes[0], 1);
        let events = nodes[0].node.get_and_clear_pending_events();
 -      assert_eq!(events.len(), 3);
 +      assert_eq!(events.len(), 5);
        let mut first_claimed = false;
        for event in events {
                match event {
                                        assert_eq!(payment_hash, payment_hash_2);
                                }
                        },
 +                      Event::PaymentPathSuccessful { .. } => {},
                        Event::ChannelClosed { reason: ClosureReason::CommitmentTxConfirmed, .. } => {},
                        _ => panic!("Unexpected event"),
                }
@@@ -3359,13 -3405,13 +3408,13 @@@ fn test_dup_events_on_peer_disconnect(
        check_added_monitors!(nodes[1], 1);
        let claim_msgs = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
        nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &claim_msgs.update_fulfill_htlcs[0]);
 -      expect_payment_sent!(nodes[0], payment_preimage);
 +      expect_payment_sent_without_paths!(nodes[0], payment_preimage);
  
        nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
        nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
  
        reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (1, 0), (0, 0), (0, 0), (0, 0), (false, false));
 -      assert!(nodes[0].node.get_and_clear_pending_events().is_empty());
 +      expect_payment_path_successful!(nodes[0]);
  }
  
  #[test]
@@@ -3405,7 -3451,7 +3454,7 @@@ fn test_simple_peer_disconnect() 
        reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (1, 0), (1, 0), (false, false));
        {
                let events = nodes[0].node.get_and_clear_pending_events();
 -              assert_eq!(events.len(), 2);
 +              assert_eq!(events.len(), 3);
                match events[0] {
                        Event::PaymentSent { payment_preimage, payment_hash, .. } => {
                                assert_eq!(payment_preimage, payment_preimage_3);
                        },
                        _ => panic!("Unexpected event"),
                }
 +              match events[2] {
 +                      Event::PaymentPathSuccessful { .. } => {},
 +                      _ => panic!("Unexpected event"),
 +              }
        }
  
        claim_payment(&nodes[0], &vec!(&nodes[1], &nodes[2]), payment_preimage_4);
@@@ -3613,7 -3655,15 +3662,7 @@@ fn do_test_drop_messages_peer_disconnec
        if messages_delivered < 2 {
                reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (1, 0), (0, 0), (0, 0), (0, 0), (false, false));
                if messages_delivered < 1 {
 -                      let events_4 = nodes[0].node.get_and_clear_pending_events();
 -                      assert_eq!(events_4.len(), 1);
 -                      match events_4[0] {
 -                              Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
 -                                      assert_eq!(payment_preimage_1, *payment_preimage);
 -                                      assert_eq!(payment_hash_1, *payment_hash);
 -                              },
 -                              _ => panic!("Unexpected event"),
 -                      }
 +                      expect_payment_sent!(nodes[0], payment_preimage_1);
                } else {
                        assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
                }
                reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
        }
  
 +      if messages_delivered == 1 || messages_delivered == 2 {
 +              expect_payment_path_successful!(nodes[0]);
 +      }
 +
        nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
        nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
        reconnect_nodes(&nodes[0], &nodes[1], (false, false), (0, 0), (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
  
 +      if messages_delivered > 2 {
 +              expect_payment_path_successful!(nodes[0]);
 +      }
 +
        // Channel should still work fine...
        let (route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[1], 1000000);
        let payment_preimage_2 = send_along_route(&nodes[0], route, &[&nodes[1]], 1000000).0;
@@@ -3954,7 -3996,6 +4003,7 @@@ fn test_drop_messages_peer_disconnect_d
        assert!(nodes[0].node.get_and_clear_pending_msg_events().is_empty());
        check_added_monitors!(nodes[0], 1);
  
 +      expect_payment_path_successful!(nodes[0]);
        claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
  }
  
@@@ -4508,7 -4549,7 +4557,7 @@@ fn test_claim_sizeable_push_msat() 
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
  
-       let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 99000000, InitFeatures::known(), InitFeatures::known());
+       let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 98_000_000, InitFeatures::known(), InitFeatures::known());
        nodes[1].node.force_close_channel(&chan.2).unwrap();
        check_closed_broadcast!(nodes[1], true);
        check_added_monitors!(nodes[1], 1);
@@@ -4537,7 -4578,7 +4586,7 @@@ fn test_claim_on_remote_sizeable_push_m
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
  
-       let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 99000000, InitFeatures::known(), InitFeatures::known());
+       let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100_000, 98_000_000, InitFeatures::known(), InitFeatures::known());
        nodes[0].node.force_close_channel(&chan.2).unwrap();
        check_closed_broadcast!(nodes[0], true);
        check_added_monitors!(nodes[0], 1);
@@@ -5622,16 -5663,24 +5671,16 @@@ fn do_htlc_claim_local_commitment_only(
        let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
        let chan = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
  
 -      let (our_payment_preimage, our_payment_hash, _) = route_payment(&nodes[0], &[&nodes[1]], if use_dust { 50000 } else { 3000000 });
 +      let (payment_preimage, _, _) = route_payment(&nodes[0], &[&nodes[1]], if use_dust { 50000 } else { 3000000 });
  
        // Claim the payment, but don't deliver A's commitment_signed, resulting in the HTLC only being
        // present in B's local commitment transaction, but none of A's commitment transactions.
 -      assert!(nodes[1].node.claim_funds(our_payment_preimage));
 +      assert!(nodes[1].node.claim_funds(payment_preimage));
        check_added_monitors!(nodes[1], 1);
  
        let bs_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
        nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.update_fulfill_htlcs[0]);
 -      let events = nodes[0].node.get_and_clear_pending_events();
 -      assert_eq!(events.len(), 1);
 -      match events[0] {
 -              Event::PaymentSent { payment_preimage, payment_hash, .. } => {
 -                      assert_eq!(payment_preimage, our_payment_preimage);
 -                      assert_eq!(payment_hash, our_payment_hash);
 -              },
 -              _ => panic!("Unexpected event"),
 -      }
 +      expect_payment_sent_without_paths!(nodes[0], payment_preimage);
  
        nodes[0].node.handle_commitment_signed(&nodes[1].node.get_our_node_id(), &bs_updates.commitment_signed);
        check_added_monitors!(nodes[0], 1);
@@@ -6053,7 -6102,15 +6102,7 @@@ fn test_free_and_fail_holding_cell_htlc
        let update_msgs = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
        nodes[0].node.handle_update_fulfill_htlc(&nodes[1].node.get_our_node_id(), &update_msgs.update_fulfill_htlcs[0]);
        commitment_signed_dance!(nodes[0], nodes[1], update_msgs.commitment_signed, false, true);
 -      let events = nodes[0].node.get_and_clear_pending_events();
 -      assert_eq!(events.len(), 1);
 -      match events[0] {
 -              Event::PaymentSent { ref payment_preimage, ref payment_hash, .. } => {
 -                      assert_eq!(*payment_preimage, payment_preimage_1);
 -                      assert_eq!(*payment_hash, payment_hash_1);
 -              }
 -              _ => panic!("Unexpected event"),
 -      }
 +      expect_payment_sent!(nodes[0], payment_preimage_1);
  }
  
  // Test that if we fail to forward an HTLC that is being freed from the holding cell that the