Onion error handling #146 #243

yuntai · 2018-11-06T05:51:25Z

Add test for onion error handling and some fixes

ariard · 2018-11-07T02:26:21Z

src/ln/channelmanager.rs

@@ -1445,6 +1445,12 @@ impl ChannelManager {
 		if let Some(mut sources) = removed_source {
 			for htlc_with_hash in sources.drain(..) {
 				if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
+				log_info!(self, "User rejected the incoming HTLC: {}", {


nit: maybe add more infos on the events sequence, "Got PaymentReceived event, ..." ?

PaymentRecieved event will be logged when it is received. So it will be duplicative here when the user calls this.

nit: the wording is a little bit weird, though - the log is for the "user"'s benefit (ie client of the library) and you're referring to them as "user". Maybe just say "Incoming HTLC rejected" since thats not ambiguous.

ariard · 2018-11-07T02:31:39Z

src/ln/channelmanager.rs

-					if let Some(update) = channel_update {
+				match &onion_error {
+					&HTLCFailReason::ErrorPacket { ref err } => {
+#[cfg(test)]


Hmm does using conditional compilation speed-up so much the tests ? Really personal, but don't find that it eases code readability..

needed not to generate additional compile warning message.

maybe #[allow(dead_code)] ? but okay

ariard · 2018-11-07T02:33:01Z

src/ln/channelmanager.rs

+#[cfg(not(test))]
+						failure_code:_,
+						data:_ } => {
+						// we get a fail_malformed_htlc from the first hop


"first hop" can we be sure it's the first one and not a previous hop there ?

if my understanding is correct, fail_malformed_htlc is converted to fail_htlc by intermediate nodes. so if the origin node received fail_malformed it must be from the first hop in the route.

oh right "here's a special malformed failure variant for the case where the peer couldn't parse it: in this case the current node instead takes action, encrypting it into a update_fail_htlc for relaying."

We should also close the channel here, no? If our next-hop can't parse what we're sending it we almost certainly don't have a use for the channel anymore.

ariard · 2018-11-07T02:39:41Z

src/ln/channelmanager.rs

 			}

 			const BADONION: u16 = 0x8000;
 			const PERM: u16 = 0x4000;
+			const NODE: u16 = 0x2000;


I think we can constify all error codes, already quoted in #49 ?

ah.. if we introduce constant error tables, it may as well to flatten out process_onion's error_code branching.

ariard

Cool stuff, protocol requirements seem good, some nits.
Don't review tests (yet)

ariard · 2018-11-07T02:43:08Z

src/ln/channelmanager.rs

-									//   required_node_feature_mssing(PERM|NODE|3)
+							error_code_ret = Some(error_code);
+
+							// eiter from intermediate or final node


nit: *either

ariard · 2018-11-07T02:53:55Z

src/ln/channelmanager.rs

+										is_permanent,
+									}), !(error_code & PERM == PERM && is_from_final_node)));
+								}
+								return;


Why assign a HTLCFailChannelUpdate to res if you return it nowhere ? Shouldn't be return res there ?

'return' returns from the callback and res is set to what should be returned from the outer function.

Ah sorry effectively yes, forgot to mental compile with callback!

ariard · 2018-11-07T02:58:31Z

src/ln/channelmanager.rs

 									res = Some((Some(msgs::HTLCFailChannelUpdate::ChannelClosed {
 										short_channel_id: route_hop.short_channel_id,
 										is_permanent: true,
-									}), false));
+									}), true));
+									return;


Same as above, return_node_fail_update ?

ariard · 2018-11-07T03:00:24Z

src/ln/channelmanager.rs

+										short_channel_id: route.hops[next_route_hop_ix].short_channel_id,
+										is_permanent: true,
+									}), true));
+									return;


Sorry, return_node_fail_update ?

ariard · 2018-11-07T03:08:07Z

src/ln/channelmanager.rs

-														incoming_htlc_msat > chan_update.contents.htlc_minimum_msat
+													11 => {
+														onion_failure_log!("amount_below_minimum", error_code, "htlc_msat", byte_utils::slice_to_be64(&err_packet.failuremsg[2..2+8]));
+														amt_to_forward > chan_update.contents.htlc_minimum_msat


switch to amt_to_forwatd because it's error code for the outgoing link so incoming_htlc_msat wasn't accurate right ?

yes.
.. from the processing node. per spec

ariard · 2018-11-07T03:08:32Z

src/ln/channelmanager.rs

-														chan_update.contents.flags & 0x01 == 0x01
+													14 => { // expiry_too_soon
+														onion_failure_log!("expiry_too_soon", error_code);
+														// alwayws valid?


ariard · 2018-11-07T03:13:18Z

src/ln/channelmanager.rs

 								}
 							}
 						}
 					}
 				}
 			}).expect("Route that we sent via spontaneously grew invalid keys in the middle of it?");
-			res.unwrap_or((None, true))
-		} else { ((None, true)) }
+			//res.unwrap_or((None, true))


thanks, all nit fixed!

TheBlueMatt

Thanks! This is really important stuff to finish up. As mentioned in #219 I hadn't previously really reviewed process_onion_failure so I just started by looking through the full function, not really just the changes here. I didn't get to a full detailed review and just looked at that function.

I left some comments in-line on some changes that I think may be helpful, but at a high-level I find the layout here a bit confusing - the spec speaks in terms of actions to take in response to different bits being set in the top byte, whereas this is structured in terms of actions to take in response to individual error types. Worse, spec doesn't seem to indicate we're allowed to use non-top-byte data for anything other than "debugging purposes"

TheBlueMatt · 2018-11-14T04:27:58Z

src/ln/channelmanager.rs

+
+			// when a node has sent invalid code or malformed failure msg
+			macro_rules! return_node_fail_update {
+				($res:ident, $node_id:expr, $retry:expr) => {{


nit: if you define this macro after the res var is let you can avoid passing in the $res argument.

TheBlueMatt · 2018-11-14T04:35:22Z

src/ln/channelmanager.rs

+						node_id: $node_id,
+						is_permanent: true,
+					}), $retry));
+					return;


nit: This reads a bit confusingly - you're using this inside the callback so it just continues the keys_callback loop, but you define it outside.

TheBlueMatt · 2018-11-14T04:37:30Z

src/ln/channelmanager.rs

+							if error_code&0xff == 1 || error_code&0xff == 2 || error_code&0xff == 3 {
+								match error_code {
+									_c if _c == PERM|1 => onion_failure_log!("invalid_realm", error_code),
+									_c if _c == NODE|2 => onion_failure_log!("temporary_node_failure", error_code),


Given these are log_info! level, it may be useful to specify a bit more than the spec-given title - something like "node specified a generic catch-all node-level failure".

TheBlueMatt · 2018-11-14T04:38:43Z

src/ln/channelmanager.rs

+									_c if _c == NODE|2 => onion_failure_log!("temporary_node_failure", error_code),
+									_c if _c == PERM|NODE|2 => onion_failure_log!("permanent_node_failure", error_code),
+									_c if _c == PERM|NODE|3 => onion_failure_log!("required_node_feature_mssing", error_code),
+									_ => return_node_fail_update!(res, route_hop.pubkey, !is_from_final_node),


Given these events only happen in response to an outgoing payment (and only once per payment), I think we should make sure we have a log entry for every case here. May be worth just merging the return_node_fail_update and onion_failure_log macros.

Further, in this particular case, I don't see why we can't continue on to the later processing? Even if we don't understand some future error code, if they set NODE|PERM we should give up routing to them.

TheBlueMatt · 2018-11-14T04:40:13Z

src/ln/channelmanager.rs

-					log_trace!(self, "{}({})", $error_code_textual, $error_code);
-				};
+				( $error_code_textual: expr, $error_code: expr, $reported_name: expr, $reported_value: expr ) => {{
+					log_info!(self, "{}({:#x}) {}({})", $error_code_textual, $error_code, $reported_name, $reported_value);


I think we should bump these from info to warn or so. They indicate a user-generated send_payment failed to complete and will only happen in response to such an event and only once per event, so they clearly meet the DoS criteria.

TheBlueMatt · 2018-11-14T04:44:14Z

src/ln/channelmanager.rs

+										// failing incoming channel to the erring node
+										short_channel_id: route_hop.short_channel_id,
+										is_permanent,
+									}), !(error_code & PERM == PERM && is_from_final_node)));


nit: You can use is_permanent in place of the explicit "error_code & PERM == PERM" here and below, no?

TheBlueMatt · 2018-11-14T04:56:51Z

src/ln/channelmanager.rs

+									_c if _c == PERM|15 => { onion_failure_log!("unknwon_payment_hash", error_code); false },
+									_c if _c == PERM|16 => { onion_failure_log!("incorrect_payment_amount", error_code); false },
+									17 => { onion_failure_log!("final_expiry_too_soon", error_code); true },
+									18 if err_packet.failuremsg.len() == 6 => {


Hmm, I don't think we should be considering a type incorrect if they send us more bytes than we wanted - updates to the spec are expected to just append bytes where new data needs to be provided.

TheBlueMatt · 2018-11-14T04:58:14Z

src/ln/channelmanager.rs

+										is_permanent,
+									}), !(error_code & PERM == PERM && is_from_final_node)));
+								}
+								return;
 							}

 							if is_from_final_node {
 								let payment_retryable = match error_code {


spec seems to indicate we should just set payment_retryable based on PERM bit and use the rest just for logging.

TheBlueMatt · 2018-11-14T05:02:32Z

src/ln/channelmanager.rs

 								}
 							}
 						}
 					}
 				}
 			}).expect("Route that we sent via spontaneously grew invalid keys in the middle of it?");
-			res.unwrap_or((None, true))
-		} else { ((None, true)) }
+			let (channel_update, payment_retryable) = res.expect("should have been set!");


If a node simply garbles the onion data we'll panic at this expect - we have to just move on since in such a case (presumably retrying the payment, since its likely an intermediate node garbled it) as we have no ability to know what happened.

yuntai · 2018-11-17T11:20:08Z

Worse, spec doesn't seem to indicate we're allowed to use non-top-byte data for anything other than "debugging purposes"

But the spec says,
A forwarding node MAY, but a final node MUST NOT ... and An intermediate hop MUST NOT, but the final node .... Then only way the receiving node marks a node violating this requirement is using the lower byte.

Also, some error codes(unknwon_payment_hash, final_expiry_too_soon, ...) from the final node does not require a route map update, but others do. These can be only distinguished by the lower byte also. Well, so I ended up (almost) error_code based action branches.

If the spec update is imminent, I'll wait and see how to clean this mess-up.

TheBlueMatt · 2018-11-19T03:25:41Z

Well specifically I was referring to the "receiving error codes" subsection, which is the thing you're implementing here https://github.com/lightningnetwork/lightning-rfc/blob/master/04-onion-routing.md#receiving-failure-codes. Indeed, if some other node is misbehaving we can ignore their crap, but, generally, we should follow the section we're doing (the spec is written in subsections applying to the nodes which (a) generate a message and (b) receive a message, and technically you're just supposed to implement the receiving part on the receiving end without considering the other part. Now that's also a very antiquated way to consider specs and the whole "be liberal in what you accept, conservative in what you send" thing has been shown to be a generally terrible idea more times than you can count, but in this case trying to parse things based on flags instead of values, where possible, does mean a bit more future compatibility).

I pointed to a few specific cases in my comments, above, though, indeed, this doesn't apply universally, as you point out.

yuntai · 2018-11-19T09:34:25Z

I'm trying to simplify the logic to more closely mirror "receiving error codes" subsection.

In the spec, when an error is from the final node, there is no mentioning about updating network map.

But, these four,
invalid_realm
temporary_node_failure
permanent_node_failure
required_node_feature_mssing

can be from a final node. When PERM bit is set ( all above except temporary_...), I think the final node should be removed from the routing DB as well as failing payment permanently. Shouldn't this be clarified in the spec? Or I am missing something and it's correct that the spec should have no opinion about the above errors from the final node.

- blockify error log macros - return node failure macros - make error_code branching tighter - fill more error logs - log onion hash with BADONION bit - fix bug reporting wrong short_channel_id for ChannleClosed - handle amount_below_minimum return error code for debugging/testing

fix htlc_fail_async_shutdown test making expect PaymentFailureNetworkUpdate

- user friendly warning message - helper for debug field reported by the erring node

More closely mirroring the spec where the process is based on mainly the top byte of the error code.

yuntai · 2018-11-22T06:07:29Z

Cleaned up error handling more based on the top byte of the error code with commented issues addressed.

TheBlueMatt

Didn't review the tests in a ton of detail, but did test them a little bit and they look awesome, well done there. I find this version way more readable, just a few small easy fixes here and there and I think this is good to go.

TheBlueMatt · 2018-11-23T20:42:32Z

src/ln/channelmanager.rs

+
+							let (description, title) = get_onion_error_description(error_code);
+							if debug_field_size > 0 && err_packet.failuremsg.len() >= 2 + debug_field_size {
+								log_warn!(self, "Onion Error[{}({:#x}) {}({})] {}", title, error_code, debug_field, DebugBytes(&err_packet.failuremsg[2..2+debug_field_size]), description);


nit: why not just use the macro instead of importing DebugBytes?

Also, isnt it failuremsg[4.. instead of 2?

src/ln/channelmanager.rs

TheBlueMatt · 2018-11-23T21:57:55Z

src/ln/channelmanager.rs

@@ -1445,6 +1445,12 @@ impl ChannelManager {
 		if let Some(mut sources) = removed_source {
 			for htlc_with_hash in sources.drain(..) {
 				if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
+				log_info!(self, "User rejected the incoming HTLC: {}", {


nit: the wording is a little bit weird, though - the log is for the "user"'s benefit (ie client of the library) and you're referring to them as "user". Maybe just say "Incoming HTLC rejected" since thats not ambiguous.

TheBlueMatt · 2018-11-25T17:19:45Z

src/ln/channelmanager.rs

+#[cfg(not(test))]
+						failure_code:_,
+						data:_ } => {
+						// we get a fail_malformed_htlc from the first hop


We should also close the channel here, no? If our next-hop can't parse what we're sending it we almost certainly don't have a use for the channel anymore.

TheBlueMatt · 2018-11-25T17:23:09Z

src/ln/channelmanager.rs

+						failure_code:_,
+						data:_ } => {
+						// we get a fail_malformed_htlc from the first hop
+						// TODO: not bother trying parsing onion for now


this TODO could be clarified more - something like "we should check the failure_code and check that it isnt something that indicates temporary payment failure before we give up on the channel entirely".

TheBlueMatt · 2018-11-25T18:12:48Z

src/ln/channelmanager.rs

+								    8|9|10 => route.hops[next_route_hop_ix].short_channel_id, // outgoing (permanent_channel_failure/required_channel_feature_missing/unknown_next_peer)
+								    _ => route_hop.short_channel_id, // incoming channel (invalid_realm & BADONION)
+								  },
+								   is_permanent: error_code & PERM == PERM,


nit: this is always true.

TheBlueMatt · 2018-11-25T18:18:21Z

src/ln/channelmanager.rs

+								})};
+							}
+							else if error_code & UPDATE == UPDATE {
+								if err_packet.failuremsg.len() >= debug_field_size + 2 {


Shouldnt this be >= +4?

TheBlueMatt · 2018-11-25T18:22:59Z

src/ln/channelmanager.rs

 								}
 							}
+
+							res = Some((fail_channel_update, !(error_code & PERM == PERM && is_from_final_node)));


If we get a None on fail_channel_update and !payment_failed (ie the data provided by the err'ing node was unreadable by us), shouldn't we default to failing something? eg if we get an UPDATE flag but couldn't parse the update, if is_chan_update_invalid, etc we should probably fail the channel, or if we get no readable flags we can fail the node wholesale.

TheBlueMatt · 2018-11-25T18:28:34Z

src/ln/channelmanager.rs

+												11 => amt_to_forward > chan_update.contents.htlc_minimum_msat,
+												12 => {
+													let new_fee = amt_to_forward.checked_mul(chan_update.contents.fee_proportional_millionths as u64).and_then(|prop_fee| { (prop_fee / 1000000).checked_add(chan_update.contents.fee_base_msat as u64) });
+													new_fee.is_none() || incoming_htlc_msat >= new_fee.unwrap() && incoming_htlc_msat >= amt_to_forward + new_fee.unwrap()


I think new_fee.is_none() is a valid update-causes failure, no? It indicates the node is setting their feerate to some mega-high value causing an overflow, which probably means they're temporarily disabling the channel by setting a high fee (or at least disabling high-value forwards).

Also, why the second condition (incoming_htlc_msat >= new_fee.unwrap()), why not just amt_to_forward.checked_add(new_fee.unwrap()) and compare?

TheBlueMatt · 2018-11-25T18:37:03Z

src/ln/channelmanager.rs

+		run_onion_failure_test_with_fail_intercept(_name, test_case, nodes, route, payment_hash, callback_msg, |_|{}, callback_node, expected_retryable, expected_error_code, expected_channel_update);
+	}
+
+	// test_case


It'd be nice to get a bit more of a description of exactly what this does and what the callbacks mean as its not super trivial to read.

TheBlueMatt · 2018-12-18T03:46:02Z

Will take as #274.

yuntai force-pushed the 201809-issue146-failurenonion2 branch from 3212322 to 593804e Compare November 6, 2018 06:02

ariard reviewed Nov 7, 2018

View reviewed changes

TheBlueMatt reviewed Nov 14, 2018

View reviewed changes

yuntai added 12 commits November 22, 2018 13:59

Add flags to update_fail_hltc when error_code is 20

3aceb37

Use a fixed session secret for testing

54d7a7a

Add log when user rejected payment

5b02494

Handle case when origin node received fail_malformed

77bbbf4

Add error_code to PaymentFailed for testing and

dc38e7e

fix htlc_fail_async_shutdown test making expect PaymentFailureNetworkUpdate

Add test for orning error handling

88579aa

nit fix

3ad9083

Add helper method for Onion error

8e5ebe7

- user friendly warning message - helper for debug field reported by the erring node

Clean up onion error handling

700c83d

More closely mirroring the spec where the process is based on mainly the top byte of the error code.

Add the current channel setting flags for channel_disabled onion error

fad983e

Fix test to expect channel update for channel_disabled onion error

07c0345

yuntai force-pushed the 201809-issue146-failurenonion2 branch from 989058b to 07c0345 Compare November 22, 2018 06:05

TheBlueMatt reviewed Nov 25, 2018

View reviewed changes

yuntai mentioned this pull request Nov 26, 2018

Add a method to get session secret for onion packet to KeysInterface #260

Merged

TheBlueMatt mentioned this pull request Dec 2, 2018

Ready for Review : Handle resolution in case of channel going onchain, pass state backward offchain #198

Closed

TheBlueMatt mentioned this pull request Dec 18, 2018

Onion Error Handling #274

Merged

TheBlueMatt closed this Dec 18, 2018

Onion error handling #146 #243

Onion error handling #146 #243

Uh oh!

Conversation

yuntai commented Nov 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuntai commented Nov 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ariard left a comment •

edited

Loading

yuntai commented Nov 17, 2018 •

edited

Loading