[rpc] [wallet] Allow specifying the output index when using bumpfee

kallewoof commented at 9:10 am on January 5, 2018: member

fanquake added the label Wallet on Jan 5, 2018

promag commented at 9:26 am on January 5, 2018: member

Missing test.

sipa commented at 12:30 pm on January 5, 2018: member

This decreases the output? In some cases that may desirable, but in general I think that’s very unexpected (I would think it would add a new input instead).

kallewoof commented at 12:40 pm on January 5, 2018: member

If you don’t have a change address chance are fairly high that you’re simply moving coins (e.g. between wallets). Especially if you only have one utxo in, you may not want to associate other utxo’s with it for privacy reasons.

wtogami commented at 12:49 pm on January 5, 2018: contributor

“single output transactions are special in that the exact amount doesn’t usually matter (the user is emptying a wallet or using coin control to move UTXOs).”

I would argue that this is not stated strongly enough. If they previously paid to a single output then it is almost always true that they do not care about the particular amount. This is especially true as the previous amount was subject to an arbitrary, variable fee.

Valid use cases where this behavior is necessary, where you really don’t want to require adding more inputs in order to bumpfee:

Emptying a wallet
Emptying particular UTXO’s

I would further suggest if the receiver was expecting a particular amount then you would expect there to be a change output.

sipa commented at 12:51 pm on January 5, 2018: member

@kallewoof You’re right that in most cases right now that you don’t care about the amount if there is only one output, but there is certainly no guarantee. The coin selection algorithm even explicitly tries to find a solution without change first - it’s just pretty terrible at its job current (#10637 will make it succeed much more often).

ryanofsky commented at 12:55 pm on January 5, 2018: member

I think this could be generalized and also be made safer by adding an reduce_output option to bumpfee (could also call it something like output_number or txout). This way you’d be able to bump single output transactions by specifying reduce_output: 0, but there’d be no risk of someone accidentally decreasing the amount they are trying to send by bumping the fee in other cases.

kallewoof commented at 12:55 pm on January 5, 2018: member

I don’t think it makes sense to arbitrarily add inputs to bumpfee. I don’t even know if RBF allows it (since you’re suggesting it, I guess it does – I thought I tried and failed).

Maybe a bump_from_output flag in options that did this behavior.

Edit: @ryanofsky That looks good to me.

sipa commented at 1:00 pm on January 5, 2018: member

I don’t see why RBF wouldn’t allow it, as long as all transactions you’re replacing have been marked as replaceable.

I like the idea of explicitly marking an output that can be reduced.

Over time (especially after #10637), I think we’ll inevitably need a way to bump transactions that don’t have change. Depending on feerates and your wallet’s UTXO set distribution, a large fraction of transactions may be created without change.

instagibbs commented at 2:11 pm on January 5, 2018: member

does the wallet track which outputs had subtractFeeFromOutput? In that case further reduction seems completely fine as well.

in test/functional/bumpfee.py:288 in 2afb0d085f outdated

283+    tx_input = dict(
284+        sequence=BIP125_SEQUENCE_NUMBER, **next(u for u in node.listunspent() if u["amount"] == Decimal("0.00100000")))
285+    rawtx = node.createrawtransaction([tx_input], {dest_address: Decimal("0.00099000")})
286+    signedtx = node.signrawtransaction(rawtx)
287+    txid = node.sendrawtransaction(signedtx["hex"])
288+    bumped_tx = node.bumpfee(txid)

promag commented at 3:11 pm on January 5, 2018:

Check new output is less than 0.00099?

kallewoof commented at 4:13 am on January 9, 2018:

Thanks, good point - fixing.

promag commented at 3:14 pm on January 5, 2018: member

Agree that there are cases where the output(s) can’t be reduced. In the cases it can be reduced, should it be re-funded?

sdaftuar commented at 3:41 pm on January 5, 2018: member

I don’t think it makes sense to arbitrarily add inputs to bumpfee. I don’t even know if RBF allows it (since you’re suggesting it, I guess it does – I thought I tried and failed). @kallewoof Currently our relay policy (and BIP 125) require that if you add a new input, it must be already confirmed – perhaps you tested with an unconfirmed input?

(The idea behind that requirement was to ensure the new transaction was more likely to be mined than the ones it was replacing; now that we use ancestor feerate mining, we could perhaps relax this requirement and compare ancestor feerates of everything instead to ensure the new transaction has a higher mining score.)

jtimon commented at 8:38 pm on January 8, 2018: contributor

concept ack after the current coin selection stop trying 1 output (I assume #10637 solves that?).

gmaxwell commented at 8:42 pm on January 8, 2018: contributor

NAK. The only time it would be acceptable to reduce the output is if the transaction was created as subtract fee from output. Bumping should add additional inputs as required.

promag commented at 8:50 pm on January 8, 2018: member

@gmaxwell even with an option to allow the subtract, default to false (since there is no subtract from output record)?

gmaxwell commented at 9:00 pm on January 8, 2018: contributor

if it were behind a bunch of warnings, … but even though if substract from wasn’t initially selected should only be a last resort if it can’t bump otherwise. The wallet shouldn’t quietly direct the user into behavior that might cause them to get criminally prosecuted.

gmaxwell commented at 3:53 am on January 9, 2018: contributor

@wtogami The wallet has explicit information about transactions created where the user requested subtracting fees from amounts. Using that information would be fine but you cannot use the lack of a change output to detect this, when 2^inputs * fees_saved_from_excluding_an_output is large relative to the amount being paid an exact match frequently exists. And the PR pieter linked to will usually find it.

kallewoof commented at 4:04 am on January 9, 2018: member

Summary:

Do not implicitly assume a single-output-no-change transaction can be altered due to better coin selection in the future.
Require the user to select the output if the user does not want additional inputs to be added to cover increased fees.
(Allow adding additional inputs to cover increased fees – separate PR)

For this PR, that translates to:

Fail if no output is explicitly chosen (pre-merge behavior)
Reduce single output to cover fees if user has explicitly picked it
Allow user to arbitrarily pick the output that is reduced for existing multi-output transactions (i.e. not restricted to change output).

kallewoof force-pushed on Jan 9, 2018

kallewoof renamed this:
~~[rpc] [wallet] Allow single-output transactions in bumpfee~~
[rpc] [wallet] Allow specifying the output index when using bumpfee
on Jan 9, 2018

kallewoof commented at 2:17 pm on January 9, 2018: member

Self-review: RPC command help needs update to reflect change

in src/wallet/feebumper.cpp:140 in 6fa618dc6f outdated

134-            nOutput = i;
135         }
136     }
137     if (nOutput == -1) {
138-        errors.push_back("Transaction does not have a change output");
139+        errors.push_back("Transaction does not have a change output (use reduce_output=0 to use the single output)");

MarcoFalke commented at 6:00 pm on January 13, 2018:

This error will also display in the gui. Not sure if it makes sense there to mention reduce_output

kallewoof commented at 9:34 pm on January 13, 2018:

Oh. Hm..

sipa commented at 2:42 am on February 20, 2018:

It also sounds confusing when there isn’t a single output?

kallewoof commented at 3:28 am on February 20, 2018:

I’ll just revert it.

in src/wallet/rpcwallet.cpp:3264 in 6fa618dc6f outdated

3261@@ -3257,6 +3262,9 @@ UniValue bumpfee(const JSONRPCRequest& request)
3262                 throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid estimate_mode parameter");
3263             }
3264         }
3265+        if (options.exists("reduce_output")) {
3266+            reduce_output = options["reduce_output"].get_int64();
3267+        }

MarcoFalke commented at 6:43 pm on January 13, 2018:

This will segfault on OOB

kallewoof commented at 9:59 pm on January 13, 2018:

Oops. Added bounds check in feebumper::CreateTransaction.

promag commented at 5:38 pm on January 18, 2018:

Where?

promag commented at 5:47 pm on January 18, 2018:

Also add assert_raises_rpc_error tests for:

when not a number;
when reduce_output < 0 and >= len(vout).

For instance, see 7f67dd0aa.

kallewoof commented at 11:20 pm on January 18, 2018:

No idea how that happened, but my changes are gone. Fixing, again.

luke-jr commented at 5:32 pm on February 26, 2018:

Needs a lower bounds check here too, to avoid accepting -1.

kallewoof force-pushed on Jan 17, 2018

in src/wallet/feebumper.cpp:81 in 02d1508d50 outdated

77@@ -78,7 +78,7 @@ bool TransactionCanBeBumped(CWallet* wallet, const uint256& txid)
78 }
79 
80 Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, CAmount total_fee, std::vector<std::string>& errors,
81-                         CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
82+                         CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx, int32_t reduce_output)

promag commented at 5:50 pm on January 18, 2018:

Place reduce_output after total_fee? Those after total_fee are output values.

in src/wallet/rpcwallet.cpp:3257 in 02d1508d50 outdated

3177@@ -3178,7 +3178,8 @@ UniValue bumpfee(const JSONRPCRequest& request)
3178             "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
3179             "An opt-in RBF transaction with the given txid must be in the wallet.\n"
3180             "The command will pay the additional fee by decreasing (or perhaps removing) its change output.\n"
3181-            "If the change output is not big enough to cover the increased fee, the command will currently fail\n"
3182+            "If an explicit \"reduce output\" has been picked, that will be decreased instead.\n"

promag commented at 5:51 pm on January 18, 2018:

`reduce_output`

kallewoof force-pushed on Jan 25, 2018

kallewoof force-pushed on Jan 26, 2018

kallewoof force-pushed on Feb 20, 2018

in src/wallet/feebumper.cpp:116 in cf02bcb3ed outdated

111@@ -111,22 +112,28 @@ Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoin
112         return Result::INVALID_ADDRESS_OR_KEY;
113     }
114     const CWalletTx& wtx = it->second;
115+    if (reduce_output < -1 || reduce_output >= int64_t(wtx.tx->vout.size())) {
116+        errors.push_back(strprintf("Change output out of bounds [-1..%zu]", wtx.tx->vout.size()-1));

luke-jr commented at 5:32 pm on February 26, 2018:

-1 shouldn’t be exposed to the user IMO (especially since the internal interfaces will probably change when we add support for additional inputs).

kallewoof commented at 4:44 am on February 27, 2018:

The default is -1 to mean auto-select. How would you propose that is expressed otherwise? As an additional bool explicit_reduce_output feels messy.

Sjors commented at 10:19 am on February 28, 2018:

rpcwallet.cpp already checks this, so just put an assert here?

luke-jr commented at 7:05 pm on March 10, 2018:

@kallewoof Just change the error message (and prevent -1 from being passed in from the user)

kallewoof commented at 1:38 am on March 12, 2018:

@luke-jr I can’t distinguish from user passing -1 and from the default (which means ‘auto-pick’ like the current behavior). I can change the message to say 0.., though. And it already prevents user from passing -1 in the RPC command itself.

luke-jr changes_requested

luke-jr commented at 5:35 pm on February 26, 2018: member

The only time it would be acceptable to reduce the output is if the transaction was created as subtract fee from output. @gmaxwell What if you want to subtract-fee-from-output after the fact?

kallewoof commented at 4:46 am on February 27, 2018: member

@luke-jr Now throws on negative reduce_output in RPC code.

in src/wallet/feebumper.cpp:104 in 624f27c4d3 outdated

129-        if (wallet->IsChange(wtx.tx->vout[i])) {
130-            if (nOutput != -1) {
131-                errors.push_back("Transaction has multiple change outputs");
132-                return Result::WALLET_ERROR;
133+    int nOutput = reduce_output;
134+    if (nOutput == -1) {

Sjors commented at 10:23 am on February 28, 2018:

Can you define a constant like ONLY_REDUCE_CHANGE_OUTPUT=-1?

in src/wallet/feebumper.cpp:110 in 624f27c4d3 outdated

135+        // figure out which output was change
136+        // if there was no change output or multiple change outputs, fail
137+        for (size_t i = 0; i < wtx.tx->vout.size(); ++i) {
138+            if (wallet->IsChange(wtx.tx->vout[i])) {
139+                if (nOutput != -1) {
140+                    errors.push_back("Transaction has multiple change outputs");

Sjors commented at 10:27 am on February 28, 2018:

Why is this an error? Why not just pick the largest (or both)?

kallewoof commented at 2:58 am on March 1, 2018:

This is beyond the scope of this PR (pre-existing behavior, and my code doesn’t really fix it). You can do ?w=1, e.g. https://github.com/bitcoin/bitcoin/pull/12096/files?w=1 which ignores whitespace-only changes.

Sjors commented at 8:40 am on March 1, 2018:

Ah, I didn’t notice it was just a whitespace change.

in src/wallet/rpcwallet.cpp:3369 in 624f27c4d3 outdated

3347@@ -3347,6 +3348,8 @@ UniValue bumpfee(const JSONRPCRequest& request)
3348             "         \"UNSET\"\n"
3349             "         \"ECONOMICAL\"\n"
3350             "         \"CONSERVATIVE\"\n"
3351+            "     \"reduce_output\"     (numeric, optional) Increase the fee by reducing the output value of the output at\n"

Sjors commented at 11:05 am on February 28, 2018:

reduce_output or reduceOutput?

kallewoof commented at 2:59 am on March 1, 2018:

I think the standard these days is to use snake case, even for new RPC commands. See e.g. address_style in getrawtransaction which was from a recent implementation.

Sjors commented at 11:07 am on February 28, 2018: member

Concept ACK, in particular that outputs should not be reduced without explicit intent.

I also tested that QT behavior is unchanged and setting reduce_output in bumpfee worked for me.

wallet_bumpfee.py fails by the way

Sjors commented at 12:53 pm on February 28, 2018: member

#12565 takes advantage of this in QT.

kallewoof force-pushed on Mar 1, 2018

kallewoof commented at 3:19 am on March 1, 2018: member

@Sjors Thanks! I fixed the wallet_bumpfee error in 765162c.

kallewoof force-pushed on Mar 12, 2018

kallewoof force-pushed on Apr 24, 2018

DrahtBot commented at 11:50 pm on July 22, 2018: member

DrahtBot closed this on Jul 22, 2018

DrahtBot reopened this on Jul 22, 2018

DrahtBot commented at 11:13 am on October 20, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15778 ([wallet] Move maxtxfee from node to wallet by jnewbery)
#15341 (rpc: Support specifying change address in bumpfee by promag)

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.

in src/qt/walletmodel.cpp:494 in f43fc28d78 outdated

490@@ -491,7 +491,7 @@ bool WalletModel::bumpFee(uint256 hash)
491     CAmount old_fee;
492     CAmount new_fee;
493     CMutableTransaction mtx;
494-    if (!m_wallet->createBumpTransaction(hash, coin_control, 0 /* totalFee */, errors, old_fee, new_fee, mtx)) {
495+    if (!m_wallet->createBumpTransaction(hash, coin_control, 0 /* totalFee */, -1, errors, old_fee, new_fee, mtx)) {

meshcollider commented at 11:35 am on November 11, 2018:

Maybe add a /* reduce_output */ comment for this new param too

kallewoof commented at 7:23 am on November 12, 2018:

Done

meshcollider commented at 12:05 pm on November 11, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/12096/commits/51826d4de0ea100f703675cae15bfaeef48b8e2a

You might want to update the first commit message like you did with the title of the PR

kallewoof force-pushed on Nov 12, 2018

kallewoof commented at 7:25 am on November 12, 2018: member

@MeshCollider Thanks for the review! I updated commit description and added comment to qt/walletmodel.cpp.

DrahtBot added the label Needs rebase on Nov 23, 2018

kallewoof force-pushed on Nov 26, 2018

DrahtBot removed the label Needs rebase on Nov 26, 2018

DrahtBot added the label Needs rebase on Dec 5, 2018

kallewoof force-pushed on Dec 10, 2018

DrahtBot removed the label Needs rebase on Dec 10, 2018

DrahtBot added the label Needs rebase on Jan 14, 2019

kallewoof force-pushed on Jan 15, 2019

DrahtBot removed the label Needs rebase on Jan 15, 2019

DrahtBot added the label Needs rebase on Feb 22, 2019

kallewoof force-pushed on Feb 25, 2019

DrahtBot removed the label Needs rebase on Feb 25, 2019

kallewoof force-pushed on Feb 25, 2019

DrahtBot added the label Needs rebase on Mar 21, 2019

kallewoof force-pushed on Mar 23, 2019

DrahtBot removed the label Needs rebase on Mar 23, 2019

DrahtBot added the label Needs rebase on Apr 15, 2019

kallewoof force-pushed on Apr 16, 2019

in src/wallet/feebumper.cpp:210 in e82c8ff85e outdated

206@@ -200,8 +207,8 @@ Result CreateTotalBumpTransaction(const CWallet* wallet, const uint256& txid, co
207 }
208 
209 
210-Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<std::string>& errors,
211-                                 CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
212+Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, int32_t reduce_output,

luke-jr commented at 6:57 pm on April 16, 2019:

Nit: Extra space on the end

in src/wallet/feebumper.cpp:233 in e82c8ff85e outdated

229@@ -223,8 +230,9 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo
230 
231     // Fill in recipients(and preserve a single change key if there is one)
232     std::vector<CRecipient> recipients;
233+    const auto* change_output = reduce_output > -1 && (size_t)reduce_output < wtx.tx->vout.size() ? &wtx.tx->vout[reduce_output] : nullptr;

luke-jr commented at 6:58 pm on April 16, 2019:

CreateTotalBumpTransaction returns an error if reduce_output is out of range. This code just ignores it. Probably should be consistent.

instagibbs commented at 7:17 pm on April 16, 2019:

“change” is really over-loaded here, please call it something else

kallewoof commented at 3:53 am on April 24, 2019:

@luke-jr Agreed - now consistent. @instagibbs Renamed to reduce_output_vout.

in src/wallet/feebumper.cpp:235 in e82c8ff85e outdated

229@@ -223,8 +230,9 @@ Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCo
230 
231     // Fill in recipients(and preserve a single change key if there is one)
232     std::vector<CRecipient> recipients;
233+    const auto* change_output = reduce_output > -1 && (size_t)reduce_output < wtx.tx->vout.size() ? &wtx.tx->vout[reduce_output] : nullptr;
234     for (const auto& output : wtx.tx->vout) {
235-        if (!wallet->IsChange(output)) {
236+        if (change_output ? output != *change_output : !wallet->IsChange(output)) {

luke-jr commented at 6:59 pm on April 16, 2019:

Couldn’t this result in the reduce_output getting increased? That could be a serious problem in some cases…

kallewoof commented at 3:54 am on April 24, 2019:

I’m not sure I follow. In which case would reduce_output increase?

DrahtBot removed the label Needs rebase on Apr 17, 2019

rpc/wallet/bumpfee: allow specifying output index

Currently, bumpfee cannot be used to bump a single output transaction. This should be possible, as single output transactions are special in that the exact amount doesn't usually matter (the user is emptying a wallet or using coin control to move UTXOs).

f8c1457b96

[test] Test single-output transaction bumpfee 086313c8b1

kallewoof force-pushed on Apr 24, 2019

DrahtBot commented at 1:37 pm on April 27, 2019: member

DrahtBot added the label Needs rebase on Apr 27, 2019

kallewoof commented at 8:18 am on June 19, 2019: member

Rebase is becoming overwhelming on this. Closing until further notice (may rewrite the code).

kallewoof closed this on Jun 19, 2019

kallewoof deleted the branch on Oct 17, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021

[rpc] [wallet] Allow specifying the output index when using bumpfee #12096

Conflicts