bumpfee: Allow the user to choose which output is change #26467

achow101 · 2022-11-07T23:38:09Z

When bumping the transaction fee, we will try to find the change output of the transaction in order to have an output whose value we can modify so that we can meet the target feerate. However we do not always find the change output which can cause us to unnecessarily add an additional output to the transaction. We can avoid this issue by outsourcing the determination of change to the user if they so desire.

This PR adds a orig_change_pos option to bumpfee which the user can use to specify the index of the change output.

Fixes #11233
Fixes #20795

luke-jr

We support subtracting fee from output, so I think the "reduce_output" name used in #12096 was better. (Would be nice if that situation could be detected, but I don't think we save the metadata needed for that.)

Also, might make sense to split the GUI part off to the GUI repo after the rest is merged. Seems like a hard thing to get a good UX for.

ghost · 2022-11-08T22:51:05Z

Concept ACK

DrahtBot · 2022-11-09T16:05:07Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ismaelsadeeq, pinheadmz, furszy
Concept ACK	ghost, john-moffett, RandyMcMillan
Stale ACK	Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/700 (Dialog for allowing the user to choose the change output when bumping a tx by achow101)
#27829 (rpc: fix data optionality for RPC calls. by miketwenty1)
#25979 ([WIP] wallet: standardize change output detection process by furszy)

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.

john-moffett · 2022-11-16T16:16:28Z

Concept ACK.

Tested and code reviewed 73d4ba2. Suggest changing the dialog UI to support scrolling and resizing in case there are lots of outputs:

Screenshot

Here is a drop-in gist to implement scrolling and resizing, which changes it to:

Screenshot

If a user creates a single-output tx and chooses that output as "change" to fee-bump, it will fail with the "Transaction must have at least one recipient" message. This makes sense if the user always expects the "payment" output to remain the same value, but if they're consolidating UTXOs or something, this may be desirable to allow.

Sjors · 2022-12-12T20:06:11Z

We support subtracting fee from output, so I think the "reduce_output" name used in #12096 was better.

Agreed. The help text could clarify that in the general case you should pick the change address output for this.

achow101 · 2023-01-23T21:15:51Z

We support subtracting fee from output, so I think the "reduce_output" name used in #12096 was better. (Would be nice if that situation could be detected, but I don't think we save the metadata needed for that.)

I've taken the name, but I'm not entirely sure that it is accurate. While we will most likely reduce the amount of the specified output, it's not the only outcome. It could be that specified output does not provide enough value, in which case we will add additional inputs and set the output's value as the resulting change value, which will be higher than the original value.

Also, might make sense to split the GUI part off to the GUI repo after the rest is merged. Seems like a hard thing to get a good UX for.

Done, moved the GUI commits to bitcoin-core/gui#700

Suggest changing the dialog UI to support scrolling and resizing in case there are lots of outputs:

Took this suggestion in the PR to the GUI repo.

If a user creates a single-output tx and chooses that output as "change" to fee-bump, it will fail with the "Transaction must have at least one recipient" message. This makes sense if the user always expects the "payment" output to remain the same value, but if they're consolidating UTXOs or something, this may be desirable to allow.

Added a commit which handles that case.

Sjors · 2023-01-24T13:31:11Z

Concept ACK

Did you mean for the middle commit to have txid: as its prefix?

On signet with an empty mempool, when I send a transaction with a single input at 1 sat/vbyte and then try to bump it to 2 sat/byte, it complains:

Insufficient total fee 0.00000222, must be at least 0.00000666 (oldFee 0.00000111 + incrementalFee 0.00000555)

getnetworkinfo says "incrementalfee": 0.00001000

Maybe maxTxSize is too pessimistic?

achow101 · 2023-01-24T18:07:01Z

Did you mean for the middle commit to have txid: as its prefix?

Nope, fixed.

On signet with an empty mempool, when I send a transaction with a single input at 1 sat/vbyte and then try to bump it to 2 sat/byte, it complains:
Insufficient total fee 0.00000222, must be at least 0.00000666 (oldFee 0.00000111 + incrementalFee 0.00000555)
getnetworkinfo says "incrementalfee": 0.00001000

Maybe maxTxSize is too pessimistic?

I'm going to say that this is an orthogonal issue. It looks like the wallet has a minimum incremental feerate of 5 sat/vb, rather than the 1 sat/vb the mempool uses.

Sjors

tACK 0b280af

Sjors · 2023-01-26T14:31:56Z

src/wallet/feebumper.cpp


-    return Result::OK;
+        return Result::OK;


0b280af nit: return could be outside the if else. If you touch it, there's also a typo in the commit description: transcation

Sjors · 2023-02-22T14:58:04Z

utACK 4167843. The latest rebase takes the new outputs argument into account from #25344. It ensures they can't be combined, so that's good.

pinheadmz

ACK 4167843

reviewed code, ran tests. Played with the feature on regtest and in particular using reduce_output to intentionally reduce the recipient output instead of the original change output added automatically by sendtoaddress. I don't think is explicitly covered in the test but could be. One other quesiton about the test below but otherwise LGTM

Show Signature

pinheadmz's public key is on keybase

pinheadmz · 2023-02-24T16:02:10Z

test/functional/wallet_bumpfee.py

+
+        new_tx = rbf_node.gettransaction(txid=new_txid, verbose=True)
+        assert_equal(len(new_tx["decoded"]["vout"]), 2)
+        new_change_pos = find_vout_for_address(rbf_node, new_txid, change_addr)


Will new_change_pos == change_pos always here? Or are the outputs of the replacement re-randomized?

They will be re-randomized.

RandyMcMillan · 2023-05-01T15:39:06Z

Concept ACK

src/wallet/feebumper.cpp

ismaelsadeeq · 2023-06-24T12:54:09Z

tACK dff7235
Tested the reduce_output option on regtest with bumpfee RPC, the value of the output in the reduced_output was decreased to fee bump the transaction instead of adding another input.
tests fail on master and pass on dff7235.
Code looks good to me

src/wallet/feebumper.h

If the user used a custom change address, it may not be detected as a change output, resulting in an additional change output being added to the bumped transaction. We can avoid this issue by allowing the user to specify the position of the change output.

ismaelsadeeq · 2023-06-27T09:31:18Z

re ACK e8c31f1

pinheadmz · 2023-07-17T14:31:27Z

Additionally, a new dialog is added to the GUI that allows the user to select the change output when they are bumping the transaction fee. This dialog will default select output that the wallet determines to be change, if it could find one.

Not seeing this in this branch.

pinheadmz

re-ACK e8c31f1

Reviewed all code changes, ran tests locally. I played with the feature in regtest with wallet TXs as well as PSBTs. Made sure invalid input threw errors and expected output was reduced by fee bump. Found one nit below. Maybe remove GUI from OP description? Is that being added in a bitcoin-core/gui PR ?

Show Signature

pinheadmz's public key is on keybase

pinheadmz · 2023-07-17T14:32:34Z

src/wallet/feebumper.cpp

-        } else {
+    for (size_t i = 0; i < txouts.size(); ++i) {
+        const CTxOut& output = txouts.at(i);
+        if (reduce_output.has_value() ?  reduce_output.value() == i : OutputIsChange(wallet, output)) {


nit: extra space?

Suggested change

if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) {

if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) {

achow101 · 2023-07-17T15:27:09Z

Additionally, a new dialog is added to the GUI that allows the user to select the change output when they are bumping the transaction fee. This dialog will default select output that the wallet determines to be change, if it could find one.

Not seeing this in this branch.

That was moved to bitcoin-core/gui#700. Dropped that from the PR description.

furszy · 2023-06-09T21:26:22Z

src/wallet/feebumper.cpp

+    for (size_t i = 0; i < txouts.size(); ++i) {
+        const CTxOut& output = txouts.at(i);
+        if (reduce_output.has_value() ?  reduce_output.value() == i : OutputIsChange(wallet, output)) {
            CTxDestination change_dest;
            ExtractDestination(output.scriptPubKey, change_dest);
            new_coin_control.destChange = change_dest;
+        } else {


If the transaction has two outputs: index 0 not change, and index 1 change for the wallet. When reduce_output=0, the first round of the loop will set destChange to the first output script, then the second round will overwrite destChange with the second output script.

This will have the bad outcome of dropping the first output (the one selected by the user) and send all coins to the second one. Leaving the bumped tx with only one output.

A solution for this could be to not discard change outputs from the recipients vector. But, for that, we need #27601. Which basically enables this feature.

So instead of having this feebumper loop dropping the change output, we can append it to the recipients list and specify inside coin control that we want to send any surplus between inputs and outputs there.

Weird, this comment was posted yesterday but is dated over a month ago (Jun 9). I thought for sure I had tested this case locally and didn't see a missing output -- is this still the case here? Since we check reduce_output.has_value() in the if condition, doesn't that mean that if that option is set only the selected option will be designated as change?

I thought for sure I had tested this case locally and didn't see a missing output -- is this still the case here? Since we check reduce_output.has_value() in the if condition, doesn't that mean that if that option is set only the selected option will be designated as change?

Yeah. Seems that I was partially blind yesterday. This issue can still be triggered by a transaction with +2 change outputs and no reduce_output (OutputIsChange() will be true for them, which overwrites destChange). But.. it isn't related to this PR.
So.. nvm. Will do another review round. Thanks @pinheadmz.

furszy

Code review ACK e8c31f1

furszy · 2023-07-18T13:49:43Z

src/wallet/rpc/spend.cpp

                        OutputsDoc(),
                        RPCArgOptions{.skip_type_check = true}},
+                    {"reduce_output", RPCArg::Type::NUM, RPCArg::DefaultHint{"not set, detect change automatically"}, "The 0-based index of the output from which the additional fees will be deducted. In general, this should be the position of change output. Cannot be provided if 'outputs' is specified."},


In 7d83502:

The first sentence "The 0-based index of the output from which the additional fees will be deducted" isn't totally accurate to what will happen.

If the bumpfee process adds another input to satisfy the new fee, the reduce_output output will also be increased by the new inputs - outputs surplus.

@furszy makes an important point. Consider the case where I am paying someone on chain under the stipulation that they will eat the mining fee. (Thus, I specify their address in subtractfeefrom when I'm constructing my transaction.) Later, they tell me that they can't wait for confirmation any longer and need an urgent fee bump. If I naïvely specify their output as the reduce_output, I may end up paying them much more than I intended to if I only have large UTxOs in my wallet.

@whitslack yes. That would be an ugly outcome. We could have fixed the RPC docs here.

Would say to work towards #27601 to fix this scenario (and others) properly.
This issue comes from a workaround code that avoids the dup change outputs bug. Basically, the change output is manually discarded from the recipients list and expected to be re-added later, with the new fees subtracted, by the inner transaction creation process logic.

With #27601, we will be able to specify the output to reduce (SFFO) and the existent change output unequivocally.

This seems like a very serious bug that should have blocked the merge... :|

Should we disable the adding of new inputs when reduce_output is set? If someone really intends to do that, they should probably use outputs.

Made a separate issue to track this: #28180

…s change

donsheddy4

Very good

luke-jr suggested changes Nov 7, 2022

View reviewed changes

achow101 force-pushed the bumpfee-choose-change-txout branch from 527b89d to 6d10ece Compare November 8, 2022 22:29

achow101 force-pushed the bumpfee-choose-change-txout branch from 6d10ece to 73d4ba2 Compare November 9, 2022 21:13

This was referenced Nov 10, 2022

[WIP] wallet: standardize change output detection process #25979

Closed

New outputs argument for bumpfee/psbtbumpfee #25344

Merged

Improve display address handling for external signer #24313

Merged

DrahtBot mentioned this pull request Dec 17, 2022

wallet, gui: bugfix, getAvailableBalance skips selected coins #26699

Merged

DrahtBot mentioned this pull request Jan 7, 2023

tests: Allow tests to use a loopback address other than 127.0.0.1 for more test runner parallelism #26841

Closed

achow101 force-pushed the bumpfee-choose-change-txout branch 2 times, most recently from bf222ab to ceab670 Compare January 23, 2023 21:11

achow101 mentioned this pull request Jan 23, 2023

Dialog for allowing the user to choose the change output when bumping a tx bitcoin-core/gui#700

Open

DrahtBot mentioned this pull request Jan 24, 2023

wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction #25273

Merged

achow101 force-pushed the bumpfee-choose-change-txout branch from ceab670 to 0b280af Compare January 24, 2023 18:05

Sjors mentioned this pull request Jan 26, 2023

bump: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee rate #26973

Closed

Sjors approved these changes Jan 26, 2023

View reviewed changes

DrahtBot added the Needs rebase label Feb 16, 2023

achow101 force-pushed the bumpfee-choose-change-txout branch from 0b280af to 4167843 Compare February 17, 2023 20:28

DrahtBot removed the Needs rebase label Feb 17, 2023

fanquake requested review from furszy, john-moffett and pinheadmz February 22, 2023 17:47

pinheadmz approved these changes Feb 24, 2023

View reviewed changes

achow101 marked this pull request as ready for review May 19, 2023 17:14

This was referenced Jun 5, 2023

Silent Payments: send and receive #27827

Closed

rpc: fix data optionality for RPC calls. #27829

Merged

DrahtBot added the CI failed label Jun 7, 2023

achow101 force-pushed the bumpfee-choose-change-txout branch from ec3b93d to dff7235 Compare June 8, 2023 10:10

DrahtBot removed the CI failed label Jun 8, 2023

ismaelsadeeq reviewed Jun 24, 2023

View reviewed changes

src/wallet/feebumper.cpp Show resolved Hide resolved

DrahtBot requested review from pinheadmz and Sjors June 24, 2023 12:54

ismaelsadeeq reviewed Jun 24, 2023

View reviewed changes

src/wallet/feebumper.h Outdated Show resolved Hide resolved

achow101 force-pushed the bumpfee-choose-change-txout branch from dff7235 to 2eeebc5 Compare June 26, 2023 17:29

achow101 added 3 commits June 26, 2023 17:49

test: Test bumpfee reduce_output

4f4d440

tests: Test for bumping single output transaction

e8c31f1

achow101 force-pushed the bumpfee-choose-change-txout branch from 2eeebc5 to e8c31f1 Compare June 26, 2023 21:57

pinheadmz approved these changes Jul 17, 2023

View reviewed changes

furszy reviewed Jul 17, 2023

View reviewed changes

furszy reviewed Jul 18, 2023

View reviewed changes

fanquake merged commit 04afe55 into bitcoin:master Jul 20, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 21, 2023

Merge bitcoin#26467: bumpfee: Allow the user to choose which output i…

f657f46

…s change

murchandamus mentioned this pull request Jul 27, 2023

wallet: don't duplicate change output if already exist #27601

Closed

Sjors mentioned this pull request Jul 28, 2023

Unsafe reduce_output when new coins are added #28180

Closed

1 task

donsheddy4 reviewed Aug 9, 2023

View reviewed changes

jonatack mentioned this pull request Sep 19, 2023

rpc: bumpfee, improve doc for 'reduce_output' arg #28505

Merged

bitcoin locked and limited conversation to collaborators Aug 8, 2024

	if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) {
	if (reduce_output.has_value() ? reduce_output.value() == i : OutputIsChange(wallet, output)) {

bumpfee: Allow the user to choose which output is change #26467

bumpfee: Allow the user to choose which output is change #26467

Uh oh!

Conversation

achow101 commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 8, 2022

Uh oh!

DrahtBot commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

john-moffett commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Dec 12, 2022

Uh oh!

achow101 commented Jan 23, 2023

Uh oh!

Sjors commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jan 24, 2023

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Feb 22, 2023

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan commented May 1, 2023

Uh oh!

Uh oh!

ismaelsadeeq commented Jun 24, 2023

Uh oh!

Uh oh!

ismaelsadeeq commented Jun 27, 2023

Uh oh!

pinheadmz commented Jul 17, 2023

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

achow101 commented Nov 7, 2022 •

edited

Loading

DrahtBot commented Nov 9, 2022 •

edited

Loading

john-moffett commented Nov 16, 2022 •

edited

Loading

Sjors commented Jan 24, 2023 •

edited

Loading