New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@kallewoof
Member

kallewoof commented Jan 5, 2018

No description provided.

@fanquake fanquake added the Wallet label Jan 5, 2018

@promag

This comment has been minimized.

Member

promag commented Jan 5, 2018

Missing test.

@sipa

This comment has been minimized.

Member

sipa commented Jan 5, 2018

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

This comment has been minimized.

Member

kallewoof commented Jan 5, 2018

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

This comment has been minimized.

Contributor

wtogami commented Jan 5, 2018

"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

This comment has been minimized.

Member

sipa commented Jan 5, 2018

@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

This comment has been minimized.

Contributor

ryanofsky commented Jan 5, 2018

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

This comment has been minimized.

Member

kallewoof commented Jan 5, 2018

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

This comment has been minimized.

Member

sipa commented Jan 5, 2018

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

This comment has been minimized.

Member

instagibbs commented Jan 5, 2018

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

rawtx = node.createrawtransaction([tx_input], {dest_address: Decimal("0.00099000")})
signedtx = node.signrawtransaction(rawtx)
txid = node.sendrawtransaction(signedtx["hex"])
bumped_tx = node.bumpfee(txid)

This comment has been minimized.

@promag

promag Jan 5, 2018

Member

Check new output is less than 0.00099?

This comment has been minimized.

@kallewoof

kallewoof Jan 9, 2018

Member

Thanks, good point - fixing.

@promag

This comment has been minimized.

Member

promag commented Jan 5, 2018

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

This comment has been minimized.

Member

sdaftuar commented Jan 5, 2018

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

This comment has been minimized.

Member

jtimon commented Jan 8, 2018

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

@gmaxwell

This comment has been minimized.

Member

gmaxwell commented Jan 8, 2018

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

This comment has been minimized.

Member

promag commented Jan 8, 2018

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

@gmaxwell

This comment has been minimized.

Member

gmaxwell commented Jan 8, 2018

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

This comment has been minimized.

Member

gmaxwell commented Jan 9, 2018

@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

This comment has been minimized.

Member

kallewoof commented Jan 9, 2018

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 kallewoof changed the title from [rpc] [wallet] Allow single-output transactions in bumpfee to [rpc] [wallet] Allow specifying the output index when using bumpfee Jan 9, 2018

@kallewoof

This comment has been minimized.

Member

kallewoof commented Jan 9, 2018

  • Self-review: RPC command help needs update to reflect change
}
}
if (nOutput == -1) {
errors.push_back("Transaction does not have a change output");
errors.push_back("Transaction does not have a change output (use reduce_output=0 to use the single output)");

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 13, 2018

Member

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

This comment has been minimized.

@kallewoof

kallewoof Jan 13, 2018

Member

Oh. Hm..

This comment has been minimized.

@sipa

sipa Feb 20, 2018

Member

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

This comment has been minimized.

@kallewoof

kallewoof Feb 20, 2018

Member

I'll just revert it.

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 13, 2018

Member

This will segfault on OOB

This comment has been minimized.

@kallewoof

kallewoof Jan 13, 2018

Member

Oops. Added bounds check in feebumper::CreateTransaction.

This comment has been minimized.

@promag

promag Jan 18, 2018

Member

Where?

This comment has been minimized.

@promag

promag Jan 18, 2018

Member

Also add assert_raises_rpc_error tests for:

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

For instance, see 7f67dd0.

This comment has been minimized.

@kallewoof

kallewoof Jan 18, 2018

Member

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

This comment has been minimized.

@luke-jr

luke-jr Feb 26, 2018

Member

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

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

This comment has been minimized.

@promag

promag Jan 18, 2018

Member

Where?

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

This comment has been minimized.

@promag

promag Jan 18, 2018

Member

Also add assert_raises_rpc_error tests for:

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

For instance, see 7f67dd0.

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

This comment has been minimized.

@promag

promag Jan 18, 2018

Member

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

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

This comment has been minimized.

@promag

promag Jan 18, 2018

Member

`reduce_output`

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

This comment has been minimized.

@luke-jr

luke-jr Feb 26, 2018

Member

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

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

This comment has been minimized.

@luke-jr

luke-jr Feb 26, 2018

Member

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

This comment has been minimized.

@kallewoof

kallewoof Feb 27, 2018

Member

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.

This comment has been minimized.

@Sjors

Sjors Feb 28, 2018

Member

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

This comment has been minimized.

@luke-jr

luke-jr Mar 10, 2018

Member

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

This comment has been minimized.

@kallewoof

kallewoof Mar 12, 2018

Member

@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

This comment has been minimized.

Member

luke-jr commented Feb 26, 2018

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

This comment has been minimized.

Member

kallewoof commented Feb 27, 2018

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

@Sjors

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

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

This comment has been minimized.

@Sjors

Sjors Feb 28, 2018

Member

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

errors.push_back("Transaction has multiple change outputs");
return Result::WALLET_ERROR;
int nOutput = reduce_output;
if (nOutput == -1) {

This comment has been minimized.

@Sjors

Sjors Feb 28, 2018

Member

Can you define a constant like ONLY_REDUCE_CHANGE_OUTPUT=-1?

for (size_t i = 0; i < wtx.tx->vout.size(); ++i) {
if (wallet->IsChange(wtx.tx->vout[i])) {
if (nOutput != -1) {
errors.push_back("Transaction has multiple change outputs");

This comment has been minimized.

@Sjors

Sjors Feb 28, 2018

Member

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

This comment has been minimized.

@kallewoof

kallewoof Mar 1, 2018

Member

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.

This comment has been minimized.

@Sjors

Sjors Mar 1, 2018

Member

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

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

This comment has been minimized.

@Sjors

Sjors Feb 28, 2018

Member

reduce_output or reduceOutput?

This comment has been minimized.

@kallewoof

kallewoof Mar 1, 2018

Member

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

This comment has been minimized.

Member

Sjors commented Feb 28, 2018

#12565 takes advantage of this in QT.

@kallewoof

This comment has been minimized.

Member

kallewoof commented Mar 1, 2018

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

@DrahtBot

This comment has been minimized.

Contributor

DrahtBot commented Jul 22, 2018

The last travis run for this pull request was 89 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

@DrahtBot DrahtBot closed this Jul 22, 2018

@DrahtBot DrahtBot reopened this Jul 22, 2018

@DrahtBot DrahtBot referenced this pull request Oct 20, 2018

Closed

Rpc help helper class #14502

@DrahtBot

This comment has been minimized.

Contributor

DrahtBot commented Oct 20, 2018

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

Conflicts

No conflicts as of last run.

@MeshCollider

utACK 51826d4

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

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

This comment has been minimized.

@MeshCollider

MeshCollider Nov 11, 2018

Member

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

This comment has been minimized.

@kallewoof

kallewoof added some commits Jan 5, 2018

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

This comment has been minimized.

Member

kallewoof commented Nov 12, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment