Skip to content

Commit 297390a

Browse files
wpaulinowpaulino
committed
Consider anchor outputs value on inbound HTLCs
This could lead us to accept HTLCs that would put the sender below their reserve, which must never happen.
1 parent d7672d4 commit 297390a

File tree

3 files changed

+130
-26
lines changed

3 files changed

+130
-26
lines changed

‎lightning/src/ln/channel.rs

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2125,7 +2125,7 @@ fn commit_tx_fee_sat(feerate_per_kw: u32, num_htlcs: usize, channel_type_feature
21252125

21262126
// Get the fee cost in MSATS of a commitment tx with a given number of HTLC outputs.
21272127
// Note that num_htlcs should not include dust HTLCs.
2128-
fn commit_tx_fee_msat(feerate_per_kw: u32, num_htlcs: usize, channel_type_features: &ChannelTypeFeatures) -> u64 {
2128+
pub(crate) fn commit_tx_fee_msat(feerate_per_kw: u32, num_htlcs: usize, channel_type_features: &ChannelTypeFeatures) -> u64 {
21292129
// Note that we need to divide before multiplying to round properly,
21302130
// since the lowest denomination of bitcoin on-chain is the satoshi.
21312131
(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
27722772
if inbound_stats.pending_htlcs_value_msat + msg.amount_msat > self.context.holder_max_htlc_value_in_flight_msat {
27732773
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)));
27742774
}
2775+
27752776
// Check holder_selected_channel_reserve_satoshis (we're getting paid, so they have to at least meet
27762777
// the reserve_satoshis we told them to always have as direct payment so that they lose
27772778
// something if we punish them for broadcasting an old state).
@@ -2831,18 +2832,29 @@ impl<SP: Deref> Channel<SP> where
28312832

28322833
// Check that the remote can afford to pay for this HTLC on-chain at the current
28332834
// feerate_per_kw, while maintaining their channel reserve (as required by the spec).
2834-
let remote_commit_tx_fee_msat = if self.context.is_outbound() { 0 } else {
2835-
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
2836-
self.context.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations
2837-
};
2838-
if pending_remote_value_msat - msg.amount_msat < remote_commit_tx_fee_msat {
2839-
return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
2840-
};
2841-
2842-
if pending_remote_value_msat - msg.amount_msat - remote_commit_tx_fee_msat < self.context.holder_selected_channel_reserve_satoshis * 1000 {
2843-
return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value".to_owned()));
2835+
{
2836+
let remote_commit_tx_fee_msat = if self.context.is_outbound() { 0 } else {
2837+
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
2838+
self.context.next_remote_commit_tx_fee_msat(htlc_candidate, None) // Don't include the extra fee spike buffer HTLC in calculations
2839+
};
2840+
let anchor_outputs_value_msat = if !self.context.is_outbound() && self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
2841+
ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
2842+
} else {
2843+
0
2844+
};
2845+
if pending_remote_value_msat.saturating_sub(msg.amount_msat).saturating_sub(anchor_outputs_value_msat) < remote_commit_tx_fee_msat {
2846+
return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees".to_owned()));
2847+
};
2848+
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 {
2849+
return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value".to_owned()));
2850+
}
28442851
}
28452852

2853+
let anchor_outputs_value_msat = if self.context.get_channel_type().supports_anchors_zero_fee_htlc_tx() {
2854+
ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000
2855+
} else {
2856+
0
2857+
};
28462858
if !self.context.is_outbound() {
28472859
// `2 *` and `Some(())` is for the fee spike buffer we keep for the remote. This deviates from
28482860
// 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
28542866
// sensitive to fee spikes.
28552867
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
28562868
let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.context.next_remote_commit_tx_fee_msat(htlc_candidate, Some(()));
2857-
if pending_remote_value_msat - msg.amount_msat - self.context.holder_selected_channel_reserve_satoshis * 1000 < remote_fee_cost_incl_stuck_buffer_msat {
2869+
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 {
28582870
// Note that if the pending_forward_status is not updated here, then it's because we're already failing
28592871
// the HTLC, i.e. its status is already set to failing.
28602872
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
28642876
// Check that they won't violate our local required channel reserve by adding this HTLC.
28652877
let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered);
28662878
let local_commit_tx_fee_msat = self.context.next_local_commit_tx_fee_msat(htlc_candidate, None);
2867-
if self.context.value_to_self_msat < self.context.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 + local_commit_tx_fee_msat {
2879+
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 {
28682880
return Err(ChannelError::Close("Cannot accept HTLC that would put our balance under counterparty-announced channel reserve value".to_owned()));
28692881
}
28702882
}

‎lightning/src/ln/monitor_tests.rs

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1400,7 +1400,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
14001400

14011401
// Create some initial channels
14021402
let (_, _, chan_id, funding_tx) =
1403-
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 11_000_000);
1403+
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 12_000_000);
14041404
let funding_outpoint = OutPoint { txid: funding_tx.txid(), index: 0 };
14051405
assert_eq!(funding_outpoint.to_channel_id(), chan_id);
14061406

@@ -1410,9 +1410,9 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
14101410
assert_eq!(revoked_local_txn[0].input.len(), 1);
14111411
assert_eq!(revoked_local_txn[0].input[0].previous_output.txid, funding_tx.txid());
14121412
if anchors {
1413-
assert_eq!(revoked_local_txn[0].output[4].value, 10000); // to_self output
1413+
assert_eq!(revoked_local_txn[0].output[4].value, 11000); // to_self output
14141414
} else {
1415-
assert_eq!(revoked_local_txn[0].output[2].value, 10000); // to_self output
1415+
assert_eq!(revoked_local_txn[0].output[2].value, 11000); // to_self output
14161416
}
14171417

14181418
// 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) {
14991499
let anchor_outputs_value = if anchors { channel::ANCHOR_OUTPUT_VALUE_SATOSHI * 2 } else { 0 };
15001500
let as_balances = sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
15011501
// to_remote output in B's revoked commitment
1502-
amount_satoshis: 1_000_000 - 11_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
1502+
amount_satoshis: 1_000_000 - 12_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
15031503
confirmation_height: to_remote_conf_height,
15041504
}, Balance::CounterpartyRevokedOutputClaimable {
15051505
// to_self output in B's revoked commitment
1506-
amount_satoshis: 10_000,
1506+
amount_satoshis: 11_000,
15071507
}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 1
15081508
amount_satoshis: 3_000,
15091509
}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
@@ -1544,11 +1544,11 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
15441544
mine_transaction(&nodes[0], &as_htlc_claim_tx[0]);
15451545
assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
15461546
// to_remote output in B's revoked commitment
1547-
amount_satoshis: 1_000_000 - 11_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
1547+
amount_satoshis: 1_000_000 - 12_000 - 3_000 - commitment_tx_fee - anchor_outputs_value,
15481548
confirmation_height: to_remote_conf_height,
15491549
}, Balance::CounterpartyRevokedOutputClaimable {
15501550
// to_self output in B's revoked commitment
1551-
amount_satoshis: 10_000,
1551+
amount_satoshis: 11_000,
15521552
}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
15531553
amount_satoshis: 1_000,
15541554
}, Balance::ClaimableAwaitingConfirmations {
@@ -1561,7 +1561,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
15611561
test_spendable_output(&nodes[0], &revoked_local_txn[0], false);
15621562
assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
15631563
// to_self output to B
1564-
amount_satoshis: 10_000,
1564+
amount_satoshis: 11_000,
15651565
}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
15661566
amount_satoshis: 1_000,
15671567
}, Balance::ClaimableAwaitingConfirmations {
@@ -1574,7 +1574,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
15741574
test_spendable_output(&nodes[0], &as_htlc_claim_tx[0], false);
15751575
assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
15761576
// to_self output in B's revoked commitment
1577-
amount_satoshis: 10_000,
1577+
amount_satoshis: 11_000,
15781578
}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
15791579
amount_satoshis: 1_000,
15801580
}]),
@@ -1623,7 +1623,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
16231623
connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1);
16241624
assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
16251625
// to_self output in B's revoked commitment
1626-
amount_satoshis: 10_000,
1626+
amount_satoshis: 11_000,
16271627
}, Balance::CounterpartyRevokedOutputClaimable { // HTLC 2
16281628
amount_satoshis: 1_000,
16291629
}]),
@@ -1632,7 +1632,7 @@ fn do_test_revoked_counterparty_htlc_tx_balances(anchors: bool) {
16321632
mine_transaction(&nodes[0], &revoked_htlc_timeout_claim);
16331633
assert_eq!(sorted_vec(vec![Balance::CounterpartyRevokedOutputClaimable {
16341634
// to_self output in B's revoked commitment
1635-
amount_satoshis: 10_000,
1635+
amount_satoshis: 11_000,
16361636
}, Balance::ClaimableAwaitingConfirmations {
16371637
amount_satoshis: revoked_htlc_timeout_claim.output[0].value,
16381638
confirmation_height: nodes[0].best_block_info().1 + ANTI_REORG_DELAY - 1,

‎lightning/src/ln/payment_tests.rs

Lines changed: 94 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,9 @@ use crate::chain::channelmonitor::{ANTI_REORG_DELAY, HTLC_FAIL_BACK_BUFFER, LATE
1616
use crate::sign::EntropySource;
1717
use crate::chain::transaction::OutPoint;
1818
use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider, PathFailure, PaymentFailureReason, PaymentPurpose};
19-
use crate::ln::channel::EXPIRE_PREV_CONFIG_TICKS;
19+
use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS, commit_tx_fee_msat, get_holder_selected_channel_reserve_satoshis, ANCHOR_OUTPUT_VALUE_SATOSHI};
2020
use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, RecentPaymentDetails, RecipientOnionFields, HTLCForwardInfo, PendingHTLCRouting, PendingAddHTLCInfo};
21-
use crate::ln::features::Bolt11InvoiceFeatures;
21+
use crate::ln::features::{Bolt11InvoiceFeatures, ChannelTypeFeatures};
2222
use crate::ln::{msgs, ChannelId, PaymentSecret, PaymentPreimage};
2323
use crate::ln::msgs::ChannelMessageHandler;
2424
use crate::ln::outbound_payment::{IDEMPOTENCY_TIMEOUT_TICKS, Retry};
@@ -4093,3 +4093,95 @@ fn test_payment_metadata_consistency() {
40934093
do_test_payment_metadata_consistency(false, true);
40944094
do_test_payment_metadata_consistency(false, false);
40954095
}
4096+
4097+
#[test]
4098+
fn test_htlc_forward_considers_anchor_outputs_value() {
4099+
// Tests that:
4100+
//
4101+
// 1) Forwarding nodes don't forward HTLCs that would cause their balance to dip below the
4102+
// reserve when considering the value of anchor outputs.
4103+
//
4104+
// 2) Recipients of `update_add_htlc` properly reject HTLCs that would cause the initiator's
4105+
// balance to dip below the reserve when considering the value of anchor outputs.
4106+
let mut config = test_default_channel_config();
4107+
config.channel_handshake_config.negotiate_anchors_zero_fee_htlc_tx = true;
4108+
config.manually_accept_inbound_channels = true;
4109+
config.channel_config.forwarding_fee_base_msat = 0;
4110+
config.channel_config.forwarding_fee_proportional_millionths = 0;
4111+
4112+
// Set up a test network of three nodes that replicates a production failure leading to the
4113+
// discovery of this bug.
4114+
let chanmon_cfgs = create_chanmon_cfgs(3);
4115+
let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
4116+
let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[Some(config), Some(config), Some(config)]);
4117+
let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
4118+
4119+
const CHAN_AMT: u64 = 1_000_000;
4120+
const PUSH_MSAT: u64 = 900_000_000;
4121+
create_announced_chan_between_nodes_with_value(&nodes, 0, 1, CHAN_AMT, 500_000_000);
4122+
let (_, _, chan_id_2, _) = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, CHAN_AMT, PUSH_MSAT);
4123+
4124+
let channel_reserve_msat = get_holder_selected_channel_reserve_satoshis(CHAN_AMT, &config) * 1000;
4125+
let commitment_fee_msat = commit_tx_fee_msat(
4126+
*nodes[1].fee_estimator.sat_per_kw.lock().unwrap(), 2, &ChannelTypeFeatures::anchors_zero_htlc_fee_and_dependencies()
4127+
);
4128+
let anchor_outpus_value_msat = ANCHOR_OUTPUT_VALUE_SATOSHI * 2 * 1000;
4129+
let sendable_balance_msat = CHAN_AMT * 1000 - PUSH_MSAT - channel_reserve_msat - commitment_fee_msat - anchor_outpus_value_msat;
4130+
let channel_details = nodes[1].node.list_channels().into_iter().find(|channel| channel.channel_id == chan_id_2).unwrap();
4131+
assert!(sendable_balance_msat >= channel_details.next_outbound_htlc_minimum_msat);
4132+
assert!(sendable_balance_msat <= channel_details.next_outbound_htlc_limit_msat);
4133+
4134+
send_payment(&nodes[0], &[&nodes[1], &nodes[2]], sendable_balance_msat);
4135+
send_payment(&nodes[2], &[&nodes[1], &nodes[0]], sendable_balance_msat);
4136+
4137+
// Send out an HTLC that would cause the forwarding node to dip below its reserve when
4138+
// considering the value of anchor outputs.
4139+
let (route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(
4140+
nodes[0], nodes[2], sendable_balance_msat + anchor_outpus_value_msat
4141+
);
4142+
nodes[0].node.send_payment_with_route(
4143+
&route, payment_hash, RecipientOnionFields::secret_only(payment_secret), PaymentId(payment_hash.0)
4144+
).unwrap();
4145+
check_added_monitors!(nodes[0], 1);
4146+
4147+
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
4148+
assert_eq!(events.len(), 1);
4149+
let mut update_add_htlc = if let MessageSendEvent::UpdateHTLCs { updates, .. } = events.pop().unwrap() {
4150+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &updates.update_add_htlcs[0]);
4151+
check_added_monitors(&nodes[1], 0);
4152+
commitment_signed_dance!(nodes[1], nodes[0], &updates.commitment_signed, false);
4153+
updates.update_add_htlcs[0].clone()
4154+
} else {
4155+
panic!("Unexpected event");
4156+
};
4157+
4158+
// The forwarding node should reject forwarding it as expected.
4159+
expect_pending_htlcs_forwardable!(nodes[1]);
4160+
expect_pending_htlcs_forwardable_and_htlc_handling_failed!(&nodes[1], vec![HTLCDestination::NextHopChannel {
4161+
node_id: Some(nodes[2].node.get_our_node_id()),
4162+
channel_id: chan_id_2
4163+
}]);
4164+
check_added_monitors(&nodes[1], 1);
4165+
4166+
let mut events = nodes[1].node.get_and_clear_pending_msg_events();
4167+
assert_eq!(events.len(), 1);
4168+
if let MessageSendEvent::UpdateHTLCs { updates, .. } = events.pop().unwrap() {
4169+
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
4170+
check_added_monitors(&nodes[0], 0);
4171+
commitment_signed_dance!(nodes[0], nodes[1], &updates.commitment_signed, false);
4172+
} else {
4173+
panic!("Unexpected event");
4174+
}
4175+
4176+
expect_payment_failed!(nodes[0], payment_hash, false);
4177+
4178+
// Assume that the forwarding node did forward it, and make sure the recipient rejects it as an
4179+
// invalid update and closes the channel.
4180+
update_add_htlc.channel_id = chan_id_2;
4181+
nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &update_add_htlc);
4182+
check_closed_event(&nodes[2], 1, ClosureReason::ProcessingError {
4183+
err: "Remote HTLC add would put them under remote reserve value".to_owned()
4184+
}, false, &[nodes[1].node.get_our_node_id()], 1_000_000);
4185+
check_closed_broadcast(&nodes[2], 1, true);
4186+
check_added_monitors(&nodes[2], 1);
4187+
}

0 commit comments

Comments
 (0)