Limit FeeRate change for the UpdateFee msg to prevent sharp changes #7805
Conversation
ziggie1984
force-pushed
the
updatefee-limiter
branch
2 times, most recently
from
3981584 to
7a4d27f
Compare
|
|
ziggie1984
force-pushed
the
updatefee-limiter
branch
5 times, most recently
from
27cb314 to
00ddb22
Compare
|
Will this PR fix stuck channels or will it only prevent channels from entering a stuck state in the future? |
It cannot undo the feeupdate which happend in the past (your case currently) tho it will prevent low feeupdates in the future.
|
lnwallet/channel.go
Outdated
| maxFee := float64(baseBalance) * maxAllocation | ||
| maxFee = math.Max(maxFee, float64(oldFee)) |
There was a problem hiding this comment.
A bit confused here - so this means our fee rate can only go up not down? And if we don't have this local balance, how can we cover the fees here?
There was a problem hiding this comment.
exactly we do not want to lower the max_feerate lower than the old_fee, because this could lead to a situation where we go all the way to 253 sats/kw, we want to decrease the feerate here: https://github.com/lightningnetwork/lnd/blob/master/lnwallet/channel.go#L8456-L8458, basically we want to either have a 3 block sample_feerate which is lower than the old_feerate before we allow a lower feeupdate, otherwise it makes not real sense to lower the MaxFeeRate.
There was a problem hiding this comment.
In that case I think we should compare it against 3 block sample feerate instead of old feerate, otherwise if the fee spikes for a while then goes down, we'd still use the higher feerate as the max feerate although the current mempool is less congested. Plus this would bypass the value maxAllocation configured by the users?
There was a problem hiding this comment.
Hmm not sure if I understand the detail of your proposal could you elaborate ?
I think binding the maxAllocation to our current amount we have on our side might cause this strange logic problem.
What about refering the maxAllocation to the ChannelCapacity, I think that makes more sense because as a channelopener you will have most of the balance on your side anyways at the beginning and are willing to pay this max amount anyways. Now when the channel becomes depeleted the maxFeeRate is always (localbalance+oldfee) but if we have a lot of balance on our side it would be capped at the (chancapacity *MaxAllocation) ?
There was a problem hiding this comment.
We would be protected against big feespikes, not allocating more that our initial willingess to pay, but if the channel is depleted we make sure we do not decrease our MaxFee the more our channel gets depleted hence reducing the local fee of the commitment all the time, that also not fair to the peer imo which now has all the balance but a crappy feerate.
There was a problem hiding this comment.
What about refering the maxAllocation to the ChannelCapacity
That's a very interesting proposal, plus it seems like to be the original definition anyway,
The maximum percentage of total funds that can be allocated to a channel's commitment fee. This only applies for the initiator of the channel. Valid values are within [0.1, 1]. (default: 0.5)
However, I think MaxFeeRate really means MaxAffordableFeeRate, which is determined by 1) the capacity, which is routing bandwidth + fee buffer + commit fee and 2) the max fee the user is willing to pay, which is specified by MaxAllocation. If we follow this strictly, we can make sure that, when necessary, there's enough channel balance, so the max fee can be deducted from it to pay the onchain fee.
The current approach doesn't guarantee there's always enough balance to cover the max fee.
After a second thought I think this is the right approach as oldFee(or the commit fee) is always there already, so it makes sense to cap at this value at least.
There was a problem hiding this comment.
Yes I think we can go with the current approach, but I think the FeeRate for the Commitment Tx is really not an easy to solve problem. So I think making sure that the Commitment has always the Relay Fee of its peer is a very important PR which should be prioritized.
ziggie1984
force-pushed
the
updatefee-limiter
branch
2 times, most recently
from
32a2b26 to
500294f
Compare
ziggie1984
force-pushed
the
updatefee-limiter
branch
from
500294f to
79314a9
Compare
|
Suffered from this behaviour on my local node as well now, I think this should be addressed asap, Channel is unusable now until we reach lower feelimits. |
There was a problem hiding this comment.
I think if we have #8096 we don't need this anymore since we would then have a fee buffer?
lnwallet/channel.go
Outdated
| maxFee := float64(baseBalance) * maxAllocation | ||
| maxFee = math.Max(maxFee, float64(oldFee)) |
There was a problem hiding this comment.
In that case I think we should compare it against 3 block sample feerate instead of old feerate, otherwise if the fee spikes for a while then goes down, we'd still use the higher feerate as the max feerate although the current mempool is less congested. Plus this would bypass the value maxAllocation configured by the users?
We would still need this, because the evaluation of the fee (we are willing to pay) would decrease over time and because the buffer is tied to the feerate we would, because of a feerate decrease, decrease the buffer => our channel would get drained more => our local balance shrinks again => the fee for the commitment would shrink as well in the next step => potential worst case fee-floor |
ziggie1984
force-pushed
the
updatefee-limiter
branch
from
79314a9 to
3fa392e
Compare
ziggie1984
force-pushed
the
updatefee-limiter
branch
from
3fa392e to
3eb8e87
Compare
yyforyongyu
added
fees
ziggie1984
force-pushed
the
updatefee-limiter
branch
from
14937cd to
2bc3e35
Compare
|
Important Auto Review SkippedAuto reviews are limited to the following labels: llm-review. Please add one of these labels to enable auto reviews. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThis update addresses several key aspects of fee management and policy updates in the Lightning Network Daemon (LND), including adjustments to fee estimation, channel fee policy handling, and the addition of Taproot witness types to RPC calls. It improves the reliability and specification compliance of fee handling during channel openings and commitment transactions, enhancing overall network stability and user experience. Changes
Assessment against linked issues
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
ziggie1984
force-pushed
the
updatefee-limiter
branch
3 times, most recently
from
d6232c3 to
ee06778
Compare
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Commits
Files that changed from the base of the PR and between 4f06f58 and ee0677859c2236da63670932c8b67c76c3b3b824.Files selected for processing (4)
- docs/release-notes/release-notes-0.18.0.md (3 hunks)
- lnwallet/chainfee/estimator.go (2 hunks)
- lnwallet/channel.go (5 hunks)
- lnwallet/channel_test.go (3 hunks)
Additional comments: 17
docs/release-notes/release-notes-0.18.0.md (3)
- 59-62: The addition of a fee buffer for locally opened channels as described in the linked PR is a significant improvement. It aligns with the BOLT 02 specification and addresses the issue of channels becoming stuck due to asynchronous HTLC additions or sharp fee changes. This change enhances the robustness of fee handling in
lndand is a proactive measure against potential channel issues.
- 81-85: The fix for proposing low fee rates due to insufficient mempool data or local channel drainage is crucial. It addresses a significant issue where channels could default to the fee floor, potentially leading to unusable channels. This change is a direct response to the problems outlined in the PR objectives and is an essential step towards more reliable fee rate handling in
lnd.
- 267-269: Updating the handling of
min_final_cltv_expiry_deltafor external invoices without the parameter is a thoughtful improvement. By ensuring a minimum value of 18 blocks,lndenhances the reliability of payment forwarding and settlement, aligning with network standards and expectations. This change contributes to a more predictable and consistent user experience.
lnwallet/chainfee/estimator.go (2)
- 477-482: The addition of a check to handle cases where Bitcoind has insufficient data for fee estimation is a crucial improvement. It ensures that the system does not proceed with potentially incorrect fee rates, thus enhancing the robustness of fee estimation. This change is particularly important for maintaining the reliability of transaction confirmations.
- 491-492: The modifications to log messages for fee rate and floor values contribute to better observability and debugging capabilities by providing clearer and more detailed information. This is beneficial for understanding the behavior of fee estimation and ensuring that the system operates as expected.
Also applies to: 498-498
lnwallet/channel.go (5)
- 7139-7139: This change appears to be a setup or minor adjustment unrelated to the PR's main objectives. It doesn't introduce any issues.
- 8683-8685: The addition of the
maxAllocationparameter to theMaxFeeRatemethod is a crucial change for implementing dynamic fee rate management based on a maximum allocation percentage. The documentation clearly specifies its intended use case.- 8695-8715: The logic implemented here effectively ensures that the fee rate does not fall below the current fee rate, addressing one of the PR's main objectives. The calculations for
maxFee,feeAllocation, andmaxFeeRateare correctly implemented and align with the intended functionality.- 8727-8727: The updated usage of
MaxFeeRatewithmaxFeeAllocas an argument correctly applies the changes made to the method in the context of calculating the commit fee rate. This aligns with the PR's objectives of managing fee rates more effectively.- 8766-8766: The logic for ensuring the commitment fee is above the minimum relay fee rate and providing a warning when the max channel fee allocation is overridden is correctly implemented. This is crucial for the network's operation and aligns with the PR's objectives.
lnwallet/channel_test.go (7)
- 7518-7520: The introduction of the
TestChannelMaxFeeRatefunction is a positive step towards ensuring that fee rate changes do not lead to unusable channels. It's crucial to have comprehensive tests covering different scenarios, especially given the impact of fee rate changes on channel usability.- 7534-7540: Creating both non-anchor and anchor channel setups for testing is a good practice, as it ensures that the changes are validated across different channel types. This thorough testing approach helps in catching potential issues early in the development process.
- 7555-7617: The addition of comprehensive test cases for different scenarios, including fee rate capping and calculation based on various conditions, is commendable. These tests are crucial for ensuring the robustness of the fee rate logic and preventing future issues related to fee rate changes.
- 7622-7658: The assertions used in the test cases are well-thought-out, especially the use of
require.InEpsilonfto account for rounding inaccuracies. This level of detail in testing demonstrates a commitment to ensuring the accuracy and reliability of the fee rate logic.- 7662-7665: The
TestIdealCommitFeeRatefunction is a valuable addition to the test suite, focusing on the computation of the ideal commitment fee rate. Testing this aspect is crucial for ensuring that fee rate adjustments align with network conditions and do not lead to unusable channels.- 7688-7715: Creating both non-anchor and anchor test channels for the
TestIdealCommitFeeRatefunction is a good practice. It ensures that the fee rate logic is validated across different channel types, which is essential for catching potential issues early.- 7717-7896: The test cases and assertions for the
TestIdealCommitFeeRatefunction are comprehensive and well-designed. The use of helper functions for calculating max fee rate and current fee rate, along with the detailed test cases for both non-anchor and anchor channels, demonstrates a thorough approach to testing the fee rate logic.
| // TODO(ziggie): introduce an interface for the client to enhance | ||
| // testability of the estimator. |
There was a problem hiding this comment.
The addition of a TODO comment to introduce an interface for client testability is a good practice for future enhancements. It's important to ensure that such TODOs are tracked in a project management tool or issue tracker to ensure they are addressed in a timely manner.
Would you like assistance in creating a GitHub issue to track this task?
ziggie1984
force-pushed
the
updatefee-limiter
branch
from
ee06778 to
d48a999
Compare
ziggie1984
force-pushed
the
updatefee-limiter
branch
2 times, most recently
from
b30879b to
decd98e
Compare
ziggie1984
force-pushed
the
updatefee-limiter
branch
from
decd98e to
ae6f11c
Compare
|
Last Push was just a rebase. |
|
Idk what's going on with CI but there's a lot of complaining checks that I can't figure out why they are failing. |
|
Related to the build cache. Going to put up a fix to reduce the cache size. |
Yes |
|
Needs a rebase. |
Bitcoind will not report any fee estimation in case it has not enough data available. We used to just set the min mempool fee in such cases but this might not represent the current fee situation of the bitcoin network. We return an error now so that we will use the fallback fee instead.
When determining the max fee rate of a channel we used to scale the fee rate depending on our available local balance on this channel. This lead to a special case that if a channel would be drained we could especially decrease the fee rate even down to the fee floor. Now we make sure that our max fee rate will not be lower than the old fee rate to make sure in case our channel is locally drained we do not continue to decrease fees too low.
Adding the release-notes for release 0.18 and fixing typos in the release doc.
ziggie1984
force-pushed
the
updatefee-limiter
branch
from
ae6f11c to
25c3849
Compare
Relates to #7666 and #7756
Detailed Problem explanation:
In lnd (lightning) there exist the feeupdate msg which allows channel openers to change the fee of the commitment transaction. For STATIC_REMOTE_KEY channels HTLCs which are part of the Commitment Transaction do have Timeout/Success Transactions which are signed with the same feerate as the Commitment Transaction (see BOLT3 for more details about the HTLC Transaction).
The FeeUpdate is only allowed to be sent by the Channel Opener but it should be in acceptable Limits, because especially for non-achor channels there is no way to accelerate this transaction (only if you pay a miner directly). CLN nodes will check for a specific range and will not allow the feeupdate from happening. If CLN does reject the feeupdate the channel will either be force-closed by the peer (lndk) or the channel becomes unusable and needs to be force-closed eventually by the peer.
This PR fixes 2 Problems:
No valid Fee Estimation by our backend
In case you either wipe your mempool or restart a new bitcoind node you want to connect to your lnd node, there is a short time period where bitcoind is not reporting any fee estimations because it does not have enough data available.
It will report:
In such cases lnd would just read this response and the fee estimation would remain zero and then would just enforce the min mempool fee instead. This is a bad choice because when our mempool is big enough this fee rate is always 1 sat/vbyte and does not represent the current fee rate conditions. This PR will return an error in such cases and lnd will use its fallback feerate of 25 sats/vbyte. It is always better to overestimate rather than underestimate in such cases.
Limit Fee Estimation when channel is drained locally
Moreover Lnd reevaluates the MaxFeeRate it is willing to allocate for a Channel every time a feeupdate msg will be sent. When the channel is locally drained this could lead to a situation where we decrease the feerate all the way to the fee floor which does not reflect the current bitcoin network fee rate correctly and could also lead to our peer rejecting these low fee rate updates. With this PR we do not lower the MaxFeeRate we are willing to allocate for this channel if it would fall below the old fee rate. This guarantees that if we had a high fee rate on a channel, that we do not decrease it when the local balance is drained.
Locking for a Concept Ack here before writing tests.
This change will not fix existing channels which ran into this issue, but will most likely prevent future channels form being unusable when proposing very low fee rate updates leading to the channel-peer rejecting those.
Change Description
This PR limits the FeeRate update to a maximum of 30% of the old feerate. It only applies to non-anchor channels because for those the negotiated fee cannot be changed when resolving on-chain.Open for discussions whether we want to pursue this route if 30% is too conservative and so one.Needs some style fixing will solve this in the coming days.Tests
I currently just updated the unit tests in the
lnwalletpackage. I think it is enough and testing this in thehtlcswitchwhere we drain the channel locally and send consecutively fee updates with the default fee-allocation might not be worth it IMO, what are your takes?Fixes #7756
Summary by CodeRabbit
min_final_cltv_expiry_deltafor external invoices.