Merge pull request #998 from TheBlueMatt/2021-07-fix-chan-reserve-msat-sat
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Mon, 26 Jul 2021 16:03:22 +0000 (16:03 +0000)
committerGitHub <noreply@github.com>
Mon, 26 Jul 2021 16:03:22 +0000 (16:03 +0000)
Fix channel reserve calculation on the sending side

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

index 0d5ffb038c9585853270e507b4664b0d3e60efaf,45f7aa73b9b6a4c2d1167d79d42a681dbd2591e3..dc2dd758a9ea97bae465a69836f79200190c97d7
@@@ -44,8 -44,8 +44,8 @@@ use util::scid_utils::scid_from_parts
  use prelude::*;
  use core::{cmp,mem,fmt};
  use core::ops::Deref;
 -#[cfg(any(test, feature = "fuzztarget"))]
 -use std::sync::Mutex;
 +#[cfg(any(test, feature = "fuzztarget", debug_assertions))]
 +use sync::Mutex;
  use bitcoin::hashes::hex::ToHex;
  use bitcoin::blockdata::opcodes::all::OP_PUSHBYTES_0;
  
@@@ -374,10 -374,10 +374,10 @@@ pub(super) struct Channel<Signer: Sign
  
        #[cfg(debug_assertions)]
        /// Max to_local and to_remote outputs in a locally-generated commitment transaction
 -      holder_max_commitment_tx_output: ::std::sync::Mutex<(u64, u64)>,
 +      holder_max_commitment_tx_output: Mutex<(u64, u64)>,
        #[cfg(debug_assertions)]
        /// Max to_local and to_remote outputs in a remote-generated commitment transaction
 -      counterparty_max_commitment_tx_output: ::std::sync::Mutex<(u64, u64)>,
 +      counterparty_max_commitment_tx_output: Mutex<(u64, u64)>,
  
        last_sent_closing_fee: Option<(u32, u64, Signature)>, // (feerate, fee, holder_sig)
  
@@@ -595,9 -595,9 +595,9 @@@ impl<Signer: Sign> Channel<Signer> 
                        monitor_pending_failures: Vec::new(),
  
                        #[cfg(debug_assertions)]
 -                      holder_max_commitment_tx_output: ::std::sync::Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
 +                      holder_max_commitment_tx_output: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
                        #[cfg(debug_assertions)]
 -                      counterparty_max_commitment_tx_output: ::std::sync::Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
 +                      counterparty_max_commitment_tx_output: Mutex::new((channel_value_satoshis * 1000 - push_msat, push_msat)),
  
                        last_sent_closing_fee: None,
  
                        monitor_pending_failures: Vec::new(),
  
                        #[cfg(debug_assertions)]
 -                      holder_max_commitment_tx_output: ::std::sync::Mutex::new((msg.push_msat, msg.funding_satoshis * 1000 - msg.push_msat)),
 +                      holder_max_commitment_tx_output: Mutex::new((msg.push_msat, msg.funding_satoshis * 1000 - msg.push_msat)),
                        #[cfg(debug_assertions)]
 -                      counterparty_max_commitment_tx_output: ::std::sync::Mutex::new((msg.push_msat, msg.funding_satoshis * 1000 - msg.push_msat)),
 +                      counterparty_max_commitment_tx_output: Mutex::new((msg.push_msat, msg.funding_satoshis * 1000 - msg.push_msat)),
  
                        last_sent_closing_fee: None,
  
                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::<Signer>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis);
+                       let holder_selected_chan_reserve_msat = Channel::<Signer>::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis) * 1000;
                        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 {
@@@ -4943,9 -4943,9 +4943,9 @@@ impl<'a, Signer: Sign, K: Deref> Readab
                        feerate_per_kw,
  
                        #[cfg(debug_assertions)]
 -                      holder_max_commitment_tx_output: ::std::sync::Mutex::new((0, 0)),
 +                      holder_max_commitment_tx_output: Mutex::new((0, 0)),
                        #[cfg(debug_assertions)]
 -                      counterparty_max_commitment_tx_output: ::std::sync::Mutex::new((0, 0)),
 +                      counterparty_max_commitment_tx_output: Mutex::new((0, 0)),
  
                        last_sent_closing_fee,
  
@@@ -5023,7 -5023,7 +5023,7 @@@ mod tests 
        use bitcoin::hashes::sha256::Hash as Sha256;
        use bitcoin::hashes::Hash;
        use bitcoin::hash_types::{Txid, WPubkeyHash};
 -      use std::sync::Arc;
 +      use sync::Arc;
        use prelude::*;
  
        struct TestFeeEstimator {
index ca835bd55cbe349c152ec6454405e3dc18c457bd,d0b2bda047b9cb3b835052423b95f41a6fb03d06..a149ed23f8a2af0c016c2f9dae42ad858137dc1c
@@@ -22,6 -22,7 +22,7 @@@ use ln::channel::{COMMITMENT_TX_BASE_WE
  use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA};
  use ln::channel::{Channel, ChannelError};
  use ln::{chan_utils, onion_utils};
+ use ln::chan_utils::HTLC_SUCCESS_TX_WEIGHT;
  use routing::router::{Route, RouteHop, RouteHint, RouteHintHop, get_route};
  use routing::network_graph::RoutingFees;
  use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures};
@@@ -53,7 -54,7 +54,7 @@@ use regex
  use prelude::*;
  use alloc::collections::BTreeSet;
  use core::default::Default;
 -use std::sync::{Arc, Mutex};
 +use sync::{Arc, Mutex};
  
  use ln::functional_test_utils::*;
  use ln::chan_utils::CommitmentTransaction;
@@@ -1700,14 -1701,24 +1701,24 @@@ 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.
-       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 = 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 node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
        let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
        let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
-       let _ = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000, InitFeatures::known(), InitFeatures::known());
  
-       let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[1], nodes[0], 4843000);
+       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 -= 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);
+       // 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);
        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"));
        assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
@@@ -1759,7 -1770,11 +1770,11 @@@ fn test_chan_reserve_violation_inbound_
  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 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 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 mut push_amt = 100_000_000;
+       push_amt -= feerate_per_kw as u64 * (COMMITMENT_TX_BASE_WEIGHT) / 1000 * 1000;
+       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());
  
-       let dust_amt = 329000; // Dust amount
+       let dust_amt = crate::ln::channel::MIN_DUST_LIMIT_SATOSHIS * 1000
+               + feerate_per_kw as u64 * HTLC_SUCCESS_TX_WEIGHT / 1000 * 1000 - 1;
        // 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);
+       // 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]