-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[anchors] HTLC update fee clamp #4795
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on board with the fee leak heuristic, it's pretty simple and rests on the existing assumptions surrounding the reserve value itself. However, I'm worried that this can cripple smaller channels that regularly make payments (or forward payments) that are above the local dust limit. Left some thought in-line where applicable.
reserve = lc.channelState.RemoteChanCfg.ChanReserve | ||
} | ||
|
||
// TODO(halseth): this limits the number of HTLCs that can be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, do you have some rough figures for the impact here? I wonder if we should expose this as a config option to avoid potentially obstructing certain services/use-cases over night with the update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean an option to skip the siphon check? That's not a bad idea..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that a problem with having the option to skip the fee leak check only works if both parties skip it. Otherwise the receiver will close the channel if the sender has disabled it and violates the invariant.
@@ -6723,6 +6736,24 @@ func (lc *LightningChannel) UpdateFee(feePerKw chainfee.SatPerKWeight) error { | |||
EntryType: FeeUpdate, | |||
} | |||
|
|||
// Make sure updating the fee won't violate any of the constraints we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially move this check into validateFeeRate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it most natural to do it here, similar to other methods validating updates, and since we need the paymentDescriptor
// We want to make sure we won't try to add an HTLC that will violate | ||
// the fee siphoning invariant. So if we are adding an HTLC on our | ||
// commitment, the HTLC timeout fee shouldn't take the total fees that | ||
// can be siphoned above our reserve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment re config option observance applies here as well.
lc.channelState.ChanType, feePerKw, | ||
) | ||
if feeSiphon+htlcTimeoutFee > lc.channelState.LocalChanCfg.ChanReserve { | ||
return 0, commitWeight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, this is nice in that it'll just show up as us having no bandwidth to service a new payment from the PoV of HTLC forwarding. However users may get confused when their payment fails even though the posted channel balance should allow it....🤔
lnwallet/channel.go
Outdated
// together with small channel reserves. Should move to a channel type | ||
// where the committed fees for HTLC transactions are zero (only BYOF) | ||
// or zero for both commitment+HTLCs (needs package relay). | ||
if !lc.skipFeeSiphonCheck && feeSiphon > reserve { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll also cause "silent failures" from the PoV of receivers with this clamp code connected to early adopter nodes that may already be running the existing anchor logic in the wild. However, I guess we can assume that this pairing will be rare in practice, since this upcoming release will be the first one that "enables" anchors network wide from the PoV of lnd
.
|
||
// For anchor channels cap the initial commit fee rate at our defined | ||
// maximum. | ||
if commitType == lnwallet.CommitmentTypeAnchors && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one other failure mode of this that we'll need to consider, is that until the second-level HTLCs are 1 sat/byte each, then rising fees can end up soft borking a channel until the fee wave subsides. However this is already more or less possible today if the fee estimate rises to a point that lnd
considered excessive, as even before that most of the initiator's balance ill have been bled over to fees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is only the case if HTLCs regularly stagnate on the commitment transaction.
One other more common impact of this change would be reducing the efficacy of MPP for smaller channels since at most (for all active concurrent payments), they'll be able to use only a handful of slots. It's difficult to try and have a better "risk assessment" based on the size of the channel itself, as then we'd also need to factor in exactly how the node is used as well.
// commitment transaction confirmed by the next few blocks (conf target | ||
// of 3). We target the near blocks here to ensure that we'll be able | ||
// to execute a timely unilateral channel closure if needed. | ||
commitFeePerKw, err := f.cfg.FeeEstimator.EstimateFeePerKW(3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're here, maybe also reduce this to 6-18 confs? As lately the 3 block estimate can be rather high
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean for all channel types, or only for anchors?
If it is for the general case, I would prefer to do it in a separate PR. Maybe even make it a config option?
Yes, this can definitely cripple smaller channels, however it will only be for anchor channels, so we can expect no existing channels to be affected A way to mitigate this before zero-fee 2nd level transactions, is to require channels to be of a certain size, or decrease the default max fee rate even more. We can do a sanity check at channel opening, something like |
3efe73f
to
abb1a38
Compare
So thought about this a bit more, and I don't think we need it after all given:
|
OK so I was wrong with that scenario above, in that ofc the fee for the HTLC output on the commitment is paid by the initiator, but the fees for the second level are taken from the HTLC value itself. |
This ensures we won't try to send an HTLC if that would violate the fee leak check. We also check that this holds before we send an update_fee.
This commit caps the update fee the initiator will send when the anchors channel type is used. We do not limit anything on the receiver side. This keeps us spec compatible, while defaulting to a reasonable feerate if initiator is lnd. Receiver side we will allow any fee and add updates as long as the fee siphon invariant is not violated. Defaults to 10 sat/vbyte. This will make give HTLC a timeout fee of 666 * 2500 / 1000 = 1665 sats An example: For a channel size of 0.01 BTC = 1,000,000 sats and a channel reserve of 1%, this leaves room for (1,000,000 / 100) / 1665 = 6 HTLCs. Bigger channels, larger reserves, or lower fee rates will open up for more room.
abb1a38
to
a108643
Compare
Mitigation for anchor channel attack brought up by @ariard in https://lists.linuxfoundation.org/pipermail/lightning-dev/2020-September/002796.html
We check that the maximum HTLC timeout fees paid (and can potentially be siphoned off as described in the attack), is always less than the channel reserve of the party. This should ensure that whatever state the channel currently is in, the party won't have an incentive to breach using this "siphoned state", since it can gain at most the channel reserve.
There is a case where the party still hasn't received a balance above the channel reserve (this is the case if the party is channel responder). But we can note that these "HTLC timeout fees" always comes from the offering party's balance, and the party cannot offer HTLCs as long as the balance is below the channel reserve.
With this change a party won't attempt to send an
update_add
orupdate_fee
that would violate this check on the receiver side.Since this new condition makes the channel close to unusable in certain fee conditions (small reserve, high fee rate, many HTLCs), we add a new option
--max-anchor-commit-feerate
that defaults to 10 sat/vbyte. This ensures the initiator won't ever try to send a fee update above this.Since for anchor channels the important thing is to get the channel to propagate, we could possibly default this to an even lower value.