Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

htlcswitch: avoid proposing fee updates exceeding max fee allowed #3401

Merged

Conversation

@wpaulino
Copy link
Collaborator

commented Aug 14, 2019

In this PR, we begin to enforce a maximum channel commitment fee for channel initiators when attempting to update their commitment fee. Now, if the new commitment fee happens to exceed their maximum, then the fee update won't be proposed and the channel will remain with its previous commitment fee. Note that this won't cause the channel to be closed, but will likely be done at a later time.

A default of up to 50% of the channel initiator's balance is enforced for the maximum channel commitment fee. It can be modified through the --max-channel-fee-allocation CLI flag.

Fixes #3006.

@wpaulino wpaulino removed the request for review from halseth Aug 14, 2019
@wpaulino wpaulino added this to the 0.8.0 milestone Aug 14, 2019
switch {
// The new fee should not exceed our maximum fee allowed.
case newFee > maxFee:
return fmt.Errorf("cannot apply fee_update=%v sat/kw, new fee "+

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 16, 2019

Member

I think we may actually just need to cooperative close at this point, as otherwise the channel won't be able to proceed as is, since we don't (but have some cut out design space for) have the ability to mark rejected updates explicitly. If we return an error here, we won't apply the fee, and the channel will be force closed by the receiving party.

What I had in mind with this PR was that we would refrain from triggering a new fee update if thee sampled fee update was above some threshold. This results in no force closures as we simply regulate our behavior on the end of the initiator. Within the protocol we're missing the ability for the responder to hav some say w.r.t what the final fee is, as they also want to ensure that they're able to close the channel in timely manner, and also this fee serves as the upper bound of what the fee can be for the cooperative closure transaction.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Aug 19, 2019

Author Collaborator

I think we may actually just need to cooperative close at this point, as otherwise the channel won't be able to proceed as is, since we don't (but have some cut out design space for) have the ability to mark rejected updates explicitly.

From a previous discussion offline, it seemed like we indeed want to do this but later down the road?

What I had in mind with this PR was that we would refrain from triggering a new fee update if thee sampled fee update was above some threshold. This results in no force closures as we simply regulate our behavior on the end of the initiator.

Wasn't aware of the force close issue. The fee update is now not proposed if it exceeds the max.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 20, 2019

Member

From a previous discussion offline, it seemed like we indeed want to do this but later down the road?

Yep, the goal of this is just to ensure we don't bleed away our entire channel. We can possibly add a flag in this PR to optionally close them out in order to avoid some fee stampede that results in a large number of channels being closed.

@wpaulino wpaulino force-pushed the wpaulino:channel-initiator-max-fee branch from 40ee38e to f9c5049 Aug 19, 2019
@wpaulino wpaulino changed the title htlcswitch+lnwallet: enforce max channel commitment fee for initiators htlcswitch: avoid proposing fee updates exceeding max fee allowed Aug 19, 2019
@wpaulino wpaulino force-pushed the wpaulino:channel-initiator-max-fee branch from f9c5049 to 2c83c14 Aug 20, 2019
@wpaulino wpaulino requested a review from Roasbeef Aug 20, 2019
Copy link
Member

left a comment

No blocking comments at this point. Only now curious to see if this direction is in line with expectations from additional reviewers. With this PR, we'll stop bleeding all our fees into channels, but ofc there's still a danger zone there which something like moving to the proposed anchor outputs will address.

func (lc *LightningChannel) CalcFee(feeRate SatPerKWeight) btcutil.Amount {
// CalcFee returns the commitment fee to use for the given fee rate
// (fee-per-kw).
func CalcFee(feeRate SatPerKWeight) btcutil.Amount {

This comment has been minimized.

Copy link
@Roasbeef

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 20, 2019

Collaborator

Can be an isolated commit.

// transaction.
commitFeeRate := l.channel.CommitFeeRate()
balance := l.channel.AvailableBalance().ToSatoshis() +
lnwallet.CalcFee(commitFeeRate)

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 20, 2019

Member

I think most of this logic can be lifted in to shouldAdjustCommitFe (first step may be to make it a method).

This comment has been minimized.

Copy link
@wpaulino

wpaulino Aug 21, 2019

Author Collaborator

I opted not to do so initially because of the additional test changes that it would require, though I can update the PR if desired.

@Roasbeef Roasbeef requested review from joostjager and removed request for cfromknecht Aug 20, 2019
Copy link
Collaborator

left a comment

With the new commitment format (always only minimal tx fee), will the link remain responsible for deciding when to coop close the channel because of fees? If the link is offline, the fees also need to be watched.

As I understand it from the issue description, this pr is a preparation for the new commitment format. But isn't it better to hold off until that format actually lands and think about max_fee in the context of all required changes?

The current pr scope doesn't include the non-initiator closing the channel if fees are not sufficient anymore. Aren't you worried that this pr will cause more unconfirmed commitment txes and users not knowing what to do about it? Or unexpected closing fees because the non-initiator needs to cpfp their outputs now?

func (lc *LightningChannel) CalcFee(feeRate SatPerKWeight) btcutil.Amount {
// CalcFee returns the commitment fee to use for the given fee rate
// (fee-per-kw).
func CalcFee(feeRate SatPerKWeight) btcutil.Amount {

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 20, 2019

Collaborator

Can be an isolated commit.

switch {
// The network fee should not exceed our maximum fee allowed.
case lnwallet.CalcFee(netFee) > maxFee:
return false

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 20, 2019

Collaborator

I can see that if the fee estimate goes up, we stop adjusting and pick it up again when the fees come down.

But suppose the commit fee does not exceed the max, but then our balance goes down. We should then update the fee to our lowered maxFee, but this clause leaves the fee unchanged.

This comment has been minimized.

Copy link
@wpaulino

wpaulino Aug 21, 2019

Author Collaborator

maxFee depends on our balance and is recomputed before every shouldAdjustCommitFee call, so this should already be handled if our balance decreases?

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 21, 2019

Collaborator

I was thinking of this scenario:

  • netFee is constant at 1000
  • maxFeeAllocation is 50%
  • AvailableBalance() is 10000 sats
  • current commitFee is 1000

All is good, as we are below 50%.

  • Then we spend 9500 sats, AvailableBalance() is 500 sats
  • balance = AvailableBalance() + current_commit_fee = 1500 sats
  • maxFee = 1500 * 50% = 750 sats

I'd say the commit fee should now be updated to 750 sats, but in the code above you return early because 1000 > 750?

This comment has been minimized.

Copy link
@wpaulino

wpaulino Aug 22, 2019

Author Collaborator

Ah, good point. The scope of the issue to me seemed just to avoid proposing the fee updates instead of limiting it to the max. Though I guess limiting it to the max would be beneficial as it puts us closer to the network fee if our current fee is way below it.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 23, 2019

Member

FWIW, in your example @joostjager, we wouldn't be able to have an available balance of 500 satoshis, as that's below our reserve.

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 23, 2019

Member

With that aside, I think example stands, but it's out of the scope of the initial target here (just to limit how high we'll go).

This comment has been minimized.

Copy link
@joostjager

joostjager Aug 26, 2019

Collaborator

Why is it out of scope? Isn't it better to keep sending updates, but cap them to the max?

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 28, 2019

Member

Depends on what the scope is here, by scope I'm referring to the original posted issue. On the other hand, this can be seen as a new bug. I think it can be resolved by setting the fee to min(maxFee, commitFee) if I understand your example correctly. So then if this value matches the current fee, no change is triggered, otherwise we use that new value.

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 2, 2019

Collaborator

Yes, that is what I thought indeed, min(maxFee, commitFee).

@wpaulino wpaulino requested review from Roasbeef and joostjager Aug 23, 2019
@Roasbeef

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

With the new commitment format (always only minimal tx fee), will the link remain responsible for deciding when to coop close the channel because of fees?

With the current design of the system, yes the link would likely still be responsible for this. However with the new format, a new upper limit in the coop close fee would need to be agreed upon (atm the commitment fee is the upper limit). As far as deployment, I think we'll end up going with a multi-stage approach, where the first deployment only adds the anchor outputs, but leaves the current update_fee semantics in place.

If the link is offline, the fees also need to be watched.

That would depend on how proactive we want to be. We may not want to close due to rising fees, but perhaps due to lack of uptime by the peer instead. When we do choose to close, we'll still eventually have the opportunity to add more/less fees if we want. It's also the case that since this is the latest state, if no HTLCs are present, then we may want to be more lax about our confirmation expectations. Once our transaction engine is complete, it can also use that pending close output as an input for other transactions such as a channel funding or regular on-chain transaction. We'll then essentially be doing a "demand based" adhoc CPFP whenever we need those funds as inputs.

As I understand it from the issue description, this pr is a preparation for the new commitment format. But isn't it better to hold off until that format actually lands and think about max_fee in the context of all required changes?

It's only meant to address complaints by users that their 50k satoshis channel has 40k allocated to fees. Moving to the new format in a safe manner will also require us to wait for new bitcoin full node software to be released, and a majority of miners upgrading to this new software.

You mentioned offline that by clamping the fee here we may exacerbate waiting issues in times of high fees, however what we should be doing in those times in for safety reasons is dynamically modifying our CLTV delta across our channels. We can regulate this value to give us more/less time to get commitments w/ HTLCs in the chain depending on the current fee climate.

The current pr scope doesn't include the non-initiator closing the channel if fees are not sufficient anymore. Aren't you worried that this pr will cause more unconfirmed commitment txes and users not knowing what to do about it? Or unexpected closing fees because the non-initiator needs to cpfp their outputs now?

We're worried mostly about mass closure events across the network if we have this be the default in the next release. Even then, if the logic is very twitchy, then responders to a channel funding may end up preemptively closing their channels due to a short lived burst in estimate fees (for example Bitmex doing their daily withdraws). We deploy any automated closing logic with caution, as we've seen in the earlier days that it can cause users unwarranted agony (for example back when CL would force close channels due to a fee disagreement).

Copy link
Member

left a comment

Running this on a few of my nodes now.

switch {
// The network fee should not exceed our maximum fee allowed.
case lnwallet.CalcFee(netFee) > maxFee:
return false

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Aug 23, 2019

Member

With that aside, I think example stands, but it's out of the scope of the initial target here (just to limit how high we'll go).

@wpaulino wpaulino force-pushed the wpaulino:channel-initiator-max-fee branch 2 times, most recently from b02689f to b8e0709 Aug 28, 2019
config.go Outdated
// Ensure a valid max channel fee allocation was set.
if cfg.MaxChannelFeeAllocation <= 0 || cfg.MaxChannelFeeAllocation > 1 {
return nil, fmt.Errorf("invalid max channel fee allocation: "+
"%v, must be within [0.1, 1]",

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 2, 2019

Collaborator

Range in error doesn't match range in if condition. Also, because range is used three times (twice here and once in config), extract to constants?

This comment has been minimized.

Copy link
@wpaulino

wpaulino Sep 3, 2019

Author Collaborator

Fixed the range. Don't think we can use the constants in the description of the flag though?

This comment has been minimized.

Copy link
@joostjager

joostjager Sep 4, 2019

Collaborator

Oh yes, that is a tag. I don't think so indeed. But the range in the tag is still [0.1, 1] instead of [0, 1]. Also the tag mentions percentage which suggests it is in the 0-100 range?

lnwallet/channel.go Show resolved Hide resolved
htlcswitch/link_test.go Show resolved Hide resolved
htlcswitch/link_test.go Outdated Show resolved Hide resolved
htlcswitch/link_test.go Show resolved Hide resolved
htlcswitch/link.go Show resolved Hide resolved
htlcswitch/link_test.go Show resolved Hide resolved
wpaulino added 3 commits Aug 23, 2019
…closure
In this commit, we begin to enforce a maximum channel commitment fee for
channel initiators when attempting to update their commitment fee. Now,
if the new commitment fee happens to exceed their maximum, then a fee
update of the maximum fee allocation will be proposed instead if needed.

A default of up to 50% of the channel initiator's balance is enforced
for the maximum channel commitment fee. It can be modified through the
`--max-channel-fee-allocation` CLI flag.
@wpaulino wpaulino force-pushed the wpaulino:channel-initiator-max-fee branch from b8e0709 to d8dd6b3 Sep 3, 2019
@wpaulino wpaulino requested review from Roasbeef and joostjager Sep 4, 2019
Copy link
Member

left a comment

LGTM 🐎

@Roasbeef Roasbeef merged commit 866867a into lightningnetwork:master Sep 5, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 61.107%
Details
@wpaulino wpaulino deleted the wpaulino:channel-initiator-max-fee branch Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.