walletrpc: allow conf_target=1 in EstimateFee and FundPsbt

[starius]

Change Description

Previously walletrpc.EstimateFee and walletrpc.FundPsbt used to fail if conf_target is 1.
This was discussed before in #9611

In this PR value 1 is allowed. An itest is added.

Steps to Test

make itest icase='estimate_fee'
make itest icase='fund_psbt'

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 is not insubstantial. Typo fixes are not accepted to fight bot spam.
• 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.

[gemini-code-assist]

Summary of Changes

Hello @starius, I'm Gemini Code Assist¹! 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 updates the walletrpc.EstimateFee RPC method to allow a confirmation target of 1, addressing a previous limitation where such requests would fail. The change includes modifying the RPC handler to remove the explicit rejection of conf_target=1 and introduces a new integration test to validate this updated behavior, ensuring the API functions as expected for immediate confirmation targets.

Highlights

• API Behavior Change: The walletrpc.EstimateFee RPC method now permits a conf_target (confirmation target) of 1, which was previously explicitly rejected. This allows users to estimate fees for transactions aiming for confirmation in the very next block.
• Improved Error Messaging: The error message returned when conf_target is 0 has been updated to reflect that the target must be 'greater than 0', aligning with the new allowance for conf_target=1.
• New Integration Test: A new integration test, testEstimateFee, has been added to comprehensively verify the updated EstimateFee RPC behavior. It specifically tests for successful fee estimation when conf_target is 1 and confirms that conf_target of 0 still correctly triggers an error.

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

1. 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. ↩

[gemini-code-assist]

Code Review

This pull request correctly allows a confirmation target of 1 in walletrpc.EstimateFee and adds an integration test to verify the behavior. The changes are straightforward and well-tested. I've added a couple of suggestions: one to improve input validation in EstimateFee to also handle negative confirmation targets, and another to refactor the new test into a table-driven format for better maintainability and to include a test case for the improved validation.

[ziggie1984]

@gemini-code-assist can you check the whole codebase where we use EstimateFeePerKW and make sure that we allow everywhere values above 0 and that we have an approriate check for 0 and negative values because EstimateFeePerKW has a uint32 as paramter and if we convert a negative int to a uint32 that would cause some bad side effects because all of a sudden we would have very high conf targets, so check every code part whether we have those checks. For example in the func FundPsbt we sill not allow 1 which is not consistent behaviour

[gemini-code-assist]

@ziggie1984 Thank you for reaching out and for the detailed explanation of your concern regarding EstimateFeePerKW and the potential int32 to uint32 conversion issues with negative or zero conf_target values. You've highlighted a critical point about type safety and unexpected behavior.

My review capabilities are focused on the changes introduced within this specific pull request. Looking at the modifications in this PR:

• lnrpc/walletrpc/walletkit_server.go: The EstimateFee function has been updated to explicitly check req.ConfTarget <= 0 and return an error if this condition is met. This change directly addresses your concern for the walletrpc.EstimateFee RPC, ensuring that negative or zero int32 values for ConfTarget are caught before being passed to EstimateFeePerKW, thus preventing the uint32 conversion issue and the resulting "very high conf targets" in this specific context.

• itest/lnd_misc_test.go: The added testEstimateFee integration test includes cases for confTarget=0 and confTarget=-1, asserting that they correctly return an error indicating the value "must be greater than 0". This further validates the fix for the EstimateFee RPC.

Regarding your request to "check the whole codebase where we use EstimateFeePerKW" and specifically the FundPsbt function, I am unable to perform a comprehensive review across the entire codebase or on files/functions not modified in this pull request. My scope is limited to the diffs presented in the PR.

If you believe there are other instances of EstimateFeePerKW usage or specific behaviors in FundPsbt that need review or modification to align with the desired consistency (e.g., allowing conf_target=1 in FundPsbt), I recommend opening a separate pull request for those changes. This allows for a focused review of those specific areas.

[ziggie1984]

seems like you still need to do it @starius see for example:

lnd/lnrpc/walletrpc/walletkit_server.go

Lines 1560 to 1563 in 6f09d96

	if targetConf < 2 {
	return nil, fmt.Errorf("confirmation target must be " +
	"greater than 1")
	}

[ziggie1984]

Please make sure the whole codebase is consistent when it comes to the estimatefee calls 🙏🙏

[lightninglabs-deploy]

@yyforyongyu: review reminder
@starius, remember to re-request review from reviewers when ready

[starius]

@ziggie1984 Thanks for pointing to FundPsbt RPC! I fixed it to accept conf_target=1 as well and checked this in itest "fund_psbt".

I checked walletkit.proto for other RPCs using conf_target. There are also BumpForceCloseFeeRequest and BumpFeeRequest. Their type is uint32, not int32 and Go code in walletkit_server.go allows value 1 there.

I haven't found other cases of comparing conf_target with 2 in the codebase.

I think it doesn't make sense to change type of confTarget argument in EstimateFeePerKW from uint32 to int32, because in some cases the type on the calling side is already uint32 and it is already checked to be positive before conversion e.g. in sweep package.

[ziggie1984]

LGTM 🙏

[yyforyongyu]

🧹

[yyforyongyu]

nit: Commit msg says testing this scenario, tho I don't think it's tested here - the branch below was not executed

[starius]

I reverted the change in lnrpc/walletrpc/walletkit_server.go and ran make itest icase='fund_psbt'. It failed:

=== RUN   TestLightningNetworkDaemon/tranche00/67-of-309/btcd/fund_psbt
    harness_node.go:395: Starting node (name=Alice) with PID=971348
    harness_node.go:395: Starting node (name=Bob) with PID=971361
    harness_rpc.go:100: 
                Error Trace:    /home/user/lnd/lntest/rpc/harness_rpc.go:100
                                                        /home/user/lnd/lntest/rpc/wallet_kit.go:66
                                                        /home/user/lnd/itest/lnd_psbt_test.go:1252
                                                        /home/user/lnd/itest/lnd_psbt_test.go:1140
                                                        /home/user/lnd/lntest/harness.go:315
                                                        /home/user/lnd/itest/lnd_test.go:130
                Error:          Received unexpected error:
                                rpc error: code = Unknown desc = confirmation target must be greater than 1
                Messages:       Alice: failed to call FundPsbt
    harness.go:393: finished test: fund_psbt, start height=438, end height=440, mined blocks=2
    harness.go:352: test failed, skipped cleanup