- 
                Notifications
    
You must be signed in to change notification settings  - Fork 2.2k
 
walletrpc: add raw_tx field to BumpFee response #10323
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
base: master
Are you sure you want to change the base?
walletrpc: add raw_tx field to BumpFee response #10323
Conversation
This commit adds an optional include_raw_tx field to BumpFeeRequest and a raw_tx field to BumpFeeResponse. When include_raw_tx is set to true, the RPC will wait for the sweep transaction to be created and return its hex-encoded raw transaction. This allows users to manually broadcast transactions in cases where they don't propagate automatically, addressing issue lightningnetwork#8470. Changes: - Add include_raw_tx bool field to BumpFeeRequest proto - Add raw_tx string field to BumpFeeResponse proto - Modify BumpFee implementation to optionally wait for sweep result - Update sweepNewInput to return result channel - Add --include_raw_tx flag to lncli wallet bumpfee command Closes lightningnetwork#8470
          Summary of ChangesHello @federicociro, 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 introduces a significant enhancement to 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 introduces a valuable feature by adding an optional include_raw_tx field to the BumpFee RPC call, allowing users to retrieve the raw transaction hex. The implementation is solid, correctly handling the asynchronous nature of sweep transaction creation using a result channel. My review includes a couple of suggestions to improve code style and documentation for better clarity and maintainability.
| return nil, fmt.Errorf("failed to "+ | ||
| "serialize tx: %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.
This string concatenation is unnecessary as the line is not exceeding the length limit. Combining it into a single string improves readability.1
return nil, fmt.Errorf("failed to serialize tx: %w", err)Style Guide References
Footnotes
- 
The style guide recommends minimizing lines for log and error messages. While it allows splitting long strings, this string is short enough to be on a single line, which improves readability. ↩
 
| /* | ||
| Optional. Whether to include the raw transaction hex in the response. This | ||
| is useful for cases where the transaction does not propagate and the caller | ||
| wants to manually broadcast it. | ||
| */ | 
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 comment for include_raw_tx should mention that setting this flag to true will cause the RPC call to block until the sweep transaction is created. This is a crucial detail for API users to understand the behavior of the call.
| /* | |
| Optional. Whether to include the raw transaction hex in the response. This | |
| is useful for cases where the transaction does not propagate and the caller | |
| wants to manually broadcast it. | |
| */ | |
| /* | |
| Optional. Whether to include the raw transaction hex in the response. This | |
| is useful for cases where the transaction does not propagate and the caller | |
| wants to manually broadcast it. Note that setting this to true will cause | |
| the RPC to block until the sweep transaction is created. | |
| */ | 
| 
           Hi @yyforyongyu! I've added the timeout fix commit to address the flaky tests. Could you approve the workflow run when you have a chance? Thanks!  | 
    
Delete CLAUDE.md
5a48d80    to
    1f6038e      
    Compare
  
    | 
           @federicociro, remember to re-request review from reviewers when ready  | 
    
| 
           Gentle ping @yyforyongyu when you have time 🙂  | 
    
Change Description
This PR adds an optional
include_raw_txfield toBumpFeeRequestand a correspondingraw_txfield toBumpFeeResponse. Wheninclude_raw_txis set totrue, the RPC willwait for the sweep transaction to be created and return its hex-encoded raw transaction in the response.
This addresses issue #8470, allowing users to manually broadcast transactions in cases where they don't propagate automatically through the network.
Changes Made:
include_raw_txbool field toBumpFeeRequestproto (lnrpc/walletrpc/walletkit.proto:3145)raw_txstring field toBumpFeeResponseproto (lnrpc/walletrpc/walletkit.proto:3255)BumpFeeimplementation to optionally wait for sweep result (lnrpc/walletrpc/walletkit_server.go:1157)sweepNewInputto return result channel for transaction tracking (lnrpc/walletrpc/walletkit_server.go:1292)--include_raw_txflag tolncli wallet bumpfeecommand (cmd/commands/walletrpc_active.go:319)Closes #8470
Steps to Test
Prerequisites:
make buildTesting the new functionality:
./lncli-debug wallet bumpfee --outpoint :
--sat_per_vbyte 10
Use the gRPC API directly with include_raw_tx: true and verify the response contains the raw transaction.
Use the returned raw transaction hex to manually broadcast via:
bitcoin-cli sendrawtransaction <raw_tx_hex>
Expected behavior:
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.