lnwallet: Make available balance HTLC fee aware #3691

halseth · 2019-11-07T18:48:46Z

This PR makes the balance calculation take into account the extra fee needed to be paid by the initiator when sending non-dust HTLCs. This is a case that could previously be hit both if we as an initiator couldn't pay the HTLC fee, and when the remote (as initiator) couldn't pay to add our HTLC to the commitment.

This PR mitigates this by in those cases reporting a lower number, ensuring the pathfinding algorithm won't attempt to use the channel in such cases.

Fixes #3787.

halseth · 2019-11-07T18:49:12Z

Unit tests not yet pushed

lnwallet/channel.go

htlcswitch/interfaces.go

halseth · 2020-02-03T15:07:18Z

This is ready for review. (ignore failing travis for now, need to amend unit tests that have expectations about available balance)

Roasbeef

Completed my initial pass. I understand the high level issue at hand, and the proposed solution, but in my mind this doesn't yet match the implementation in this PR. However it's possible I'm missing some additional context, I've asked some clarifying questions in-line.

lnwallet/channel_test.go

lnwallet/channel.go

lnwallet/channel_test.go

lnwallet/channel.go

lnwallet/channel_test.go

lnwallet/channel.go

lnwallet/channel_test.go

lnwallet/channel.go

lnwallet/channel_test.go

joostjager · 2020-02-07T10:51:10Z

lnwallet/channel.go

+		// If our balance is high enough to pay for a non-dust HTLC and
+		// the resulting commitment fee, subtract this commitment fee
+		// to reflect what we have left for such payments.
+		case ourBalance >= htlcCommitFee+nonDustHtlcAmt:


Isn't there a gap here when ourBalance == htlcCommitFee+nonDustHtlcAmt - 1? That in that case we only subtract baseCommitFee and the remainder is still enough to create a non dust htlc?

joostjager · 2020-02-07T10:53:08Z

lnwallet/channel.go

 	// If we are the channel initiator, we must to subtract the commitment
 	// fee from our available balance.
 	if lc.channelState.IsInitiator {
-		ourBalance -= baseCommitFee
+		switch {


Shouldn't some kind of check for the local commitment transaction be added in here too? Similar to the earlier validations that were added to AddHtlc and ReceiveHtlc. So not only make sure we can afford adding it, but also that the remote can afford it if we are the non-initiator?

lnwallet/channel.go

joostjager · 2020-02-10T07:46:45Z

Tested PR and can confirm that I now get a single insufficient local balance failure instead of a never-ending loop.

lnwallet/channel.go

halseth · 2020-02-17T10:38:36Z

Pushed new update where channel reserve calculation is moved into availableBalance and we now report a 0 balance for the initiator if only the HTLC fee is left, to avoid getting into the unusable channel situation. PTAL.

joostjager · 2020-02-17T12:33:19Z

lnwallet/channel_test.go

+	bobChannel.channelState.RemoteChanCfg.ChanReserve = aliceMinReserve
+
+	// Now let Bob attempt to add an HTLC of 0.1 BTC. He has plenty of
+	// money, but Alice, which is the initiator, cannot afford any more


Well, plenty of money? Bob just has commitFee above his reserve?

He has 5 BTC. Don't know about you, but I think that's plenty of money :)

Alice also has plenty of money with that definition. I mean, it is about amount spendable?

Bob has 5 BTC on his end of the channel when it's created. Does that clear things up?

Expanded the comment. Hopefully more clear now.

lnwallet/channel_test.go

lnwallet/channel.go

lnwallet/channel_test.go

joostjager · 2020-02-17T13:15:52Z

lnwallet/channel_test.go

+	// sending yet another HTLC. In practice this can also happen if a fee
+	// update eats into Alice's balance.
+	htlcAmt = 1
+	sendHtlc(htlcAmt)


Hmm is it good that this is still possible if there is no amount sendable anymore?

It would be changing AddHTLC to not allow sending HTLCs even though there is balance left. IMO it is a nice upper layer heuristic to not attempt this by reporting this balance not being available. We can revisit if this becomes a hard protocol requirement, re lightning/bolts#728

It could lead to getting caught in the danger zone if there are concurrent payments. Both payments used the same reported availableBalance, which was enough at that point, but when executed the second payment pushes us below the threshold. If AddHTLC would fail, this wouldn't happen and a new path for the second payment could be searched for.

But this is arguably a rare condition, especially because we plan to do non-concurrent pathfinding for mpp. Maybe it is worth a comment near the AddHTLC implementation.

Btw, does this mean that you can still go below the threshold with SendToRoute?

SendToRoute will still eventually send an HTLC through the link, so none of these checks are bypassed.

SendToRoute will also be forwarded by the switch, and Bandwidth is called here:

lnd/htlcswitch/link.go

Line 2339 in 92b79f6

if amt > l.Bandwidth() {

lnwallet/channel.go

halseth · 2020-02-17T15:16:19Z

Won't that be equivalent to what we do here? (Only diff in which order we subtract)

…

On Mon, Feb 17, 2020, 16:00 Joost Jager ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In htlcswitch/link.go <#3691 (comment)>: > return 0 } - // Else the amount that is available to flow through the link at this - // point is the available balance minus the reserve amount we are - // required to keep as collateral. - return linkBandwidth - reserve + return channelBandwidth - overflowBandwidth Maybe you don't need to subtract len * htlcFee, because the htlcs can also all be send and settled one after the other. The htlcFee will be returned every time. I was thinking to first subtract the sum of the overflow queue and only then see that is left and compare that with htlcFee. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3691?email_source=notifications&email_token=AA4XLZF5B6ACJWN53QAYFC3RDKQ7NA5CNFSM4JKLXYNKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCVZA26A#discussion_r380229910>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4XLZBQHG7BIJUYUCJ7KUTRDKQ7NANCNFSM4JKLXYNA> .

joostjager

LGTM

Tested PR, works as expected. The initiator cannot get itself stuck anymore.

Will do a final pass after second reviewer review

Roasbeef

Only one modification request here (typo), the rest of the comments either add some commentary for future follow up work, or attempt to resolve lingering unresolved comments.

Ideally this is the last time we add additional heuristics or checks in this area w.r.t HTLC accounting, as things are really starting to stack up. Thankfully we have a pretty good commit trail and set of comments here, so it should be easy to re-asses our assumptions in the future if we need to.

In the end, there's some pretty fundamental fixes here, so that itself alone warrants moving forwarding with the fix. Albeit, imo the code in this area is starting to get a bit cluttered since we're now layering on heuristic after heuristics when the prior is found to be lacking, similar to a game of whack-a-mole.

TBH, I anticipated a number of issues (not these explicit ones of course) years ago when we start to draft the HTLC accounting logic in the spec. However then, the issues seemed more distant as our main goal was just to get something up and deployed that mostly worked in order to prove that this entire thing was even possible. Nevertheless, I think it has become apparent to us all at this point that we may require some more fundamental fixes here to just attempt to eliminate certain classes of these edge cases all together.

LGTM 🛸

Roasbeef · 2020-02-19T01:53:54Z

lnwallet/channel_test.go

+	bobChannel.channelState.RemoteChanCfg.ChanReserve = aliceMinReserve
+
+	// Now let Bob attempt to add an HTLC of 0.1 BTC. He has plenty of
+	// money, but Alice, which is the initiator, cannot afford any more


Bob has 5 BTC on his end of the channel when it's created. Does that clear things up?

htlcswitch/link.go

Roasbeef · 2020-02-19T02:29:24Z

lnwallet/channel.go

 	feePerKw := filteredView.feePerKw
-	baseCommitFee := lnwire.NewMSatFromSatoshis(
-		feePerKw.FeeForWeight(commitWeight),
+	htlcCommitFee := lnwire.NewMSatFromSatoshis(


Similar to the predictAdded field in validateCommitmentSanity, shouldn't we only conditionally add this weight if we're attempting to look ahead by one HTLC to see the expected balance after this HTLC is added.

Or is the assumption that this method is only used atm by the switch when it is attempting to add a new HTLC to the commitment transaction?

Non blocking, more of a side point, but if we do add this param, then we can use this to display the current balance of a channel in ListChannels and WalletBalance, if we opt to just mask the concept of a channel reserver all together. If we go this route, then at least the user's wallet will show zero satoshis, rather than showing a balance which can't actually be sent/received (though there're other implications).

Or is the assumption that this method is only used atm by the switch when it is attempting to add a new HTLC to the commitment transaction?

Yeah, that is the assumption, that it returns the "balance available for HTLCs". And that has to include the HTLC fee if we are the initiator.

Roasbeef · 2020-02-19T02:46:34Z

lnwallet/channel_test.go

+	// sending yet another HTLC. In practice this can also happen if a fee
+	// update eats into Alice's balance.
+	htlcAmt = 1
+	sendHtlc(htlcAmt)


SendToRoute will still eventually send an HTLC through the link, so none of these checks are bypassed.

add To ba able to validate the commitment sanity both for remote and local commitments, and at the same time predict both our and their add, we let validateCommitmentSanity take an extra payment descriptor to make this possible.

This commit adds a test that was previously not performed, namely that adding a HTLC would dip the remote initiator below its channel reserve.

This commit adds an extra validation step when adding HTLCs. Previously we would only validate the remote commitment resulting from adding an HTLC, which in most cases is enough. However, there are situations where the dustlimits are different, which could lead to the resulting remote commitment from adding the HTLC being valid, but not the local commitment. Now we also validate the local commitment. A test to trigger the case is added.

Since we want to handle the edge case where paying the HTLC fee would take the initiator below the reserve, we move the subtraction of the reserve into availableBalance where this calculation will be performed.

When we send non-dust HTLCs as the non-initiator, the remote party will have to pay the extra commitment fee. To account for this we figure out if they can afford paying this fee, if not we report that we only have balance available for dust HTLCs, since these HTLCs won't increase the commitment fee.

…alculation Since our HTLC must also be added to the remote commitment, we do the balance caluclation also from the remote chain perspective and report our minimum balance from the two commit views as our available balance.

halseth requested a review from joostjager November 7, 2019 18:48

joostjager reviewed Nov 8, 2019

View reviewed changes

lnwallet/channel.go Outdated Show resolved Hide resolved

htlcswitch/interfaces.go Outdated Show resolved Hide resolved

Roasbeef added enhancement Improvements to existing features / behaviour htlcswitch incomplete WIP PR, not fully finalized, but light review possible P3 might get fixed, nice to have labels Nov 9, 2019

halseth added this to the 0.9.1 milestone Jan 8, 2020

joostjager mentioned this pull request Jan 28, 2020

routing: multi-part mpp send #3967

Merged

halseth force-pushed the link-bandwitch-amt-aware branch 2 times, most recently from 4f01c31 to a5bd181 Compare February 3, 2020 13:51

halseth changed the title ~~Make link bandwidth amount aware~~ lnwallet: Make available balance HTLC fee aware Feb 3, 2020

halseth marked this pull request as ready for review February 3, 2020 15:06

halseth requested a review from Roasbeef as a code owner February 3, 2020 15:06

Roasbeef requested changes Feb 4, 2020

View reviewed changes

halseth force-pushed the link-bandwitch-amt-aware branch from a5bd181 to 3c9f52d Compare February 4, 2020 13:16

halseth requested a review from cfromknecht as a code owner February 4, 2020 13:16

halseth requested review from joostjager and Roasbeef February 5, 2020 15:25

joostjager reviewed Feb 7, 2020

View reviewed changes

halseth force-pushed the link-bandwitch-amt-aware branch from 6389d29 to 9ab0d82 Compare February 7, 2020 15:19

halseth force-pushed the link-bandwitch-amt-aware branch from 9ab0d82 to 8ad42df Compare February 10, 2020 12:35

halseth mentioned this pull request Feb 12, 2020

Fix TestNewBreachRetributionSkipsDustHtlcs incorrect state transtion #3998

Merged

joostjager reviewed Feb 14, 2020

View reviewed changes

lnwallet/channel.go Outdated Show resolved Hide resolved

joostjager reviewed Feb 14, 2020

View reviewed changes

lnwallet/channel.go Outdated Show resolved Hide resolved

halseth force-pushed the link-bandwitch-amt-aware branch from 8ad42df to 354fc27 Compare February 17, 2020 10:26

halseth requested a review from joostjager February 17, 2020 10:37

joostjager reviewed Feb 17, 2020

View reviewed changes

halseth force-pushed the link-bandwitch-amt-aware branch from 354fc27 to 27404d1 Compare February 17, 2020 14:57

halseth force-pushed the link-bandwitch-amt-aware branch 2 times, most recently from 65fa022 to 4a2bde1 Compare February 18, 2020 12:57

halseth requested a review from joostjager February 18, 2020 13:08

joostjager reviewed Feb 18, 2020

View reviewed changes

Roasbeef approved these changes Feb 19, 2020

View reviewed changes

halseth force-pushed the link-bandwitch-amt-aware branch from 4a2bde1 to dc38285 Compare February 19, 2020 10:05

halseth requested a review from joostjager February 19, 2020 10:08

joostjager approved these changes Feb 19, 2020

View reviewed changes

halseth added 9 commits February 19, 2020 12:27

lnwallet tests: add test for dipping remote below chan reserve

4ea822e

This commit adds a test that was previously not performed, namely that adding a HTLC would dip the remote initiator below its channel reserve.

lnwallet/channel: break up availableBalance

58dec10

link+lnwallet: move bandwidth channel reserve validation into channel

5e89d5b

Since we want to handle the edge case where paying the HTLC fee would take the initiator below the reserve, we move the subtraction of the reserve into availableBalance where this calculation will be performed.

lnwallet/channel: account for HTLC fee when reporting available balance

9ff79ae

lnwallet/channel: add TODO

3e5f2e4

halseth force-pushed the link-bandwitch-amt-aware branch from dc38285 to 3e5f2e4 Compare February 19, 2020 11:27

halseth merged commit 5d5069c into lightningnetwork:master Feb 20, 2020

Roasbeef mentioned this pull request Aug 23, 2023

[bug]: commitment transaction dips peer below chan reserve: internal error causing eclair to FC #7657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lnwallet: Make available balance HTLC fee aware #3691

lnwallet: Make available balance HTLC fee aware #3691

halseth commented Nov 7, 2019 •

edited

Loading

halseth commented Nov 7, 2019

halseth commented Feb 3, 2020

Roasbeef left a comment

joostjager Feb 7, 2020

joostjager Feb 7, 2020

joostjager commented Feb 10, 2020

halseth commented Feb 17, 2020

joostjager Feb 17, 2020

halseth Feb 18, 2020

joostjager Feb 18, 2020

Roasbeef Feb 19, 2020

halseth Feb 19, 2020

joostjager Feb 17, 2020

halseth Feb 18, 2020

joostjager Feb 18, 2020

joostjager Feb 18, 2020

Roasbeef Feb 19, 2020

halseth Feb 19, 2020

halseth commented Feb 17, 2020 via email

joostjager left a comment

Roasbeef left a comment

Roasbeef Feb 19, 2020

Roasbeef Feb 19, 2020

Roasbeef Feb 19, 2020

Roasbeef Feb 19, 2020

halseth Feb 19, 2020

Roasbeef Feb 19, 2020

lnwallet: Make available balance HTLC fee aware #3691

lnwallet: Make available balance HTLC fee aware #3691

Conversation

halseth commented Nov 7, 2019 • edited Loading

halseth commented Nov 7, 2019

halseth commented Feb 3, 2020

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostjager commented Feb 10, 2020

halseth commented Feb 17, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halseth commented Feb 17, 2020 via email

joostjager left a 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

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

halseth commented Nov 7, 2019 •

edited

Loading