-
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
sweep+lnrpc: enforce provided fee rate is no less than relay fee #7645
sweep+lnrpc: enforce provided fee rate is no less than relay fee #7645
Conversation
for fundpsbt it would be nice to provide a minimum relay fee to funding, maybe locally the minimum relay fee is higher than the chain daemon ultimately where it will be broadcast - a funded psbt can be broadcast outside of the local chain backend |
Why would you not just specify a custom |
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.
Looks very good, thanks a lot for the PR!
right but wouldn't this block that if the specified rate was too low? |
The change in this PR wouldn't block you from using that lower fee rate but increase the manually specified rate to the minimum local rate. I can see why this might not always be the desired outcome. Maybe we need a |
yeah that's what i mean exactly for fundpsbt definitely yes, especially in cases where psbts are being collaboratively formed but not all participants agree on a minimum relay fee. a lot of operators run a limited mempool to save on operational costs. for the other calls I can't think why you would care, only fundpsbt because that isn't necessarily going to be broadcast locally or maybe it will not be broadcast locally immediately |
Okay, that case makes sense. Though I guess my question would be: Does it really make sense to publish a transaction with a fee rate that you know upfront won't make it into the default mempools? Assuming the resulting transaction most likely is a channel funding TX where you'd risk it not being confirmed in the necessary 2 weeks if the fee rate is too low? Or would the use case here be to low-ball it with 1 sat/vByte and then later CPFP it higher? |
Yeah the issue is the publish time and place which can be variable - potentially low-balling with cpfp or just waiting for publish at a later date and creating a series of potential txs that all can be used, but at different fee rates |
Thanks for the review @guggero ! I'll get started on the feedback you provided. I would also like a bit of help tackling this failing itest: Finally, I tried to follow the discussion between you and @alexbosworth -- is the ask to additionally provide a floor for
? |
For FundPsbt the idea is just to avoid the fee guardrails being mandatory Another idea I had though is to allow for clamping the fee estimate, seems like a lot of times that this error condition happens it's because Bitcoin Core is returning a low end fee estimate that is below its minimum relay fee This makes more sense in Bitcoin Core because the idea is that you rebroadcast your transactions and the mempool is more of a temporary cache rather than a guaranteed queue |
Yes, I think we should go with option 2, address
Ugh, yeah, that test is a bit of a pain with all the hard coded values. Since this sounds like a pure rounding thing (where the 3 additional sat/kw sometime result in a full satoshi more in fees), I think you just need to update the values until you get no failure. You can run that test in isolation by running |
e733db3
to
75777c7
Compare
Thanks @guggero -- I removed the The failing itest was due to The |
by clamping the fee estimate i mean just make sure that if you ask for a long confirm conf target like 1000 that it doesn't return or use a fee rate that is below the minimum relay fee rate |
@shaurya947 thanks for the updates. Can you apply the fixup commits please? I'll give this another round of review afterwards. |
75777c7
to
0d333dd
Compare
Done @guggero 🙂 |
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.
Only nits left, LGTM 🎉
0d333dd
to
218c75a
Compare
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.
Thanks for the PR⚡️ Major question is whether we should default to the min relay fee or abort the transaction so the user can decide to not open a channel.
I've tried my best to follow the required practices (code style, linting, comments etc.). Since this is my first PR, I'd love any feedback about what I may have overlooked 🙂
I'd probably squash the first two commits and the refactor in the third commit seems to be not applied anywhere, but this is good enough for a first-time contributor, good job!
sweep/walletsweep.go
Outdated
|
||
feePerKW = chainfee.FeePerKwFloor | ||
feePerKW = minFeePerKW |
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.
Maybe we should disallow creating transactions instead of defaulting to the min relay fee? Maybe I'd choose to not open a channel if the min relay fee is too high, but I don't know until I open the channel. Not sure tho, cc @guggero
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.
Hmm, good question... Maybe failing with an appropriate error message is actually the better option UX wise.
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.
+1 for error message
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.
Thanks for the input guys. Happy to make the change but just wondering if there is an argument to be made about preserving existing behavior? Granted that the bump today is only to chainfee.FeePerKwFloor
which, if I'm not mistaken, translates to 1sat/vbyte.
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 most RPCs disallow setting 0 or a fraction of sat/vByte. So the existing behavior would only come into place in RPCs where we have sat/kWU (which isn't the case here). But if we want to be 100% exact, we would split the check into two: if the fee rate is below the absolute floor value, bump it. If it's still lower than the min relay/mempool fee, return an error (probably mentioning the original, user-set value, not the bumped one).
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 most RPCs disallow setting 0 or a fraction of sat/vByte. So the existing behavior would only come into place in RPCs where we have sat/kWU (which isn't the case here).
This makes sense.
split the check into two: if the fee rate is below the absolute floor value, bump it. If it's still lower than the min relay/mempool fee, return an error (probably mentioning the original, user-set value, not the bumped one).
I would imagine in most instances the relay/mempool fee would be >= the absolute floor value. Is that accurate? If so, it doesn't seem very useful to introduce additional complexity to handle the case where the inequality is reversed (and a bumped fee would actually go through). What do you think?
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 would imagine in most instances the relay/mempool fee would be >= the absolute floor value.
Yes, that's correct. I was just thinking about RPCs that use sat/kWU, if they also go through DetermineFeePerKw
then users could actually enter a value lower than the absolute floor value. In that cases it would make sense to just bump that to the floor. But returning an error in both cases probably makes sense to keep the code simple.
We should probably be pretty specific in the error message, mentioning something along the lines of "fee of %d sat/vByte is too low to be accepted into the mempool or relayed to the network".
Also, any ideas why the itest check for |
Itests are still flaky, we're working on that. |
218c75a
to
dd30199
Compare
Hey @guggero and @yyforyongyu , I updated the PR to error out when the provided fee is too low. Updated the tests as well. Let me know how it looks now 🙂 |
Looks like we actually use a fee rate lower than 253 sat/kWU in one of the integration tests: |
Hey @guggero thanks for looking into the test failure. I wanted to make sure I fully understand your suggestion. So we're considering two values,
Is that accurate? If so, we consider the 3 possible inequalities:
Let me know if I'm misinterpreting or overthinking this. Otherwise I can just change the itest to use a higher fee value. |
Hmm, you are right, there might be weird edge cases here. To me
I wasn't sure how many tests would be affected. And it sounded a bit like a workaround to me. But maybe that is indeed the better option? I guess what we always have to check is that the fee rate returned by So perhaps do this in
|
Hey @guggero thanks for the feedback!
This was actually going to be my next question 🙂 because I believe this already happens. lnd/lnwallet/chainfee/minfeemanager.go Line 73 in bbbf7d3
So what do you think then, just update the tests? |
Updated itests. Hopefully I caught everything :) |
Ah, sorry, I should've taken a closer look at the failing tests first. But this latest change basically makes it impossible to ever use 1 sat/vByte again for anything on-chain. Which is definitely not what we want. I was expecting the itest changes to only be necessary for RPCs where we use sat/kw and had a value of 250 instead of 253. So what we actually actually want (I believe) is:
|
418f6cb
to
3ba454c
Compare
Makes sense @guggero , updated! |
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.
Nice work, LGTM 🎉
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.
LGTM🏖
This ensures that for transactions where a fee rate is specified (instead of a confirmation target), lnd doesn't accept transactions which would be ultimately ignored by the underlying chain's RPC.
Handle error in test case, as well as add new test case for below relay fee input.
This refactor aims to house the CalculateFeeRate function in a more "shareable" location. It used to be a non-exported function inside of rpcserver.go. However, other routines, such as FundPsbt inside of walletkit_server.go, could also rely on the same fee calculation functionality. So we move the function to rpc_utils.go and export it, and we will reuse it in the FundPsbt workflow in a future PR.
3ba454c
to
e2bd34b
Compare
Change Description
Fixes #7512 by enforcing that if a manual fee rate is provided in RPC calls then that fee rate is no less than the relay fee of the underlying chain node. To achieve this, as suggested, the
calculateFeeRate()
function was refactored out ofrpcserver.go
and intorpc_utils.go
, and reused inwalletkit_server.go
. This wayFundPsbt
rpc call can use the same fee estimation logic asOpenChannel
,SendCoins
, etc.FundPsbt
will be handled in a follow-up PR.I've tried my best to follow the required practices (code style, linting, comments etc.). Since this is my first PR, I'd love any feedback about what I may have overlooked 🙂
Steps to Test
Attempt any one of the rpc calls whilst providing a manual sat per vbyte which would fall below the relay fee of the chain node that lnd is connected to. Notice that the fee rate used is actually the relay fee.
OpenChannel
SendCoins
SendMany
CloseChannel
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.