Skip to content

Conversation

@hieblmi
Copy link
Collaborator

@hieblmi hieblmi commented Oct 15, 2025

EstimateFee now takes optional inputs to give a fee estimate.

Examples:

reg_zane estimatefee '{"bcrt1q7dnuwzl87hunndngmgzs9s8t279jjkrdgkjp4t" : 500600001}' --conf_target 1 --utxo ca1c4fc61331b35adc8032e8b61461986f40c7f97c1e83c27a181defe5a08ee4:1 --utxo d9665f3fe1190098b2bc0836e25e7a1019e2532b2cca4b5acfb974de5d01b90c:0 --utxo 734d2c21da9e95b412bef63c036b7d72f8c365abab05cc1e5973b292a5d976a2:0 --utxo f766ae254d150cd2556efa096503fba86b29674e606b44a8bbf44b504bdd31d0:1
{
    "fee_sat": "8950",
    "feerate_sat_per_byte": "25",
    "sat_per_vbyte": "25"
}
reg_zane estimatefee '{"bcrt1q7dnuwzl87hunndngmgzs9s8t279jjkrdgkjp4t" : 300001}' --conf_target 2
{
    "fee_sat": "3825",
    "feerate_sat_per_byte": "25",
    "sat_per_vbyte": "25",
     "outpoints": [
        {
            "txid_bytes": "",
            "txid_str": "ba88c81dd1456de4714a2c09d38355374362d3e5d97daf00c76f4b918a280b3b",
            "output_index": 0
        }
    ]
}

@hieblmi hieblmi marked this pull request as draft October 15, 2025 14:05
@gemini-code-assist
Copy link

Summary of Changes

Hello @hieblmi, 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 significantly improves the estimatefee RPC by introducing the ability for users to explicitly define which unspent transaction outputs (UTXOs) should be considered when estimating transaction fees. Additionally, it provides an option to return the exact outpoints used in the estimation, enhancing transparency and control for users who need precise fee calculations based on specific inputs.

Highlights

  • Enhanced estimatefee command: The estimatefee command now supports specifying custom UTXOs for fee estimation using a new --utxo flag.
  • Outpoint visibility: A new --show_outpoints flag has been introduced, allowing users to view the specific outpoints (UTXOs) that were used in the fee estimation process.
  • API and Protocol Updates: The EstimateFeeRequest and EstimateFeeResponse protobuf messages have been extended to accommodate the new outpoints input and show_outpoints output.
  • RPC Server Logic: The RPC server's EstimateFee handler has been updated to parse and validate user-provided outpoints, pass them to the wallet's transaction creation logic, and return the selected outpoints in the response when requested.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

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.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances the estimatefee command by allowing users to specify UTXOs for fee estimation and to view the selected outpoints. The implementation is solid, with appropriate changes to the RPC interface, command-line flags, and server-side logic, including validation for duplicate UTXOs. My feedback includes a couple of minor suggestions to improve error messages for better clarity and to align with the repository's style guide.

@saubyk saubyk added this to the v0.21.0 milestone Oct 15, 2025
@saubyk saubyk added this to v0.21 Oct 15, 2025
@saubyk saubyk moved this to In progress in v0.21 Oct 15, 2025
repeated OutPoint outpoints = 6;

// Whether to include the selected outpoints in the response.
bool show_outpoints = 7;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason to not show them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review! In case we provide a list of outpoints manually I thought they don't have to be displayed again in the output. Only if we estimate for an amount would it make sense to show them imo.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gemini-code-assist I think it would make sense to acutally return them as well instead of having this clumsy boolean in the API

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @ziggie1984. From an API design perspective, it's generally beneficial to have predictable responses. Always returning the outpoints in the EstimateFeeResponse, regardless of whether they were provided as input or auto-selected, would simplify client-side logic and provide a more consistent and complete view of the transaction being estimated. This removes the need for the show_outpoints boolean flag in the request, making the API cleaner and easier to use.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the LLM here as well, let's always show the inputs

@hieblmi hieblmi force-pushed the estimate-inputs branch 2 times, most recently from 6d12c75 to 04bfef2 Compare November 25, 2025 10:31
@hieblmi hieblmi requested a review from sputn1ck November 25, 2025 10:32
@hieblmi hieblmi marked this pull request as ready for review November 25, 2025 10:32
@hieblmi hieblmi force-pushed the estimate-inputs branch 2 times, most recently from 716fd8b to 3a24364 Compare November 25, 2025 10:45
@hieblmi hieblmi requested a review from ziggie1984 December 2, 2025 10:39
@hieblmi
Copy link
Collaborator Author

hieblmi commented Dec 23, 2025

Rebased

Copy link
Collaborator

@ziggie1984 ziggie1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small change, I wonder when this is useful without being able to finally force this transaction to happen via sendcoins, or is this already possible ?

AddrToAmount: amountToAddr,
TargetConf: int32(ctx.Int64("conf_target")),
CoinSelectionStrategy: coinSelectionStrategy,
Outpoints: outpoints,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the outpoints (inputs) are not enough what is the behaviour ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case CreateSimpleTx inside EstimateFee will return an error, see

lnd/rpcserver.go

Line 1291 in 013f5c9

tx, err = wallet.CreateSimpleTx(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment how it behaves in that case

rpcserver.go Outdated

// Return the outpoints the estimate is for if the client requested so.
outpoints := make([]*lnrpc.OutPoint, 0, len(tx.Tx.TxIn))
if in.ShowOutpoints {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

always return the outpoints

Copy link
Collaborator Author

@hieblmi hieblmi Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought if the client specifically states the inputs they don't need to be shown to them again. But for consistency of the output I think that's ok.
I removed this option and always show them.

@hieblmi hieblmi force-pushed the estimate-inputs branch 2 times, most recently from 2319f1a to fee5d40 Compare December 24, 2025 08:33
uint64 sat_per_vbyte = 3;

// A list of selected outpoints as inputs for the transaction.
repeated OutPoint outpoints = 4;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to inputs as well.

@hieblmi hieblmi force-pushed the estimate-inputs branch 2 times, most recently from cac8a05 to 4679aee Compare December 24, 2025 12:38
@hieblmi hieblmi requested a review from ziggie1984 December 24, 2025 21:26
Copy link
Collaborator

@ziggie1984 ziggie1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think you could also add an itest to at least verify that this new feature works.

@lightninglabs-deploy
Copy link

@sputn1ck: review reminder
@hieblmi, remember to re-request review from reviewers when ready

@hieblmi
Copy link
Collaborator Author

hieblmi commented Jan 5, 2026

Thank for the review @ziggie1984. I've added itests in 1bbdb21.

They ensure that a fee estimation for selected inputs matches the actual fee produced by SendCoins for the same inputs.

@hieblmi hieblmi requested a review from ziggie1984 January 5, 2026 08:37
@saubyk saubyk moved this from In progress to In review in v0.21 Jan 5, 2026
Comment on lines 136 to 148
feeDiff := actualFee - estimateResp.FeeSat
if feeDiff < 0 {
feeDiff = -feeDiff
}

// Allow up to 1% difference or 100 sats, whichever is larger.
maxDiff := actualFee / 100
if maxDiff < 100 {
maxDiff = 100
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you use the fee-service and taproot inputs there shouldn't be a error tho, normally we don't need this.

}

// Allow up to 1% difference or 100 sats, whichever is larger.
maxDiff := actualFee / 100
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here can we avoid this ?

@ziggie1984
Copy link
Collaborator

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the ability to specify inputs for fee estimation in the EstimateFee RPC and its corresponding estimatefee lncli command. This is a useful feature for users who want more control over coin selection for fee estimation. The implementation involves extending the RPC interface, updating the server-side logic to handle these inputs, and adding new integration tests to validate the functionality. A significant portion of the changes also involves refactoring helper types and functions into a more central package for better code reuse across the application. My review identified a bug in the new integration tests where outpoint indices were being incorrectly converted to strings, which could lead to test failures. I've also included some minor suggestions to improve error message formatting for better readability and consistency with the style guide. Overall, the changes are well-structured and the new feature is a valuable addition.

// Create a map of input outpoints from the estimate for easy lookup.
estimateInputs := make(map[string]bool)
for _, input := range estimateResp.Inputs {
key := input.TxidStr + ":" + string(rune(input.OutputIndex))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The conversion of input.OutputIndex to a string is incorrect. string(rune(input.OutputIndex)) converts the integer to its UTF-8 character representation, which is not the desired decimal representation for an outpoint index. This can lead to incorrect map keys and test failures. You should use fmt.Sprintf or strconv.Itoa for a correct conversion.

Suggested change
key := input.TxidStr + ":" + string(rune(input.OutputIndex))
key := fmt.Sprintf("%s:%d", input.TxidStr, input.OutputIndex)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines 92 to 93
key := outpoint.TxidStr + ":" +
string(rune(outpoint.OutputIndex))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to the previous loop, the conversion of outpoint.OutputIndex to a string is incorrect. string(rune(outpoint.OutputIndex)) converts the integer to its UTF-8 character representation. This should be converted to its decimal string representation to form a correct outpoint string.

Suggested change
key := outpoint.TxidStr + ":" +
string(rune(outpoint.OutputIndex))
key := fmt.Sprintf("%s:%d", outpoint.TxidStr, outpoint.OutputIndex)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

rpcserver.go Outdated
Comment on lines 1290 to 1291
return nil, fmt.Errorf("can't create outpoints "+
"%w", err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error message formatting can be improved for clarity. Instead of concatenating a space before the format verb, it's cleaner to include the colon and space within the format string itself. This improves readability and aligns with the style guide's goal of clear error messages.

return nil, fmt.Errorf("can't create outpoints: %w", err)
References
  1. The style guide recommends minimizing lines for log and error messages and provides examples for clear formatting. Using string concatenation for short error messages like this is unnecessary and less readable than a single format string. (link)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines +1295 to +1287
return nil, fmt.Errorf("selected outpoints contain " +
"duplicate values")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error message can be simplified by removing the string concatenation for better readability.

return nil, fmt.Errorf("selected outpoints contain duplicate values")
References
  1. The style guide recommends minimizing lines for log and error messages. While this isn't a long line, avoiding unnecessary string concatenation improves readability. (link)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a long line.

rpcserver.go Outdated
Comment on lines 1334 to 1335
return nil, fmt.Errorf("can't convert outpoints "+
"%w", err)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The error message formatting can be improved for clarity. Instead of concatenating a space before the format verb, it's cleaner to include the colon and space within the format string itself. This improves readability and aligns with the style guide's goal of clear error messages.

return nil, fmt.Errorf("can't convert outpoints: %w", err)
References
  1. The style guide recommends minimizing lines for log and error messages and provides examples for clear formatting. Using string concatenation for short error messages like this is unnecessary and less readable than a single format string. (link)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Collaborator

@sputn1ck sputn1ck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ziggie1984 ziggie1984 merged commit 39a1421 into lightningnetwork:master Jan 7, 2026
39 of 41 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in v0.21 Jan 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants