rpc: add change_address parameter to SendCoins #10283
Conversation
Summary of ChangesHello @anumukul, 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature by adding a change_address parameter to the SendCoins RPC, allowing for more control over UTXOs. The changes across the RPC definitions, CLI, and wallet interfaces are well-implemented. However, I've identified a critical flaw in the btcwallet implementation that could lead to incorrect transaction creation. Additionally, there's a minor style guide violation regarding a missing function comment. Please see the detailed comments for more information.
| // sendOutputsWithCustomChange creates a transaction with a custom change | ||
| // address, signs it, and broadcasts it. | ||
| func (b *BtcWallet) sendOutputsWithCustomChange(inputs fn.Set[wire.OutPoint], | ||
| outputs []*wire.TxOut, feeRate chainfee.SatPerKWeight, minConfs int32, | ||
| label string, strategy base.CoinSelectionStrategy, | ||
| changeAddr btcutil.Address) (*wire.MsgTx, error) { | ||
|
|
||
| // First, create the transaction WITHOUT the custom change address. | ||
| // This will add a default change output if needed. | ||
| authoredTx, err := b.CreateSimpleTx( | ||
| inputs, outputs, feeRate, minConfs, strategy, false, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating transaction: %w", err) | ||
| } | ||
|
|
||
| // If there's a change output (ChangeIndex >= 0), we need to: | ||
| // 1. Replace it with our custom change address | ||
| // 2. Re-create and re-sign the transaction | ||
| if authoredTx.ChangeIndex >= 0 { | ||
| changeOutput := authoredTx.Tx.TxOut[authoredTx.ChangeIndex] | ||
| changeAmount := changeOutput.Value | ||
|
|
||
| // Create the script for the custom change address. | ||
| changePkScript, err := txscript.PayToAddrScript(changeAddr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error creating change script: %w", err) | ||
| } | ||
|
|
||
| // Create new outputs list with the custom change address. | ||
| newOutputs := make([]*wire.TxOut, 0, len(outputs)+1) | ||
|
|
||
| // Add all original outputs (not including change). | ||
| for i, out := range authoredTx.Tx.TxOut { | ||
| if i != int(authoredTx.ChangeIndex) { | ||
| newOutputs = append(newOutputs, out) | ||
| } | ||
| } | ||
|
|
||
| // Add our custom change output. | ||
| newOutputs = append(newOutputs, &wire.TxOut{ | ||
| Value: changeAmount, | ||
| PkScript: changePkScript, | ||
| }) | ||
|
|
||
| // Re-create the transaction with the custom change output. | ||
| authoredTx, err = b.CreateSimpleTx( | ||
| inputs, newOutputs, feeRate, minConfs, strategy, false, | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error re-creating transaction with custom change: %w", err) | ||
| } | ||
| } | ||
|
|
||
| // CreateSimpleTx returns a signed transaction, so we just need to publish it. | ||
| err = b.PublishTransaction(authoredTx.Tx, label) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error publishing transaction: %w", err) | ||
| } | ||
|
|
||
| return authoredTx.Tx, nil | ||
| } |
There was a problem hiding this comment.
The current implementation of sendOutputsWithCustomChange has a fundamental flaw that can lead to incorrect transaction creation or failures.
The function attempts a two-step process:
- It calls
CreateSimpleTxto generate a transaction and determine the change amount. - It then calls
CreateSimpleTxa second time with a new set of outputs that includes the custom change address, using the change amount from the first call.
This approach is problematic for a few reasons:
- Incorrect Change Amount: The second call to
CreateSimpleTxre-runs coin selection and fee calculation. If the custom change address has a different script size than the wallet's default change address, the transaction size will change, leading to a different fee. The change amount from the first call will then be incorrect for the second transaction. This could result in a transaction wheresum(outputs) > sum(inputs), making it invalid. - Double Change Output: Because the second call to
CreateSimpleTxperforms coin selection and change calculation again, if the inputs and outputs don't perfectly balance (which is likely if the fee changes), it will add another change output, this time to a wallet-generated address. This would result in a transaction with two change outputs, which is not the desired behavior.
A correct implementation needs to avoid this two-step creation process. Since the underlying btcwallet API does not seem to support providing a custom change address directly during transaction creation, a more robust approach is required. This might involve manually constructing the transaction after a single coin selection pass to ensure all values are calculated correctly for the final transaction structure.
| func (r *rpcServer) sendCoinsOnChain(paymentMap map[string]int64, | ||
| feeRate chainfee.SatPerKWeight, minConfs int32, label string, | ||
| strategy wallet.CoinSelectionStrategy, | ||
| selectedUtxos fn.Set[wire.OutPoint]) (*chainhash.Hash, error) { | ||
| selectedUtxos fn.Set[wire.OutPoint], | ||
| changeAddr btcutil.Address) (*chainhash.Hash, error) { |
|
Thanks for your PR @anumukul! Could you please place your changes into different commits to ease review. Please also check the review that gemini code assist gave. |
|
@anumukul please go through the code contribution guidelines: https://github.com/lightningnetwork/lnd/blob/master/docs/code_contribution_guidelines.md |
There was a problem hiding this comment.
Build Issue:
lnrpc/walletrpc/walletkit_server.go:824:20: not enough arguments in call to w.cfg.Wallet.SendOutputs
have (nil, []*"github.com/btcsuite/btcd/wire".TxOut, chainfee.SatPerKWeight, int32, string, wallet.CoinSelectionStrategy)
want (fn.Set["github.com/btcsuite/btcd/wire".OutPoint], []*"github.com/btcsuite/btcd/wire".TxOut, chainfee.SatPerKWeight, int32, string, wallet.CoinSelectionStrategy, btcutil.Address)
few additional comments:
- Please try running
make fmt- there seem to be some formatting inconsistencies. - Run
make rpcto ensure all generated files are updated. - It would be great if you could add tests for your changes.
- Also, can you ensure that your changes (hint: exported function arguments) are backward compatible?
Update WalletController.SendOutputs to accept an optional change address parameter, allowing callers to specify where change should be sent.
Add sendOutputsWithCustomChange function that creates transactions with custom change addresses. If no change is needed or no custom address is provided, falls back to standard wallet behavior. The implementation creates an initial transaction to determine change amount, then recreates the transaction with the custom change output using btcwallet's SendOutputs method for proper signing.
Update SendOutputs to accept changeAddr parameter for interface compatibility.
Update SendOutputs call to include nil for the new changeAddr parameter, maintaining existing behavior.
- Parse and validate change_address from SendCoinsRequest - Support custom change address in both regular sends and sweepall - Pass change address through sendCoinsOnChain to wallet - Add function documentation for sendCoinsOnChain - Update SendMany to pass nil for backwards compatibility
Add optional --change_address flag allowing users to specify where change should be sent, enabling systematic draining of addresses.
|
Thanks for the detailed reviews! I've addressed all the feedback: Changes Made:Fixed critical bug in sendOutputsWithCustomChange:
Added function comment:
Split into separate commits:
Fixed build error:
Ran make fmt:
Make rpc:
Tests: I'd like to add tests but would appreciate guidance on:
Backward compatibility: The interface change adds a new optional parameter at the end. All existing callers pass Ready to iterate based on your feedback! |
I think it would be better if you compile using
Preferably both. I haven’t fully gone through the concept of the PR yet, but you can check the existing
I don’t think this is how backward compatibility works. It might help to read more about it to understand the concept better. Also, I recommend going through the https://github.com/lightningnetwork/lnd/blob/master/docs/development_guidelines.md as there are still a few conventions that need to be followed, for eg.- the 80 character line length. |
Keep the original SendOutputs method unchanged and add a new SendOutputsWithChangeAddr method to maintain backward compatibility with existing code that uses the WalletController interface.
Implement the new SendOutputsWithChangeAddr method while keeping the original SendOutputs for backward compatibility. The old method now delegates to the new one with nil changeAddr.
Add SendOutputsWithChangeAddr method to all mock implementations while preserving the original SendOutputs method.
Implement both SendOutputs and SendOutputsWithChangeAddr methods for backward compatibility.
Update sendCoinsOnChain to use SendOutputsWithChangeAddr when a custom change address is provided, otherwise use standard SendOutputs. This maintains backward compatibility.
Add optional change_address parameter to SendOutputsRequest proto. The RPC handler continues to use SendOutputs (without custom change) for now.
Regenerate all protobuf files using Docker-based proto compiler.
Thanks for the detailed feedback! I've addressed all the issues: Fixed Backward CompatibilityThe original approach broke backward compatibility by changing the interface signature. I've now:
This ensures any external code using the Ran
|
@anumukul – This may came across as unsolicited advice but please try to write responses in your own words instead of using Large Language Model. That way it would feel the OP responses more humanized and to the point |
5 participants
Summary
Adds
change_addressparameter toSendCoinsRPC andsendcoinsCLI command, allowing users to drain addresses by controlling where change is sent.Closes #10271
Changes
change_addressfield toSendCoinsRequest(lightning.proto) andSendOutputsRequest(walletkit.proto)WalletController.SendOutputsinterface to accept change address parameter--change_addressflag tosendcoinsCLI commandUsage