add sats_per_kweight option when crafting a transaction

[ziggie1984]

Replaces #7443 because a wrong from branch was used.

For the commands SendCoins, SendMany, CloseChannel, OpenChannel, BatchOpenChannel FundPsbt, BumpFee, PendingSweeps and in the response of the EstimateFee command the option sats_per_kweight is added to allow for more granular control of fees when creating transactions. Commands with the deprecated option sat_per_byte are replaced with the new option sat_per_kweight. For commands which did not have this option the new one is just added.

In the current fee environment one cannot use decimal fee estimates for sats_per_vbyte, therefore using sats_per_kweight one will be able to use decimal sats_per_vbyte feerates indirectly without introducing floating point math.

Currently in the lnd backend the option sats_per_kweight is prioritized in case both options are present at the same time (sats_per_kweight and sats_per_vbyte). Though having both of them set is prevented on lncli level. This might be open for discussion.

Steps to Test

4 new itests were added.
make itest icase="fundpsbt_feerate"
make itest icase="openchannel_feerate"
make itest icase="sendcoins_feerate"
make itest icase="sendmany_feerate"

1 itest was modified to account for the new option:
make itest icase="cpfp"

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.
• There is a change description in the release notes, or [skip ci] in the commit message for small changes.

📝 Please see our Contribution Guidelines for further guidance.

[alexbosworth]

FundPsbt also uses sats_per_vbyte

[ziggie1984]

takes a bit longer because I want to add itests for all the functions to easily test that changes work.

[ziggie1984]

Still some open questions I have:

1. Currently I replaced the deprecated option sat_per_byte and added the new option sat_per_kweight in one commit which leads to a bigger commit with several subsystems involved. Doing it separately would cause some overhead.
   How are things deprecated normally, especially when its replaced by a new option right after?

3. I did not include the itest for bumpfee for now, I guess it also makes sense to add an itest for it ?

4. I added the sat_per_kweight option for almost all commands/function except for the watchertower feerate. I did not find it that important to include it there as well, any thoughts ?

5. Regarding the tests I choose to compare the result including an error-rate (inepsilon) as seen in other place in the code. I choose 1% error tolerance. Can we go with this approach ?

6. I might have introduced a timing problem for the itest suite. The cpfp tests normally runs through without problems but it needs a bit of time for the sweeper to broadcast the transaction, somehow the failing itest seem to have some problems when broadcasting the RBF transaction for the child. Not sure how to approach this problem.

[ziggie1984]

This PR needs some rework from my side because I did renumber protofile nummeration not thinking of backwards compatibility issues. So will rework in the next days.

Learned this from guggero and his comment here:
#7473 (comment)

[ziggie1984]

I updated this PR to make the RPC chane backwards-compatible. I included itests for commands which now have the new sat_per_kweight option.

Only for estimatefee cmd where I added the sat_per_kweight option I did not include a test, because nothing really changed compared to the sat_per_vbyte option.

From my POV this PR is ready for review. Don't be intimidated by the amount of lines which were changed. The change is minor but the walletrpc and lnrpc commands changed therefore the change appears to be big.

[lightninglabs-deploy]

@joostjager: review reminder

[ziggie1984]

!lightninglabs-deploy mute

[C-Otto]

FYI: I'd love to see this merged. Currently, it has a bunch of merge conflicts.

[coderabbitai]

Important

Auto Review Skipped

Auto 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 .coderabbit.yaml file in this repository.

To trigger a single review, invoke the @coderabbitai review command.

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[ziggie1984]

FYI: I'd love to see this merged. Currently, it has a bunch of merge conflicts.

Fixed the merge conflicts should be good now, will take a closer look in LND 19 but you could check by yourself if you want this feature now and merge it into your version.

[joostjager]

PR has been untouched for a long time, but cannot remove myself as reviewer.

[starius]

I propose to use another ID for this field and not to change the deprecated field, not to break binary backwards compatibility. If someone has an old client and runs against new server (or vice-versa), they can send a request with this field, which has changed meaning, resulting in huge overpayment or underpayment of fee.