wallet: don't duplicate change output if already exist #27601
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK |
7ec4284 to
e46c127
Compare
There was a problem hiding this comment.
Updated per feedback, thanks @xekyo.
Added test coverage verifying that the same fee is paid when the wallet creates a change output automatically vs when the user provides the change output and the wallet just use it as recipient for the fee surplus.
e46c127 to
8f49902
Compare
ffcfe08 to
a8ac2ad
Compare
|
How would it work if coinselection produced result without change output? |
There was a problem hiding this comment.
How would it work if coinselection produced result without change output?
There is a test exercising the behavior inside a8ac2ad and also an explanation there.
But essentially, the process must adhere to the outputs specified by the user. If the change destination option in 'fundrawtransaction' matches one of the output scripts, the wallet is instructed to 'increase the specified output only if there is any change' and must not discard outputs predefined by the user under any circumstances
|
From CI https://cirrus-ci.com/task/4675435070488576?logs=ci#L3365: |
a8ac2ad to
a8f9633
Compare
a8f9633 to
06e0248
Compare
There was a problem hiding this comment.
Updated. Added required changes to make coin selection, change and fee calculations take into consideration the presence of an existing change output. Avoiding cases where we end up having to increase the change output at the end of the process by the fee surplus.
Changes:
- Cleaned two change related members from the
CoinSelectedParamsstruct that are not used inside the coin selection process. They were only intermediate results used to calculate other values. - Excluded the change cost from the waste score when the transaction already accounts for it.
- In transactions with an existing change output, the spending process will no longer compute all the change related coin selection params. By setting them to 0, we instruct coin selection to produce results closer to the target.
src/wallet/spend.cpp
Outdated
| // Calculate the cost of change | ||
| // Cost of change is the cost of creating the change output + cost of spending the change output in the future. | ||
| // For creating the change output now, we use the effective feerate. | ||
| // For spending the change output in the future, we use the discard feerate for now. | ||
| // So cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate) | ||
| coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); | ||
| coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(change_spend_size) + coin_selection_params.m_change_fee; | ||
| coin_selection_params.m_cost_of_change = discard_feerate.GetFee(change_spend_size) + coin_selection_params.m_change_fee; |
There was a problem hiding this comment.
in 9dc5bbe
I know this preserves current behaviour, but don't we want to use long_term_fee_rate to determine cost of change?
There was a problem hiding this comment.
I don’t know why the implementer of the waste metric in Bitcoin Core chose to use discard_feerate instead of LTFRE, but it does use discard_feerate throughout, so this would be consistent.
|
Approach ACK I think setting change related params to 0 in case where we reuse existing output is the correct way to go. |
06e0248 to
88916b1
Compare
If the transaction includes an output that goes to the custom change address (CoinControl::destChange), the transaction creation process should avoid creating duplicate outputs to the same address. Instead, it should reuse the existing output to prevent revealing which address is the change address, which could compromise privacy. This will also be useful to make other wallet processes less confusing and error-prone. For instance, within the bumpfee functionality, we are extracting the change recipient from the transaction before calling 'CreateTransaction' to ensure that the change output is not duplicated.
Coverage for fundrawtxn: 1) Should not create a second change output if there is one already going to that address. 2) The wallet changeless predilection should never discard a manually set output by the user. e.g. the user manually crafted a tx that already contain a change output with certain value. This output shouldn't get lost as it was predefined by the user. Only should be used as recipient of the "inputs - outputs" surplus during 'fundrawtransaction'.
The field is an intermediate result used to calculate other values and not read anywhere inside coin selection.
The field is an intermediate result used to calculate other values and not read anywhere inside coin selection.
Useful when the transaction already accounted for the change cost. Which happens when the user, or wallet process, set the coin control custom change address equal to one of the existing output addresses. In other words, when the user selects one of the transaction existing outputs to be increased by the difference between inputs - output. This will be connected to the transaction creation process in the following-up commit.
No behavior change. The initialization of all the CoinSelectionParams fields related to 'change' were moved to its own standalone function.
Instructing coin selection to not account for the 'change' costs.
For example:
* The minimum viable change will be 0, which means that the surplus
between inputs and outputs will never be sent to fees.
* With change_fee=0:
- The target value being sought will be the exact target (and not
target + change_fee).
- 'GetChange' will not deduce the change_fee from the value
calculation.
* The waste score calculation will exclude the change cost.
8c39dee to
349a8f0
Compare
Then it should just fail instead of merging the outputs. It should tell the user that they've done something wrong, as we do in the RPCs. I agree with @murchandamus's suggestion.
No, that forbids duplicates too.
I think the issue is mainly that this is not expected typical user behavior. We don't expect users to have an address as both a normal output and as the change address, in the same way that we don't expect users to have the same address twice. Because this is unexpected of our users, we shouldn't make assumptions about why they may be doing this and should instead tell them that they've done something unexpected. |
There was a problem hiding this comment.
Thanks achow for the feedback :).
That is not really how the wallet behaves for regular outputs. Not sure why it should
behave differently for change outputs only.Then it should just fail instead of merging the outputs. It should tell the user that they've done something wrong, as we do in the RPCs. I agree with @murchandamus's suggestion.
Ok, great. We are almost on the same page. There is one point about Murch's suggestion that makes some noise in my head.
Describing the general process first:
When the user manually specifies a custom change address/script, the transaction creation and transaction funding processes are instructed to follow one of the following option:
- If there is an existing output matching the user provided change destination: the process will send the change amount to the existing output.
- If there is no output matching the user provided change destination: the process will create a new output under the provided destination and send the change amount to it.
Murch's suggestion:
The only difference between the general process description and what murch suggested is the following one; he said to only perform (1) when the wallet organically detect the custom change address provided by the user as change (which makes the existing output matching this address also change). In other words, he said to only perform (1) when the wallet's IsChange() returns true for the provided destination.
Now, the discussion point.
What is the point behind forbidding this action? Does it have any advantage over the current status?
If the concerns are related to sending the change amount to an address not owned by the wallet; that is already possible today. Simply by specifying a custom change address, the user can send the change amount to any destination. This is part of the contract behind the utilization of the custom change address option.
My perspective is that what this PR introduces isn't radically different from our current process. The only difference arises when one of the user's manually specified outputs matches the user's manually specified change address. In such cases, we will increase the existing output instead of creating a second output going to the same address.
I mean, I feel that what we are introducing here is a natural behavior rather than something unexpected. As a user, if I provide a custom change address that relates to one of the outputs that I provided, it suggests my intention to send the change to the existing output. I don't think that anyone would expect another behavior.
But, having that said, I agree with you that we should explicitly state this behavior on the RPC custom change address option description. So there are no doubts about it.
Would you agree with this rationale, or do you still have second thoughts about it?
I think there is a difference in these two cases. a) When user provided the same address twice and we show an error, then the user can easily achieve the intended goal by just summing the amounts. Having two UTXOs don't give an advantage over a one with combined amount as spending conditions are the same anyway, so there is no point. b) If user provided the same address for output and change and we show an error. What the user should do? Pick a different change address? But new change address will have different spending conditions, so this is not an equivalent substitute. And even then, it's more expensive as it forces an extra output. I could be wrong, but there is no way for the user to specify they want to send change to an existing output |
|
For the But the above doesn't answer what should happen if someone does |
I disagree. I think it's a mistake if that happens. If the user wants to use something for change, then they shouldn't be giving it some value first, because it's change, not an recipient. On second thought, I think we should actually just always error, regardless of whether the wallet thinks the address is change.
Either pick a different change address, or remove the output.
There isn't, but it also doesn't make sense to me for someone to want to do that. Why specify an existing amount instead of just dropping it and letting the software add one if it has to? Maybe coin selection finds a solution that doesn't have change and the output doesn't actually need to exist. |
I think this is the crux of the discussion here, and we should agree on the use-case before talking about implementation. Specifying one of the existing outputs as change would be useful when a user wants to pay someone and send to themselves (at least X sats) in the same tx. I can think of following examples:
What do you think? From your experience, does it sound like realistic use-case? |
|
|
||
| # Instead of creating a new change value, the wallet should have increased the existing one | ||
| desc_tx = wallet.decoderawtransaction(funded_tx['hex']) | ||
| assert_equal(wallet.gettransaction(desc_tx['vin'][0]['txid'])['amount'], Decimal(50)) # assert input value |
There was a problem hiding this comment.
test 2023-09-11T08:16:14.393000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 127, in run_test
self.test_double_change()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 331, in test_double_change
assert_equal(wallet.gettransaction(desc_tx['vin'][0]['txid'])['amount'], Decimal(50)) # assert input value
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 57, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(-5.00000000 == 50)
Yeah those sound like things that I would consider doing if I were operating a high-volume wallet, e.g. in a multi-wallet setup the receive-only wallet could send change to the cold-wallet when topping up the send-only withdrawal wallet. |
| wallet = self.nodes[0] | ||
| wallet.syncwithvalidationinterfacequeue() # up-to-date wallet | ||
|
|
||
| # 1) fundrawtxn shouldn't create a second change output if there is one already going to that address |
There was a problem hiding this comment.
Regarding the commitment message and test in 84a37f8 ("test: fundrawtxn, add coverage for double change output creation"): If there is an output with a destination address and a manually set amount, it should not be considered change but a recipient. If there is an output that was calculated automatically and goes to the change address, it can be treated as ephemeral. Is there a way for us to reliably distinguish those two scenarios?
Either way, if there is a recipient with an explicitly set amount and the same address is provided as a custom change destination, that sounds like a conflicting instruction to me.
src/wallet/spend.cpp
Outdated
| // Calculate the cost of change | ||
| // Cost of change is the cost of creating the change output + cost of spending the change output in the future. | ||
| // For creating the change output now, we use the effective feerate. | ||
| // For spending the change output in the future, we use the discard feerate for now. | ||
| // So cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate) | ||
| coin_selection_params.m_change_fee = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.change_output_size); | ||
| coin_selection_params.m_cost_of_change = coin_selection_params.m_discard_feerate.GetFee(change_spend_size) + coin_selection_params.m_change_fee; | ||
| coin_selection_params.m_cost_of_change = discard_feerate.GetFee(change_spend_size) + coin_selection_params.m_change_fee; |
There was a problem hiding this comment.
I don’t know why the implementer of the waste metric in Bitcoin Core chose to use discard_feerate instead of LTFRE, but it does use discard_feerate throughout, so this would be consistent.
| @@ -449,7 +449,7 @@ void OutputGroupTypeMap::Push(const OutputGroup& group, OutputType type, bool in | |||
| } | |||
| } | |||
|
|
|||
| CAmount GetSelectionWaste(const std::set<std::shared_ptr<COutput>>& inputs, CAmount change_cost, CAmount target, bool use_effective_value) | |||
| CAmount GetSelectionWaste(const std::set<std::shared_ptr<COutput>>& inputs, std::optional<CAmount> change_cost, CAmount target, bool use_effective_value) | |||
There was a problem hiding this comment.
The majority of 83e85c0 "coinselection: option to exclude change cost from waste score" feels like a layer violation. If we don’t create change, that should be obvious from the excess being smaller than min_viable_change + change_fee. Let’s please not use change_cost as a dual-intent variable. change_cost/cost_of_change is the amount that it would cost to create and spend a change if there is one. If you want to communicate the intent of not creating change explicitly, please use a separate designated variable that only communicates that and only that.
If we can identify the change output, we should remove it before coin selection, redo coin selection with the existing inputs as preset inputs and then create a new change output. The change output should absolutely be part of the waste calculation, since the replacement transaction at a higher feerate may have multiple possible input sets, some of which might be changeless, which then should be preferred by the scoring accordingly.
Generally, if a custom change output address is provided, I would read that as “if there will be change, please send it to this”. If the user wants a specific output, they should add it as a recipient. If the user wants a specific output of at least some amount without creating change, they should send a higher amount and subtract the fee from that output, or use a sendall with preset inputs. So I don’t see how there should ever be a case of “the user explicitly wanted a specific change”—that would be the description of a recipient output.
| @@ -1071,7 +1071,7 @@ BOOST_AUTO_TEST_CASE(check_max_weight) | |||
| rand, | |||
| /*change_output_size=*/34, | |||
| /*min_change_target=*/CENT, | |||
| /*effective_feerate=*/CFeeRate(0), | |||
| /*effective_feerate=*/CFeeRate(1), // tiny feerate so the waste score isn't always 0. | |||
There was a problem hiding this comment.
I would like us to get away from predominantly using values that the wallet would never encounter in the normal operation in our tests. Unless we are testing edge cases, using values that are out-of-scope for the normal operation just confuses. Please use at least 1,000 for the effective feerate.
Also see: #27754
| if (!existent_change_out_index) { | ||
| ComputeChangeParams(coin_selection_params, wallet, scriptChange, recipients_sum, vecSend.size()); | ||
| } |
There was a problem hiding this comment.
No, please don’t do this. This is hacky and muddles meaning of variables.
The parameters of creating change are independent of whether change is created or not. In fact, we use some of these value in some of the coinselection algorithms to determine whether we have a changeless solution as well as in transaction building to determine the size of the change output. By fiddling with these values, it feels like we are coding against the implementation of the GetChange(…) function rather than against the function definition. Please use a new designated variable to explicitly track that transaction building should not create a change output if we absolutely must have that.
I would much prefer an approach that sidesteps this entire issue by starting the creation of an RBF transaction higher in the stack where we can remove a pre-existing identifiable change output from the draft transaction before just kicking off an entire transaction building process with some preset inputs.
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
Maybe mark as draft while this is stale? |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Are you still working on this? |
Probably next month. But no problem on closing it for now. Will re-open it or create a new one whenever I back to it. |
If the transaction includes an output that goes to the custom change
address (
CoinControl::destChange), the transaction creation processshould avoid creating duplicate outputs to the same address.
Instead, it should reuse the existing output to prevent revealing which
address is the change address, which could compromise privacy.
This will also be useful to make other wallet processes less confusing
and error-prone. For instance, within the bumpfee functionality, we
manually extract the change recipient from the transaction before calling
'CreateTransaction' to ensure that the change output is not duplicated.
Side note:
This is something that will be using for the #26732 rework.