-
Notifications
You must be signed in to change notification settings - Fork 2.2k
estimatefee: tx fee estimate for selected inputs #10296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @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 Highlights
Using Gemini Code AssistThe 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
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 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
|
There was a problem hiding this 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.
lnrpc/lightning.proto
Outdated
| repeated OutPoint outpoints = 6; | ||
|
|
||
| // Whether to include the selected outpoints in the response. | ||
| bool show_outpoints = 7; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
6d12c75 to
04bfef2
Compare
716fd8b to
3a24364
Compare
3a24364 to
658f073
Compare
|
Rebased |
ziggie1984
left a comment
There was a problem hiding this 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 ?
cmd/commands/commands.go
Outdated
| AddrToAmount: amountToAddr, | ||
| TargetConf: int32(ctx.Int64("conf_target")), | ||
| CoinSelectionStrategy: coinSelectionStrategy, | ||
| Outpoints: outpoints, |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
Line 1291 in 013f5c9
| tx, err = wallet.CreateSimpleTx( |
There was a problem hiding this comment.
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
658f073 to
f639036
Compare
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always return the outpoints
There was a problem hiding this comment.
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.
2319f1a to
fee5d40
Compare
lnrpc/lightning.proto
Outdated
| uint64 sat_per_vbyte = 3; | ||
|
|
||
| // A list of selected outpoints as inputs for the transaction. | ||
| repeated OutPoint outpoints = 4; |
There was a problem hiding this comment.
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.
cac8a05 to
4679aee
Compare
ziggie1984
left a comment
There was a problem hiding this 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.
4679aee to
ebd6dd0
Compare
|
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 |
ebd6dd0 to
e38ecd2
Compare
e38ecd2 to
8b70b79
Compare
| 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 | ||
| } |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 ?
|
/gemini review |
There was a problem hiding this 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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| key := input.TxidStr + ":" + string(rune(input.OutputIndex)) | |
| key := fmt.Sprintf("%s:%d", input.TxidStr, input.OutputIndex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| key := outpoint.TxidStr + ":" + | ||
| string(rune(outpoint.OutputIndex)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| key := outpoint.TxidStr + ":" + | |
| string(rune(outpoint.OutputIndex)) | |
| key := fmt.Sprintf("%s:%d", outpoint.TxidStr, outpoint.OutputIndex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
rpcserver.go
Outdated
| return nil, fmt.Errorf("can't create outpoints "+ | ||
| "%w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
- 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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
| return nil, fmt.Errorf("selected outpoints contain " + | ||
| "duplicate values") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message can be simplified by removing the string concatenation for better readability.
return nil, fmt.Errorf("selected outpoints contain duplicate values")References
- 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)
There was a problem hiding this comment.
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
| return nil, fmt.Errorf("can't convert outpoints "+ | ||
| "%w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
- 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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
8b70b79 to
52815a0
Compare
52815a0 to
202e473
Compare
sputn1ck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
EstimateFeenow takes optional inputs to give a fee estimate.Examples: