Fix in checking channel_reserve #189

yuntai · 2018-09-19T11:49:18Z

No description provided.

yuntai · 2018-09-19T11:50:11Z

src/ln/channel.rs

@@ -2609,8 +2609,8 @@ impl Channel {
 		if htlc_outbound_value_msat + amount_msat > self.their_max_htlc_value_in_flight_msat {
 			return Err(HandleError{err: "Cannot send value that would put us over our max HTLC value in flight", action: None});
 		}
-		// Check their_channel_reserve_satoshis:
-		if htlc_inbound_value_msat + htlc_outbound_value_msat + amount_msat + (self.channel_value_satoshis * 1000 - self.value_to_self_msat) > (self.channel_value_satoshis - self.their_channel_reserve_satoshis) * 1000 {


The previous eqn seems double counting htlc_inbound_value_msat and htlc_outbound_value_msat?

TheBlueMatt · 2018-09-20T02:29:40Z

Hmm, we definitely need to be subtracting pending HTLCs from the value_to_self_msat here (since any pending HTLCs can't count towards channel reserve and aren't included in value_to_self_msat), though it looks pretty awkward and I'd buy that its wrong. Can you add a test either way?

yuntai · 2018-09-25T09:54:07Z

Since value_to_self_msat is adjusted when revoke_and_ack'd, when evaluating this condition, value_to_self contains pending outgoing htlc values, so, yes, they need to be subtracted. But pending incoming htlc values have nothing to do with value_to_self_msat at this point.

New conditions are the same except not including value for incoming HTLC to be subtracted from value_to_self_msat. The conditions are just tighter (more money available). Also, I added outgoing value in the pending_cell.

Hard to figure a good test for this.
One thing I could think of is to add debug_assert!(self.value_to_self_msat >= channel_reserve) inside revoke_and_ack(when debited) and make fuzz catch any error.

TheBlueMatt · 2018-09-26T17:01:25Z

Hmm, ok, yes, I buy that the new version is correct, but really needs tests. One thing you could do it add a debug_assert in build_commitment_transaction that the finally-calculated value_to_self and value_to_remote match channel reserves (which is broader than revoke_and_ack), though I don't like relying on fuzz testing for that as the full_stack_target is still way too broad to reliably find things like that.

If you did that and also added a number of tests which tested sending one less than and one more than the limit in the ChannelManager network tests (incl various states like awaiting-revoke, etc) then we'd probably be OK.

Testing the holding cell updates is going to be much harder, I think, though, cause free_holding_cell_htlcs handles the error case instead of sending the new HTLC.

TheBlueMatt · 2018-09-29T19:49:35Z

src/ln/channel.rs

+			}
+		}
+
+		// Check self.their_channel_reserve_satoshis (i.e our channel reserve):


Not sure that that is any more clear...maybe s/our channel reserve/the amount we must keep as reserve for them to have something to claim if we misbehave/?

yuntai · 2018-10-01T11:09:12Z

still need another test involving holding cell.

yuntai · 2018-10-02T05:26:22Z

Testing the holding cell updates is going to be much harder, I think, though, cause free_holding_cell_htlcs handles the error case instead of sending the new HTLC.

I'm not clear about this. HTLCUpdateAwaitingACK::FailHTLC won't be affecting to_self_value_msat ever?
HTLCUpdateAwaitingACK::ClaimHTLC will increase to_self_value_msat later on but including this as + to to_self_value_msat when evaluating channel reserve condition with anticipation that we will get money soon will increase the chance of channel reserve violation error from the remote.

and max_htlc_value_in_flight_msat

yuntai · 2018-10-02T07:12:33Z

added a test scenario with holding cell.

TheBlueMatt

Nice! Changes look good and I tried to falsify the tests a bit and they caught it.

TheBlueMatt · 2018-10-01T14:45:30Z

src/ln/channel.rs

@@ -2725,7 +2763,7 @@ impl Channel {
 			return Err(HandleError{err: "Cannot send an HTLC while disconnected", action: Some(ErrorAction::IgnoreError)});
 		}

-		let (_, outbound_htlc_count, htlc_outbound_value_msat, htlc_inbound_value_msat) = self.get_pending_htlc_stats(false);
+		let (_, outbound_htlc_count, htlc_outbound_value_msat, _) = self.get_pending_htlc_stats(false);


Oops, with this and the change to split up pending_htlcs into two vecs it probably makes sense to get rid of get_pending_htlc_stats - currently all callsites make it iterate over both HTLC vecs but only care about the results from one.

TheBlueMatt · 2018-10-02T18:23:51Z

src/ln/channelmanager.rs

+		use ln::msgs::HandleError;
+
+		macro_rules! get_channel_value_stat {
+      ($node: expr, $channel_id: expr) => {{


nit: indentation is wrong around here.

TheBlueMatt · 2018-10-02T18:24:01Z

src/ln/channelmanager.rs

+		macro_rules! get_route_and_payment_hash {
+			($recv_value: expr) => {{
+				let route = nodes[0].router.get_route(&nodes.last().unwrap().node.get_our_node_id(), None, &Vec::new(), $recv_value, TEST_FINAL_CLTV).unwrap();
+				let payment_preimage = [*nodes[0].network_payment_count.borrow(); 32];


Should be able to use get_payment_preimage_hash!!

TheBlueMatt · 2018-10-02T18:56:39Z

You can also take the change at TheBlueMatt@1f31e03 if you want, its a pretty aggressive check but once it gets adapted to udpate_fee/etc it may make sense to have something more aggressive like that.

to get_inbound_pending_htlc_stats and get_outbound_pending_htlc_stats

yuntai · 2018-10-03T01:19:32Z

Took TheBlueMatt/rust-lightning@1f31e03 and others fixed.

TheBlueMatt · 2018-10-06T03:08:32Z

Will take as #206.

#189 rebased

yuntai commented Sep 19, 2018

View reviewed changes

yuntai force-pushed the 201809-channel-reserve branch 2 times, most recently from c8fef0b to a3e103c Compare September 22, 2018 14:30

yuntai mentioned this pull request Sep 25, 2018

channel_reserve_satoshis variable names confusing #181

Closed

TheBlueMatt mentioned this pull request Sep 26, 2018

Implement update_fee handling #161 #187

Closed

TheBlueMatt reviewed Sep 29, 2018

View reviewed changes

yuntai added 5 commits September 30, 2018 12:29

Fix in checking channel_reserve

1c31cc9

add a nice comment

f5c50c3

Some initial work to print out detailed log

0714869

struct for the state of channel values for testing

28c402e

Allow to specifiy values

23c12a2

yuntai force-pushed the 201809-channel-reserve branch from a3e103c to af89dc1 Compare October 1, 2018 11:06

Testing for channel_reserve check

0ae339b

and max_htlc_value_in_flight_msat

yuntai force-pushed the 201809-channel-reserve branch from ab13c24 to 0000294 Compare October 2, 2018 07:09

TheBlueMatt reviewed Oct 2, 2018

View reviewed changes

yuntai and others added 3 commits October 3, 2018 10:07

Add a test scenario with holding cell output

8dc6c4e

Check that reserves are met or progress is made on commitment tx gen

ff6ea71

Split get_pending_htlc_stats

5f1b366

to get_inbound_pending_htlc_stats and get_outbound_pending_htlc_stats

yuntai force-pushed the 201809-channel-reserve branch from 0000294 to 5f1b366 Compare October 3, 2018 01:18

yuntai mentioned this pull request Oct 3, 2018

Check commitment transaction fees when testing channel_reserve #205

Closed

TheBlueMatt closed this Oct 6, 2018

TheBlueMatt added a commit that referenced this pull request Oct 6, 2018

Merge pull request #206 from TheBlueMatt/2018-10-189-rebase

84d779a

#189 rebased

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix in checking channel_reserve #189

Fix in checking channel_reserve #189

Uh oh!

yuntai commented Sep 19, 2018

Uh oh!

yuntai Sep 19, 2018

Uh oh!

TheBlueMatt commented Sep 20, 2018

Uh oh!

yuntai commented Sep 25, 2018 •

edited

Loading

Uh oh!

TheBlueMatt commented Sep 26, 2018

Uh oh!

TheBlueMatt Sep 29, 2018

Uh oh!

yuntai commented Oct 1, 2018

Uh oh!

yuntai commented Oct 2, 2018

Uh oh!

yuntai commented Oct 2, 2018

Uh oh!

TheBlueMatt left a comment

Uh oh!

TheBlueMatt Oct 1, 2018

Uh oh!

TheBlueMatt Oct 2, 2018

Uh oh!

TheBlueMatt Oct 2, 2018

Uh oh!

TheBlueMatt commented Oct 2, 2018

Uh oh!

yuntai commented Oct 3, 2018

Uh oh!

TheBlueMatt commented Oct 6, 2018

Uh oh!

Uh oh!

Fix in checking channel_reserve #189

Fix in checking channel_reserve #189

Uh oh!

Conversation

yuntai commented Sep 19, 2018

Uh oh!

yuntai Sep 19, 2018

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Sep 20, 2018

Uh oh!

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

Uh oh!

Uh oh!

TheBlueMatt commented Sep 26, 2018

Uh oh!

TheBlueMatt Sep 29, 2018

Choose a reason for hiding this comment

Uh oh!

yuntai commented Oct 1, 2018

Uh oh!

yuntai commented Oct 2, 2018

Uh oh!

yuntai commented Oct 2, 2018

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Oct 2, 2018

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Oct 2, 2018

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Oct 2, 2018

Uh oh!

yuntai commented Oct 3, 2018

Uh oh!

TheBlueMatt commented Oct 6, 2018

Uh oh!

Uh oh!

yuntai commented Sep 25, 2018 •

edited

Loading