lnwallet: Introduce a fee buffer.

[ziggie1984]

Fixes #7657/#7721

This change introduces a fee buffer which accounts for a 100% fee rate increase plus an additional htlc output on the commitment transaction at the current fee rate. This equals the behaviour what CLN does currently and is spec compliant IMO. It's a conservative approach because we are not taking any trimmed Htlcs at higher fee rates into account, but this has a positive side effect when fee rates decrease and dust htlcs become normal outputs (only relevant for non-anchor channels because they have to pay for the second-level covenant transaction).

Still need to write tests in the coming days. Change seems small at first glance but lets wait until tests are ready.

EDIT: Before updating all the unit tests and thinking of itests, I would be happy to receive first feedback on this approach (especially the additonal argument enforceFeeBuffer extra argument, maybe there is a cleaner approach?).

It is important to underline that the fee buffer is only a BOLT02 requirement, all the requirements regarding BOLT03 where the testcase go below the imaginable fee buffer still need hold.

This PR takes the recommendation from BOTL02 and calculates the fee buffer like the following:

feeBuffer = currentFeeRate *2 * commitweight + additionalHTLCWeigth * currentFeeRate *2

This is the same behaviour as eclair and cln follow. Subtle difference to eclair is that eclair accounts for the potential trimmed htlcs at higher fee rates whereas we do not account for this trimming (CLN does not as well). This is only relevant for non-anchor channels because anchor channels have zerofee htlc second-level covenant transactions

eclair: https://github.com/ACINQ/eclair/blob/97c9b579f319952c215f2a1930357de25a0640ab/eclair-core/src/main/scala/fr/acinq/eclair/channel/Commitments.scala#L135-L151

cln: https://github.com/ElementsProject/lightning/blob/62ddf84b4f15a460ed5a8f72b313998a83eefe19/channeld/full_channel.c#L565-L569

So there are two occasions we have to differentiate when we want to enforce the fee buffer and when not.

1. When sending an payment or forwarding a payment we will enforce this fee buffer.
2. When evaluating a new FeeUpdate msg to increase the fee rate for a channel commitment we will not (only account for the additional HTLC a peer might add).

In addition what do you think of a new field when showing the channel: CanSend, which show the exact max amount one can send currently ?

[Roasbeef]

I wonder if we can write a fuzzer that it able to trigger the original edge case, which would then add confidence that this properly resolves the issue.

[DerEwige]

@Roasbeef

I encounter this edge case about once per week on my live node.
The reason why this happens so much for me, is that I generate a lot of htlcs.
On a single channel it is usually about 1 htlc every 2-3s (if this channel is a source to me)

I stopped accepting channels from LND nodes that are sources to me, becaue if I accept those channels they end up in a FC usually in less than a week.

So if you wanna trigger this case.
Just empty the opner LND side untill you have only a few sats left and then spamm htlcs from both sides (and let them fail after a short time).
You should run into this edge case within a few minutes

[ziggie1984]

I wonder if we can write a fuzzer that it able to trigger the original edge case, which would then add confidence that this properly resolves the issue.

I will definitely look into ways reproducing this error (fuzzing,itesting) and see whether this PR fixes the issues, good point.

[Crypt-iQ]

I wonder if we can write a fuzzer that it able to trigger the original edge case, which would then add confidence that this properly resolves the issue.

I will definitely look into ways reproducing this error (fuzzing,itesting) and see whether this PR fixes the issues, good point.

It would probably be easiest to write a unit test in channel_test.go using these steps #7657 (comment)

[ziggie1984]

So I reworked this PR a bit from the initial version, so I generalized the part where we account for the CommitmentFee of the Channel introducing a new BufferType. So now we do have the feebuffer logic at one place only. Tho not super happy with the design therefore looking for some ideas of you guys how to make this thing better. So looking for an ApproachAck so that I can make it ready for the final release.

I am actually not sure if we really need to FuzzTest this scenario, if so I would love to get some ideas from the pros here 😀.
I added the relevant unit tests which I think follow the case which eugene pointed out in his comment.

I left some TODO's in this PR which I want you to focus on while reviewing, especially the proposal in #8249 where we would allow the peer to put us below our reserve if we are the opener of the channel. (only relevant if the channel is already stuck when running this new code, so I guess we need to think about it?)

So this PR is not final yet but ready to receive a Design Review.

[yyforyongyu]

Very important work! Concept ACK. Some notes may be helpful for other reviewers, the core change happens in applyCommitFee, which is called by,

`validateCommitmentSanity`, and used in various methods:
	- `SignNextCommitment` - `NoBuffer`
	- `ReceiveNewCommitment` - `NoBuffer`
	- `ReceiveHTLC` - `NoBuffer`
	- `validateAddHtlc`
		- `AddHTLC` - options - `FeeBuffer`
		- `MayAddOutgoingHtlc` - `FeeBuffer`

`availableCommitmentBalance`, and used in various methods:
	- `availableBalance`
		- `validateFeeRate` - `AdditionalHtlc`
			- `UpdateFee` which sends `update_fee` msg
		- `AvailableBalance` - `FeeBuffer` - used in link to decide channel bandwith
		- `MaxFeeRate` - `AdditionalHtlc` TODO(yy): downgrade it to private method
			- `IdealCommitFeeRate` - used in link to decide new commit fee rate

To summarize, when we add a htlc, we enforce a fee buffer. When we receive an HTLC or commit_sig or sign a new commit_sig, we don't check for fee buffer. When we send a update_fee message, we only account one more htlc as our fee buffer.

My questions and suggestions,

• not sure about AdditionalHtlc's use case. I think it makes sense to only account for one extra HTLC when we know we will do a coop close, to make sure there's enough budget to pay the final tx fees. Not sure tho, if there another round of commitment dance, then this budget may not be enough? I guess in that case we can always fail it if it cannot meet the fee buffer?

• maybe we don't change validateCommitmentSanity, use FeeBuffer and add code/method to validate fee buffer in validateAddHtlc directly, seems to give us less code change.

[yyforyongyu]

Let's focus on adding the fee buffer in this PR and add a dedicated PR to fix the chan reserve thing?

[ziggie1984]

maybe we don't change validateCommitmentSanity, use FeeBuffer and add code/method to validate fee buffer in validateAddHtlc directly, seems to give us less code change.

I think this might be a good idea, will try to code this variant and see if I can make it more clean 👍

EDIT: So I tried this solution as well but I think this makes the code change more narrow (we don't have to add the NoBuffer argument for all other calls of validateCommitmentSanity but instead introduces a lot of duplicate code when calculating the balances which we then need to compare. But open for suggesstions for sure.

not sure about AdditionalHtlc's use case. I think it makes sense to only account for one extra HTLC when we know we will do a coop close, to make sure there's enough budget to pay the final tx fees. Not sure tho, if there another round of commitment dance, then this budget may not be enough? I guess in that case we can always fail it if it cannot meet the fee buffer?

hmm I think the AdditionalHtlc makes sense when we handle the calculation of the MaxFeeRate, because the maxAllocation can be 1 and if fees spiked extremely we could end up in an unusable channel (only dust would be sendable)? I haven't thought about keeping an extra buffer for the coop close, but I think the coop close should not negotiate fees which are higher than the current commitment feerate, because that would reveal that the commitment feerate is too low and should be updated in the first place? But maybe I am missing something here.

[yyforyongyu]

Looking good. Still need to do another round to review the tests. Meanwhile sharing some notes, I think it's helpful to unify the concepts that are used here re the value stored in a channel,

┌─────────────────────┐
│ ┌─────────────────┐ │
│ │                 │ │
│ │     routing     │ │
│ │                 │ │
│ │    bandwidth    │ │
│ │                 │ │
│ ├─────────────────┤ │
│ │    fee buffer   │ │
│ ├─────────────────┤ │
│ │ channel reserve │ │
│ └─────────────────┘ │
│                     │
│   channel balance   │
│                     │
├─────────────────────┤
│     commit fees     │
└─────────────────────┘

commit fees is the onchain fee, and channel balance is what the user will receive once the channel is closed, which is made of three parts,

• channel reserve, the security deposit.
• fee buffer, a dedicated amount that can be used to deal with fee spikes, and we expect commit fees <= fee buffer.
• routing bandwidth, the value that can be used for routing, ie, add htlcs.

If we can agree on these concepts we could make future PRs to apply them in our code, which can make things easier.

In a following PR I think we should make the fee buffer configurable, something like currentFeeRate * BufferFactor * (commitweight + additionalHTLCWeigth * NumBufferedHTLCs)

[yyforyongyu]

Very nice👍 Think we can finally fix this issue, also great unit tests! No major comments, only questions and nits. Haven't finished link_test.go and itest yet, but I think this is veeeerry close!

[yyforyongyu]

Q: for anchor channels do we still need this buffer? My understanding for non-anchor channel is we build a fee buffer based on the commit fee, so I guess for anchor channels we'd build it based on relaying fee?

[ziggie1984]

Hmm, so I think we should always look at the current fee rate of the commitment transaction and go from there? As you correctly pointed out, these should be different for anchor and non-anchor channels (which currently they are but not in a smart way - for anchors its just hardcapped but should rather be at least floored at the min_relay fee of the avarage peers.) I agree that we should add a floor for the buffer in case we have very low feerates but I would do this in a future PR.

[yyforyongyu]

I guess this doesn't fix #8315 since if our peer is using an older LND, we'd still have this issue. If we could define a different behavior based on our channel balance, such that,

• when it dips below channel reserve, fail the link and force close
• when it dips below fee buffer, fail the ADD but keep the link

Then this could be resolved. However, this would require a larger change(and specs change?) so I think we should do it in a future PR.

[ziggie1984]

Yeah it will not fix 8315 until we carve-out the cases where we acutally allow the peer dip us below our reserve (outlined in the TODO).

when it dips below fee buffer, fail the ADD but keep the link

I think this is already in place when this PR is merged, we do not account for the fee buffer when receiving, and when sending we can easily roll back the ADD.

[yyforyongyu]

I guess the reason we are not using FeeBuffer here is because for our balance this HTLC will increase it and for their balance we don't ever check it in validateCommitmentSanity.

[ziggie1984]

Rephrased the comment, so the main reason we do not use it here is for the concurrency problem we try to solve:

	// We do not enforce the FeeBuffer here because one of the reasons it
	// was introduced is to protect against asynchronous sending of htlcs so
	// we use it here. The current lightning protocol does not allow to
	// reject ADDs already sent by the peer.

[ziggie1984]

The point you are making, that we should look at whether the htlc ADDs balance to our local will be implemented in a future PR, this relates to the "allow dipping the peer below the reserve" question.

[Crypt-iQ]

LGTM besides nits 🌵 great work on this

[yyforyongyu]

LGTM⛵️ Thank you for all the effort, and, looking forward to the followup PR on fixing the dip-below-reserve-error on the remote side!

[C-Otto]

nit: typo