From: Matt Corallo Date: Sun, 4 Jul 2021 14:46:17 +0000 (+0000) Subject: Fix channel reserve calculation on the sending side X-Git-Tag: v0.0.100~22^2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=306e9a5acf7553b966030cc0ba177d2743675ea5;p=rust-lightning Fix channel reserve calculation on the sending side As the variable name implies holder_selected_chan_reserve_msat is intended to be in millisatoshis, but is instead calculated in satoshis. We fix that error here and update the relevant tests to more accurately calculate the expected reserve value and test both success and failure cases. Bug discovered by chanmon_consistency fuzz target. --- diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index dea87715e..b879ec9b2 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -241,6 +241,7 @@ fn check_api_err(api_err: APIError) { _ if err.starts_with("Cannot push more than their max accepted HTLCs ") => {}, _ if err.starts_with("Cannot send value that would put us over the max HTLC value in flight our peer will accept ") => {}, _ if err.starts_with("Cannot send value that would put our balance under counterparty-announced channel reserve value") => {}, + _ if err.starts_with("Cannot send value that would put counterparty balance under holder-announced channel reserve value") => {}, _ if err.starts_with("Cannot send value that would overdraw remaining funds.") => {}, _ if err.starts_with("Cannot send value that would not leave enough to pay for fees.") => {}, _ => panic!("{}", err), diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 87f4e6ea9..45f7aa73b 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -4087,7 +4087,7 @@ impl Channel { 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::::get_holder_selected_channel_reserve_satoshis(self.channel_value_satoshis); + let holder_selected_chan_reserve_msat = Channel::::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 { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index ad4d68ae8..d0b2bda04 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -22,6 +22,7 @@ use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC}; 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}; @@ -1700,14 +1701,24 @@ fn test_chan_reserve_violation_outbound_htlc_inbound_chan() { // 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::::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 @@ fn test_chan_reserve_violation_inbound_htlc_outbound_channel() { 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); @@ -1767,13 +1782,22 @@ fn test_chan_reserve_dust_inbound_htlcs_outbound_chan() { // 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::::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]