Skip to content

Commit c09c383

Browse files
committed
Mind htlc_minimum_msat when truncating overpaid value
At truncating the overpaid value, if we fall below htlc_minimum_msat, reach it by increasing fees.
1 parent 2110da2 commit c09c383

File tree

1 file changed

+230
-68
lines changed

1 file changed

+230
-68
lines changed

lightning/src/routing/router.rs

Lines changed: 230 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ pub struct RouteHop {
3939
/// to reach this node.
4040
pub channel_features: ChannelFeatures,
4141
/// The fee taken on this hop (for paying for the use of the *next* channel in the path).
42-
/// For the last hop, this should be the full value of the payment.
42+
/// For the last hop, this should be the full value of the payment (might be more than
43+
/// requested if we had to match htlc_minimum_msat).
4344
pub fee_msat: u64,
4445
/// The CLTV delta added for this hop. For the last hop, this should be the full CLTV value
4546
/// expected at the destination, in excess of the current block height.
@@ -186,6 +187,10 @@ struct PathBuildingHop {
186187
/// an estimated cost of reaching this hop.
187188
/// Might get stale when fees are recomputed. Primarily for internal use.
188189
total_fee_msat: u64,
190+
/// This is useful for update_value_and_recompute_fees to make sure
191+
/// we don't fall below the minimum. Should not be updated manually and
192+
/// generally should not be accessed.
193+
htlc_minimum_msat: u64,
189194
}
190195

191196
impl PathBuildingHop {
@@ -230,14 +235,52 @@ impl PaymentPath {
230235

231236
// If an amount transferred by the path is updated, the fees should be adjusted.
232237
// Any other way to change fees may result in an inconsistency.
233-
// The caller should make sure values don't fall below htlc_minimum_msat of the used channels.
238+
// Also, it's only safe to reduce the value, to not violate limits like
239+
// htlc_minimum_msat.
234240
fn update_value_and_recompute_fees(&mut self, value_msat: u64) {
241+
if value_msat == self.hops.last().unwrap().route_hop.fee_msat {
242+
// Nothing to change.
243+
return;
244+
}
245+
assert!(value_msat < self.hops.last().unwrap().route_hop.fee_msat);
246+
235247
let mut total_fee_paid_msat = 0 as u64;
236-
for i in (1..self.hops.len()).rev() {
237-
let cur_hop_amount_msat = total_fee_paid_msat + value_msat;
248+
for i in (0..self.hops.len()).rev() {
249+
let last_hop = i == self.hops.len() - 1;
250+
251+
// For non-last-hop, this value will represent the fees paid on the current hop. It
252+
// will consist of the fees for the use of the next hop, and extra fees to match
253+
// htlc_minimum_msat of the current channel. Last hop is handled separately.
254+
let mut cur_hop_fees_msat = 0;
255+
if !last_hop {
256+
cur_hop_fees_msat = self.hops.get(i + 1).unwrap().hop_use_fee_msat;
257+
}
258+
238259
let mut cur_hop = self.hops.get_mut(i).unwrap();
239260
cur_hop.next_hops_fee_msat = total_fee_paid_msat;
240-
if let Some(new_fee) = compute_fees(cur_hop_amount_msat, cur_hop.channel_fees) {
261+
// Overpay in fees if we can't save these funds due to htlc_minimum_msat.
262+
// We try to account for htlc_minimum_msat in scoring (add_entry!), so that nodes don't
263+
// set it too high just to maliciously take more fees by exploiting this
264+
// match htlc_minimum_msat logic.
265+
let mut cur_hop_transferred_amount_msat = total_fee_paid_msat + value_msat;
266+
if let Some(extra_fees_msat) = cur_hop.htlc_minimum_msat.checked_sub(cur_hop_transferred_amount_msat) {
267+
cur_hop_transferred_amount_msat += extra_fees_msat;
268+
total_fee_paid_msat += extra_fees_msat;
269+
cur_hop_fees_msat += extra_fees_msat;
270+
}
271+
272+
if last_hop {
273+
// Final hop is a special case: it usually has just value_msat (by design), but also
274+
// it still could overpay for the htlc_minimum_msat.
275+
cur_hop.route_hop.fee_msat = cur_hop_transferred_amount_msat;
276+
} else {
277+
// Propagate updated fees for the use of the channels to one hop back, where they
278+
// will be actually paid (fee_msat). The last hop is handled above separately.
279+
cur_hop.route_hop.fee_msat = cur_hop_fees_msat;
280+
}
281+
282+
// Fee for the use of the current hop which will be deducted on the previous hop.
283+
if let Some(new_fee) = compute_fees(cur_hop_transferred_amount_msat, cur_hop.channel_fees) {
241284
cur_hop.hop_use_fee_msat = new_fee;
242285
total_fee_paid_msat += new_fee;
243286
} else {
@@ -247,16 +290,6 @@ impl PaymentPath {
247290
unreachable!();
248291
}
249292
}
250-
251-
// Propagate updated fees for the use of the channels to one hop back,
252-
// where they will be actually paid (fee_msat).
253-
// For the last hop it will represent the value being transferred over this path.
254-
for i in 0..self.hops.len() - 1 {
255-
let next_hop_use_fee_msat = self.hops.get(i + 1).unwrap().hop_use_fee_msat;
256-
self.hops.get_mut(i).unwrap().route_hop.fee_msat = next_hop_use_fee_msat;
257-
}
258-
self.hops.last_mut().unwrap().route_hop.fee_msat = value_msat;
259-
self.hops.last_mut().unwrap().hop_use_fee_msat = 0;
260293
}
261294
}
262295

@@ -466,17 +499,13 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
466499
None => unreachable!(),
467500
};
468501

469-
// If HTLC minimum is larger than the whole value we're going to transfer, we shouldn't bother
470-
// even considering this channel, because it won't be useful.
502+
// If HTLC minimum is larger than the amount we're going to transfer, we shouldn't
503+
// bother considering this channel.
504+
// Since we're choosing amount_to_transfer_over_msat as maximum possible, it can
505+
// be only reduced later (not increased), so this channel should just be skipped
506+
// as not sufficient.
471507
// TODO: Explore simply adding fee to hit htlc_minimum_msat
472-
// This is separate from checking amount_to_transfer_over_msat, which also should be
473-
// lower-bounded by htlc_minimum_msat. Since we're choosing it as maximum possible,
474-
// this channel should just be skipped if it's not sufficient.
475-
// amount_to_transfer_over_msat can be both larger and smaller than final_value_msat,
476-
// so this can't be derived.
477-
let final_value_satisfies_htlc_minimum_msat = final_value_msat >= $directional_info.htlc_minimum_msat;
478-
if final_value_satisfies_htlc_minimum_msat &&
479-
contributes_sufficient_value && amount_to_transfer_over_msat >= $directional_info.htlc_minimum_msat {
508+
if contributes_sufficient_value && amount_to_transfer_over_msat >= $directional_info.htlc_minimum_msat {
480509
let hm_entry = dist.entry(&$src_node_id);
481510
let old_entry = hm_entry.or_insert_with(|| {
482511
// If there was previously no known way to access
@@ -507,6 +536,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
507536
next_hops_fee_msat: u64::max_value(),
508537
hop_use_fee_msat: u64::max_value(),
509538
total_fee_msat: u64::max_value(),
539+
htlc_minimum_msat: $directional_info.htlc_minimum_msat,
510540
}
511541
});
512542

@@ -554,7 +584,29 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
554584
// (considering the cost to "reach" this channel from the route destination,
555585
// the cost of using this channel,
556586
// and the cost of routing to the source node of this channel).
557-
if old_entry.total_fee_msat > total_fee_msat {
587+
// Also, consider that htlc_minimum_msat_difference, because we might end up
588+
// paying it. Consider the following exploit:
589+
// we use 2 paths to transfer 1.5 BTC. One of them is 0-fee normal 1 BTC path,
590+
// and for the other one we picked a 1sat-fee path with htlc_minimum_msat of
591+
// 1 BTC. Now, since the latter is more expensive, we gonna try to cut it
592+
// by 0.5 BTC, but then match htlc_minimum_msat by paying a fee of 0.5 BTC
593+
// to this channel.
594+
// TODO: this scoring could be smarter (e.g. 0.5*htlc_minimum_msat here).
595+
let mut old_cost = old_entry.total_fee_msat;
596+
if let Some(increased_old_cost) = old_cost.checked_add(old_entry.htlc_minimum_msat) {
597+
old_cost = increased_old_cost;
598+
} else {
599+
old_cost = u64::max_value();
600+
}
601+
602+
let mut new_cost = total_fee_msat;
603+
if let Some(increased_new_cost) = new_cost.checked_add($directional_info.htlc_minimum_msat) {
604+
new_cost = increased_new_cost;
605+
} else {
606+
new_cost = u64::max_value();
607+
}
608+
609+
if new_cost < old_cost {
558610
targets.push(new_graph_node);
559611
old_entry.next_hops_fee_msat = $next_hops_fee_msat;
560612
old_entry.hop_use_fee_msat = hop_use_fee_msat;
@@ -568,6 +620,9 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
568620
cltv_expiry_delta: $directional_info.cltv_expiry_delta as u32,
569621
};
570622
old_entry.channel_fees = $directional_info.fees;
623+
// It's probably fine to replace the old entry, because the new one
624+
// passed the htlc_minimum-related checks above.
625+
old_entry.htlc_minimum_msat = $directional_info.htlc_minimum_msat;
571626
}
572627
}
573628
}
@@ -852,7 +907,7 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
852907
// Sort by value so that we drop many really-low values first, since
853908
// fewer paths is better: the payment is less likely to fail.
854909
// TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
855-
// so that the sender pays less fees overall.
910+
// so that the sender pays less fees overall. And also htlc_minimum_msat.
856911
cur_route.sort_by_key(|path| path.get_value_msat());
857912
// We should make sure that at least 1 path left.
858913
let mut paths_left = cur_route.len();
@@ -878,11 +933,20 @@ pub fn get_route<L: Deref>(our_node_id: &PublicKey, network: &NetworkGraph, paye
878933

879934
// Step (7).
880935
// Now, substract the overpaid value from the most-expensive path.
936+
// TODO: this could also be optimized by also sorting by feerate_per_sat_routed,
937+
// so that the sender pays less fees overall. And also htlc_minimum_msat.
881938
cur_route.sort_by_key(|path| { path.hops.iter().map(|hop| hop.channel_fees.proportional_millionths as u64).sum::<u64>() });
882-
let expensive_payment_path = cur_route.last_mut().unwrap();
883-
let expensive_path_new_value_msat = expensive_payment_path.get_value_msat() - overpaid_value_msat;
884-
// TODO: make sure expensive_path_new_value_msat doesn't cause to fall behind htlc_minimum_msat.
885-
expensive_payment_path.update_value_and_recompute_fees(expensive_path_new_value_msat);
939+
for expensive_payment_path in &mut cur_route {
940+
if let Some(expensive_path_new_value_msat) = expensive_payment_path.get_value_msat().checked_sub(overpaid_value_msat) {
941+
expensive_payment_path.update_value_and_recompute_fees(expensive_path_new_value_msat);
942+
break;
943+
} else {
944+
// We already dropped all the small channels above, meaning all the
945+
// remaining channels are larger than remaining overpaid_value_msat.
946+
// Thus, this can't be negative.
947+
unreachable!();
948+
}
949+
}
886950
break;
887951
}
888952
}
@@ -1385,33 +1449,6 @@ mod tests {
13851449
let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx);
13861450

13871451
// Simple route to 2 via 1
1388-
// Should fail if htlc_minimum can't be reached
1389-
1390-
update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
1391-
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
1392-
short_channel_id: 2,
1393-
timestamp: 2,
1394-
flags: 0,
1395-
cltv_expiry_delta: 0,
1396-
htlc_minimum_msat: 50_000,
1397-
htlc_maximum_msat: OptionalField::Absent,
1398-
fee_base_msat: 0,
1399-
fee_proportional_millionths: 0,
1400-
excess_data: Vec::new()
1401-
});
1402-
// Make 0 fee.
1403-
update_channel(&net_graph_msg_handler, &secp_ctx, &privkeys[1], UnsignedChannelUpdate {
1404-
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
1405-
short_channel_id: 4,
1406-
timestamp: 2,
1407-
flags: 0,
1408-
cltv_expiry_delta: 0,
1409-
htlc_minimum_msat: 0,
1410-
htlc_maximum_msat: OptionalField::Absent,
1411-
fee_base_msat: 0,
1412-
fee_proportional_millionths: 0,
1413-
excess_data: Vec::new()
1414-
});
14151452

14161453
// Disable other paths
14171454
update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
@@ -1475,16 +1512,7 @@ mod tests {
14751512
excess_data: Vec::new()
14761513
});
14771514

1478-
if let Err(LightningError{err, action: ErrorAction::IgnoreError}) = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, &Vec::new(), 49_999, 42, Arc::clone(&logger)) {
1479-
assert_eq!(err, "Failed to find a path to the given destination");
1480-
} else { panic!(); }
1481-
1482-
// A payment above the minimum should pass
1483-
let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, &Vec::new(), 50_000, 42, Arc::clone(&logger)).unwrap();
1484-
assert_eq!(route.paths[0].len(), 2);
1485-
1486-
// This was a check against final_value_msat.
1487-
// Now let's check against amount_to_transfer_over_msat.
1515+
// Check against amount_to_transfer_over_msat.
14881516
// We will be transferring 300_000_000 msat.
14891517
// Set minimal HTLC of 200_000_000 msat.
14901518
update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
@@ -1540,6 +1568,140 @@ mod tests {
15401568
assert_eq!(route.paths[0].len(), 2);
15411569
}
15421570

1571+
#[test]
1572+
fn htlc_minimum_overpay_test() {
1573+
let (secp_ctx, net_graph_msg_handler, _, logger) = build_graph();
1574+
let (our_privkey, our_id, privkeys, nodes) = get_nodes(&secp_ctx);
1575+
1576+
// A route to node#2 via two paths.
1577+
// One path allows transferring 35-40 sats, another one also allows 35-40 sats.
1578+
// Thus, they can't send 60 without overpaying.
1579+
update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
1580+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
1581+
short_channel_id: 2,
1582+
timestamp: 2,
1583+
flags: 0,
1584+
cltv_expiry_delta: 0,
1585+
htlc_minimum_msat: 35_000,
1586+
htlc_maximum_msat: OptionalField::Present(40_000),
1587+
fee_base_msat: 0,
1588+
fee_proportional_millionths: 0,
1589+
excess_data: Vec::new()
1590+
});
1591+
update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
1592+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
1593+
short_channel_id: 12,
1594+
timestamp: 3,
1595+
flags: 0,
1596+
cltv_expiry_delta: 0,
1597+
htlc_minimum_msat: 35_000,
1598+
htlc_maximum_msat: OptionalField::Present(40_000),
1599+
fee_base_msat: 0,
1600+
fee_proportional_millionths: 0,
1601+
excess_data: Vec::new()
1602+
});
1603+
1604+
// Make 0 fee.
1605+
update_channel(&net_graph_msg_handler, &secp_ctx, &privkeys[7], UnsignedChannelUpdate {
1606+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
1607+
short_channel_id: 13,
1608+
timestamp: 2,
1609+
flags: 0,
1610+
cltv_expiry_delta: 0,
1611+
htlc_minimum_msat: 0,
1612+
htlc_maximum_msat: OptionalField::Absent,
1613+
fee_base_msat: 0,
1614+
fee_proportional_millionths: 0,
1615+
excess_data: Vec::new()
1616+
});
1617+
update_channel(&net_graph_msg_handler, &secp_ctx, &privkeys[1], UnsignedChannelUpdate {
1618+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
1619+
short_channel_id: 4,
1620+
timestamp: 2,
1621+
flags: 0,
1622+
cltv_expiry_delta: 0,
1623+
htlc_minimum_msat: 0,
1624+
htlc_maximum_msat: OptionalField::Absent,
1625+
fee_base_msat: 0,
1626+
fee_proportional_millionths: 0,
1627+
excess_data: Vec::new()
1628+
});
1629+
1630+
// Disable other paths
1631+
update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
1632+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
1633+
short_channel_id: 1,
1634+
timestamp: 3,
1635+
flags: 2, // to disable
1636+
cltv_expiry_delta: 0,
1637+
htlc_minimum_msat: 0,
1638+
htlc_maximum_msat: OptionalField::Absent,
1639+
fee_base_msat: 0,
1640+
fee_proportional_millionths: 0,
1641+
excess_data: Vec::new()
1642+
});
1643+
1644+
let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, &Vec::new(), 60_000, 42, Arc::clone(&logger)).unwrap();
1645+
// Overpay fees to hit htlc_minimum_msat.
1646+
let overpaid_fees = route.paths[0][0].fee_msat + route.paths[1][0].fee_msat;
1647+
// TODO: this could be better balanced to overpay 10k and not 15k.
1648+
assert_eq!(overpaid_fees, 15_000);
1649+
1650+
// Now, test that if there are 2 paths, a "cheaper" by fee path wouldn't be prioritized
1651+
// while taking even more fee to match htlc_minimum_msat.
1652+
update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
1653+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
1654+
short_channel_id: 12,
1655+
timestamp: 4,
1656+
flags: 0,
1657+
cltv_expiry_delta: 0,
1658+
htlc_minimum_msat: 65_000,
1659+
htlc_maximum_msat: OptionalField::Present(80_000),
1660+
fee_base_msat: 0,
1661+
fee_proportional_millionths: 0,
1662+
excess_data: Vec::new()
1663+
});
1664+
update_channel(&net_graph_msg_handler, &secp_ctx, &our_privkey, UnsignedChannelUpdate {
1665+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
1666+
short_channel_id: 2,
1667+
timestamp: 3,
1668+
flags: 0,
1669+
cltv_expiry_delta: 0,
1670+
htlc_minimum_msat: 0,
1671+
htlc_maximum_msat: OptionalField::Absent,
1672+
fee_base_msat: 0,
1673+
fee_proportional_millionths: 0,
1674+
excess_data: Vec::new()
1675+
});
1676+
update_channel(&net_graph_msg_handler, &secp_ctx, &privkeys[1], UnsignedChannelUpdate {
1677+
chain_hash: genesis_block(Network::Testnet).header.block_hash(),
1678+
short_channel_id: 4,
1679+
timestamp: 4,
1680+
flags: 0,
1681+
cltv_expiry_delta: 0,
1682+
htlc_minimum_msat: 0,
1683+
htlc_maximum_msat: OptionalField::Absent,
1684+
fee_base_msat: 0,
1685+
fee_proportional_millionths: 100_000,
1686+
excess_data: Vec::new()
1687+
});
1688+
1689+
let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, &Vec::new(), 60_000, 42, Arc::clone(&logger)).unwrap();
1690+
// Fine to overpay for htlc_minimum_msat if it allows us to save fee.
1691+
assert_eq!(route.paths.len(), 1);
1692+
assert_eq!(route.paths[0][0].short_channel_id, 12);
1693+
let fees = route.paths[0][0].fee_msat;
1694+
assert_eq!(fees, 5_000);
1695+
1696+
let route = get_route(&our_id, &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2], None, &Vec::new(), 50_000, 42, Arc::clone(&logger)).unwrap();
1697+
// Not fine to overpay for htlc_minimum_msat if it requires paying more than fee on
1698+
// the other channel.
1699+
assert_eq!(route.paths.len(), 1);
1700+
assert_eq!(route.paths[0][0].short_channel_id, 2);
1701+
let fees = route.paths[0][0].fee_msat;
1702+
assert_eq!(fees, 5_000);
1703+
}
1704+
15431705
#[test]
15441706
fn disable_channels_test() {
15451707
let (secp_ctx, net_graph_msg_handler, _, logger) = build_graph();

0 commit comments

Comments
 (0)