[feature]: add WithChangeAddress TxCreateOption for custom change output destination#1235
Open
murraystewart96 wants to merge 6 commits into
Open
[feature]: add WithChangeAddress TxCreateOption for custom change output destination#1235murraystewart96 wants to merge 6 commits into
murraystewart96 wants to merge 6 commits into
Conversation
yyforyongyu
reviewed
Jun 2, 2026
yyforyongyu
left a comment
yyforyongyu
left a comment
Collaborator
There was a problem hiding this comment.
LGTM 🏷️ (deepseek-v4-pro)
yyforyongyu
reviewed
Jun 2, 2026
yyforyongyu
reviewed
Jun 2, 2026
yyforyongyu
reviewed
Jun 2, 2026
yyforyongyu
reviewed
Jun 2, 2026
yyforyongyu
reviewed
Jun 2, 2026
yyforyongyu
reviewed
Jun 2, 2026
yyforyongyu
left a comment
yyforyongyu
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the PR. I've started the agents to perform an initial review, meanwhile, I would suggest putting this PR on hold, as we are in the process of a major refactor/upgrade, the milesteon can be tracked here.
FundPsbt calls ScriptForOutput on the change output to attach BIP32 derivation metadata, which requires the address to be wallet-owned. An external change address has no such record and would cause the call to fail. Return a clear error if WithChangeAddress is set rather than letting it fail deep inside the PSBT funding path.
…fication Two fixes for external custom change addresses: 1. Validate that the change address belongs to the wallet's network before building the pkScript. 2. Skip the NotifyReceived call for external change addresses. Registering a non-wallet address with the chain backend causes future payments to that address to be delivered as wallet notifications. Update TestTxToOutputsCustomChangeAddr to use a testnet address, which now correctly exercises the network validation.
Author
|
@yyforyongyu Thanks for reviewing. I have addressed the issues highlighted by the agents. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change Description
Description:
This change extends txToOutputs and addrMgrWithChangeSource to accept an optional changeAddr btcutil.Address parameter. When non-nil, the change source is built as a static script paying to that address rather than deriving a fresh HD wallet address. When nil, the existing derivation path is taken unchanged — there is no behaviour change for existing callers.
Steps to Test
added new test in createtx_test.go - TestTxToOutputsCustomChangeAddr
Pull Request Checklist
Testing
Code Style and Documentation
📝 Please see our Contribution Guidelines for further guidance.