bumpfee: Allow the user to choose which output is change

achow101 commented at 11:38 pm on November 7, 2022: member

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 changes_requested

luke-jr commented at 11:43 pm on November 7, 2022: member

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.

achow101 force-pushed on Nov 8, 2022

ghost commented at 10:51 pm on November 8, 2022: none

Concept ACK

DrahtBot commented at 4:05 pm on November 9, 2022: contributor

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.

achow101 force-pushed on Nov 9, 2022

john-moffett commented at 4:16 pm on November 16, 2022: contributor

Concept ACK.

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

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

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 commented at 8:06 pm on December 12, 2022: member

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 force-pushed on Jan 23, 2023

achow101 commented at 9:15 pm on January 23, 2023: member

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 https://github.com/bitcoin-core/gui/pull/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 commented at 1:31 pm on January 24, 2023: member

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:

0Insufficient 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 force-pushed on Jan 24, 2023

achow101 commented at 6:07 pm on January 24, 2023: member

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:
0Insufficient 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.

in src/wallet/feebumper.cpp:313 in 0b280affd4 outdated

307-    mtx = CMutableTransaction(*txr.tx);
308+        // Write back transaction
309+        mtx = CMutableTransaction(*txr.tx);
310 
311-    return Result::OK;
312+        return Result::OK;

Sjors commented at 2:31 pm on January 26, 2023:

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

Sjors approved

Sjors commented at 2:34 pm on January 26, 2023: member

tACK 0b280affd4c755d8f9eb809b3676389b2c2c52c0

DrahtBot added the label Needs rebase on Feb 16, 2023

achow101 force-pushed on Feb 17, 2023

DrahtBot removed the label Needs rebase on Feb 17, 2023

Sjors commented at 2:58 pm on February 22, 2023: member

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

fanquake requested review from furszy on Feb 22, 2023

fanquake requested review from john-moffett on Feb 22, 2023

fanquake requested review from pinheadmz on Feb 22, 2023

in test/functional/wallet_bumpfee.py:262 in 4167843e07 outdated

200+        bumped = rbf_node.bumpfee(txid, {"reduce_output": change_pos})
201+        new_txid = bumped["txid"]
202+
203+        new_tx = rbf_node.gettransaction(txid=new_txid, verbose=True)
204+        assert_equal(len(new_tx["decoded"]["vout"]), 2)
205+        new_change_pos = find_vout_for_address(rbf_node, new_txid, change_addr)

pinheadmz commented at 4:02 pm on February 24, 2023:

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

achow101 commented at 5:29 pm on March 15, 2023:

They will be re-randomized.

pinheadmz approved

pinheadmz commented at 4:39 pm on February 24, 2023: member

ACK 4167843e0729994954317e2a4ac8a96a453bad79

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

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 4167843e0729994954317e2a4ac8a96a453bad79
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmP4544ACgkQ5+KYS2KJ
 7yTr/hw//RCFzObS/NFeiJjG4Btb6kuDMDW7SYmXaEazT0mX91QpiQlq+cW92/FZa
 8FDqycJs43hMMu0ytmTJ8qs9S0sROU0IYRJghK16Ne16TBhtJwbpS93NIQo4LQY7j
 9a77Ql9DmDGt/c5EkN+lQsvk6W6PXlo1NqwvXT5Rn3NXnbJ0yD4PCo5ZQW1GQ305i
106pGboMlx97PGRtnCsFvfO6+lOAUlAPiwYXt8a+yfT2jhhFzmoi0KG4NlcPLHmwCL
11OPKken36AE/2WMpZLu12nDMAb6qM3ia7/GiKxEB6+C0j0RBxYF4RAjmKxgNo/XJ/
12f2yZiPmDSsGnZWaYRMpOjR5s0HgQS98WRUK17ZJfz8tN7qR9Y8L8wUwLTpvqHM/a
13i+Je0mXY1k5iUtkOG9D9PKAAmsQ6OQzoCW1vzuKL1Z1jc2nKZHbuYbuE8OzyKlGc
14A4gv/pNSyaWLHP7TqwvzxHTVlCNlbp+bN+PB21ojd4xsItCU5AGbLAxAMF+LJnwO
15YL5xI+29voZcnZBiQI9/jHV8VJ5mUYYDT4f/nQYkTlMmAcBx7BzPF4Zh3h7gKOBA
16ybhrHHsSNiXVcm2VIqI6Zb/Wvn/zRTJscxvRQNDuYvEGeCMjQSc1uaGulFflWLmq
17qtGtR6U4j4GRTL8TyQcAZu4VfuaUH3up2Km3cKAAFvNqbaKOqlo=
18=A42I
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

in src/wallet/feebumper.h:47 in a902eaf5be outdated

42@@ -43,6 +43,7 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid);
43  * @param[out] new_fee the fee that the bump transaction pays
44  * @param[out] mtx The bump transaction itself
45  * @param[in] require_mine Whether the original transaction must consist of inputs that can be spent by the wallet
46+ * @param[in] reduce_output The position of the change output to deduct the fee from in the transaction being bumped

furszy commented at 10:52 pm on February 27, 2023:

In a902eaf5: While you are here, could add the missing doc for the @param[in] outputs arg.

achow101 commented at 5:33 pm on March 15, 2023:

Done

in src/wallet/feebumper.cpp:252 in a902eaf5be outdated

252-            recipients.push_back(recipient);
253-        } else {
254+    const auto& tx_outs = outputs.empty() ? wtx.tx->vout : outputs;
255+    for (size_t i = 0; i < tx_outs.size(); ++i) {
256+        const CTxOut& output = tx_outs.at(i);
257+        if ((reduce_output.has_value() && reduce_output.value() == i) || (!reduce_output.has_value() && OutputIsChange(wallet, output))) {

furszy commented at 11:00 pm on February 27, 2023:

In https://github.com/bitcoin/bitcoin/commit/a902eaf5be4a83dff37f28a988e5f3b2425551c0:

Could write it prettier:

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

achow101 commented at 5:33 pm on March 15, 2023:

Done

furszy commented at 11:38 pm on February 27, 2023: member

Concept ACK, left two nits. Will keep reviewing.

in src/wallet/feebumper.cpp:325 in 4167843e07 outdated

331+        if (IsDust(temp_mtx.vout[0], wallet.chain().relayDustFee())) {
332+            errors.push_back(Untranslated("Single output amount too small"));
333+            return Result::MISC_ERROR;
334+        }
335+        mtx = temp_mtx;
336+        return Result::OK;

furszy commented at 3:26 pm on March 3, 2023:

This is not contemplating the user changing the outputs for a single change output. E.g. bump tx to send coins back to yourself.

The generated temp_mtx uses the same outputs as the previous transaction, so the provided output is discarded. And only deduces the new fee from the previous tx outputs.

Previously, in master, we would just fail on this situation. While here, we return a tx that is not what the user wants. So, created #27195 to solve the issue in master first.

achow101 commented at 5:52 pm on March 15, 2023:

I’ve added a fix, and adopted your test. The fix is simple enough to do here that I think a separate PR is unnecessary.

achow101 force-pushed on Mar 15, 2023

in src/wallet/feebumper.cpp:252 in ffc4389022 outdated

252-            recipients.push_back(recipient);
253-        } else {
254+    const auto& tx_outs = outputs.empty() ? wtx.tx->vout : outputs;
255+    for (size_t i = 0; i < tx_outs.size(); ++i) {
256+        const CTxOut& output = tx_outs.at(i);
257+        if ((reduce_output.has_value() ?  reduce_output.value() == i : OutputIsChange(wallet, output))) {

furszy commented at 9:40 pm on March 17, 2023:

nit: double unneeded ()

achow101 commented at 11:20 pm on March 17, 2023:

Fixed.

in src/wallet/feebumper.cpp:319 in 5ac20d3e7b outdated

326+    } else {
327+        // The transaction only has one output, and it is either detected as change,
328+        // or selected as the one to take the fee from.
329+        // In that case, just reduce its value to meet the new feerate.
330+        new_fee = new_coin_control.m_feerate->GetFee(maxTxSize);
331+        temp_mtx.vout[0].nValue = input_value - new_fee;

furszy commented at 10:15 pm on March 17, 2023:

There still is a problem with this approach when the user sets a value for the single output.

Basically, the user provided value is being discarded. We don’t check if it’s within the inputs amount bounds nor if need to create a change output for the remaining amount (value can be lower than the inputs amount).

Current code instead of create a change output when the value is lower than the inputs amount, it sets the value equal to inputs total minus new fee. We should code a test for it too.

Thus why thought about using SFFO in 27195 to mark the output from where we want to subtract fees, so the apparent “single output tx” can be crafted using CreateTransaction instead of making an special case here.

Could use that instead of using the change destination. Outputs cannot be changed if the “reduce” option is set.

Side from that, all good about closing 27195. I made it just because it’s a bug in master.

achow101 commented at 11:19 pm on March 17, 2023:

Ah, I see. Actually I’ve decided to just rebase this PR on top of #27195 and drop the single output exception commit, although I’ve kept the test.

furszy commented at 3:24 am on March 22, 2023:

k, either way is fine for me. Cool about the special case exception removal and the new test. just updated 27195 with further tests too.

achow101 force-pushed on Mar 17, 2023

achow101 force-pushed on Mar 22, 2023

achow101 marked this as a draft on Mar 22, 2023

achow101 force-pushed on Mar 22, 2023

DrahtBot added the label Needs rebase on Apr 15, 2023

achow101 force-pushed on Apr 18, 2023

DrahtBot removed the label Needs rebase on Apr 18, 2023

DrahtBot added the label Needs rebase on May 1, 2023

achow101 force-pushed on May 1, 2023

DrahtBot removed the label Needs rebase on May 1, 2023

RandyMcMillan commented at 3:39 pm on May 1, 2023: contributor

Concept ACK

achow101 marked this as ready for review on May 19, 2023

DrahtBot added the label CI failed on Jun 7, 2023

achow101 force-pushed on Jun 8, 2023

DrahtBot removed the label CI failed on Jun 8, 2023

in src/wallet/feebumper.cpp:254 in fa87a8b2b2 outdated

254+        const CTxOut& output = txouts.at(i);
255+        if (reduce_output.has_value() ?  reduce_output.value() == i : OutputIsChange(wallet, output)) {
256             CTxDestination change_dest;
257             ExtractDestination(output.scriptPubKey, change_dest);
258             new_coin_control.destChange = change_dest;
259+        } else {

furszy commented at 9:26 pm on June 9, 2023:

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.

pinheadmz commented at 11:32 am on July 18, 2023:

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?

furszy commented at 1:07 pm on July 18, 2023:

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.

in src/wallet/feebumper.cpp:158 in dff7235f9a outdated

151@@ -152,8 +152,14 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid)
152 }
153 
154 Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<bilingual_str>& errors,
155-                                 CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector<CTxOut>& outputs)
156+                                 CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, bool require_mine, const std::vector<CTxOut>& outputs, std::optional<uint32_t> reduce_output)
157 {
158+    // Cannot both specify new outputs and an output to reduce
159+    if (!outputs.empty() && reduce_output.has_value()) {

ismaelsadeeq commented at 12:35 pm on June 24, 2023:

Is this not redundant since it was already checked in spend.cpp? before calling CreateRateBumpTransaction?

achow101 commented at 5:30 pm on June 26, 2023:

This function may be used by other callers (in the future).

ismaelsadeeq commented at 8:46 pm on June 26, 2023:

Could we perhaps consider relying on the check here? It seems like a duplicate of the same process in two different places if you agree.

achow101 commented at 9:57 pm on June 26, 2023:

Done

ismaelsadeeq commented at 12:54 pm on June 24, 2023: contributor

tACK dff7235f9a6 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 dff7235f9a6. Code looks good to me

DrahtBot requested review from pinheadmz on Jun 24, 2023

DrahtBot requested review from Sjors on Jun 24, 2023

in src/wallet/feebumper.h:46 in dff7235f9a outdated

42@@ -43,6 +43,8 @@ bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid);
43  * @param[out] new_fee the fee that the bump transaction pays
44  * @param[out] mtx The bump transaction itself
45  * @param[in] require_mine Whether the original transaction must consist of inputs that can be spent by the wallet
46+ * #param[in] outputs Vector of new outputs to replace the bumped transaction's outputs

ismaelsadeeq commented at 1:00 pm on June 24, 2023:

nit

0* [@param](/bitcoin-bitcoin/contributor/param/)[in] outputs Vector of new outputs to replace the bumped transaction's outputs

achow101 commented at 5:30 pm on June 26, 2023:

Fixed

achow101 force-pushed on Jun 26, 2023

bumpfee: Allow original change position to be specified

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.

7d83502d3d

test: Test bumpfee reduce_output 4f4d4407e3

tests: Test for bumping single output transaction e8c31f135c

achow101 force-pushed on Jun 26, 2023

ismaelsadeeq commented at 9:31 am on June 27, 2023: contributor

re ACK e8c31f135c6e9a5f57325dbf4feceafd384f7762

pinheadmz commented at 2:31 pm on July 17, 2023: member

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.

in src/wallet/feebumper.cpp:250 in e8c31f135c

250-            CRecipient recipient = {output.scriptPubKey, output.nValue, false};
251-            recipients.push_back(recipient);
252-        } else {
253+    for (size_t i = 0; i < txouts.size(); ++i) {
254+        const CTxOut& output = txouts.at(i);
255+        if (reduce_output.has_value() ?  reduce_output.value() == i : OutputIsChange(wallet, output)) {

pinheadmz commented at 2:32 pm on July 17, 2023:

nit: extra space?

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

pinheadmz approved

pinheadmz commented at 3:27 pm on July 17, 2023: member

re-ACK e8c31f135c6e9a5f57325dbf4feceafd384f7762

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 ?

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK e8c31f135c6e9a5f57325dbf4feceafd384f7762
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmS1V/kACgkQ5+KYS2KJ
 7yTon4g/+JpVTIdCwb4fvLOiulKLu+poVhET+Wz9iyC+47m4GJUPgcR8RyEcPFa+6
 8eEn7V1md7o9pOekPQnBLStZJZSFQwuJgG0utgt/M0a5yzQlNNsTcEh2nWqSAI1X2
 9RAzxOlBjomFzQndm8zQ6QVQRX4tmrwKdnJxumZxWrnRBToJCvikdDVzHIP1yS0CE
10d9Z2yWNgJKkocuWACMOgfjt5GjR/Y0Y4qPDahcI10qpWjD2yFJMrx5DUjUnMxXyD
11H/3+kYUga9yfFAlE1J9OyCWpL1k//zRcttBZxDoHZ/Hh3MIMvcla6eRAjndd0iU0
12o92BVZS1+Ylz4mY2+aoja1yoXQa9JJwJavyOVNR0HtzXXtkzkWdf8shMcUe7jTXb
13wlUJwm4+ItIZ05ms05eTmcxfDOc6aQjKud26hIG0TaztfLAZU3ITDdU9lHeZB8qC
14szHpyVxLJ63BtaQCOdC//asAqxsn44EZlM3LY9iUE1jvxLAaaHN7yPgSO62A1B3N
15RbQ+5BYLYWv6ceAnyUKxX7XnzEGjsFQnSe45KVYZ42zy4eO6kuc/xKy7his3oJ9r
167JT1FgA41wY8mi4KazB6JRyTwhcx0QQ3UxwbnLy3tzsTzWw0aU3dPqwG7iR2dTqH
17Ac08JiqHzUCtTiYbRVrrATkgoZNeBqjCRhVr3o0z+2iKZLm6lWg=
18=8/+K
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

achow101 commented at 3:27 pm on July 17, 2023: member

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 https://github.com/bitcoin-core/gui/pull/700. Dropped that from the PR description.

in src/wallet/rpc/spend.cpp:1022 in 7d83502d3d outdated

1014@@ -1015,9 +1015,11 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
1015                              "\"" + FeeModes("\"\n\"") + "\""},
1016                     {"outputs", RPCArg::Type::ARR, RPCArg::Default{UniValue::VARR}, "New outputs (key-value pairs) which will replace\n"
1017                              "the original ones, if provided. Each address can only appear once and there can\n"
1018-                             "only be one \"data\" object.\n",
1019+                             "only be one \"data\" object.\n"
1020+                             "Cannot be provided if 'reduce_output' is specified.",
1021                         OutputsDoc(),
1022                         RPCArgOptions{.skip_type_check = true}},
1023+                    {"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."},

furszy commented at 1:49 pm on July 18, 2023:

In 7d83502d:

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.

whitslack commented at 9:11 am on July 20, 2023:

@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.

furszy commented at 1:32 pm on July 20, 2023:

@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.

luke-jr commented at 2:42 am on July 27, 2023:

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

Sjors commented at 11:06 am on July 28, 2023:

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.

Sjors commented at 11:11 am on July 28, 2023:

Made a separate issue to track this: #28180

furszy commented at 2:09 pm on July 18, 2023: member

Code review ACK e8c31f13

fanquake merged this on Jul 20, 2023

fanquake closed this on Jul 20, 2023

sidhujag referenced this in commit f657f46c10 on Jul 21, 2023

donsheddy4 commented at 9:12 pm on August 9, 2023: none

Very good

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

Reviews

Conflicts