]> git.bitcoin.ninja Git - rust-lightning/commitdiff
Consider anchor outputs value on inbound HTLCs
authorWilmer Paulino <wilmer@wilmerpaulino.com>
Thu, 19 Oct 2023 16:27:30 +0000 (09:27 -0700)
committerWilmer Paulino <wilmer@wilmerpaulino.com>
Fri, 20 Oct 2023 18:04:41 +0000 (11:04 -0700)
This could lead us to accept HTLCs that would put the sender below
their reserve, which must never happen.

lightning/src/ln/channel.rs
lightning/src/ln/monitor_tests.rs
lightning/src/ln/payment_tests.rs

index 8c835ed068cff4632cfb36224cf3d29e1b674b39..4552e6176ad21b16b094df3e2821f255ab9de820 100644 (file)
@@ -2125,7 +2125,7 @@ fn commit_tx_fee_sat(feerate_per_kw: u32, num_htlcs: usize, channel_type_feature
 
 // 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(feerate_per_kw: u32, num_htlcs: usize, channel_type_features: &ChannelTypeFeatures) -> u64 {
+pub(crate) fn commit_tx_fee_msat(feerate_per_kw: u32, num_htlcs: usize, channel_type_features: &ChannelTypeFeatures) -> 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(channel_type_features) + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * feerate_per_kw as u64 / 1000 * 1000
@@ -2772,6 +2772,7 @@ impl<SP: Deref> Channel<SP> where
                if inbound_stats.pending_htlcs_value_msat + msg.amount_msat > self.context.holder_max_htlc_value_in_flight_msat {
                        return Err(ChannelError::Close(format!("Remote HTLC add would put them over our max HTLC value ({})", self.context.holder_max_htlc_value_in_flight_msat)));
                }
+
                // Check holder_selected_channel_reserve_satoshis (we're getting paid, so they have to at least meet
                // the reserve_satoshis we told them to always have as direct payment so that they lose
                // something if we punish them for broadcasting an old state).
@@ -2831,18 +2832,29 @@ impl<SP: Deref> Channel<SP> where
 
                // Check that the remote can afford to pay for this HTLC on-chain at the current
                // feerate_per_kw, while maintaining their channel reserve (as required by the spec).
-               let remote_commit_tx_fee_msat = if self.context.is_outbound() { 0 } else {
-                       let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
-                       self.context.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations
-               };
-               if pending_remote_value_msat - msg.amount_msat < remote_commit_tx_fee_msat {
-                       return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
-               };
-
-               if pending_remote_value_msat - msg.amount_msat - remote_commit_tx_fee_msat < self.context.holder_selected_channel_reserve_satoshis * 1000 {
-                       return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value".to_owned()));
+               {
+                       let remote_commit_tx_fee_msat = if self.context.is_outbound() { 0 } else {
+                               let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
+                               self.context.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations
+                       };
+                       let anchor_outputs_value_msat = if !self.context.is_outbound() && self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
+                               ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
+                       } else {
+                               0
+                       };
+                       if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(anchor_outputs_value_msat) < remote_commit_tx_fee_msat {
+                               return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
+                       };
+                       if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(remote_commit_tx_fee_msat).saturating_sub(anchor_outputs_value_msat) < self.context.holder_selected_channel_reserve_satoshis * 1000 {
+                               return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value".to_owned()));
+                       }
                }
 
+               let anchor_outputs_value_msat = if self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
+                       ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
+               } else {
+                       0
+               };
                if !self.context.is_outbound() {
                        // `2 *` and `Some(())` is for the fee spike buffer we keep for the remote. This deviates from
                        // the spec because in the spec, the fee spike buffer requirement doesn't exist on the
@@ -2854,7 +2866,7 @@ impl<SP: Deref> Channel<SP> where
                        // sensitive to fee spikes.
                        let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
                        let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.context.next_remote_commit_tx_fee_msat(htlc_candidate, Some(()));
-                       if pending_remote_value_msat - msg.amount_msat - self.context.holder_selected_channel_reserve_satoshis * 1000 < remote_fee_cost_incl_stuck_buffer_msat {
+                       if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(self.context.holder_selected_channel_reserve_satoshis * 1000).saturating_sub(anchor_outputs_value_msat) < remote_fee_cost_incl_stuck_buffer_msat {
                                // Note that if the pending_forward_status is not updated here, then it's because we're already failing
                                // the HTLC, i.e. its status is already set to failing.
                                log_info!(logger, "Attempting to fail HTLC due to fee spike buffer violation in channel {}. Rebalancing is required.", &self.context.channel_id());
@@ -2864,7 +2876,7 @@ impl<SP: Deref> Channel<SP> where
                        // Check that they won't violate our local required channel reserve by adding this HTLC.
                        let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
                        let local_commit_tx_fee_msat = self.context.next_local_commit_tx_fee_msat(htlc_candidate, None);
-                       if self.context.value_to_self_msat < self.context.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat {
+                       if self.context.value_to_self_msat < self.context.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat + anchor_outputs_value_msat {
                                return Err(ChannelError::Close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned()));
                        }
                }
index 0399ed38251ba7bc155af80323add37b86c9eef2..dcc0d57409bbd7abc4a5abae8a4970f8ea1c0de7 100644 (file)
@@ -1400,7 +1400,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
 
        // Create some initial channels
        let (_, _, chan_id, funding_tx) =
-               create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 11_000_000);
+               create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 12_000_000);
        let funding_outpoint = OutPoint { txid: funding_tx.txid(), index: 0 };
        assert_eq!(funding_outpoint.to_channel_id(), chan_id);
 
@@ -1410,9 +1410,9 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
        assert_eq!(revoked_local_txn[0].input.len(), 1);
        assert_eq!(revoked_local_txn[0].input[0].previous_output.txid, funding_tx.txid());
        if anchors {
-               assert_eq!(revoked_local_txn[0].output[4].value, 10000); // to_self output
+               assert_eq!(revoked_local_txn[0].output[4].value, 11000); // to_self output
        } else {
-               assert_eq!(revoked_local_txn[0].output[2].value, 10000); // to_self output
+               assert_eq!(revoked_local_txn[0].output[2].value, 11000); // to_self output
        }
 
        // The to-be-revoked commitment tx should have two HTLCs, an output for each side, and an
@@ -1499,11 +1499,11 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
        let anchor_outputs_value = if anchors { channel::ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 };
        let as_balances = sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
                        // to_remote output in B's revoked commitment
-                       amount_satoshis: 1_000_000 - 11_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
+                       amount_satoshis: 1_000_000 - 12_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
                        confirmation_height: to_remote_conf_height,
                }, Balance::CounterpartyRevokedOutputClaimable {
                        // to_self output in B's revoked commitment
-                       amount_satoshis: 10_000,
+                       amount_satoshis: 11_000,
                }, Balance::CounterpartyRevokedOutputClaimable { // HTLC 1
                        amount_satoshis: 3_000,
                }, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
@@ -1544,11 +1544,11 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
        mine_transaction(&nodes[0], &as_htlc_claim_tx[0]);
        assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
                        // to_remote output in B's revoked commitment
-                       amount_satoshis: 1_000_000 - 11_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
+                       amount_satoshis: 1_000_000 - 12_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
                        confirmation_height: to_remote_conf_height,
                }, Balance::CounterpartyRevokedOutputClaimable {
                        // to_self output in B's revoked commitment
-                       amount_satoshis: 10_000,
+                       amount_satoshis: 11_000,
                }, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
                        amount_satoshis: 1_000,
                }, Balance::ClaimableAwaitingConfirmations {
@@ -1561,7 +1561,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
        test_spendable_output(&nodes[0], &revoked_local_txn[0], false);
        assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
                        // to_self output to B
-                       amount_satoshis: 10_000,
+                       amount_satoshis: 11_000,
                }, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
                        amount_satoshis: 1_000,
                }, Balance::ClaimableAwaitingConfirmations {
@@ -1574,7 +1574,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
        test_spendable_output(&nodes[0], &as_htlc_claim_tx[0], false);
        assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
                        // to_self output in B's revoked commitment
-                       amount_satoshis: 10_000,
+                       amount_satoshis: 11_000,
                }, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
                        amount_satoshis: 1_000,
                }]),
@@ -1623,7 +1623,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
        connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
        assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
                        // to_self output in B's revoked commitment
-                       amount_satoshis: 10_000,
+                       amount_satoshis: 11_000,
                }, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
                        amount_satoshis: 1_000,
                }]),
@@ -1632,7 +1632,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
        mine_transaction(&nodes[0], &revoked_htlc_timeout_claim);
        assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
                        // to_self output in B's revoked commitment
-                       amount_satoshis: 10_000,
+                       amount_satoshis: 11_000,
                }, Balance::ClaimableAwaitingConfirmations {
                        amount_satoshis: revoked_htlc_timeout_claim.output[0].value,
                        confirmation_height: nodes[0].best_block_info().1 + ANTI_REORG_DELAY - 1,
index e25d8f06e45688446b13c2a5db68986ff6403423..8882af6836268f557a304796ee983f23fb750394 100644 (file)
@@ -16,9 +16,9 @@ use crate::chain::channelmonitor::{ANTI_REORG_DELAY, HTLC_FAIL_BACK_BUFFER, LATE
 use crate::sign::EntropySource;
 use crate::chain::transaction::OutPoint;
 use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose};
-use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
+use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS, commit_tx_fee_msat, get_holder_selected_channel_reserve_satoshis, ANCHOR_OUTPUT_VALUE_SATOSHI};
 use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, RecentPaymentDetails, RecipientOnionFields, HTLCForwardInfo, PendingHTLCRouting, PendingAddHTLCInfo};
-use crate::ln::features::Bolt11InvoiceFeatures;
+use crate::ln::features::{Bolt11InvoiceFeatures, ChannelTypeFeatures};
 use crate::ln::{msgs, ChannelId, PaymentSecret, PaymentPreimage};
 use crate::ln::msgs::ChannelMessageHandler;
 use crate::ln::outbound_payment::{IDEMPOTENCY_TIMEOUT_TICKS, Retry};
@@ -4093,3 +4093,95 @@ fn test_payment_metadata_consistency() {
        do_test_payment_metadata_consistency(false, true);
        do_test_payment_metadata_consistency(false, false);
 }
+
+#[test]
+fn  test_htlc_forward_considers_anchor_outputs_value() {
+       // Tests that:
+       //
+       // 1) Forwarding nodes don't forward HTLCs that would cause their balance to dip below the
+       //    reserve when considering the value of anchor outputs.
+       //
+       // 2) Recipients of `update_add_htlc` properly reject HTLCs that would cause the initiator's
+       //    balance to dip below the reserve when considering the value of anchor outputs.
+       let mut config = test_default_channel_config();
+       config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
+       config.manually_accept_inbound_channels = true;
+       config.channel_config.forwarding_fee_base_msat = 0;
+       config.channel_config.forwarding_fee_proportional_millionths = 0;
+
+       // Set up a test network of three nodes that replicates a production failure leading to the
+       // discovery of this bug.
+       let chanmon_cfgs = create_chanmon_cfgs(3);
+       let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+       let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(config), Some(config), Some(config)]);
+       let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+
+       const CHAN_AMT: u64 = 1_000_000;
+       const PUSH_MSAT: u64 = 900_000_000;
+       create_announced_chan_between_nodes_with_value(&nodes, 0, 1, CHAN_AMT, 500_000_000);
+       let (_, _, chan_id_2, _) = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, CHAN_AMT, PUSH_MSAT);
+
+       let channel_reserve_msat = get_holder_selected_channel_reserve_satoshis(CHAN_AMT, &config) * 1000;
+       let commitment_fee_msat = commit_tx_fee_msat(
+               *nodes[1].fee_estimator.sat_per_kw.lock().unwrap(), 2, &ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()
+       );
+       let anchor_outpus_value_msat = ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000;
+       let sendable_balance_msat = CHAN_AMT * 1000 - PUSH_MSAT - channel_reserve_msat - commitment_fee_msat - anchor_outpus_value_msat;
+       let channel_details = nodes[1].node.list_channels().into_iter().find(|channel| channel.channel_id == chan_id_2).unwrap();
+       assert!(sendable_balance_msat >= channel_details.next_outbound_htlc_minimum_msat);
+       assert!(sendable_balance_msat <= channel_details.next_outbound_htlc_limit_msat);
+
+       send_payment(&nodes[0], &[&nodes[1], &nodes[2]], sendable_balance_msat);
+       send_payment(&nodes[2], &[&nodes[1], &nodes[0]], sendable_balance_msat);
+
+       // Send out an HTLC that would cause the forwarding node to dip below its reserve when
+       // considering the value of anchor outputs.
+       let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(
+               nodes[0], nodes[2], sendable_balance_msat + anchor_outpus_value_msat
+       );
+       nodes[0].node.send_payment_with_route(
+               &route, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)
+       ).unwrap();
+       check_added_monitors!(nodes[0], 1);
+
+       let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+       assert_eq!(events.len(), 1);
+       let mut update_add_htlc = if let MessageSendEvent::UpdateHTLCs { updates, .. } = events.pop().unwrap() {
+               nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]);
+               check_added_monitors(&nodes[1], 0);
+               commitment_signed_dance!(nodes[1], nodes[0], &updates.commitment_signed, false);
+               updates.update_add_htlcs[0].clone()
+       } else {
+               panic!("Unexpected event");
+       };
+
+       // The forwarding node should reject forwarding it as expected.
+       expect_pending_htlcs_forwardable!(nodes[1]);
+       expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[1], vec![HTLCDestination::NextHopChannel {
+               node_id: Some(nodes[2].node.get_our_node_id()),
+               channel_id: chan_id_2
+       }]);
+       check_added_monitors(&nodes[1], 1);
+
+       let mut events = nodes[1].node.get_and_clear_pending_msg_events();
+       assert_eq!(events.len(), 1);
+       if let MessageSendEvent::UpdateHTLCs { updates, .. } = events.pop().unwrap() {
+               nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
+               check_added_monitors(&nodes[0], 0);
+               commitment_signed_dance!(nodes[0], nodes[1], &updates.commitment_signed, false);
+       } else {
+               panic!("Unexpected event");
+       }
+
+       expect_payment_failed!(nodes[0], payment_hash, false);
+
+       // Assume that the forwarding node did forward it, and make sure the recipient rejects it as an
+       // invalid update and closes the channel.
+       update_add_htlc.channel_id = chan_id_2;
+       nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &update_add_htlc);
+       check_closed_event(&nodes[2], 1, ClosureReason::ProcessingError {
+               err: "Remote HTLC add would put them under remote reserve value".to_owned()
+       }, false, &[nodes[1].node.get_our_node_id()], 1_000_000);
+       check_closed_broadcast(&nodes[2], 1, true);
+       check_added_monitors(&nodes[2], 1);
+}