lnwallet: add new config group and configurable cache for web fee estimator

[mrfelton]

Change Description

Add new fee config group with options fee.min-update-timeout and fee.max-update-timeout to allow configuration of the web fee estimator cache refresh rate. Also adds fee.url which deprecates the old top level feeurl setting. The previously deprecated neutrino.feeurl setting has now been removed with this change.

Closes #8482

This pull request includes changes to the fee estimation configuration in the chainreg/chainregistry.go and config.go files, and the fee estimation process in the chainfee/estimator.go file. The changes deprecate the FeeURL field and introduce a new Fee field with URL, MinUpdateTimeout, and MaxUpdateTimeout subfields. The FeeURL field is mapped to the Fee.URL field for backward compatibility. The MinUpdateTimeout and MaxUpdateTimeout fields determine the minimum and maximum intervals for updating fees from the fee estimation URL. The pull request also includes changes to the test files to accommodate these changes.

Changes to fee estimation configuration:

• chainreg/chainregistry.go: Deprecated the FeeURL field in the Config struct and introduced a new Fee field with URL, MinUpdateTimeout, and MaxUpdateTimeout subfields. Mapped the FeeURL field to the Fee.URL field for backward compatibility. [1] [2] [3] [4]
• config.go: Made similar changes to the Config struct and the DefaultConfig function. [1] [2] [3]

Changes to fee estimation process:

• chainfee/estimator.go: Removed the minFeeUpdateTimeout and maxFeeUpdateTimeout constants and added MinUpdateTimeout and MaxUpdateTimeout fields to the WebAPIEstimator struct. Modified the NewWebAPIEstimator function to take these fields as parameters and check that MinUpdateTimeout is less than MaxUpdateTimeout. Adjusted the randomFeeUpdateTimeout method to use these fields. (Faf38103L12R12, [1] [2] [3]

Changes to tests:

• config_builder.go, htlcswitch/link.go, htlcswitch/link_test.go, htlcswitch/test_utils.go, lntest/fee_service.go, lntest/node/config.go, lnwallet/chainfee/estimator_test.go: Updated the tests to accommodate the changes to the Config struct and the NewWebAPIEstimator function. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]

New file:

• lncfg/fee.go: Introduced a new file to define the Fee struct and the DefaultMinUpdateTimeout and DefaultMaxUpdateTimeout constants.

Deprecation:

• lncfg/neutrino.go: Deprecated the FeeURL field in the Neutrino struct.

Steps to Test

1. Start lnd with fee.min-update-timeout and/or fee.max-update-timeout set as cli args or in lnd.conf.
2. Verify that that the web fee estimator uses a cache refresh time between the supplied min and max values (you can see the cache refresh time used from new debug log entry that is printed on startup right after the web api fee estimator is started).
3. Verify that setting the min update timeout higher than the max update timeout results in an error.
4. Verify the values 5 min and 20 min are still used as the defaults to ensure no change in default behaviour.
5. Verify that fee.url can be used to set the web estimator fee url
6. Verify that feeurl can still be used
7. Verify that using both fee.url and feeurl results in a startup error

Example

Start lnd with a custom fee estimator timeout window:

./lnd-debug \
    --lnddir=./test-data \
    --bitcoin.active \
    --bitcoin.testnet \
    --debuglevel=debug \
    --bitcoin.node=neutrino \
    --fee.url=https://nodes.lightning.computer/fees/v1/btc-fee-estimates.json \
    --fee.min-update-timeout=1m \
    --fee.max-update-timeout=5m

Create/Unlock the wallet the wallet to trigger activation of the web fee estimator:

./lncli-debug --lnddir ./test-data --network testnet [create|unlock]

Review the logs:

2024-02-17 20:54:25.965 [INF] LNWL: Opened wallet
2024-02-17 20:54:26.072 [INF] CHRE: Using external fee estimator https://nodes.lightning.computer/fees/v1/btc-fee-estimates.json: cached=true: min update timeout=1m0s, max update timeout=5m0s
2024-02-17 20:54:26.072 [INF] LNWL: Starting web API fee estimator
2024-02-17 20:54:26.072 [INF] LNWL: Web API fee estimator using update timeout of 1m31s

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.
• Any new lncli commands have appropriate tags in the comments for the rpc in the proto file.
• 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.

[coderabbitai]

Important

Auto Review Skipped

Auto reviews are disabled on this repository.

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.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

---

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 testing code 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 testing code 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 testing code.
  • @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.
• Please see the configuration documentation for more information.
• 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/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[yyforyongyu]

Looks great! And a very helpful PR description! Just some nits.

[guggero]

Two more nits, otherwise LGTM 🎉

[lightninglabs-deploy]

@saubyk: review reminder
@yyforyongyu: review reminder

[saubyk]

Ack

Given that we are already in the rc phase for 0.18. Tagging this for 0.18.1 instead.
Would require updating the release notes accordingly

[mrfelton]

Given that we are already in the rc phase for 0.18. Tagging this for 0.18.1 instead.
Would require updating the release notes accordingly

Given the included breaking change (removal of deprecated neutrino.feeurl setting), shouldn't this go in 0.18.0? I've rebased the PR in any case.

[guggero]

Noticed a small issue with the sample config file that should be fixed.

@saubyk @Roasbeef since this technically could be considered to be a bug fix, should we still get it into 0.18 during the RC phase?

[guggero]

Since the prefix creates a new sub config part, this whole section should get a section title ([fee]) and be moved to just above the [prometheus] section, otherwise this becomes syntactically invalid for the config file parser (even though people shouldn't use this as a template directly, a lot of people probably still will).

[yyforyongyu]

Merged to 0.18.1 branch.

[yyforyongyu]

sorry didn't see the above comments, feel free to reopen and address the issue. If this goes to 0.18.1, then a new release notes doc is needed.

[mrfelton]

Merged to 0.18.1 branch.
sorry didn't see the above comments, feel free to reopen and address the issue. If this goes to 0.18.1, then a new release notes doc is needed.

I wouldn't feel comfortable introducing a breaking change into a patch release like that. That seems problematic. Either this should be ported in 0.18.0, or the breaking change should be removed from 0.18.1. I'd prefer the former, since otherwise we're maintaining multiple levels of backwards compatibility which is pretty messy and confusing (hence why the super old neutrino.feeurl setting was removed along with this change).

[saubyk]

Merged to 0.18.1 branch.
sorry didn't see the above comments, feel free to reopen and address the issue. If this goes to 0.18.1, then a new release notes doc is needed.

I wouldn't feel comfortable introducing a breaking change into a patch release like that. That seems problematic. Either this should be ported in 0.18.0, or the breaking change should be removed from 0.18.1. I'd prefer the former, since otherwise we're maintaining multiple levels of backwards compatibility which is pretty messy and confusing (hence why the super old neutrino.feeurl setting was removed along with this change).

yeah. I was waiting to chat with @Roasbeef about it and see if we can merge it in 0.18. Agree that this shouldn't be in 0.18.1

[saubyk]

Approved to merge in 0.18.0