Implement checks amt_to_forward and outgoing_cltv_value acceptable ranges #114

ariard · 2018-08-17T01:52:34Z

Just implemented checks as described in rfc 04 Packet structure. Maybe we should check also against an older fee value as recommended in rfc 07 due to propagation delay, but if so need to log old fee somewhere?

TheBlueMatt · 2018-08-17T21:26:53Z

Looks like maybe the fake network tests are violating the rules somehow?

ariard · 2018-08-18T02:56:07Z

Yeah, it's the 6th send_payment in fake_network_test with a channel_outbound increase. I'm still digging to see if this case is right handle by get_route to generate first hop amount_msat..

ariard · 2018-08-18T02:59:58Z

Looking on how others implementations manage get_our_fee_base_msat, spec is unclear on what is the right way to do it.

ariard · 2018-08-20T01:38:52Z

Was a funny one to track :)
Amount msat in send_payment in fake_network_test were all false due to the way Router::handle_channel_update manages the msg. When this method receives a msg, it checks if it's a two_to_one or one_to_two one and completes DirectionalChannelInfo accordingly.
The problem is that cltv_expiry_delta, htlc_minimum_msat, fee_base_msat, as included in the message, aren't the policy that the source node will seen apply for its htlc but the policy it will apply for the other node policy. So we need to reverse.
If not, get_route uses this false information when adding node for Route and the final amount msat is inaccurate.
The fix is maybe a bit ugly, I think we can also fix it at route generation?

I wouldn't have fall on this if we didn't used get_our_fee_base_msat, I find it a little bit complex due to the channel_outbound case but I guess it's better than a magic default value of 1000 msat as lnd and c-lightning do.

90d626 is log stuff for NetworkMap, really useful for route bug tracking , but it's need network map fields as public so maybe not?

TheBlueMatt · 2018-08-20T15:45:04Z

src/ln/router.rs

-	htlc_minimum_msat: u64,
-	fee_base_msat: u32,
-	fee_proportional_millionths: u32,
+pub struct DirectionalChannelInfo {


These probably want pub(crate), not pub, and maybe a comment that they're only crate-pub for the logger's access.

TheBlueMatt · 2018-08-20T15:55:31Z

src/ln/channelmanager.rs

@@ -1625,7 +1625,18 @@ impl ChannelMessageHandler for ChannelManager {
 					hmac: next_hop_data.hmac.clone(),
 				};

-				//TODO: Check amt_to_forward and outgoing_cltv_value are within acceptable ranges!
+				let channel_state = self.channel_state.lock().unwrap();


Hmm, can you move this check down past the existing channel_state.lock() call? It really sucks to lock, do a channel lookup, check something, unlock, then lock and do the same channel lookup again. That's likely to introduce strange races as well.

Done, but it's add a check to prevent the case of the HTLC being our payment

TheBlueMatt · 2018-08-20T17:15:47Z

src/ln/router.rs

-						$target.htlc_minimum_msat = msg.contents.htlc_minimum_msat;
-						$target.fee_base_msat = msg.contents.fee_base_msat;
-						$target.fee_proportional_millionths = msg.contents.fee_proportional_millionths;
+						chan_was_enabled = $first_side.enabled;


Nice debugging, but sadly I think you came to the wrong conclusion. The way I read the second paragraph in https://github.com/lightningnetwork/lightning-rfc/blob/master/07-routing-gossip.md#the-channel_update-message reads to me like the original code here is correct and the issue is in the new check - fees/cltv_deltas are announced on the basis of the channel where the HTLC will be forwarded to, not the basis of which channel the HTLC came in on.

Yeah but seems that my code follows rfc 07 fee calculation : fee_base_msat + ( amount_msat * fee_proportional_millionths / 1000000 ) and the rfc 04 check : incoming_htlc_amt - fee >= amt_to_forward ? The amounts don't match as seen there, running cargo test fake_network_test -- --no-capture on my demo_bug_channelinfo_handling branch : https://0bin.net/paste/lixegfyl2cDb+n25#B9BdwQDf4pqNYa5S4dZuK6i1ZGM+OQMYqjIcMgSuilW

Yes, fees/cltv_deltas are based on the receiving node policy where the HTLC will be forwarded to, the issue is on the way handle_channel_update is recording the channel_update as one_to_two or two_to_one. When the node is one, and uses the one_to_two side, it has to use the two_to_one directional info and so the receiving node policy. That's this policy in ChannelInfo, i think, is inverted now.

The information going into the channels map here looks right to me - its setting the fees on a channel on the entry that represents the outgoing hop (as indicated by it being the side with the src_node_id being equal to the node which signed the message). In order to be calculating the fee on the basis of where the payment is being forwarded to (ie if you route A -> B -> C, the fees paid are the ones that B announced for the B -> C hop) you'd need to change the check that was added to update_add_htlc.

TheBlueMatt · 2018-08-20T17:17:56Z

src/util/macro_logger.rs

+		Ok(())
+	}
+}
+macro_rules! log_networkmap {


These are really awesome, but it sucks to add unused code cause it generates way more compile warnings. Honestly its probably worth adding a pub fn Router::log_network function to keep these around.

Why not use [allow(dead_code)] and [allow(unused_macros], seems that these attributes been there for avoid lint warnings ?

Just adding allow(dead_code) doesn't resolve the issue - the point is that code should be used or it won't be kept up-to-date and probably doesn't belong in the repo at all.

ariard · 2018-08-21T00:43:02Z

travis failures on fuzz part due to amount in/out fee required mismatch, what is test:test_no_existing_test_breakage? it's calling handle_update_add_htlc somewhere?

TheBlueMatt

The new full_stack_target test tests that existing fuzz test cases aren't broken by changes in behavior. This is mostly for my tracking so I know what bits to not take until I want to restart fuzzing on all new test-cases. It will break generally when you add new feerate requests and new checks like this. If you don't feel like fiing the test case that's written out there that's find, I'd generally be fine merging things that comment it out and then fixing it myself.

There is another issue here that I forgot about - if we let routing fees charged float with network fees (which I think is prudent, but may be more complexity than its worth) then we have to pay close attention to when those fees change and send channel_update messages when the feerate info causes us to change our feerate, which is hopefully not too often.

TheBlueMatt · 2018-08-21T17:21:25Z

src/ln/channelmanager.rs

+		{
+			if next_hop_data.hmac != [0;32] {
+				// check doesn't apply if we are last node (check happens here due to lock optimizations)
+				let chan = channel_state_lock.by_id.get(&msg.channel_id).unwrap();


This is still wrong - we should be doing the fee checks against the place we're sending the payment (ie pending_forward_info.short_channel_id ie the channel that is looked up in the next if statement.

TheBlueMatt · 2018-08-21T17:24:12Z

src/ln/router.rs

-						$target.htlc_minimum_msat = msg.contents.htlc_minimum_msat;
-						$target.fee_base_msat = msg.contents.fee_base_msat;
-						$target.fee_proportional_millionths = msg.contents.fee_proportional_millionths;
+						chan_was_enabled = $first_side.enabled;


The information going into the channels map here looks right to me - its setting the fees on a channel on the entry that represents the outgoing hop (as indicated by it being the side with the src_node_id being equal to the node which signed the message). In order to be calculating the fee on the basis of where the payment is being forwarded to (ie if you route A -> B -> C, the fees paid are the ones that B announced for the B -> C hop) you'd need to change the check that was added to update_add_htlc.

ariard · 2018-08-22T00:37:08Z

Aaaaah finally got it! After reading again and scrupulously this time the rfcs and lnd, c-lightning codes! Sorry for the lost time, i was perpexled at first i think by the channel_outbound estimation thing case and from then, misjudged all the rest.. At least i've seen the Router internals.

For the routing fees coupled on network fees I think that's an issue with FeeEstimator implementation to avoid too much channel_update messages. We should warn it to have an update limit for a given window time and not necessarily to echo all network fees moves (but don't know algorithm fees so surely harder to implement than to describe I guess).

ariard · 2018-08-22T00:38:35Z

I commented out the new fuzzing case, will dig into the fuzz tools and set it up on my environnement soon, need to understand this too

TheBlueMatt · 2018-08-22T18:58:28Z

src/ln/channelmanager.rs

@@ -1656,6 +1654,20 @@ impl ChannelMessageHandler for ChannelManager {
 				Some(id) => id.clone(),
 			};
 			let chan = channel_state.by_id.get_mut(&forwarding_id).unwrap();
+			let fee = 0;
+			if next_hop_data.hmac != [0;32] {
+				let fee = chan.get_our_fee_base_msat(&*self.fee_estimator) + (pending_forward_info.amt_to_forward * self.fee_proportional_millionths as u64 / 1000000) as u32;


Because you have a "let" here, fee is always 0 in the next if statement. Also, the pending_forward_info.onion_packet.is_some() check above is duplicative with this - if next_hop_data.hmac is 0s onion_packet is always None.

TheBlueMatt · 2018-08-22T18:59:26Z

src/ln/router.rs

-	fee_base_msat: u32,
-	fee_proportional_millionths: u32,
+//Here, DirectionalChannelInfo, ChannelInfo, NodeInfo and NetworkMap are pub(crate) only for log_* macros access
+pub(crate) struct DirectionalChannelInfo {


Hmm, it seems weird to put the logger in log macros and then expose all these things, maybe just implement Display for these types directly instead of going through the indirection?

acceptable ranges add log_route macro

ariard · 2018-08-23T02:25:22Z

Updated, with Display trait on networkmap structs.

TheBlueMatt · 2018-08-23T18:19:09Z

Cool, thanks, will take as #125.

Rebase of #114

Fix several panics introduced by #125 (#114) and #124

ariard force-pushed the ranges_amt_to_forward_outgoing_cltv_values branch from 4b2281d to a5a1f7e Compare August 18, 2018 02:51

ariard force-pushed the ranges_amt_to_forward_outgoing_cltv_values branch from a5a1f7e to 90d6260 Compare August 20, 2018 01:33

TheBlueMatt reviewed Aug 20, 2018

View reviewed changes

ariard force-pushed the ranges_amt_to_forward_outgoing_cltv_values branch 2 times, most recently from 4762f02 to 65d8210 Compare August 21, 2018 00:09

ariard force-pushed the ranges_amt_to_forward_outgoing_cltv_values branch from 65d8210 to bfce91c Compare August 21, 2018 00:56

TheBlueMatt reviewed Aug 21, 2018

View reviewed changes

ariard force-pushed the ranges_amt_to_forward_outgoing_cltv_values branch 3 times, most recently from 9ff8238 to 50bcfda Compare August 22, 2018 00:22

TheBlueMatt reviewed Aug 22, 2018

View reviewed changes

TheBlueMatt mentioned this pull request Aug 22, 2018

Do not return UpdateFailHTLC updates until the HTLC is committed #124

Merged

Antoine Riard added 2 commits August 23, 2018 02:00

Add checks that amt_to_forward and outgoing_cltv_value are within

327828d

acceptable ranges add log_route macro

Add Display trait on network structs for routing bug track

8462362

ariard force-pushed the ranges_amt_to_forward_outgoing_cltv_values branch from 50bcfda to 8462362 Compare August 23, 2018 02:20

TheBlueMatt closed this Aug 23, 2018

TheBlueMatt added a commit that referenced this pull request Aug 23, 2018

Merge pull request #125 from TheBlueMatt/2018-08-114-rebase

d0e1e7f

Rebase of #114

ariard deleted the ranges_amt_to_forward_outgoing_cltv_values branch August 23, 2018 21:05

TheBlueMatt added a commit that referenced this pull request Aug 23, 2018

Merge pull request #127 from TheBlueMatt/2018-08-fuzz-fixes-5

a66e597

Fix several panics introduced by #125 (#114) and #124

Implement checks amt_to_forward and outgoing_cltv_value acceptable ranges #114

Implement checks amt_to_forward and outgoing_cltv_value acceptable ranges #114

Uh oh!

Conversation

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

Uh oh!

Uh oh!

TheBlueMatt commented Aug 17, 2018

Uh oh!

ariard commented Aug 18, 2018

Uh oh!

ariard commented Aug 18, 2018

Uh oh!

ariard commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 commented Aug 21, 2018

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!

ariard commented Aug 22, 2018

Uh oh!

ariard commented Aug 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Aug 23, 2018

Uh oh!

TheBlueMatt commented Aug 23, 2018

Uh oh!

Uh oh!

ariard commented Aug 17, 2018 •

edited

Loading

ariard commented Aug 20, 2018 •

edited

Loading