lightningnetwork / lnd

Lightning Network Daemon ⚡️
MIT License
7.69k stars 2.08k forks source link

[bug]: `minFeeRate` not capped by budget in `LinearFeeFunction` #9101

Open feelancer21 opened 1 month ago

feelancer21 commented 1 month ago

Studied a little bit the code around the LinearFeeFunction and found the following edge case. If the conf_target is greather equal than 1008 we return the min relay fee. This is not capped by the budget (endrate) of the fee function. E.g. if you have a min relay fee rate of 100 sat/vb an ad max fee rate given by the budget of 50 sat/vb NewLinearFeeFunction will return a fee function with a startingFeeRate of 100 sat/vb and an endingFeeRate of 50 sat/vb. Imho this will lead to a broadcasted sweep tx with 100 sat/vb.

https://github.com/lightningnetwork/lnd/blob/e8c5e7d5ce4b79117ef9ab497745fea3a1bffb36/sweep/fee_function.go#L294-L299

ziggie1984 commented 1 month ago

Thank you for your investigation:

We definitely need to cap a high relay feerate with the endFeerate otherwise this can cause your explained scenario especially when the maxFeeRate config setting is the determining factor limiting the fees. In the case when budget is the limiting factor we might catch this case here (but only maybe depending on the different inputs and their different budget sizes):

https://github.com/lightningnetwork/lnd/blob/750770e1904dd7db54b2cf1aa510d5e92308e290/sweep/aggregator.go#L229-L238

Roasbeef commented 1 month ago

If the min relay fee is 100 sat/vb, whether we use that or not depends on the budget available for the set of inputs. If there's a spike that causes the min relay fee to grow quickly, then the end result will just be that we delay trying to sweep it under such conditions.

I think what we are missing though is a sanity check that start < end, the Alloy model enforces this: https://github.com/lightningnetwork/lnd/blob/750770e1904dd7db54b2cf1aa510d5e92308e290/docs/alloy-models/linear-fee-function/linear-fee.als#L233-L237