lnwallet: limit received htlc's to MaxAcceptedHTLCs #3910

Crypt-iQ · 2020-01-14T04:42:50Z

This PR clamps down the number of received HTLCs to MaxAcceptedHTLCs.

halseth · 2020-01-15T13:31:00Z

lnwallet/channel.go

+	// Clamp down on the number of HTLC's we can receive. It should never
+	// exceed our configurable MaxAcceptedHTLCs.
+	maxHTLCs := lc.channelState.LocalChanCfg.MaxAcceptedHtlcs
+	remoteACKedIndex := lc.localCommitChain.tail().theirHtlcIndex


This will be the index of the last acked htlc, not the first one active, which results in checking the number of unACKed HTLCs, which I don't think is what we want?

What we want to check is the number of active HTLCs, which will include both ACKed and unACKed. I think we must check the update logs or the commitments to see which HTLCs are actually active.

Ahhh you're right

halseth

see comment

Crypt-iQ · 2020-01-15T19:34:01Z

PTAL @halseth

halseth

LGTM 💯

halseth · 2020-01-21T08:52:53Z

Travis failing?

Crypt-iQ · 2020-01-21T12:39:38Z

Travis failing?

timeout flake with TestMppPayment I think

Crypt-iQ · 2020-01-21T15:34:38Z

@halseth Updated the calculation to start from the tip rather than tail since any new adds will be on top of that. Also moved the calculation to a function + updated the unit test.

halseth · 2020-01-22T08:18:08Z

@halseth Updated the calculation to start from the tip rather than tail since any new adds will be on top of that. Also moved the calculation to a function + updated the unit test.

Yes, makes sense! Looks good 👍

halseth · 2020-01-22T08:19:49Z

There might be some unexpected implication for the itests, looks like they are all failing the same test now.

halseth · 2020-01-30T14:25:40Z

lntest/itest/lnd_test.go

@@ -10145,7 +10145,7 @@ func testBidirectionalAsyncPayments(net *lntest.NetworkHarness, t *harnessTest)
 	}

 	// Calculate the number of invoices.
-	numInvoices := int(info.LocalBalance / paymentAmt)
+	numInvoices := input.MaxHTLCNumber / 2


Do we know why this didn't fail before?

Since the new check is on the receive side that could mean we are not properly checking sender side we are not overflowing the commitment.

You're right - the last commit is not the correct fix. The sender is overflowing the commitment, but there is a check there. I suspect an off-by-one error somewhere...

cfromknecht

async bidi test seems happy with latest fix, LGTM 🤘

lnwallet/channel.go

cfromknecht · 2020-02-08T00:41:40Z

lnwallet/channel.go

+	// channel. If, however, we adjust the committed HTLC's count to ignore
+	// HTLC's which will be removed in the next signature that Alice sends,
+	// then the add in step 6 won't overflow. This is because the signature
+	// received in step 7 will cover the settle/fail in step 1 and makes space


excellent comments, very helpful!

I updated the code to just use validateCommitmentSanity since it was doing the same thing. I didn't feel the need to add a unit test for the specific case outlined above since it's not a new function like calcIncomingHtlcs. Curious what you think.

lnwallet/channel.go

cfromknecht · 2020-02-11T18:51:16Z

lnwallet/channel.go

@@ -2788,7 +2788,7 @@ func (lc *LightningChannel) createCommitDiff(
 // in the state when adding a new HTLC, or nil otherwise.
 func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
 	ourLogCounter uint64, remoteChain bool,
-	predictAdded *PaymentDescriptor) error {
+	predictAdded, predictTheirs *PaymentDescriptor) error {


the godoc should be updated to describe this new argument. alternatively, seems like we can just pass it in via predictAdded and then add it to the appropriate log based on remoteChain

Possibly we would need this extra argument, to allow the case where we are predicting adding our add to the remote commitment chain, but would need to look at the callsites for the method.

Yes can do this, if remoteChain is true, add to our updates. Otherwise, add to theirs.

halseth · 2020-02-12T08:24:47Z

lnwallet/channel.go

+
+	// Clamp down on the number of HTLC's we can receive by checking the
+	// commitment sanity.
+	err := lc.validateCommitmentSanity(


yeah, nice I think this is it. If I'm not mistaken this will also handle the CASE 2 you outlined earlier, since it will apply the settles/fails accordingly?

Yup it applies the same logic that the sender applies so it handles case 2

halseth · 2020-02-12T08:26:15Z

lnwallet/channel.go

@@ -2788,7 +2788,7 @@ func (lc *LightningChannel) createCommitDiff(
 // in the state when adding a new HTLC, or nil otherwise.
 func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
 	ourLogCounter uint64, remoteChain bool,
-	predictAdded *PaymentDescriptor) error {
+	predictAdded, predictTheirs *PaymentDescriptor) error {


Possibly we would need this extra argument, to allow the case where we are predicting adding our add to the remote commitment chain, but would need to look at the callsites for the method.

cfromknecht

new unit test looks solid, could use a squash :)

lnwallet/channel_test.go

joostjager · 2020-02-14T20:55:46Z

lnwallet/channel.go

+
+	// Clamp down on the number of HTLC's we can receive by checking the
+	// commitment sanity.
+	err := lc.validateCommitmentSanity(


Doesn't this early validation violate the protocol? For example if we'd receive and settle an htlc in the same batch with no more commitment slot left? If validating only for the whole batch, that would still be possible perhaps?

I'm not entirely sure I follow - the prior behavior was to do no validation and wait until a commitment was received to do the validation. This just makes the validation earlier so there shouldn't be a problem. It also mimics lnd's sender-side logic (to validate upon calling AddHTLC) so if this is a protocol violation, then that logic would be too.

I think it wouldn't be a protocol violation since it will only add an additional HTLC if their next signature for us must have a settle/fail that would remove one HTLC to make room for this new one.

Yes, the question is indeed if it is a protocol violation. If the commitment is full, should we allow another htlc to be added? Because before the signature comes, a settle may still be added.

So there are two places in the spec I can see where this is elaborated on:

if result would be offering more than the remote's max_accepted_htlcs HTLCs, in the remote commitment transaction: MUST NOT add an HTLC.

and

if a sending node adds more than receiver max_accepted_htlcs HTLCs to its local commitment transaction [...]: SHOULD fail the channel.

So they only seem concerned about the effect on the commitment transaction. So I think this won't violate the spec since ReceiveHTLC just goes into the update logs. Plus it's only applied to the commit tx later because there is a settle/fail that ensures only the max can be applies to the commit tx.

It still feels a little inconclusive. But as the second quote is from the description of the add message, I indeed think that validation should take place already then and not be postponed until the signature arrives.

Is this worth bringing up / making an issue in the spec repo? I don't see the receive logic being the problem, but rather the sending logic. If we think a send in this async case is ok but our peer thinks it's a violation (esp w/ diff implementations), then I'd see a problem as we may be disconnected. All in all though, the receive should match the send logic.

Worth bringing up as a spec issue I think, although I think it is pretty obvious that each received message would have to lead to a valid commitment transaction. Otherwise how many updates would you allow while waiting fo a settel to open up the commitment. But agree that this could be more clearly spelled out in the spec.

This commit checks the commitment sanity when receiving an HTLC so that if a commitment transaction will overflow from an ADD, it is caught earlier rather than in ReceiveNewCommitment.

This commit fixes the TestMaxAcceptedHTLCs, TestMaxPendingAmount, TestMinHTLC, & TestChanReserve unit tests to pass with the new ReceiveHTLC logic. Instead of asserting specific failures upon receiving a new commitment signature, the various assertions were moved to assert on the error returned from ReceiveHTLC.

Adds a new test which asserts that the new ReceiveHTLC logic can handle proper commitment overflow calculation in the face of asynchronous updates.

halseth · 2020-02-17T11:52:13Z

Noticed travis was failing, but unrelated to this change: #4010

lnwallet/channel.go

halseth · 2020-02-17T12:01:07Z

lnwallet/channel_test.go

+// Alice sends to Bob in step 6 does not overflow Bob's commitment transaction.
+// This is because validateCommitmentSanity counts the HTLC's by ignoring
+// HTLC's which will be removed in the next signature that Alice sends. Thus,
+// the add won't overflow. This is because the signature received in step 7


Won't this contradict with the conclusion in #3910 (review) ?

In the end, we can only really execute these heuristics on a best effort basis. There're other classes of the concurrent send case that aren't covered in this test/scenario. At the end of the day, the real defense point is at the sender, in that they should be careful to not send items that would result in a force close.

Roasbeef

LGTM ✈️

Latest version is much simpler than the prior version I reviewed, which is great! As we dig into this area more and more it becomes more apparent how underspecified the spec w.r.t proper adherence to the current flow control limits. As discussed offline, it may actually take some protocol state machine level changes in order to fully rectify this set of issues.

Roasbeef · 2020-02-19T01:21:26Z

lnwallet/channel_test.go

+// Alice sends to Bob in step 6 does not overflow Bob's commitment transaction.
+// This is because validateCommitmentSanity counts the HTLC's by ignoring
+// HTLC's which will be removed in the next signature that Alice sends. Thus,
+// the add won't overflow. This is because the signature received in step 7


In the end, we can only really execute these heuristics on a best effort basis. There're other classes of the concurrent send case that aren't covered in this test/scenario. At the end of the day, the real defense point is at the sender, in that they should be careful to not send items that would result in a force close.

Crypt-iQ requested a review from Roasbeef January 14, 2020 04:42

Crypt-iQ requested a review from halseth as a code owner January 14, 2020 04:42

Crypt-iQ added this to the 0.9.1 milestone Jan 14, 2020

Crypt-iQ force-pushed the htlc_add_0113 branch from 8c86cb3 to f1e84ed Compare January 14, 2020 10:11

cfromknecht approved these changes Jan 15, 2020

View reviewed changes

halseth reviewed Jan 15, 2020

View reviewed changes

halseth suggested changes Jan 15, 2020

View reviewed changes

Crypt-iQ force-pushed the htlc_add_0113 branch from f1e84ed to ac91616 Compare January 15, 2020 19:28

halseth self-requested a review January 20, 2020 12:35

halseth approved these changes Jan 20, 2020

View reviewed changes

Crypt-iQ force-pushed the htlc_add_0113 branch from ac91616 to 528c964 Compare January 21, 2020 15:33

halseth reviewed Jan 30, 2020

View reviewed changes

Crypt-iQ force-pushed the htlc_add_0113 branch 2 times, most recently from b27d519 to a92f0e9 Compare February 7, 2020 20:38

Crypt-iQ requested a review from halseth February 8, 2020 00:21

cfromknecht approved these changes Feb 8, 2020

View reviewed changes

joostjager reviewed Feb 10, 2020

View reviewed changes

lnwallet/channel.go Outdated Show resolved Hide resolved

halseth reviewed Feb 10, 2020

View reviewed changes

lnwallet/channel.go Outdated Show resolved Hide resolved

cfromknecht reviewed Feb 11, 2020

View reviewed changes

halseth reviewed Feb 12, 2020

View reviewed changes

cfromknecht reviewed Feb 14, 2020

View reviewed changes

lnwallet/channel_test.go Outdated Show resolved Hide resolved

joostjager reviewed Feb 14, 2020

View reviewed changes

Crypt-iQ force-pushed the htlc_add_0113 branch 2 times, most recently from 3970197 to b783829 Compare February 15, 2020 14:53

Crypt-iQ added 3 commits February 15, 2020 09:59

lnwallet: call valdiateCommitmentSanity in ReceiveHTLC

56c07a5

This commit checks the commitment sanity when receiving an HTLC so that if a commitment transaction will overflow from an ADD, it is caught earlier rather than in ReceiveNewCommitment.

lnwallet: adding TestMaxAsynchronousHtlcs unit test

5a5e095

Adds a new test which asserts that the new ReceiveHTLC logic can handle proper commitment overflow calculation in the face of asynchronous updates.

Crypt-iQ force-pushed the htlc_add_0113 branch from b783829 to 5a5e095 Compare February 15, 2020 15:00

halseth suggested changes Feb 17, 2020

View reviewed changes

Crypt-iQ mentioned this pull request Feb 17, 2020

clarification: when does max_accepted_htlcs apply? lightning/bolts#745

Open

Roasbeef approved these changes Feb 19, 2020

View reviewed changes

Roasbeef merged commit 92b79f6 into lightningnetwork:master Feb 19, 2020

Crypt-iQ deleted the htlc_add_0113 branch February 19, 2020 02:11

lnwallet: limit received htlc's to MaxAcceptedHTLCs #3910

lnwallet: limit received htlc's to MaxAcceptedHTLCs #3910

Conversation

Crypt-iQ commented Jan 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth left a comment

Choose a reason for hiding this comment

Crypt-iQ commented Jan 15, 2020

halseth left a comment

Choose a reason for hiding this comment

halseth commented Jan 21, 2020

Crypt-iQ commented Jan 21, 2020

Crypt-iQ commented Jan 21, 2020 • edited Loading

halseth commented Jan 22, 2020

halseth commented Jan 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfromknecht left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ Feb 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ Feb 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ Feb 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth commented Feb 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ commented Jan 21, 2020 •

edited

Loading

Crypt-iQ Feb 15, 2020 •

edited

Loading

Crypt-iQ Feb 15, 2020 •

edited

Loading

Crypt-iQ Feb 15, 2020 •

edited

Loading