Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one#670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ariard wants to merge 3 commits into from
Closed

Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one #670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8b94722
[channelmanager] Move htlc_minimum_msat policy check on outgoing channel
Aug 19, 2020
1cf928e
[channel] Remove get_their_htlc_minimum_msat
Aug 22, 2020
0e6d0b0
[channelmanager] Partially document usage/encoding of onion errors
Aug 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -3158,11 +3158,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
);
}

/// Allowed in any state (including after shutdown)
pub fn get_their_htlc_minimum_msat(&self) -> u64 {
self.our_htlc_minimum_msat
}

pub fn get_value_satoshis(&self) -> u64 {
self.channel_value_satoshis
}
Expand Down
15 changes: 11 additions & 4 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1158,9 +1158,6 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
if !chan.is_live() { // channel_disabled
break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap())));
}
if *amt_to_forward < chan.get_their_htlc_minimum_msat() { // amount_below_minimum

@TheBlueMattTheBlueMattAug 23, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.

@ariardariardAug 23, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay you're right, actual code is correct, that just a badly-named method.

But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :

  • performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
  • correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.

I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.

@TheBlueMattTheBlueMattAug 23, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.

In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.

As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.

break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap())));
}
let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_our_fee_base_msat(&self.fee_estimator) as u64) });
if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
Expand DownExpand Up@@ -1188,7 +1185,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
{
let mut res = Vec::with_capacity(8 + 128);
if let Some(chan_update) = chan_update {
if code == 0x1000 | 11 || code == 0x1000 | 12 {
if code == 0x1000 | 12 { // fee_insufficient
res.extend_from_slice(&byte_utils::be64_to_array(msg.amount_msat));
}
else if code == 0x1000 | 13 {
Expand DownExpand Up@@ -1587,6 +1584,16 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
htlc_id: prev_htlc_id,
incoming_packet_shared_secret: incoming_shared_secret,
});
if amt_to_forward < chan.get().get_our_htlc_minimum_msat() {
let mut data = Vec::with_capacity(8 + 128); // 8-bytes-htlc_msat + 2-byte-length + length-byte-channel_update
data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
let chan_update = self.get_channel_update(chan.get()).unwrap();
data.extend_from_slice(&chan_update.encode_with_len()[..]);
failed_forwards.push((htlc_source, payment_hash,
HTLCFailReason::Reason { failure_code: 0x1000 | 11, data } // amount_below_minimum
));
continue;
}
match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) {
Err(e) => {
if let ChannelError::Ignore(msg) = e {
Expand Down
30 changes: 26 additions & 4 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -5909,10 +5909,11 @@ fn run_onion_failure_test<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>,
}

// test_case
// 0: node1 fails backward
// 0: final node fails backward
// 1: final node fails backward
// 2: payment completed but the user rejects the payment
// 3: final node fails backward (but tamper onion payloads from node0)
// 4: intermediate node failure, fails backward
// 100: trigger error in the intermediate node and tamper returning fail_htlc
// 200: trigger error in the final node and tamper returning fail_htlc
fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case: u8, nodes: &Vec<Node>, route: &Route, payment_hash: &PaymentHash, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option<u16>, expected_channel_update: Option<HTLCFailChannelUpdate>)
Expand DownExpand Up@@ -6013,13 +6014,25 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
assert!(update_1_0.update_fail_htlcs.len() == 1);
update_1_0
},
4 => { // intermediate node failure; failing backward to start node
assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
// forwarding on 1
expect_htlc_forward!(&nodes[1]);

// backward fail on 1
expect_htlc_forward!(&nodes[1]);
check_added_monitors!(nodes[1], 1);
let update_1_0 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
assert!(update_1_0.update_fail_htlcs.len() == 1);
update_1_0
},
_ => unreachable!(),
};

// 1 => 0 commitment_signed_dance
if update_1_0.update_fail_htlcs.len() > 0 {
let mut fail_msg = update_1_0.update_fail_htlcs[0].clone();
if test_case == 100 {
if test_case == 100 || test_case == 4 {
callback_fail(&mut fail_msg);
}
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
Expand DownExpand Up@@ -6269,11 +6282,20 @@ fn test_onion_failure() {
run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(PERM|10),
Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true}));

let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_their_htlc_minimum_msat() - 1;
let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_our_htlc_minimum_msat() - 1;
let mut bogus_route = route.clone();
let route_len = bogus_route.paths[0].len();
bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward;
run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
run_onion_failure_test_with_fail_intercept("amount_below_minimum", 4, &nodes, &bogus_route, &payment_hash, |_| {}, |msg| {
let session_priv = SecretKey::from_slice(&[3; 32]).unwrap();
let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
let mut data = Vec::with_capacity(8 + 128);
data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
let mut chan_update = ChannelUpdate::dummy();
chan_update.contents.htlc_minimum_msat = amt_to_forward + 1;
data.extend_from_slice(&chan_update.encode_with_len()[..]);
msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], UPDATE|11, &data);
}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));

//TODO: with new config API, we will be able to generate both valid and
//invalid channel_update cases.
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one#670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

ariard
wants to merge
3
commits into

from

 Closed


Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one
#670
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8b94722

[channelmanager] Move htlc_minimum_msat policy check on outgoing channel

Aug 19, 2020
1cf928e

[channel] Remove get_their_htlc_minimum_msat

Aug 22, 2020
0e6d0b0

[channelmanager] Partially document usage/encoding of onion errors

Aug 22, 2020

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


5 changes: 0 additions & 5 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3158,11 +3158,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 );
	}

	/// Allowed in any state (including after shutdown)
	pub fn get_their_htlc_minimum_msat(&self) -> u64 {
 self.our_htlc_minimum_msat
	}

	pub fn get_value_satoshis(&self) -> u64 {
 self.channel_value_satoshis
	}
Expand Down

15 changes: 11 additions & 4 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1158,9 +1158,6 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 if !chan.is_live() { // channel_disabled
 break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap())));
 }
 if *amt_to_forward < chan.get_their_htlc_minimum_msat() { // amount_below_minimum
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.
@ariardariardAug 23, 2020

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay you're right, actual code is correct, that just a badly-named method.
But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :
  • performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
  • correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.
I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.
In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.
As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.
 break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap())));
 }
 let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_our_fee_base_msat(&self.fee_estimator) as u64) });
 if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
 break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
Expand DownExpand Up@@ -1188,7 +1185,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 {
 let mut res = Vec::with_capacity(8 + 128);
 if let Some(chan_update) = chan_update {
 if code == 0x1000 | 11 || code == 0x1000 | 12 {
 if code == 0x1000 | 12 { // fee_insufficient
 res.extend_from_slice(&byte_utils::be64_to_array(msg.amount_msat));
 }
 else if code == 0x1000 | 13 {
Expand DownExpand Up@@ -1587,6 +1584,16 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 htlc_id: prev_htlc_id,
 incoming_packet_shared_secret: incoming_shared_secret,
 });
 if amt_to_forward < chan.get().get_our_htlc_minimum_msat() {
 let mut data = Vec::with_capacity(8 + 128); // 8-bytes-htlc_msat + 2-byte-length + length-byte-channel_update
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let chan_update = self.get_channel_update(chan.get()).unwrap();
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 failed_forwards.push((htlc_source, payment_hash,
 HTLCFailReason::Reason { failure_code: 0x1000 | 11, data } // amount_below_minimum
 ));
 continue;
 }
 match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) {
 Err(e) => {
 if let ChannelError::Ignore(msg) = e {
Expand Down

30 changes: 26 additions & 4 deletions
lightning/src/ln/functional_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5909,10 +5909,11 @@ fn run_onion_failure_test<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>,
}

// test_case
// 0: node1 fails backward
// 0: final node fails backward
// 1: final node fails backward
// 2: payment completed but the user rejects the payment
// 3: final node fails backward (but tamper onion payloads from node0)
// 4: intermediate node failure, fails backward
// 100: trigger error in the intermediate node and tamper returning fail_htlc
// 200: trigger error in the final node and tamper returning fail_htlc
fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case: u8, nodes: &Vec<Node>, route: &Route, payment_hash: &PaymentHash, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option<u16>, expected_channel_update: Option<HTLCFailChannelUpdate>)
Expand DownExpand Up@@ -6013,13 +6014,25 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 4 => { // intermediate node failure; failing backward to start node
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 // forwarding on 1
 expect_htlc_forward!(&nodes[1]);

 // backward fail on 1
 expect_htlc_forward!(&nodes[1]);
 check_added_monitors!(nodes[1], 1);
 let update_1_0 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 _ => unreachable!(),
	};

	// 1 => 0 commitment_signed_dance
	if update_1_0.update_fail_htlcs.len() > 0 {
 let mut fail_msg = update_1_0.update_fail_htlcs[0].clone();
 if test_case == 100 {
 if test_case == 100 || test_case == 4 {
 callback_fail(&mut fail_msg);
 }
 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
Expand DownExpand Up@@ -6269,11 +6282,20 @@ fn test_onion_failure() {
	run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(PERM|10),
 Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true}));

	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_their_htlc_minimum_msat() - 1;
	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_our_htlc_minimum_msat() - 1;
	let mut bogus_route = route.clone();
	let route_len = bogus_route.paths[0].len();
	bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward;
	run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
	run_onion_failure_test_with_fail_intercept("amount_below_minimum", 4, &nodes, &bogus_route, &payment_hash, |_| {}, |msg| {
 let session_priv = SecretKey::from_slice(&[3; 32]).unwrap();
 let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
 let mut data = Vec::with_capacity(8 + 128);
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let mut chan_update = ChannelUpdate::dummy();
 chan_update.contents.htlc_minimum_msat = amt_to_forward + 1;
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], UPDATE|11, &data);
	}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));

	//TODO: with new config API, we will be able to generate both valid and
	//invalid channel_update cases.
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one#670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

ariard
wants to merge
3
commits into

from

 Closed


Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one
#670
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8b94722

[channelmanager] Move htlc_minimum_msat policy check on outgoing channel

Aug 19, 2020
1cf928e

[channel] Remove get_their_htlc_minimum_msat

Aug 22, 2020
0e6d0b0

[channelmanager] Partially document usage/encoding of onion errors

Aug 22, 2020

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


5 changes: 0 additions & 5 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3158,11 +3158,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 );
	}

	/// Allowed in any state (including after shutdown)
	pub fn get_their_htlc_minimum_msat(&self) -> u64 {
 self.our_htlc_minimum_msat
	}

	pub fn get_value_satoshis(&self) -> u64 {
 self.channel_value_satoshis
	}
Expand Down

15 changes: 11 additions & 4 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1158,9 +1158,6 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 if !chan.is_live() { // channel_disabled
 break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap())));
 }
 if *amt_to_forward < chan.get_their_htlc_minimum_msat() { // amount_below_minimum
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.
@ariardariardAug 23, 2020

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay you're right, actual code is correct, that just a badly-named method.
But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :
  • performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
  • correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.
I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.
In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.
As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.
 break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap())));
 }
 let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_our_fee_base_msat(&self.fee_estimator) as u64) });
 if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
 break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
Expand DownExpand Up@@ -1188,7 +1185,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 {
 let mut res = Vec::with_capacity(8 + 128);
 if let Some(chan_update) = chan_update {
 if code == 0x1000 | 11 || code == 0x1000 | 12 {
 if code == 0x1000 | 12 { // fee_insufficient
 res.extend_from_slice(&byte_utils::be64_to_array(msg.amount_msat));
 }
 else if code == 0x1000 | 13 {
Expand DownExpand Up@@ -1587,6 +1584,16 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 htlc_id: prev_htlc_id,
 incoming_packet_shared_secret: incoming_shared_secret,
 });
 if amt_to_forward < chan.get().get_our_htlc_minimum_msat() {
 let mut data = Vec::with_capacity(8 + 128); // 8-bytes-htlc_msat + 2-byte-length + length-byte-channel_update
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let chan_update = self.get_channel_update(chan.get()).unwrap();
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 failed_forwards.push((htlc_source, payment_hash,
 HTLCFailReason::Reason { failure_code: 0x1000 | 11, data } // amount_below_minimum
 ));
 continue;
 }
 match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) {
 Err(e) => {
 if let ChannelError::Ignore(msg) = e {
Expand Down

30 changes: 26 additions & 4 deletions
lightning/src/ln/functional_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5909,10 +5909,11 @@ fn run_onion_failure_test<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>,
}

// test_case
// 0: node1 fails backward
// 0: final node fails backward
// 1: final node fails backward
// 2: payment completed but the user rejects the payment
// 3: final node fails backward (but tamper onion payloads from node0)
// 4: intermediate node failure, fails backward
// 100: trigger error in the intermediate node and tamper returning fail_htlc
// 200: trigger error in the final node and tamper returning fail_htlc
fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case: u8, nodes: &Vec<Node>, route: &Route, payment_hash: &PaymentHash, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option<u16>, expected_channel_update: Option<HTLCFailChannelUpdate>)
Expand DownExpand Up@@ -6013,13 +6014,25 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 4 => { // intermediate node failure; failing backward to start node
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 // forwarding on 1
 expect_htlc_forward!(&nodes[1]);

 // backward fail on 1
 expect_htlc_forward!(&nodes[1]);
 check_added_monitors!(nodes[1], 1);
 let update_1_0 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 _ => unreachable!(),
	};

	// 1 => 0 commitment_signed_dance
	if update_1_0.update_fail_htlcs.len() > 0 {
 let mut fail_msg = update_1_0.update_fail_htlcs[0].clone();
 if test_case == 100 {
 if test_case == 100 || test_case == 4 {
 callback_fail(&mut fail_msg);
 }
 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
Expand DownExpand Up@@ -6269,11 +6282,20 @@ fn test_onion_failure() {
	run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(PERM|10),
 Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true}));

	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_their_htlc_minimum_msat() - 1;
	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_our_htlc_minimum_msat() - 1;
	let mut bogus_route = route.clone();
	let route_len = bogus_route.paths[0].len();
	bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward;
	run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
	run_onion_failure_test_with_fail_intercept("amount_below_minimum", 4, &nodes, &bogus_route, &payment_hash, |_| {}, |msg| {
 let session_priv = SecretKey::from_slice(&[3; 32]).unwrap();
 let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
 let mut data = Vec::with_capacity(8 + 128);
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let mut chan_update = ChannelUpdate::dummy();
 chan_update.contents.htlc_minimum_msat = amt_to_forward + 1;
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], UPDATE|11, &data);
	}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));

	//TODO: with new config API, we will be able to generate both valid and
	//invalid channel_update cases.
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one#670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

ariard
wants to merge
3
commits into

from

 Closed


Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one
#670
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8b94722

[channelmanager] Move htlc_minimum_msat policy check on outgoing channel

Aug 19, 2020
1cf928e

[channel] Remove get_their_htlc_minimum_msat

Aug 22, 2020
0e6d0b0

[channelmanager] Partially document usage/encoding of onion errors

Aug 22, 2020

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


5 changes: 0 additions & 5 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3158,11 +3158,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 );
	}

	/// Allowed in any state (including after shutdown)
	pub fn get_their_htlc_minimum_msat(&self) -> u64 {
 self.our_htlc_minimum_msat
	}

	pub fn get_value_satoshis(&self) -> u64 {
 self.channel_value_satoshis
	}
Expand Down

15 changes: 11 additions & 4 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1158,9 +1158,6 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 if !chan.is_live() { // channel_disabled
 break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap())));
 }
 if *amt_to_forward < chan.get_their_htlc_minimum_msat() { // amount_below_minimum
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.
@ariardariardAug 23, 2020

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay you're right, actual code is correct, that just a badly-named method.
But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :
  • performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
  • correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.
I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.
In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.
As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.
 break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap())));
 }
 let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_our_fee_base_msat(&self.fee_estimator) as u64) });
 if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
 break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
Expand DownExpand Up@@ -1188,7 +1185,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 {
 let mut res = Vec::with_capacity(8 + 128);
 if let Some(chan_update) = chan_update {
 if code == 0x1000 | 11 || code == 0x1000 | 12 {
 if code == 0x1000 | 12 { // fee_insufficient
 res.extend_from_slice(&byte_utils::be64_to_array(msg.amount_msat));
 }
 else if code == 0x1000 | 13 {
Expand DownExpand Up@@ -1587,6 +1584,16 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 htlc_id: prev_htlc_id,
 incoming_packet_shared_secret: incoming_shared_secret,
 });
 if amt_to_forward < chan.get().get_our_htlc_minimum_msat() {
 let mut data = Vec::with_capacity(8 + 128); // 8-bytes-htlc_msat + 2-byte-length + length-byte-channel_update
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let chan_update = self.get_channel_update(chan.get()).unwrap();
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 failed_forwards.push((htlc_source, payment_hash,
 HTLCFailReason::Reason { failure_code: 0x1000 | 11, data } // amount_below_minimum
 ));
 continue;
 }
 match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) {
 Err(e) => {
 if let ChannelError::Ignore(msg) = e {
Expand Down

30 changes: 26 additions & 4 deletions
lightning/src/ln/functional_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5909,10 +5909,11 @@ fn run_onion_failure_test<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>,
}

// test_case
// 0: node1 fails backward
// 0: final node fails backward
// 1: final node fails backward
// 2: payment completed but the user rejects the payment
// 3: final node fails backward (but tamper onion payloads from node0)
// 4: intermediate node failure, fails backward
// 100: trigger error in the intermediate node and tamper returning fail_htlc
// 200: trigger error in the final node and tamper returning fail_htlc
fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case: u8, nodes: &Vec<Node>, route: &Route, payment_hash: &PaymentHash, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option<u16>, expected_channel_update: Option<HTLCFailChannelUpdate>)
Expand DownExpand Up@@ -6013,13 +6014,25 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 4 => { // intermediate node failure; failing backward to start node
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 // forwarding on 1
 expect_htlc_forward!(&nodes[1]);

 // backward fail on 1
 expect_htlc_forward!(&nodes[1]);
 check_added_monitors!(nodes[1], 1);
 let update_1_0 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 _ => unreachable!(),
	};

	// 1 => 0 commitment_signed_dance
	if update_1_0.update_fail_htlcs.len() > 0 {
 let mut fail_msg = update_1_0.update_fail_htlcs[0].clone();
 if test_case == 100 {
 if test_case == 100 || test_case == 4 {
 callback_fail(&mut fail_msg);
 }
 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
Expand DownExpand Up@@ -6269,11 +6282,20 @@ fn test_onion_failure() {
	run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(PERM|10),
 Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true}));

	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_their_htlc_minimum_msat() - 1;
	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_our_htlc_minimum_msat() - 1;
	let mut bogus_route = route.clone();
	let route_len = bogus_route.paths[0].len();
	bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward;
	run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
	run_onion_failure_test_with_fail_intercept("amount_below_minimum", 4, &nodes, &bogus_route, &payment_hash, |_| {}, |msg| {
 let session_priv = SecretKey::from_slice(&[3; 32]).unwrap();
 let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
 let mut data = Vec::with_capacity(8 + 128);
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let mut chan_update = ChannelUpdate::dummy();
 chan_update.contents.htlc_minimum_msat = amt_to_forward + 1;
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], UPDATE|11, &data);
	}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));

	//TODO: with new config API, we will be able to generate both valid and
	//invalid channel_update cases.
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one#670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

ariard
wants to merge
3
commits into

from

 Closed


Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one
#670
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8b94722

[channelmanager] Move htlc_minimum_msat policy check on outgoing channel

Aug 19, 2020
1cf928e

[channel] Remove get_their_htlc_minimum_msat

Aug 22, 2020
0e6d0b0

[channelmanager] Partially document usage/encoding of onion errors

Aug 22, 2020

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


5 changes: 0 additions & 5 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3158,11 +3158,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 );
	}

	/// Allowed in any state (including after shutdown)
	pub fn get_their_htlc_minimum_msat(&self) -> u64 {
 self.our_htlc_minimum_msat
	}

	pub fn get_value_satoshis(&self) -> u64 {
 self.channel_value_satoshis
	}
Expand Down

15 changes: 11 additions & 4 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1158,9 +1158,6 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 if !chan.is_live() { // channel_disabled
 break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap())));
 }
 if *amt_to_forward < chan.get_their_htlc_minimum_msat() { // amount_below_minimum
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.
@ariardariardAug 23, 2020

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay you're right, actual code is correct, that just a badly-named method.
But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :
  • performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
  • correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.
I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.
In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.
As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.
 break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap())));
 }
 let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_our_fee_base_msat(&self.fee_estimator) as u64) });
 if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
 break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
Expand DownExpand Up@@ -1188,7 +1185,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 {
 let mut res = Vec::with_capacity(8 + 128);
 if let Some(chan_update) = chan_update {
 if code == 0x1000 | 11 || code == 0x1000 | 12 {
 if code == 0x1000 | 12 { // fee_insufficient
 res.extend_from_slice(&byte_utils::be64_to_array(msg.amount_msat));
 }
 else if code == 0x1000 | 13 {
Expand DownExpand Up@@ -1587,6 +1584,16 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 htlc_id: prev_htlc_id,
 incoming_packet_shared_secret: incoming_shared_secret,
 });
 if amt_to_forward < chan.get().get_our_htlc_minimum_msat() {
 let mut data = Vec::with_capacity(8 + 128); // 8-bytes-htlc_msat + 2-byte-length + length-byte-channel_update
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let chan_update = self.get_channel_update(chan.get()).unwrap();
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 failed_forwards.push((htlc_source, payment_hash,
 HTLCFailReason::Reason { failure_code: 0x1000 | 11, data } // amount_below_minimum
 ));
 continue;
 }
 match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) {
 Err(e) => {
 if let ChannelError::Ignore(msg) = e {
Expand Down

30 changes: 26 additions & 4 deletions
lightning/src/ln/functional_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5909,10 +5909,11 @@ fn run_onion_failure_test<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>,
}

// test_case
// 0: node1 fails backward
// 0: final node fails backward
// 1: final node fails backward
// 2: payment completed but the user rejects the payment
// 3: final node fails backward (but tamper onion payloads from node0)
// 4: intermediate node failure, fails backward
// 100: trigger error in the intermediate node and tamper returning fail_htlc
// 200: trigger error in the final node and tamper returning fail_htlc
fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case: u8, nodes: &Vec<Node>, route: &Route, payment_hash: &PaymentHash, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option<u16>, expected_channel_update: Option<HTLCFailChannelUpdate>)
Expand DownExpand Up@@ -6013,13 +6014,25 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 4 => { // intermediate node failure; failing backward to start node
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 // forwarding on 1
 expect_htlc_forward!(&nodes[1]);

 // backward fail on 1
 expect_htlc_forward!(&nodes[1]);
 check_added_monitors!(nodes[1], 1);
 let update_1_0 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 _ => unreachable!(),
	};

	// 1 => 0 commitment_signed_dance
	if update_1_0.update_fail_htlcs.len() > 0 {
 let mut fail_msg = update_1_0.update_fail_htlcs[0].clone();
 if test_case == 100 {
 if test_case == 100 || test_case == 4 {
 callback_fail(&mut fail_msg);
 }
 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
Expand DownExpand Up@@ -6269,11 +6282,20 @@ fn test_onion_failure() {
	run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(PERM|10),
 Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true}));

	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_their_htlc_minimum_msat() - 1;
	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_our_htlc_minimum_msat() - 1;
	let mut bogus_route = route.clone();
	let route_len = bogus_route.paths[0].len();
	bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward;
	run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
	run_onion_failure_test_with_fail_intercept("amount_below_minimum", 4, &nodes, &bogus_route, &payment_hash, |_| {}, |msg| {
 let session_priv = SecretKey::from_slice(&[3; 32]).unwrap();
 let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
 let mut data = Vec::with_capacity(8 + 128);
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let mut chan_update = ChannelUpdate::dummy();
 chan_update.contents.htlc_minimum_msat = amt_to_forward + 1;
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], UPDATE|11, &data);
	}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));

	//TODO: with new config API, we will be able to generate both valid and
	//invalid channel_update cases.
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one#670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

ariard
wants to merge
3
commits into

from

 Closed


Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one
#670
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8b94722

[channelmanager] Move htlc_minimum_msat policy check on outgoing channel

Aug 19, 2020
1cf928e

[channel] Remove get_their_htlc_minimum_msat

Aug 22, 2020
0e6d0b0

[channelmanager] Partially document usage/encoding of onion errors

Aug 22, 2020

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


5 changes: 0 additions & 5 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3158,11 +3158,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 );
	}

	/// Allowed in any state (including after shutdown)
	pub fn get_their_htlc_minimum_msat(&self) -> u64 {
 self.our_htlc_minimum_msat
	}

	pub fn get_value_satoshis(&self) -> u64 {
 self.channel_value_satoshis
	}
Expand Down

15 changes: 11 additions & 4 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1158,9 +1158,6 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 if !chan.is_live() { // channel_disabled
 break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap())));
 }
 if *amt_to_forward < chan.get_their_htlc_minimum_msat() { // amount_below_minimum
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.
@ariardariardAug 23, 2020

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay you're right, actual code is correct, that just a badly-named method.
But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :
  • performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
  • correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.
I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.
In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.
As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.
 break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap())));
 }
 let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_our_fee_base_msat(&self.fee_estimator) as u64) });
 if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
 break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
Expand DownExpand Up@@ -1188,7 +1185,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 {
 let mut res = Vec::with_capacity(8 + 128);
 if let Some(chan_update) = chan_update {
 if code == 0x1000 | 11 || code == 0x1000 | 12 {
 if code == 0x1000 | 12 { // fee_insufficient
 res.extend_from_slice(&byte_utils::be64_to_array(msg.amount_msat));
 }
 else if code == 0x1000 | 13 {
Expand DownExpand Up@@ -1587,6 +1584,16 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 htlc_id: prev_htlc_id,
 incoming_packet_shared_secret: incoming_shared_secret,
 });
 if amt_to_forward < chan.get().get_our_htlc_minimum_msat() {
 let mut data = Vec::with_capacity(8 + 128); // 8-bytes-htlc_msat + 2-byte-length + length-byte-channel_update
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let chan_update = self.get_channel_update(chan.get()).unwrap();
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 failed_forwards.push((htlc_source, payment_hash,
 HTLCFailReason::Reason { failure_code: 0x1000 | 11, data } // amount_below_minimum
 ));
 continue;
 }
 match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) {
 Err(e) => {
 if let ChannelError::Ignore(msg) = e {
Expand Down

30 changes: 26 additions & 4 deletions
lightning/src/ln/functional_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5909,10 +5909,11 @@ fn run_onion_failure_test<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>,
}

// test_case
// 0: node1 fails backward
// 0: final node fails backward
// 1: final node fails backward
// 2: payment completed but the user rejects the payment
// 3: final node fails backward (but tamper onion payloads from node0)
// 4: intermediate node failure, fails backward
// 100: trigger error in the intermediate node and tamper returning fail_htlc
// 200: trigger error in the final node and tamper returning fail_htlc
fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case: u8, nodes: &Vec<Node>, route: &Route, payment_hash: &PaymentHash, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option<u16>, expected_channel_update: Option<HTLCFailChannelUpdate>)
Expand DownExpand Up@@ -6013,13 +6014,25 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 4 => { // intermediate node failure; failing backward to start node
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 // forwarding on 1
 expect_htlc_forward!(&nodes[1]);

 // backward fail on 1
 expect_htlc_forward!(&nodes[1]);
 check_added_monitors!(nodes[1], 1);
 let update_1_0 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 _ => unreachable!(),
	};

	// 1 => 0 commitment_signed_dance
	if update_1_0.update_fail_htlcs.len() > 0 {
 let mut fail_msg = update_1_0.update_fail_htlcs[0].clone();
 if test_case == 100 {
 if test_case == 100 || test_case == 4 {
 callback_fail(&mut fail_msg);
 }
 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
Expand DownExpand Up@@ -6269,11 +6282,20 @@ fn test_onion_failure() {
	run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(PERM|10),
 Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true}));

	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_their_htlc_minimum_msat() - 1;
	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_our_htlc_minimum_msat() - 1;
	let mut bogus_route = route.clone();
	let route_len = bogus_route.paths[0].len();
	bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward;
	run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
	run_onion_failure_test_with_fail_intercept("amount_below_minimum", 4, &nodes, &bogus_route, &payment_hash, |_| {}, |msg| {
 let session_priv = SecretKey::from_slice(&[3; 32]).unwrap();
 let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
 let mut data = Vec::with_capacity(8 + 128);
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let mut chan_update = ChannelUpdate::dummy();
 chan_update.contents.htlc_minimum_msat = amt_to_forward + 1;
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], UPDATE|11, &data);
	}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));

	//TODO: with new config API, we will be able to generate both valid and
	//invalid channel_update cases.
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one#670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

ariard
wants to merge
3
commits into

from

 Closed


Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one
#670
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8b94722

[channelmanager] Move htlc_minimum_msat policy check on outgoing channel

Aug 19, 2020
1cf928e

[channel] Remove get_their_htlc_minimum_msat

Aug 22, 2020
0e6d0b0

[channelmanager] Partially document usage/encoding of onion errors

Aug 22, 2020

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


5 changes: 0 additions & 5 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3158,11 +3158,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 );
	}

	/// Allowed in any state (including after shutdown)
	pub fn get_their_htlc_minimum_msat(&self) -> u64 {
 self.our_htlc_minimum_msat
	}

	pub fn get_value_satoshis(&self) -> u64 {
 self.channel_value_satoshis
	}
Expand Down

15 changes: 11 additions & 4 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1158,9 +1158,6 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 if !chan.is_live() { // channel_disabled
 break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap())));
 }
 if *amt_to_forward < chan.get_their_htlc_minimum_msat() { // amount_below_minimum
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.
@ariardariardAug 23, 2020

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay you're right, actual code is correct, that just a badly-named method.
But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :
  • performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
  • correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.
I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.
In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.
As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.
 break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap())));
 }
 let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_our_fee_base_msat(&self.fee_estimator) as u64) });
 if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
 break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
Expand DownExpand Up@@ -1188,7 +1185,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 {
 let mut res = Vec::with_capacity(8 + 128);
 if let Some(chan_update) = chan_update {
 if code == 0x1000 | 11 || code == 0x1000 | 12 {
 if code == 0x1000 | 12 { // fee_insufficient
 res.extend_from_slice(&byte_utils::be64_to_array(msg.amount_msat));
 }
 else if code == 0x1000 | 13 {
Expand DownExpand Up@@ -1587,6 +1584,16 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 htlc_id: prev_htlc_id,
 incoming_packet_shared_secret: incoming_shared_secret,
 });
 if amt_to_forward < chan.get().get_our_htlc_minimum_msat() {
 let mut data = Vec::with_capacity(8 + 128); // 8-bytes-htlc_msat + 2-byte-length + length-byte-channel_update
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let chan_update = self.get_channel_update(chan.get()).unwrap();
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 failed_forwards.push((htlc_source, payment_hash,
 HTLCFailReason::Reason { failure_code: 0x1000 | 11, data } // amount_below_minimum
 ));
 continue;
 }
 match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) {
 Err(e) => {
 if let ChannelError::Ignore(msg) = e {
Expand Down

30 changes: 26 additions & 4 deletions
lightning/src/ln/functional_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5909,10 +5909,11 @@ fn run_onion_failure_test<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>,
}

// test_case
// 0: node1 fails backward
// 0: final node fails backward
// 1: final node fails backward
// 2: payment completed but the user rejects the payment
// 3: final node fails backward (but tamper onion payloads from node0)
// 4: intermediate node failure, fails backward
// 100: trigger error in the intermediate node and tamper returning fail_htlc
// 200: trigger error in the final node and tamper returning fail_htlc
fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case: u8, nodes: &Vec<Node>, route: &Route, payment_hash: &PaymentHash, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option<u16>, expected_channel_update: Option<HTLCFailChannelUpdate>)
Expand DownExpand Up@@ -6013,13 +6014,25 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 4 => { // intermediate node failure; failing backward to start node
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 // forwarding on 1
 expect_htlc_forward!(&nodes[1]);

 // backward fail on 1
 expect_htlc_forward!(&nodes[1]);
 check_added_monitors!(nodes[1], 1);
 let update_1_0 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 _ => unreachable!(),
	};

	// 1 => 0 commitment_signed_dance
	if update_1_0.update_fail_htlcs.len() > 0 {
 let mut fail_msg = update_1_0.update_fail_htlcs[0].clone();
 if test_case == 100 {
 if test_case == 100 || test_case == 4 {
 callback_fail(&mut fail_msg);
 }
 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
Expand DownExpand Up@@ -6269,11 +6282,20 @@ fn test_onion_failure() {
	run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(PERM|10),
 Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true}));

	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_their_htlc_minimum_msat() - 1;
	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_our_htlc_minimum_msat() - 1;
	let mut bogus_route = route.clone();
	let route_len = bogus_route.paths[0].len();
	bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward;
	run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
	run_onion_failure_test_with_fail_intercept("amount_below_minimum", 4, &nodes, &bogus_route, &payment_hash, |_| {}, |msg| {
 let session_priv = SecretKey::from_slice(&[3; 32]).unwrap();
 let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
 let mut data = Vec::with_capacity(8 + 128);
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let mut chan_update = ChannelUpdate::dummy();
 chan_update.contents.htlc_minimum_msat = amt_to_forward + 1;
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], UPDATE|11, &data);
	}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));

	//TODO: with new config API, we will be able to generate both valid and
	//invalid channel_update cases.
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one#670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Closed

ariard
wants to merge
3
commits into

from

 Closed


Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one
#670
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


3 commits


Select commit
Hold shift + click to select a range
8b94722

[channelmanager] Move htlc_minimum_msat policy check on outgoing channel

Aug 19, 2020
1cf928e

[channel] Remove get_their_htlc_minimum_msat

Aug 22, 2020
0e6d0b0

[channelmanager] Partially document usage/encoding of onion errors

Aug 22, 2020

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


5 changes: 0 additions & 5 deletions
lightning/src/ln/channel.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -3158,11 +3158,6 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 );
	}

	/// Allowed in any state (including after shutdown)
	pub fn get_their_htlc_minimum_msat(&self) -> u64 {
 self.our_htlc_minimum_msat
	}

	pub fn get_value_satoshis(&self) -> u64 {
 self.channel_value_satoshis
	}
Expand Down

15 changes: 11 additions & 4 deletions
lightning/src/ln/channelmanager.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -1158,9 +1158,6 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 if !chan.is_live() { // channel_disabled
 break Some(("Forwarding channel is not in a ready state.", 0x1000 | 20, Some(self.get_channel_update(chan).unwrap())));
 }
 if *amt_to_forward < chan.get_their_htlc_minimum_msat() { // amount_below_minimum
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.
@ariardariardAug 23, 2020

Copy link


Copy Markdown

Author
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay you're right, actual code is correct, that just a badly-named method.
But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :
  • performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
  • correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.
I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.
@TheBlueMattTheBlueMattAug 23, 2020

Copy link


Copy Markdown

Collaborator
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.
In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.
As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.
 break Some(("HTLC amount was below the htlc_minimum_msat", 0x1000 | 11, Some(self.get_channel_update(chan).unwrap())));
 }
 let fee = amt_to_forward.checked_mul(chan.get_fee_proportional_millionths() as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan.get_our_fee_base_msat(&self.fee_estimator) as u64) });
 if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
 break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
Expand DownExpand Up@@ -1188,7 +1185,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 {
 let mut res = Vec::with_capacity(8 + 128);
 if let Some(chan_update) = chan_update {
 if code == 0x1000 | 11 || code == 0x1000 | 12 {
 if code == 0x1000 | 12 { // fee_insufficient
 res.extend_from_slice(&byte_utils::be64_to_array(msg.amount_msat));
 }
 else if code == 0x1000 | 13 {
Expand DownExpand Up@@ -1587,6 +1584,16 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 htlc_id: prev_htlc_id,
 incoming_packet_shared_secret: incoming_shared_secret,
 });
 if amt_to_forward < chan.get().get_our_htlc_minimum_msat() {
 let mut data = Vec::with_capacity(8 + 128); // 8-bytes-htlc_msat + 2-byte-length + length-byte-channel_update
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let chan_update = self.get_channel_update(chan.get()).unwrap();
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 failed_forwards.push((htlc_source, payment_hash,
 HTLCFailReason::Reason { failure_code: 0x1000 | 11, data } // amount_below_minimum
 ));
 continue;
 }
 match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet) {
 Err(e) => {
 if let ChannelError::Ignore(msg) = e {
Expand Down

30 changes: 26 additions & 4 deletions
lightning/src/ln/functional_tests.rs

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -5909,10 +5909,11 @@ fn run_onion_failure_test<F1,F2>(_name: &str, test_case: u8, nodes: &Vec<Node>,
}

// test_case
// 0: node1 fails backward
// 0: final node fails backward
// 1: final node fails backward
// 2: payment completed but the user rejects the payment
// 3: final node fails backward (but tamper onion payloads from node0)
// 4: intermediate node failure, fails backward
// 100: trigger error in the intermediate node and tamper returning fail_htlc
// 200: trigger error in the final node and tamper returning fail_htlc
fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case: u8, nodes: &Vec<Node>, route: &Route, payment_hash: &PaymentHash, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option<u16>, expected_channel_update: Option<HTLCFailChannelUpdate>)
Expand DownExpand Up@@ -6013,13 +6014,25 @@ fn run_onion_failure_test_with_fail_intercept<F1,F2,F3>(_name: &str, test_case:
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 4 => { // intermediate node failure; failing backward to start node
 assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 // forwarding on 1
 expect_htlc_forward!(&nodes[1]);

 // backward fail on 1
 expect_htlc_forward!(&nodes[1]);
 check_added_monitors!(nodes[1], 1);
 let update_1_0 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 assert!(update_1_0.update_fail_htlcs.len() == 1);
 update_1_0
 },
 _ => unreachable!(),
	};

	// 1 => 0 commitment_signed_dance
	if update_1_0.update_fail_htlcs.len() > 0 {
 let mut fail_msg = update_1_0.update_fail_htlcs[0].clone();
 if test_case == 100 {
 if test_case == 100 || test_case == 4 {
 callback_fail(&mut fail_msg);
 }
 nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg);
Expand DownExpand Up@@ -6269,11 +6282,20 @@ fn test_onion_failure() {
	run_onion_failure_test("unknown_next_peer", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(PERM|10),
 Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: bogus_route.paths[0][1].short_channel_id, is_permanent:true}));

	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_their_htlc_minimum_msat() - 1;
	let amt_to_forward = nodes[1].node.channel_state.lock().unwrap().by_id.get(&channels[1].2).unwrap().get_our_htlc_minimum_msat() - 1;
	let mut bogus_route = route.clone();
	let route_len = bogus_route.paths[0].len();
	bogus_route.paths[0][route_len-1].fee_msat = amt_to_forward;
	run_onion_failure_test("amount_below_minimum", 0, &nodes, &bogus_route, &payment_hash, |_| {}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
	run_onion_failure_test_with_fail_intercept("amount_below_minimum", 4, &nodes, &bogus_route, &payment_hash, |_| {}, |msg| {
 let session_priv = SecretKey::from_slice(&[3; 32]).unwrap();
 let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
 let mut data = Vec::with_capacity(8 + 128);
 data.extend_from_slice(&byte_utils::be64_to_array(amt_to_forward));
 let mut chan_update = ChannelUpdate::dummy();
 chan_update.contents.htlc_minimum_msat = amt_to_forward + 1;
 data.extend_from_slice(&chan_update.encode_with_len()[..]);
 msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], UPDATE|11, &data);
	}, ||{}, true, Some(UPDATE|11), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));

	//TODO: with new config API, we will be able to generate both valid and
	//invalid channel_update cases.
Expand Down