Skip to content

Commit 54ed39d

Browse files
philipr-zaphilipr-za
committed
Address PR comments
1 parent 0719055 commit 54ed39d

File tree

2 files changed

+76
-115
lines changed

2 files changed

+76
-115
lines changed

‎src/ln/channel.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,9 @@ pub(super) struct Channel {
271271
// is received. holding_cell_update_fee is updated when there are additional
272272
// update_fee() during ChannelState::AwaitingRemoteRevoke.
273273
holding_cell_update_fee: Option<u64>,
274+
#[cfg(test)]
275+
pub next_local_htlc_id: u64,
276+
#[cfg(not(test))]
274277
next_local_htlc_id: u64,
275278
#[cfg(test)]
276279
pub next_remote_htlc_id: u64,

‎src/ln/functional_tests.rs

Lines changed: 73 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -6603,153 +6603,112 @@ fn test_onion_failure() {
66036603
}, ||{}, true, Some(21), None);
66046604
}
66056605

6606-
#[test]
6607-
fn test_update_add_htlc_bolt2_sender() {
6608-
use util::rng;
6609-
use std::sync::atomic::Ordering;
6610-
use super::channelmanager::HTLCSource;
6611-
use super::channel::ChannelError;
6612-
6613-
let secp_ctx = Secp256k1::new();
6614-
6615-
// BOLT 2 Requirements for the Sender when constructing and sending an update_add_htlc message.
6606+
// BOLT 2 Requirements for the Sender when constructing and sending an update_add_htlc message.
6607+
// BOLT 2 Requirement: MUST NOT offer amount_msat it cannot pay for in the remote commitment transaction at the current feerate_per_kw (see "Updating Fees") while maintaining its channel reserve.
6608+
//TODO: I don't believe this is explicitly enforced when sending an HTLC but as the Fee aspect of the BOLT specs is in flux leaving this as a TODO.
66166609

6617-
// BOLT 2 Requirement: MUST NOT offer amount_msat it cannot pay for in the remote commitment transaction at the current feerate_per_kw (see "Updating Fees") while maintaining its channel reserve.
6618-
//TODO: I don't believe this is explicitly enforced when sending an HTLC but as the Fee aspect of the BOLT specs is in flux leaving this as a TODO.
6619-
6620-
// BOLT2 Requirement: MUST offer amount_msat greater than 0.
6621-
// BOLT2 Requirement: MUST NOT offer amount_msat below the receiving node's htlc_minimum_msat (same validation check catches both of these)
6610+
#[test]
6611+
fn test_update_add_htlc_bolt2_sender_value_below_minimum_msat() {
6612+
//BOLT2 Requirement: MUST offer amount_msat greater than 0.
6613+
//BOLT2 Requirement: MUST NOT offer amount_msat below the receiving node's htlc_minimum_msat (same validation check catches both of these)
66226614
let mut nodes = create_network(2);
6623-
let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000);
6624-
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], 100000, TEST_FINAL_CLTV).unwrap();
6615+
let _chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000);
6616+
let mut route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], 100000, TEST_FINAL_CLTV).unwrap();
66256617
let (_, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
66266618

6627-
let session_priv = SecretKey::from_slice(&secp_ctx, &{
6628-
let mut session_key = [0; 32];
6629-
rng::fill_bytes(&mut session_key);
6630-
session_key
6631-
}).expect("RNG is bad!");
6619+
route.hops[0].fee_msat = 0;
66326620

6633-
let cur_height = nodes[0].node.latest_block_height.load(Ordering::Acquire) as u32 + 1;
6634-
let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route, &session_priv).unwrap();
6635-
let (onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height).unwrap();
6636-
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, &our_payment_hash);
6621+
let err = nodes[0].node.send_payment(route, our_payment_hash);
66376622

6638-
let err = nodes[0].node.channel_state.lock().unwrap().by_id.get_mut(&chan.2).unwrap().send_htlc(0, our_payment_hash, TEST_FINAL_CLTV, HTLCSource::OutboundRoute {
6639-
route: route.clone(),
6640-
session_priv: session_priv.clone(),
6641-
first_hop_htlc_msat: 0,
6642-
}, onion_packet);
6643-
6644-
if let Err(ChannelError::Ignore(msg)) = err {
6645-
assert_eq!(msg, "Cannot send less than their minimum HTLC value");
6623+
if let Err(APIError::ChannelUnavailable{err}) = err {
6624+
assert_eq!(err, "Cannot send less than their minimum HTLC value");
66466625
} else {
66476626
assert!(false);
66486627
}
6628+
}
66496629

6630+
#[test]
6631+
fn test_update_add_htlc_bolt2_sender_cltv_expiry_too_high() {
66506632
//BOLT 2 Requirement: MUST set cltv_expiry less than 500000000.
66516633
//TODO: This is not currently explicitly checked when sending an HTLC and exists as TODO in the channel::send_htlc(...) function
66526634
//It is enforced when constructing a route.
6653-
6654-
// BOLT 2 Requirement: if result would be offering more than the remote's max_accepted_htlcs HTLCs, in the remote commitment transaction: MUST NOT add an HTLC.
66556635
let mut nodes = create_network(2);
6656-
let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 0);
6657-
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], 100000, TEST_FINAL_CLTV).unwrap();
6636+
let _chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 0);
6637+
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], 100000000, 500000001).unwrap();
66586638
let (_, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
66596639

6660-
let session_priv = SecretKey::from_slice(&secp_ctx, &{
6661-
let mut session_key = [0; 32];
6662-
rng::fill_bytes(&mut session_key);
6663-
session_key
6664-
}).expect("RNG is bad!");
6640+
let err = nodes[0].node.send_payment(route, our_payment_hash);
66656641

6666-
let cur_height = nodes[0].node.latest_block_height.load(Ordering::Acquire) as u32 + 1;
6667-
let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route, &session_priv).unwrap();
6668-
let (onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height).unwrap();
6669-
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, &our_payment_hash);
6670-
6671-
let max_accepted_htlcs = nodes[1].node.channel_state.lock().unwrap().by_id.get(&chan.2).unwrap().their_max_accepted_htlcs;
6672-
6673-
for _i in 0..max_accepted_htlcs {
6674-
let _ = nodes[0].node.channel_state.lock().unwrap().by_id.get_mut(&chan.2).unwrap().send_htlc(10000, our_payment_hash, TEST_FINAL_CLTV, HTLCSource::OutboundRoute {
6675-
route: route.clone(),
6676-
session_priv: session_priv.clone(),
6677-
first_hop_htlc_msat: 0,
6678-
}, onion_packet.clone());
6679-
}
6680-
6681-
let err = nodes[0].node.channel_state.lock().unwrap().by_id.get_mut(&chan.2).unwrap().send_htlc(10000, our_payment_hash, TEST_FINAL_CLTV, HTLCSource::OutboundRoute {
6682-
route: route.clone(),
6683-
session_priv: session_priv.clone(),
6684-
first_hop_htlc_msat: 0,
6685-
}, onion_packet);
6686-
6687-
if let Err(ChannelError::Ignore(msg)) = err {
6688-
assert_eq!(msg, "Cannot push more than their max accepted HTLCs");
6642+
if let Err(APIError::RouteError{err}) = err {
6643+
assert_eq!(err, "Channel CLTV overflowed?!");
66896644
} else {
66906645
assert!(false);
66916646
}
6647+
}
66926648

6693-
// BOLT 2 Requirement: if the sum of total offered HTLCs would exceed the remote's max_htlc_value_in_flight_msat: MUST NOT add an HTLC.
6694-
let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 0);
6695-
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], 100000, TEST_FINAL_CLTV).unwrap();
6696-
let (_, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
6649+
#[test]
6650+
fn test_update_add_htlc_bolt2_sender_exceed_max_htlc_num_and_htlc_id_increment() {
6651+
//BOLT 2 Requirement: if result would be offering more than the remote's max_accepted_htlcs HTLCs, in the remote commitment transaction: MUST NOT add an HTLC.
6652+
//BOLT 2 Requirement: for the first HTLC it offers MUST set id to 0.
6653+
//BOLT 2 Requirement: MUST increase the value of id by 1 for each successive offer.
6654+
let mut nodes = create_network(2);
6655+
let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 0);
6656+
let max_accepted_htlcs = nodes[1].node.channel_state.lock().unwrap().by_id.get(&chan.2).unwrap().their_max_accepted_htlcs as u64;
66976657

6698-
let session_priv = SecretKey::from_slice(&secp_ctx, &{
6699-
let mut session_key = [0; 32];
6700-
rng::fill_bytes(&mut session_key);
6701-
session_key
6702-
}).expect("RNG is bad!");
6658+
//Confirm the first HTLC ID is zero
6659+
assert_eq!(nodes[0].node.channel_state.lock().unwrap().by_id.get(&chan.2).unwrap().next_local_htlc_id, 0);
67036660

6704-
let cur_height = nodes[0].node.latest_block_height.load(Ordering::Acquire) as u32 + 1;
6705-
let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route, &session_priv).unwrap();
6706-
let (onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height).unwrap();
6707-
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, &our_payment_hash);
6661+
for i in 0..max_accepted_htlcs {
6662+
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], 100000, TEST_FINAL_CLTV).unwrap();
6663+
let (_, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
6664+
let mut payment_event = {
6665+
nodes[0].node.send_payment(route, our_payment_hash).unwrap();
6666+
check_added_monitors!(nodes[0], 1);
67086667

6709-
let err = nodes[0].node.channel_state.lock().unwrap().by_id.get_mut(&chan.2).unwrap().send_htlc(10000001, our_payment_hash, TEST_FINAL_CLTV, HTLCSource::OutboundRoute {
6710-
route: route.clone(),
6711-
session_priv: session_priv.clone(),
6712-
first_hop_htlc_msat: 0,
6713-
}, onion_packet);
6668+
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
6669+
assert_eq!(events.len(), 1);
6670+
SendEvent::from_event(events.remove(0))
6671+
};
6672+
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]).unwrap();
6673+
check_added_monitors!(nodes[1], 0);
6674+
commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false);
67146675

6715-
if let Err(ChannelError::Ignore(msg)) = err {
6716-
assert_eq!(msg, "Cannot send value that would put us over our max HTLC value in flight");
6717-
} else {
6718-
assert!(false);
6719-
}
6676+
expect_pending_htlcs_forwardable!(nodes[1]);
6677+
let _ = nodes[1].node.get_and_clear_pending_events();
67206678

6721-
// BOLT 2 Requirement: if the sum of total offered HTLCs would exceed the remote's max_htlc_value_in_flight_msat: MUST NOT add an HTLC.
6722-
// BOLT 2 Requirement: MUST increase the value of id by 1 for each successive offer.
6723-
let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 0);
6679+
//Confirm the value of the id is increased
6680+
assert_eq!(nodes[0].node.channel_state.lock().unwrap().by_id.get(&chan.2).unwrap().next_local_htlc_id, i+1);
6681+
}
67246682
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], 100000, TEST_FINAL_CLTV).unwrap();
67256683
let (_, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
6684+
let err = nodes[0].node.send_payment(route, our_payment_hash);
67266685

6727-
let session_priv = SecretKey::from_slice(&secp_ctx, &{
6728-
let mut session_key = [0; 32];
6729-
rng::fill_bytes(&mut session_key);
6730-
session_key
6731-
}).expect("RNG is bad!");
6686+
if let Err(APIError::ChannelUnavailable{err}) = err {
6687+
assert_eq!(err, "Cannot push more than their max accepted HTLCs");
6688+
} else {
6689+
assert!(false);
6690+
}
6691+
}
67326692

6733-
let cur_height = nodes[0].node.latest_block_height.load(Ordering::Acquire) as u32 + 1;
6734-
let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route, &session_priv).unwrap();
6735-
let (onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height).unwrap();
6736-
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, &our_payment_hash);
6693+
#[test]
6694+
fn test_update_add_htlc_bolt2_sender_exceed_max_htlc_value_in_flight() {
6695+
//BOLT 2 Requirement: if the sum of total offered HTLCs would exceed the remote's max_htlc_value_in_flight_msat: MUST NOT add an HTLC.
6696+
let mut nodes = create_network(2);
6697+
let _chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 0);
67376698

6738-
for expected_id in 0..2 {
6739-
let res = nodes[0].node.channel_state.lock().unwrap().by_id.get_mut(&chan.2).unwrap().send_htlc(100000, our_payment_hash, TEST_FINAL_CLTV, HTLCSource::OutboundRoute {
6740-
route: route.clone(),
6741-
session_priv: session_priv.clone(),
6742-
first_hop_htlc_msat: 0,
6743-
}, onion_packet.clone());
6699+
let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &[], 100000000, TEST_FINAL_CLTV).unwrap();
6700+
let (_, our_payment_hash) = get_payment_preimage_hash!(nodes[0]);
67446701

6745-
if let Ok(Some(msg)) = res {
6746-
assert_eq!(msg.htlc_id, expected_id);
6747-
} else {
6748-
assert!(false);
6749-
}
6702+
let err = nodes[0].node.send_payment(route, our_payment_hash);
6703+
6704+
if let Err(APIError::ChannelUnavailable{err}) = err {
6705+
assert_eq!(err, "Cannot send value that would put us over our max HTLC value in flight");
6706+
} else {
6707+
assert!(false);
67506708
}
67516709
}
67526710

6711+
// BOLT 2 Requirements for the Receiver when handling an update_add_htlc message.
67536712
#[test]
67546713
fn test_update_add_htlc_bolt2_receiver_check_amount_received_more_than_min() {
67556714
use super::msgs::HandleError;
@@ -6930,8 +6889,6 @@ fn test_update_add_htlc_bolt2_receiver_check_cltv_expiry() {
69306889

69316890
#[test]
69326891
fn test_update_add_htlc_bolt2_receiver_check_repeated_id_ignore() {
6933-
use super::msgs::HandleError;
6934-
69356892
//BOLT 2 requirement: if the sender did not previously acknowledge the commitment of that HTLC: MUST ignore a repeated id value after a reconnection.
69366893
let mut nodes = create_network(2);
69376894
let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 95000000);
@@ -6962,4 +6919,5 @@ fn test_update_add_htlc_bolt2_receiver_check_repeated_id_ignore() {
69626919

69636920
//Clear unhandled msg events
69646921
let _ = nodes[1].node.get_and_clear_pending_msg_events();
6965-
}
6922+
}
6923+

0 commit comments

Comments
 (0)