Mark failed counterparty-is-destination HTLCs retryable
[rust-lightning] / lightning / src / ln / channel.rs
index bc891070a15947ed93672f0dd0e3c80e3dc681b5..624f4d6b688b0e2d81964d39e2deb77e1233af8a 100644 (file)
@@ -31,7 +31,7 @@ use ln::channelmanager::{CounterpartyForwardingInfo, PendingHTLCStatus, HTLCSour
 use ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, htlc_success_tx_weight, htlc_timeout_tx_weight, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor, ClosingTransaction};
 use ln::chan_utils;
 use chain::BestBlock;
-use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
+use chain::chaininterface::{FeeEstimator, ConfirmationTarget, LowerBoundedFeeEstimator};
 use chain::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpdateStep, LATENCY_GRACE_PERIOD_BLOCKS};
 use chain::transaction::{OutPoint, TransactionData};
 use chain::keysinterface::{Sign, KeysInterface};
@@ -795,6 +795,9 @@ pub const MAX_CHAN_DUST_LIMIT_SATOSHIS: u64 = MAX_STD_OUTPUT_DUST_LIMIT_SATOSHIS
 /// See https://github.com/lightning/bolts/issues/905 for more details.
 pub const MIN_CHAN_DUST_LIMIT_SATOSHIS: u64 = 354;
 
+// Just a reasonable implementation-specific safe lower bound, higher than the dust limit.
+pub const MIN_THEIR_CHAN_RESERVE_SATOSHIS: u64 = 1000;
+
 /// Used to return a simple Error back to ChannelManager. Will get converted to a
 /// msgs::ErrorAction::SendErrorMessage or msgs::ErrorAction::IgnoreError as appropriate with our
 /// channel_id in ChannelManager.
@@ -843,16 +846,25 @@ impl<Signer: Sign> Channel<Signer> {
        }
 
        /// Returns a minimum channel reserve value the remote needs to maintain,
-       /// required by us.
+       /// required by us according to the configured or default
+       /// [`ChannelHandshakeConfig::their_channel_reserve_proportional_millionths`]
        ///
        /// Guaranteed to return a value no larger than channel_value_satoshis
        ///
-       /// This is used both for new channels and to figure out what reserve value we sent to peers
-       /// for channels serialized before we included our selected reserve value in the serialized
-       /// data explicitly.
-       pub(crate) fn get_holder_selected_channel_reserve_satoshis(channel_value_satoshis: u64) -> u64 {
+       /// This is used both for outbound and inbound channels and has lower bound
+       /// of `MIN_THEIR_CHAN_RESERVE_SATOSHIS`.
+       pub(crate) fn get_holder_selected_channel_reserve_satoshis(channel_value_satoshis: u64, config: &UserConfig) -> u64 {
+               let calculated_reserve = channel_value_satoshis.saturating_mul(config.channel_handshake_config.their_channel_reserve_proportional_millionths as u64) / 1_000_000;
+               cmp::min(channel_value_satoshis, cmp::max(calculated_reserve, MIN_THEIR_CHAN_RESERVE_SATOSHIS))
+       }
+
+       /// This is for legacy reasons, present for forward-compatibility.
+       /// LDK versions older than 0.0.104 don't know how read/handle values other than default
+       /// from storage. Hence, we use this function to not persist default values of
+       /// `holder_selected_channel_reserve_satoshis` for channels into storage.
+       pub(crate) fn get_legacy_default_holder_selected_channel_reserve_satoshis(channel_value_satoshis: u64) -> u64 {
                let (q, _) = channel_value_satoshis.overflowing_div(100);
-               cmp::min(channel_value_satoshis, cmp::max(q, 1000)) //TODO
+               cmp::min(channel_value_satoshis, cmp::max(q, 1000))
        }
 
        pub(crate) fn opt_anchors(&self) -> bool {
@@ -886,7 +898,7 @@ impl<Signer: Sign> Channel<Signer> {
 
        // Constructors:
        pub fn new_outbound<K: Deref, F: Deref>(
-               fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures,
+               fee_estimator: &LowerBoundedFeeEstimator<F>, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures,
                channel_value_satoshis: u64, push_msat: u64, user_id: u64, config: &UserConfig, current_chain_height: u32,
                outbound_scid_alias: u64
        ) -> Result<Channel<Signer>, APIError>
@@ -912,12 +924,14 @@ impl<Signer: Sign> Channel<Signer> {
                if holder_selected_contest_delay < BREAKDOWN_TIMEOUT {
                        return Err(APIError::APIMisuseError {err: format!("Configured with an unreasonable our_to_self_delay ({}) putting user funds at risks", holder_selected_contest_delay)});
                }
-               let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(channel_value_satoshis);
+               let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(channel_value_satoshis, config);
                if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
+                       // Protocol level safety check in place, although it should never happen because
+                       // of `MIN_THEIR_CHAN_RESERVE_SATOSHIS`
                        return Err(APIError::APIMisuseError { err: format!("Holder selected channel  reserve below implemention limit dust_limit_satoshis {}", holder_selected_channel_reserve_satoshis) });
                }
 
-               let feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
+               let feerate = fee_estimator.bounded_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, opt_anchors);
@@ -1056,7 +1070,7 @@ impl<Signer: Sign> Channel<Signer> {
                })
        }
 
-       fn check_remote_fee<F: Deref>(fee_estimator: &F, feerate_per_kw: u32) -> Result<(), ChannelError>
+       fn check_remote_fee<F: Deref>(fee_estimator: &LowerBoundedFeeEstimator<F>, feerate_per_kw: u32) -> Result<(), ChannelError>
                where F::Target: FeeEstimator
        {
                // We only bound the fee updates on the upper side to prevent completely absurd feerates,
@@ -1064,11 +1078,11 @@ impl<Signer: Sign> Channel<Signer> {
                // We generally don't care too much if they set the feerate to something very high, but it
                // could result in the channel being useless due to everything being dust.
                let upper_limit = cmp::max(250 * 25,
-                       fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 * 10);
+                       fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 * 10);
                if feerate_per_kw as u64 > upper_limit {
                        return Err(ChannelError::Close(format!("Peer's feerate much too high. Actual: {}. Our expected upper limit: {}", feerate_per_kw, upper_limit)));
                }
-               let lower_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
+               let lower_limit = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background);
                // Some fee estimators round up to the next full sat/vbyte (ie 250 sats per kw), causing
                // occasional issues with feerate disagreements between an initiator that wants a feerate
                // of 1.1 sat/vbyte and a receiver that wants 1.1 rounded up to 2. Thus, we always add 250
@@ -1082,7 +1096,7 @@ impl<Signer: Sign> Channel<Signer> {
        /// Creates a new channel from a remote sides' request for one.
        /// Assumes chain_hash has already been checked and corresponds with what we expect!
        pub fn new_from_req<K: Deref, F: Deref, L: Deref>(
-               fee_estimator: &F, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures,
+               fee_estimator: &LowerBoundedFeeEstimator<F>, keys_provider: &K, counterparty_node_id: PublicKey, their_features: &InitFeatures,
                msg: &msgs::OpenChannel, user_id: u64, config: &UserConfig, current_chain_height: u32, logger: &L,
                outbound_scid_alias: u64
        ) -> Result<Channel<Signer>, ChannelError>
@@ -1204,12 +1218,14 @@ impl<Signer: Sign> Channel<Signer> {
                        }
                }
 
-               let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(msg.funding_satoshis);
+               let holder_selected_channel_reserve_satoshis = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(msg.funding_satoshis, config);
                if holder_selected_channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
+                       // Protocol level safety check in place, although it should never happen because
+                       // of `MIN_THEIR_CHAN_RESERVE_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)));
                }
                if holder_selected_channel_reserve_satoshis * 1000 >= full_channel_value_msat {
-                       return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({}). Channel value is ({} - {}).", holder_selected_channel_reserve_satoshis, full_channel_value_msat, msg.push_msat)));
+                       return Err(ChannelError::Close(format!("Suitable channel reserve not found. remote_channel_reserve was ({})msats. Channel value is ({} - {})msats.", holder_selected_channel_reserve_satoshis * 1000, full_channel_value_msat, msg.push_msat)));
                }
                if msg.channel_reserve_satoshis < MIN_CHAN_DUST_LIMIT_SATOSHIS {
                        log_debug!(logger, "channel_reserve_satoshis ({}) is smaller than our dust limit ({}). We can broadcast stale states without any risk, implying this channel is very insecure for our counterparty.",
@@ -2209,7 +2225,7 @@ impl<Signer: Sign> Channel<Signer> {
                                                          &self.channel_transaction_parameters,
                                                          funding_redeemscript.clone(), self.channel_value_satoshis,
                                                          obscure_factor,
-                                                         holder_commitment_tx, best_block);
+                                                         holder_commitment_tx, best_block, self.counterparty_node_id);
 
                channel_monitor.provide_latest_counterparty_commitment_tx(counterparty_initial_commitment_txid, Vec::new(), self.cur_counterparty_commitment_transaction_number, self.counterparty_cur_commitment_point.unwrap(), logger);
 
@@ -2286,7 +2302,7 @@ impl<Signer: Sign> Channel<Signer> {
                                                          &self.channel_transaction_parameters,
                                                          funding_redeemscript.clone(), self.channel_value_satoshis,
                                                          obscure_factor,
-                                                         holder_commitment_tx, best_block);
+                                                         holder_commitment_tx, best_block, self.counterparty_node_id);
 
                channel_monitor.provide_latest_counterparty_commitment_tx(counterparty_initial_bitcoin_tx.txid, Vec::new(), self.cur_counterparty_commitment_transaction_number, self.counterparty_cur_commitment_point.unwrap(), logger);
 
@@ -3687,7 +3703,7 @@ impl<Signer: Sign> Channel<Signer> {
                }
        }
 
-       pub fn update_fee<F: Deref>(&mut self, fee_estimator: &F, msg: &msgs::UpdateFee) -> Result<(), ChannelError>
+       pub fn update_fee<F: Deref>(&mut self, fee_estimator: &LowerBoundedFeeEstimator<F>, msg: &msgs::UpdateFee) -> Result<(), ChannelError>
                where F::Target: FeeEstimator
        {
                if self.is_outbound() {
@@ -4013,7 +4029,8 @@ impl<Signer: Sign> Channel<Signer> {
        /// Calculates and returns our minimum and maximum closing transaction fee amounts, in whole
        /// satoshis. The amounts remain consistent unless a peer disconnects/reconnects or we restart,
        /// at which point they will be recalculated.
-       fn calculate_closing_fee_limits<F: Deref>(&mut self, fee_estimator: &F) -> (u64, u64)
+       fn calculate_closing_fee_limits<F: Deref>(&mut self, fee_estimator: &LowerBoundedFeeEstimator<F>)
+               -> (u64, u64)
                where F::Target: FeeEstimator
        {
                if let Some((min, max)) = self.closing_fee_limits { return (min, max); }
@@ -4021,8 +4038,8 @@ impl<Signer: Sign> Channel<Signer> {
                // Propose a range from our current Background feerate to our Normal feerate plus our
                // force_close_avoidance_max_fee_satoshis.
                // If we fail to come to consensus, we'll have to force-close.
-               let mut proposed_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
-               let normal_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
+               let mut proposed_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Background);
+               let normal_feerate = fee_estimator.bounded_sat_per_1000_weight(ConfirmationTarget::Normal);
                let mut proposed_max_feerate = if self.is_outbound() { normal_feerate } else { u32::max_value() };
 
                // The spec requires that (when the channel does not have anchors) we only send absolute
@@ -4087,7 +4104,8 @@ impl<Signer: Sign> Channel<Signer> {
                Ok(())
        }
 
-       pub fn maybe_propose_closing_signed<F: Deref, L: Deref>(&mut self, fee_estimator: &F, logger: &L)
+       pub fn maybe_propose_closing_signed<F: Deref, L: Deref>(
+               &mut self, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
                -> Result<(Option<msgs::ClosingSigned>, Option<Transaction>), ChannelError>
                where F::Target: FeeEstimator, L::Target: Logger
        {
@@ -4241,7 +4259,9 @@ impl<Signer: Sign> Channel<Signer> {
                tx
        }
 
-       pub fn closing_signed<F: Deref>(&mut self, fee_estimator: &F, msg: &msgs::ClosingSigned) -> Result<(Option<msgs::ClosingSigned>, Option<Transaction>), ChannelError>
+       pub fn closing_signed<F: Deref>(
+               &mut self, fee_estimator: &LowerBoundedFeeEstimator<F>, msg: &msgs::ClosingSigned)
+               -> Result<(Option<msgs::ClosingSigned>, Option<Transaction>), ChannelError>
                where F::Target: FeeEstimator
        {
                if self.channel_state & BOTH_SIDES_SHUTDOWN_MASK != BOTH_SIDES_SHUTDOWN_MASK {
@@ -5761,7 +5781,7 @@ impl<Signer: Sign> Channel<Signer> {
        /// those explicitly stated to be allowed after shutdown completes, eg some simple getters).
        /// Also returns the list of payment_hashes for channels which we can safely fail backwards
        /// immediately (others we will have to allow to time out).
-       pub fn force_shutdown(&mut self, should_broadcast: bool) -> (Option<(OutPoint, ChannelMonitorUpdate)>, Vec<(HTLCSource, PaymentHash)>) {
+       pub fn force_shutdown(&mut self, should_broadcast: bool) -> (Option<(OutPoint, ChannelMonitorUpdate)>, Vec<(HTLCSource, PaymentHash, PublicKey, [u8; 32])>) {
                // Note that we MUST only generate a monitor update that indicates force-closure - we're
                // called during initialization prior to the chain_monitor in the encompassing ChannelManager
                // being fully configured in some cases. Thus, its likely any monitor events we generate will
@@ -5771,10 +5791,11 @@ impl<Signer: Sign> Channel<Signer> {
                // We go ahead and "free" any holding cell HTLCs or HTLCs we haven't yet committed to and
                // return them to fail the payment.
                let mut dropped_outbound_htlcs = Vec::with_capacity(self.holding_cell_htlc_updates.len());
+               let counterparty_node_id = self.get_counterparty_node_id();
                for htlc_update in self.holding_cell_htlc_updates.drain(..) {
                        match htlc_update {
                                HTLCUpdateAwaitingACK::AddHTLC { source, payment_hash, .. } => {
-                                       dropped_outbound_htlcs.push((source, payment_hash));
+                                       dropped_outbound_htlcs.push((source, payment_hash, counterparty_node_id, self.channel_id));
                                },
                                _ => {}
                        }
@@ -6102,7 +6123,7 @@ impl<Signer: Sign> Writeable for Channel<Signer> {
                // a different percentage of the channel value then 10%, which older versions of LDK used
                // to set it to before the percentage was made configurable.
                let serialized_holder_selected_reserve =
-                       if self.holder_selected_channel_reserve_satoshis != Self::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis)
+                       if self.holder_selected_channel_reserve_satoshis != Self::get_legacy_default_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis)
                        { Some(self.holder_selected_channel_reserve_satoshis) } else { None };
 
                let mut old_max_in_flight_percent_config = UserConfig::default().channel_handshake_config;
@@ -6377,7 +6398,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
                let mut announcement_sigs = None;
                let mut target_closing_feerate_sats_per_kw = None;
                let mut monitor_pending_finalized_fulfills = Some(Vec::new());
-               let mut holder_selected_channel_reserve_satoshis = Some(Self::get_holder_selected_channel_reserve_satoshis(channel_value_satoshis));
+               let mut holder_selected_channel_reserve_satoshis = Some(Self::get_legacy_default_holder_selected_channel_reserve_satoshis(channel_value_satoshis));
                let mut holder_max_htlc_value_in_flight_msat = Some(Self::get_holder_max_htlc_value_in_flight_msat(channel_value_satoshis, &UserConfig::default().channel_handshake_config));
                // Prior to supporting channel type negotiation, all of our channels were static_remotekey
                // only, so we default to that if none was written.
@@ -6557,6 +6578,7 @@ impl<'a, Signer: Sign, K: Deref> ReadableArgs<(&'a K, u32)> for Channel<Signer>
 
 #[cfg(test)]
 mod tests {
+       use std::cmp;
        use bitcoin::blockdata::script::{Script, Builder};
        use bitcoin::blockdata::transaction::{Transaction, TxOut};
        use bitcoin::blockdata::constants::genesis_block;
@@ -6566,14 +6588,14 @@ mod tests {
        use ln::PaymentHash;
        use ln::channelmanager::{HTLCSource, PaymentId};
        use ln::channel::{Channel, InboundHTLCOutput, OutboundHTLCOutput, InboundHTLCState, OutboundHTLCState, HTLCCandidate, HTLCInitiator};
-       use ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS};
+       use ln::channel::{MAX_FUNDING_SATOSHIS_NO_WUMBO, TOTAL_BITCOIN_SUPPLY_SATOSHIS, MIN_THEIR_CHAN_RESERVE_SATOSHIS};
        use ln::features::{InitFeatures, ChannelTypeFeatures};
-       use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate};
+       use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate, MAX_VALUE_MSAT};
        use ln::script::ShutdownScript;
        use ln::chan_utils;
        use ln::chan_utils::{htlc_success_tx_weight, htlc_timeout_tx_weight};
        use chain::BestBlock;
-       use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
+       use chain::chaininterface::{FeeEstimator, LowerBoundedFeeEstimator, ConfirmationTarget};
        use chain::keysinterface::{InMemorySigner, Recipient, KeyMaterial, KeysInterface};
        use chain::transaction::OutPoint;
        use util::config::UserConfig;
@@ -6581,14 +6603,16 @@ mod tests {
        use util::errors::APIError;
        use util::test_utils;
        use util::test_utils::OnGetShutdownScriptpubkey;
-       use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature};
+       use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature, Scalar};
        use bitcoin::secp256k1::ffi::Signature as FFISignature;
        use bitcoin::secp256k1::{SecretKey,PublicKey};
+       use bitcoin::secp256k1::ecdh::SharedSecret;
        use bitcoin::secp256k1::ecdsa::RecoverableSignature;
        use bitcoin::hashes::sha256::Hash as Sha256;
        use bitcoin::hashes::Hash;
        use bitcoin::hash_types::WPubkeyHash;
        use bitcoin::bech32::u5;
+       use bitcoin::PackedLockTime;
        use bitcoin::util::address::WitnessVersion;
        use prelude::*;
 
@@ -6612,7 +6636,9 @@ mod tests {
        fn test_no_fee_check_overflow() {
                // Previously, calling `check_remote_fee` with a fee of 0xffffffff would overflow in
                // arithmetic, causing a panic with debug assertions enabled.
-               assert!(Channel::<InMemorySigner>::check_remote_fee(&&TestFeeEstimator { fee_est: 42 }, u32::max_value()).is_err());
+               let fee_est = TestFeeEstimator { fee_est: 42 };
+               let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&fee_est);
+               assert!(Channel::<InMemorySigner>::check_remote_fee(&bounded_fee_estimator, u32::max_value()).is_err());
        }
 
        struct Keys {
@@ -6622,6 +6648,7 @@ mod tests {
                type Signer = InMemorySigner;
 
                fn get_node_secret(&self, _recipient: Recipient) -> Result<SecretKey, ()> { panic!(); }
+               fn ecdh(&self, _recipient: Recipient, _other_key: &PublicKey, _tweak: Option<&Scalar>) -> Result<SharedSecret, ()> { panic!(); }
                fn get_inbound_payment_key_material(&self) -> KeyMaterial { panic!(); }
                fn get_destination_script(&self) -> Script {
                        let secp_ctx = Secp256k1::signing_only();
@@ -6662,11 +6689,10 @@ mod tests {
                        returns: non_v0_segwit_shutdown_script.clone(),
                });
 
-               let fee_estimator = TestFeeEstimator { fee_est: 253 };
                let secp_ctx = Secp256k1::new();
                let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let config = UserConfig::default();
-               match Channel::<EnforcingSigner>::new_outbound(&&fee_estimator, &&keys_provider, node_id, &features, 10000000, 100000, 42, &config, 0, 42) {
+               match Channel::<EnforcingSigner>::new_outbound(&LowerBoundedFeeEstimator::new(&TestFeeEstimator { fee_est: 253 }), &&keys_provider, node_id, &features, 10000000, 100000, 42, &config, 0, 42) {
                        Err(APIError::IncompatibleShutdownScript { script }) => {
                                assert_eq!(script.into_inner(), non_v0_segwit_shutdown_script.into_inner());
                        },
@@ -6681,6 +6707,7 @@ mod tests {
        fn test_open_channel_msg_fee() {
                let original_fee = 253;
                let mut fee_est = TestFeeEstimator{fee_est: original_fee };
+               let bounded_fee_estimator = LowerBoundedFeeEstimator::new(&fee_est);
                let secp_ctx = Secp256k1::new();
                let seed = [42; 32];
                let network = Network::Testnet;
@@ -6688,7 +6715,7 @@ mod tests {
 
                let node_a_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let config = UserConfig::default();
-               let node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&fee_est, &&keys_provider, node_a_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap();
+               let node_a_chan = Channel::<EnforcingSigner>::new_outbound(&bounded_fee_estimator, &&keys_provider, node_a_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap();
 
                // Now change the fee so we can check that the fee in the open_channel message is the
                // same as the old fee.
@@ -6701,7 +6728,7 @@ mod tests {
        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 feeest = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
                let secp_ctx = Secp256k1::new();
                let seed = [42; 32];
                let network = Network::Testnet;
@@ -6714,13 +6741,13 @@ mod tests {
                // 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::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap();
+               let mut node_a_chan = Channel::<EnforcingSigner>::new_outbound(&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).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());
                let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
-               let mut node_b_chan = Channel::<EnforcingSigner>::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger, 42).unwrap();
+               let mut node_b_chan = Channel::<EnforcingSigner>::new_from_req(&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger, 42).unwrap();
 
                // Node B --> Node A: accept channel, explicitly setting B's dust limit.
                let mut accept_channel_msg = node_b_chan.accept_inbound_channel(0);
@@ -6776,7 +6803,7 @@ mod tests {
                // 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 fee_est = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 253 });
                let secp_ctx = Secp256k1::new();
                let seed = [42; 32];
                let network = Network::Testnet;
@@ -6784,7 +6811,7 @@ mod tests {
 
                let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let config = UserConfig::default();
-               let mut chan = Channel::<EnforcingSigner>::new_outbound(&&fee_est, &&keys_provider, node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap();
+               let mut chan = Channel::<EnforcingSigner>::new_outbound(&fee_est, &&keys_provider, node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap();
 
                let commitment_tx_fee_0_htlcs = Channel::<EnforcingSigner>::commit_tx_fee_msat(chan.feerate_per_kw, 0, chan.opt_anchors());
                let commitment_tx_fee_1_htlc = Channel::<EnforcingSigner>::commit_tx_fee_msat(chan.feerate_per_kw, 1, chan.opt_anchors());
@@ -6819,7 +6846,7 @@ mod tests {
 
        #[test]
        fn channel_reestablish_no_updates() {
-               let feeest = TestFeeEstimator{fee_est: 15000};
+               let feeest = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
                let logger = test_utils::TestLogger::new();
                let secp_ctx = Secp256k1::new();
                let seed = [42; 32];
@@ -6833,12 +6860,12 @@ mod tests {
                // 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::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap();
+               let mut node_a_chan = Channel::<EnforcingSigner>::new_outbound(&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap();
 
                // Create Node B's channel by receiving Node A's open_channel message
                let open_channel_msg = node_a_chan.get_open_channel(chain_hash);
                let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
-               let mut node_b_chan = Channel::<EnforcingSigner>::new_from_req(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger, 42).unwrap();
+               let mut node_b_chan = Channel::<EnforcingSigner>::new_from_req(&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger, 42).unwrap();
 
                // Node B --> Node A: accept channel
                let accept_channel_msg = node_b_chan.accept_inbound_channel(0);
@@ -6846,7 +6873,7 @@ mod tests {
 
                // Node A --> Node B: funding created
                let output_script = node_a_chan.get_funding_redeemscript();
-               let tx = Transaction { version: 1, lock_time: 0, input: Vec::new(), output: vec![TxOut {
+               let tx = Transaction { version: 1, lock_time: PackedLockTime::ZERO, input: Vec::new(), output: vec![TxOut {
                        value: 10000000, script_pubkey: output_script.clone(),
                }]};
                let funding_outpoint = OutPoint{ txid: tx.txid(), index: 0 };
@@ -6885,7 +6912,7 @@ mod tests {
 
        #[test]
        fn test_configured_holder_max_htlc_value_in_flight() {
-               let feeest = TestFeeEstimator{fee_est: 15000};
+               let feeest = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
                let logger = test_utils::TestLogger::new();
                let secp_ctx = Secp256k1::new();
                let seed = [42; 32];
@@ -6906,12 +6933,12 @@ mod tests {
                // Test that `new_outbound` creates a channel with the correct value for
                // `holder_max_htlc_value_in_flight_msat`, when configured with a valid percentage value,
                // which is set to the lower bound + 1 (2%) of the `channel_value`.
-               let chan_1 = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, outbound_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config_2_percent, 0, 42).unwrap();
+               let chan_1 = Channel::<EnforcingSigner>::new_outbound(&feeest, &&keys_provider, outbound_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config_2_percent, 0, 42).unwrap();
                let chan_1_value_msat = chan_1.channel_value_satoshis * 1000;
                assert_eq!(chan_1.holder_max_htlc_value_in_flight_msat, (chan_1_value_msat as f64 * 0.02) as u64);
 
                // Test with the upper bound - 1 of valid values (99%).
-               let chan_2 = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, outbound_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config_99_percent, 0, 42).unwrap();
+               let chan_2 = Channel::<EnforcingSigner>::new_outbound(&feeest, &&keys_provider, outbound_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config_99_percent, 0, 42).unwrap();
                let chan_2_value_msat = chan_2.channel_value_satoshis * 1000;
                assert_eq!(chan_2.holder_max_htlc_value_in_flight_msat, (chan_2_value_msat as f64 * 0.99) as u64);
 
@@ -6920,45 +6947,103 @@ mod tests {
                // Test that `new_from_req` creates a channel with the correct value for
                // `holder_max_htlc_value_in_flight_msat`, when configured with a valid percentage value,
                // which is set to the lower bound - 1 (2%) of the `channel_value`.
-               let chan_3 = Channel::<EnforcingSigner>::new_from_req(&&feeest, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_1_open_channel_msg, 7, &config_2_percent, 0, &&logger, 42).unwrap();
+               let chan_3 = Channel::<EnforcingSigner>::new_from_req(&feeest, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_1_open_channel_msg, 7, &config_2_percent, 0, &&logger, 42).unwrap();
                let chan_3_value_msat = chan_3.channel_value_satoshis * 1000;
                assert_eq!(chan_3.holder_max_htlc_value_in_flight_msat, (chan_3_value_msat as f64 * 0.02) as u64);
 
                // Test with the upper bound - 1 of valid values (99%).
-               let chan_4 = Channel::<EnforcingSigner>::new_from_req(&&feeest, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_1_open_channel_msg, 7, &config_99_percent, 0, &&logger, 42).unwrap();
+               let chan_4 = Channel::<EnforcingSigner>::new_from_req(&feeest, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_1_open_channel_msg, 7, &config_99_percent, 0, &&logger, 42).unwrap();
                let chan_4_value_msat = chan_4.channel_value_satoshis * 1000;
                assert_eq!(chan_4.holder_max_htlc_value_in_flight_msat, (chan_4_value_msat as f64 * 0.99) as u64);
 
                // Test that `new_outbound` uses the lower bound of the configurable percentage values (1%)
                // if `max_inbound_htlc_value_in_flight_percent_of_channel` is set to a value less than 1.
-               let chan_5 = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, outbound_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config_0_percent, 0, 42).unwrap();
+               let chan_5 = Channel::<EnforcingSigner>::new_outbound(&feeest, &&keys_provider, outbound_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config_0_percent, 0, 42).unwrap();
                let chan_5_value_msat = chan_5.channel_value_satoshis * 1000;
                assert_eq!(chan_5.holder_max_htlc_value_in_flight_msat, (chan_5_value_msat as f64 * 0.01) as u64);
 
                // Test that `new_outbound` uses the upper bound of the configurable percentage values
                // (100%) if `max_inbound_htlc_value_in_flight_percent_of_channel` is set to a larger value
                // than 100.
-               let chan_6 = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, outbound_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config_101_percent, 0, 42).unwrap();
+               let chan_6 = Channel::<EnforcingSigner>::new_outbound(&feeest, &&keys_provider, outbound_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config_101_percent, 0, 42).unwrap();
                let chan_6_value_msat = chan_6.channel_value_satoshis * 1000;
                assert_eq!(chan_6.holder_max_htlc_value_in_flight_msat, chan_6_value_msat);
 
                // Test that `new_from_req` uses the lower bound of the configurable percentage values (1%)
                // if `max_inbound_htlc_value_in_flight_percent_of_channel` is set to a value less than 1.
-               let chan_7 = Channel::<EnforcingSigner>::new_from_req(&&feeest, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_1_open_channel_msg, 7, &config_0_percent, 0, &&logger, 42).unwrap();
+               let chan_7 = Channel::<EnforcingSigner>::new_from_req(&feeest, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_1_open_channel_msg, 7, &config_0_percent, 0, &&logger, 42).unwrap();
                let chan_7_value_msat = chan_7.channel_value_satoshis * 1000;
                assert_eq!(chan_7.holder_max_htlc_value_in_flight_msat, (chan_7_value_msat as f64 * 0.01) as u64);
 
                // Test that `new_from_req` uses the upper bound of the configurable percentage values
                // (100%) if `max_inbound_htlc_value_in_flight_percent_of_channel` is set to a larger value
                // than 100.
-               let chan_8 = Channel::<EnforcingSigner>::new_from_req(&&feeest, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_1_open_channel_msg, 7, &config_101_percent, 0, &&logger, 42).unwrap();
+               let chan_8 = Channel::<EnforcingSigner>::new_from_req(&feeest, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_1_open_channel_msg, 7, &config_101_percent, 0, &&logger, 42).unwrap();
                let chan_8_value_msat = chan_8.channel_value_satoshis * 1000;
                assert_eq!(chan_8.holder_max_htlc_value_in_flight_msat, chan_8_value_msat);
        }
 
+       #[test]
+       fn test_configured_holder_selected_channel_reserve_satoshis() {
+
+               // Test that `new_outbound` and `new_from_req` create a channel with the correct
+               // channel reserves, when `their_channel_reserve_proportional_millionths` is configured.
+               test_self_and_counterparty_channel_reserve(10_000_000, 0.02, 0.02);
+
+               // Test with valid but unreasonably high channel reserves
+               // Requesting and accepting parties have requested for 49%-49% and 60%-30% channel reserve
+               test_self_and_counterparty_channel_reserve(10_000_000, 0.49, 0.49);
+               test_self_and_counterparty_channel_reserve(10_000_000, 0.60, 0.30);
+
+               // Test with calculated channel reserve less than lower bound
+               // i.e `MIN_THEIR_CHAN_RESERVE_SATOSHIS`
+               test_self_and_counterparty_channel_reserve(100_000, 0.00002, 0.30);
+
+               // Test with invalid channel reserves since sum of both is greater than or equal
+               // to channel value
+               test_self_and_counterparty_channel_reserve(10_000_000, 0.50, 0.50);
+               test_self_and_counterparty_channel_reserve(10_000_000, 0.60, 0.50);
+       }
+
+       fn test_self_and_counterparty_channel_reserve(channel_value_satoshis: u64, outbound_selected_channel_reserve_perc: f64, inbound_selected_channel_reserve_perc: f64) {
+               let fee_est = LowerBoundedFeeEstimator::new(&TestFeeEstimator { fee_est: 15_000 });
+               let logger = test_utils::TestLogger::new();
+               let secp_ctx = Secp256k1::new();
+               let seed = [42; 32];
+               let network = Network::Testnet;
+               let keys_provider = test_utils::TestKeysInterface::new(&seed, network);
+               let outbound_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
+               let inbound_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
+
+
+               let mut outbound_node_config = UserConfig::default();
+               outbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (outbound_selected_channel_reserve_perc * 1_000_000.0) as u32;
+               let chan = Channel::<EnforcingSigner>::new_outbound(&&fee_est, &&keys_provider, outbound_node_id, &InitFeatures::known(), channel_value_satoshis, 100_000, 42, &outbound_node_config, 0, 42).unwrap();
+
+               let expected_outbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.channel_value_satoshis as f64 * outbound_selected_channel_reserve_perc) as u64);
+               assert_eq!(chan.holder_selected_channel_reserve_satoshis, expected_outbound_selected_chan_reserve);
+
+               let chan_open_channel_msg = chan.get_open_channel(genesis_block(network).header.block_hash());
+               let mut inbound_node_config = UserConfig::default();
+               inbound_node_config.channel_handshake_config.their_channel_reserve_proportional_millionths = (inbound_selected_channel_reserve_perc * 1_000_000.0) as u32;
+
+               if outbound_selected_channel_reserve_perc + inbound_selected_channel_reserve_perc < 1.0 {
+                       let chan_inbound_node = Channel::<EnforcingSigner>::new_from_req(&&fee_est, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_open_channel_msg, 7, &inbound_node_config, 0, &&logger, 42).unwrap();
+
+                       let expected_inbound_selected_chan_reserve = cmp::max(MIN_THEIR_CHAN_RESERVE_SATOSHIS, (chan.channel_value_satoshis as f64 * inbound_selected_channel_reserve_perc) as u64);
+
+                       assert_eq!(chan_inbound_node.holder_selected_channel_reserve_satoshis, expected_inbound_selected_chan_reserve);
+                       assert_eq!(chan_inbound_node.counterparty_selected_channel_reserve_satoshis.unwrap(), expected_outbound_selected_chan_reserve);
+               } else {
+                       // Channel Negotiations failed
+                       let result = Channel::<EnforcingSigner>::new_from_req(&&fee_est, &&keys_provider, inbound_node_id, &InitFeatures::known(), &chan_open_channel_msg, 7, &inbound_node_config, 0, &&logger, 42);
+                       assert!(result.is_err());
+               }
+       }
+
        #[test]
        fn channel_update() {
-               let feeest = TestFeeEstimator{fee_est: 15000};
+               let feeest = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
                let secp_ctx = Secp256k1::new();
                let seed = [42; 32];
                let network = Network::Testnet;
@@ -6968,7 +7053,7 @@ mod tests {
                // Create a channel.
                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::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap();
+               let mut node_a_chan = Channel::<EnforcingSigner>::new_outbound(&feeest, &&keys_provider, node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap();
                assert!(node_a_chan.counterparty_forwarding_info.is_none());
                assert_eq!(node_a_chan.holder_htlc_minimum_msat, 1); // the default
                assert!(node_a_chan.counterparty_forwarding_info().is_none());
@@ -6982,7 +7067,7 @@ mod tests {
                                flags: 0,
                                cltv_expiry_delta: 100,
                                htlc_minimum_msat: 5,
-                               htlc_maximum_msat: OptionalField::Absent,
+                               htlc_maximum_msat: MAX_VALUE_MSAT,
                                fee_base_msat: 110,
                                fee_proportional_millionths: 11,
                                excess_data: Vec::new(),
@@ -7047,7 +7132,7 @@ mod tests {
                let counterparty_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let mut config = UserConfig::default();
                config.channel_handshake_config.announced_channel = false;
-               let mut chan = Channel::<InMemorySigner>::new_outbound(&&feeest, &&keys_provider, counterparty_node_id, &InitFeatures::known(), 10_000_000, 100000, 42, &config, 0, 42).unwrap(); // Nothing uses their network key in this test
+               let mut chan = Channel::<InMemorySigner>::new_outbound(&LowerBoundedFeeEstimator::new(&feeest), &&keys_provider, counterparty_node_id, &InitFeatures::known(), 10_000_000, 100000, 42, &config, 0, 42).unwrap(); // Nothing uses their network key in this test
                chan.holder_dust_limit_satoshis = 546;
                chan.counterparty_selected_channel_reserve_satoshis = Some(0); // Filled in in accept_channel
 
@@ -7885,7 +7970,7 @@ mod tests {
 
        #[test]
        fn test_zero_conf_channel_type_support() {
-               let feeest = TestFeeEstimator{fee_est: 15000};
+               let feeest = LowerBoundedFeeEstimator::new(&TestFeeEstimator{fee_est: 15000});
                let secp_ctx = Secp256k1::new();
                let seed = [42; 32];
                let network = Network::Testnet;
@@ -7894,7 +7979,7 @@ mod tests {
 
                let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap());
                let config = UserConfig::default();
-               let node_a_chan = Channel::<EnforcingSigner>::new_outbound(&&feeest, &&keys_provider,
+               let node_a_chan = Channel::<EnforcingSigner>::new_outbound(&feeest, &&keys_provider,
                        node_b_node_id, &InitFeatures::known(), 10000000, 100000, 42, &config, 0, 42).unwrap();
 
                let mut channel_type_features = ChannelTypeFeatures::only_static_remote_key();
@@ -7903,7 +7988,7 @@ mod tests {
                let mut open_channel_msg = node_a_chan.get_open_channel(genesis_block(network).header.block_hash());
                open_channel_msg.channel_type = Some(channel_type_features);
                let node_b_node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[7; 32]).unwrap());
-               let res = Channel::<EnforcingSigner>::new_from_req(&&feeest, &&keys_provider,
+               let res = Channel::<EnforcingSigner>::new_from_req(&feeest, &&keys_provider,
                        node_b_node_id, &InitFeatures::known(), &open_channel_msg, 7, &config, 0, &&logger, 42);
                assert!(res.is_ok());
        }