Fees: add fractional sat/vB support (lncli) and sats_per_kw (RPC)#10067
Fees: add fractional sat/vB support (lncli) and sats_per_kw (RPC)#10067MPins wants to merge 8 commits intolightningnetwork:masterfrom
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @MPins, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances fee rate control within the system by introducing a sats_per_kweight option for various on-chain and channel-related transactions. This change provides more granular control over transaction fees, which is particularly beneficial for advanced users managing Lightning channels. The update involves modifications to the command-line interface, RPC definitions, internal fee calculation logic, and comprehensive integration tests to ensure correctness and prevent conflicting fee specifications.
Highlights
- New Fee Rate Option: Introduced a new
--sat_per_kweightoption across variouslnclicommands andwalletrpccalls, allowing users to specify transaction fees in satoshis per kiloweight (sat/kweight). This provides finer control over fee rates, especially useful for advanced channel management. - API and Protocol Updates: Updated
lnrpc/lightning.protoandlnrpc/walletrpc/walletkit.prototo include the newsat_per_kweightfield, replacing the deprecatedsat_per_bytefield. Corresponding Swagger and generated Go files have been updated. - Fee Calculation Logic Enhancement: Modified the internal
CalculateFeeRateutility to properly handle the newsat_per_kweightinput, ensuring that only one fee rate type (sat_per_vbyteorsat_per_kweight) can be specified at a time. A security measure was also added to cap the effective fee rate at a default maximum. - Expanded Command Support: The
--sat_per_kweightoption is now available forlncli openchannel,batchopenchannel,sendcoins,sendmany, andclosechannelcommands, as well aswalletrpc FundPsbtandBumpFeecalls. - Integration Test Coverage: Added new integration tests to verify the correct application of
sat_per_kweightfor channel opening, sending coins, and sending to multiple addresses, including tests for mutual exclusivity withsat_per_vbyte.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a sats_per_kweight option for more granular fee control in various transaction-related commands. The changes are extensive, touching lncli commands, RPC definitions, and wallet logic. I've added several new integration tests to validate this new functionality.
My review focuses on ensuring consistency in argument validation across different commands, adherence to the style guide, and clarity in documentation and error messages. I've identified a few places where command-line argument validation can be improved and some documentation that could be more precise. Overall, the implementation is solid, and the new feature is a great addition.
ziggie1984
left a comment
There was a problem hiding this comment.
Left some initial comments
I’m doing a final double-check and plan to push the PR tomorrow. |
1bddccb to
e23345b
Compare
|
@ziggie1984 ready for review. Thanks! |
|
Hello @saubyk could you please assign it to me. |
ziggie1984
left a comment
There was a problem hiding this comment.
Looking good thank you for taking this PR over. I had some suggestions how to make this fractional feerate more user friendly on the lncli level.
|
Regarding dropping the deprecated sat_per_byte option, can you create a release-note signaling the dropping of this feature for LND 20, so that we can drop the support in LND 21.0, this gives users time to change their setup, similar how we did it for the payment RPC endpoints. |
|
Thanks for working on this! This is very useful! I think about this feature every time I send a transaction and have to choose between 1 and 2 sats/vbyte. I propose modifying Note that 1 sat/vbyte = 250 sats/kw, so this modification does not introduce rounding errors for integer values. This is an example of how it would work: |
Good idea, thanks for your time! |
|
Thanks for the review, @ziggie1984 and @starius . To summarize, I plan to:
|
That makes sense! Release 20 should add the release-notes for removing the sat_per_byte option, and then we should open a PR dropping it for 21. But this PR should definitely keep the sat_per_byte option. |
|
I have a couple of proposals.
|
That makes sense. If we go in that direction, we should also introduce a kweight-based field (perhaps MaxFeeRateKw) for MaxFeeRate in the closechannel RPC, and deprecate the current MaxFeeRate at the same time.
That would be great — I’d be more than happy to review your PR.
I will, thanks for the heads-up. |
Include lightningnetwork/lnd#10067 add sats_per_kweight option when crafting a transaction (continue)
Use SatPerKw instead of SatPerVbyte in SendCoins, CloseChannel, and walletrpc.BumpFee APIs. Added new option WithSendCoinsFeerate for SendCoins and WithOpenChannelFeerate for OpenChannel API specifying feerate in sats/kw. Use new fields provded by lightningnetwork/lnd#10067
Use SatPerKw instead of SatPerVbyte in SendCoins, CloseChannel, and walletrpc.BumpFee APIs. Added new option WithSendCoinsFeerate for SendCoins and WithOpenChannelFeerate for OpenChannel API specifying feerate in sats/kw. Use new fields provided by lightningnetwork/lnd#10067
I sent PR lightninglabs/lndclient#241 with updates for lndclient. It switches lnclient from |
Include lightningnetwork/lnd#10067 add sats_per_kweight option when crafting a transaction (continue)
Use SatPerKw instead of SatPerVbyte in SendCoins, CloseChannel, and walletrpc.BumpFee APIs. Added new option WithSendCoinsFeerate for SendCoins and WithOpenChannelFeerate for OpenChannel API specifying feerate in sats/kw. Use new fields provided by lightningnetwork/lnd#10067
Use SatPerKw instead of SatPerVbyte in SendCoins, CloseChannel, and walletrpc.BumpFee APIs. Added new option WithSendCoinsFeerate for SendCoins and WithOpenChannelFeerate for OpenChannel API specifying feerate in sats/kw. Use new fields provided by lightningnetwork/lnd#10067
b5852ad to
aa73d97
Compare
|
@ziggie1984 thanks for the review. I've addressed the comments. Let's wait the @starius on deprecated related comments to address them. |
aa73d97 to
2d3e101
Compare
|
Hey @ziggie1984 and @starius! I’ve updated the release note — whenever you get a chance, I’d love your feedback again. |
ziggie1984
left a comment
There was a problem hiding this comment.
I propose splitting at least the coop-close feerate update to sat/kW, and also a separate PR for the release not notification that support for sat_per_byte is dropped.
Moreover I don't really like that we are removing a valid option from the RPC interface (sat_per_vbyte) I think that should and. stay a valid option.
| return uint64(feeRate.FeePerVByte()) | ||
| }) | ||
|
|
||
| startingFeeRateKw := fn.MapOptionZ( |
There was a problem hiding this comment.
shouldn't we report an error if both of them are set ?
There was a problem hiding this comment.
or does it happen later in the caller stack ?
There was a problem hiding this comment.
The feerate is always stored internally in sat/kw only; pendingsweeps simply displays it in both formats (sat/kw and sat/vB).
|
|
||
| // parseFeeRate converts fee from sat/vB to sat/kw using fixed-point | ||
| // math to avoid rounding errors from floating-point arithmetic. | ||
| func parseFeeRate(ctx *cli.Context, flagName string) (uint64, error) { |
There was a problem hiding this comment.
let's also proper unit test this helper once rewritten to use string.
| require.Equal(ht, alicePendingUpdate.FeePerVbyte, int64(bobFeeRate)) | ||
| // Using InDelta as fee rate calc might differ due to weight estimation | ||
| // versuns actual tx weight. | ||
| require.InDelta( |
There was a problem hiding this comment.
my question is more, normally LND defaults to taproot addresses and I don't understand why there is an inaccuracy in the test ? Was this test flaky before ? Because before it was not using any delta
|
Let's keep /*
Optional manual fee rate override. Specify either sat_per_vbyte or
sat_per_kw to force a particular rate for the funding transaction. If left
unset, the wallet's confirmation target/estimator is used.
*/
oneof manual_fee_rate {
/*
Manual fee rate for the funding transaction expressed in satoshis per
virtual byte. Use this when mirroring external wallet or mempool
policies that quote fees per vbyte.
*/
uint64 sat_per_vbyte = 1;
/*
Manual fee rate for the funding transaction expressed in satoshis per
kilo-weight. Use this when quoting fees from weight-based estimators or
when you want deterministic weight accounting.
*/
uint64 sat_per_kw = 29;
}(this is for This is needed to prevent sending both of the fields sententiously with a compile-time check in Go code. Note that this change is backwards-compatible on the wire. Old clients sending |
Hello @ziggie1984 and @starius, as far as I know, if we go with the proposal above, the scope of changes becomes significantly larger. We would need to update the code everywhere those request structures are used or manipulated. One option would make this change (using ‘oneof’ structure) with the PR that it will be done to drop the sat_per_byte deprecated field … we will have to do it in the v0.22 |
For EstimateFeeResponse, SendManyRequest, SendCoinsRequest, CloseChannelRequest, BatchOpenChannelRequest, OpenChannelRequest, PendingSweeps, BumpFeeRequest the sat_per_kw field is added. This allows more fine granular control of transaction fees.
Fee calculation for sendcoins, sendmany, openchannel, closechannel, PendingSweeps, BumpFee has now the option sat_per_kw. In addition a safety check is added to prevent very high fees. For the estimatefee cmd the fee in sat_per_kw is added to the response.
2d3e101 to
90a40c0
Compare
The sat_per_vbyte input option now accepts fractional values and is internally converted into sat_per_kw. The max_fee_per_vbyte input option now accepts fractional values and is internally converted into max_fee_per_kw for fee calculation. Add the sat_per_kw option for the lnrpc cmds sendcoins, sendmany, openchannel, batchopenchannel, closechannel and closeallchannels. Add the sat_per_kw for estimatefee in the response. Add sat_per_kw option for walletrpc cmds "wallet bumpfee" and in the response of "wallet pending sweeps". Add max_fee_per_kw for closechannel command.
Add new itests for openchannel, sendcoins and sendmany.
Fix the test "rbf coop close" that was broken by a safety check added to prevent very high fees.
Change RBF cooperative close from sat/vB to sat/kw, along with the minimal changes needed for existing tests.
90a40c0 to
18d23a5
Compare
|
@starius: review reminder |
This PR continues the work started in #7454 by @ziggie1984
The goal is to allow RPC callers to specify a custom
sats_per_kweightvalue when crafting transactions, enabling finer control over fee rates. This is particularly useful for advanced channel management.The lncli users will continue using the
--sat_per_vbyteoption, but fractions are now accepted (e.g.,--sat_per_vbyte=1.08). Internally, the value will be converted to sat/kw when calling the RPC.For the
closechannelcommand, the--max_fee_rateoption will also accept fractions and will likewise be converted to sat/kw internally when calling the RPC.Original PR:
#7454
Happy to continue the discussion and iterate on this.