[RPC] Simplified bumpfee command. #8456

mrbandrews · 2016-08-04T13:52:22Z

This purports to be a simplified "bumpfee" RPC command.

This pull was motivated by the discussion in #7865, where at the end, the idea was floated of submitting a simplified version of the code. I put a little thought into how a simple "bumpfee" command should work and here is what I came up with:

The user should specify which output to decrement in order to pay increased fee. This saves us from trying to figure out which address(es) are "change" addresses. This is preferable not only because it simplifies the code, but because as far as I can tell, the code that identifies change outputs is not particularly robust and may be modified in the future. If it's desirable to have user-friendly code that figures out which output to decrement, perhaps that logic could be placed in the GUI?
The command should not add new inputs. In other words, if the user can't identify an output that can be decremented in an amount sufficient to bump the fee then the command should fail. It seems likely that the vast majority of the time, identifying such an output would not be a problem. Adding new inputs complicates the code because the size of the transaction increases, perhaps significantly, so then the user would have to pay more fee for that reason as well, as opposed to just bumping the fee on the tx as it currently exists.
If the tx has descendant transactions, the bumped tx has to pay the fees for all of the descendants since they will get evicted from the mempool, and the rule as I understand it is that replacing a tx cannot cause the total fees in the mempool to go down.

So, the required inputs are the txid and the output index.

The optional inputs are:

confirmTarget - perhaps the reason the user feels the need to bump the fee is because the original confirmTarget was too slow
totalFee - for full control, the user can simply specify how many satoshis to pay as a fee on the bumped transaction (this may be useful when the parent tx has children tx'es)

This pull includes a python test.

jonasschnelli · 2016-08-05T10:56:24Z

Thanks! Looks good.
Concept ACK will review and test soon.

NicolasDorier · 2016-08-05T22:39:53Z

src/wallet/rpcwallet.cpp

nit: Double negation makes it hards to read. I suggest to replace by

CAmount nOldFee = wtx.IsFromMe(ISMINE_SPENDABLE) ? nDebit - wtx.GetValueOut() : 0;

jonasschnelli · 2016-08-19T16:50:24Z

Needs rebase and reviewers... setting 0.14 milestone.

mrbandrews · 2016-10-18T18:07:09Z

Rebased and addressed feedback.

@jonasschnelli

Allows a user to increase the fee on a wallet transaction in the mempool. Requires user to specify which output to decrement. Includes code by @jonasschnelli Github-Pull: bitcoin#8456 Rebased-From: 41572b183d33080602b8522ff7085f472ef3e4a7

Github-Pull: bitcoin#8456 Rebased-From: a8f28c25144f07035f73d90515e8135a933fa723

Github-Pull: bitcoin#8456 Rebased-From: a5ab6421af5de2255a359bcb3d758990cca279e6

jonasschnelli · 2016-10-20T19:55:23Z

Needs rebase again...

mrbandrews · 2016-10-21T15:44:36Z

Rebased and edited to use JSONRPCRequest, consistent with #8788.

luke-jr · 2016-10-22T14:35:52Z

src/wallet/rpcwallet.cpp

This line is outdated

RHavar · 2016-10-26T00:30:55Z

I think it's very important for this command to actual figure out the change output on its own. I understand that it's a bit messier and more fragile doing it here -- but it actually has enough information to do this, and by avoiding it, it just pushes that mess into the caller which comes at a very significant usability issue.

Compare:
Transaction stuck? Use bumpfee $txid

vs

Transaction stuck? First use gettransaction $txid, and now get the data from the "hex" field to feed into decoderawtransaction $hex now go through the "vout" fields until you find "addresses" nested in the "scriptPubKey". Now try figure out which is a change address. (I can't even see this exposed over rpc at all?). So maybe go validateaddress $address and check the ismine field. If one of them is yours, but the other one isn't -- then the one that is yours is a change address, so now go back through the results of decoderawtransaction to figure out which index that is. And finally bumpfee $txid $index. Oh yeah, becareful to handle all the edge cases, like the transaction only having 1 output, some of the outputs not having address, both having the same address etc.

I think you'll find the usability problem of needing to know the index will prevent the majority of use by users and services for this =)

RHavar · 2016-10-26T00:34:39Z

Also ideally, I think the txid argument should be an array of transaction ids to bump. And then it creates a single transaction that bumps the fees on all of those transactions (stripping out extraneous change outputs as it goes). However, I imagine this can be done separately and later as the txid argument could be overloaded to either accept a string or array of strings?

JeremyRubin

Overall seems like the right behavior for bumpfee, just a few minor suggestions.

It seems that the descendants fee adds additional complexity that probably can't be addressed in the scope of this PR but at least could merit some documentation so that a stuck user can find out what's wrong.

JeremyRubin · 2016-10-26T19:38:20Z

src/wallet/rpcwallet.cpp

Perhaps make this fail if there are any options other than confTarget or totalFee passed in to guard against the case where a user has a typo or something.

JeremyRubin · 2016-10-26T19:48:41Z

src/wallet/rpcwallet.cpp

Maybe add a tighter limit to your bumpfee code, bumpfeeMaxTxFee. Set this to be an optional default parameter in options. maxTxFee is huge, so might be safer to have something smaller and not much added code. This parameter can be overridden if need be, but not to more the maxTxFee.

JeremyRubin · 2016-10-26T20:12:09Z

src/wallet/rpcwallet.cpp

There is kind of a weird deal here where nDelta can be <= 0 if totalFee is set, and will be accepted on a race condition with mempool, I would explicitly guard against this case by restricting nDelta > 0.

@JeremyRubin how could nDelta be negative here?

JeremyRubin · 2016-10-26T20:13:26Z

src/wallet/rpcwallet.cpp

Using a nNewFee = 0 is fine here, it just slightly bothers me to use this as a null value when indeed 0 is a valid fee amount.

-1 would be an invalid fee amount (creates coins) so I have a slight preference to use this.

JeremyRubin · 2016-10-26T20:21:00Z

src/wallet/rpcwallet.cpp

I would suggest that this should also return in the error message the txid of the furthest child, and suggest bumping the fee on that one if possible to take advantage of ancestor fee based mining and keep txs valid?

As far as I can tell it's a little tricky to track down the furthest child (and there could be numerous furthest children at the same level) but I edited the error message to give the user more info (number of children and size of those transactions) and explaining the situation a bit better.

JeremyRubin · 2016-10-26T20:48:30Z

src/wallet/rpcwallet.cpp

There is something kinda funky when a user is running in say blocksonlymode and doesn't know about any child transactions that may exist and therefore has trouble setting the fee correctly for those that they will invalidate.

This is probably a hard problem to solve; so I'm just pointing it out.

mrbandrews · 2016-10-27T21:10:51Z

Addressed JeremyRubin feedback, edited the python test, and made a few other small changes I noticed with further testing.

RHavar: I understand your point but I still think it's better for this command to be low-level and not fragile. A more user-friendly RPC (e.g., "bumpfeeauto" or something) could be layered on top, identifying the change output and then using this code. Then if the change-output-identifying code breaks, it might break the user-friendly version but it wouldn't dismantle RBF entirely.

Victorsueca · 2016-11-11T13:13:31Z

ACK f3833f4
Tested on Windows x64

bumpfee 8c56b13830405a55ec4bc58b26b531f1b187d2349ee19bd0dd01aa835972929a 1

{
  "txid": "67d6af1a3e29e246eaef0b7ce272f745e2ae6178050ccb78fca515b13c0f9e92",
  "oldfee": 0.00000260,
  "fee": 0.00000520
}

ryanofsky · 2017-01-17T17:20:09Z

Squashed 28fd457 -> ce3a363.

TheBlueMatt · 2017-01-17T18:08:18Z

utACK ce3a363

morcos · 2017-01-17T18:09:11Z

tested previous iteration ACK ce3a363

luke-jr · 2017-01-18T18:06:40Z

src/rpc/server.cpp

    }
 }

+void RPCTypeCheckArgument(const UniValue& value, UniValue::VType typeExpected)


This seems redundant. Type-checking is the purpose of the .get_<type> methods on UniValue.

We absolutely do not use .get_'s built-in type check anywhere - it throws a different type of exception which will not be correctly reported as an RPC_TYPE_ERROR.

sdaftuar · 2017-01-18T19:59:55Z

utACK ce3a363 (reviewed diff from last iteration I tested and ACKed above).

luke-jr

Looks okay. A few suggested changes, but not a big deal if they don't go in.

luke-jr · 2017-01-18T18:07:52Z

src/wallet/rpcwallet.cpp

+            "4. include_unsafe (bool, optional, default=true) Include outputs that are not safe to spend\n"
+            "                  because they come from unconfirmed untrusted transactions or unconfirmed\n"
+            "                  replacement transactions (cases where we can't be sure a conflicting\n"
+            "                  transaction won't be mined).\n"


We can't be sure 1-block confirmed transactions won't have a replacement mined either...

Changed wording in 4beb7c9.

luke-jr · 2017-01-18T18:26:53Z

src/wallet/rpcwallet.cpp

+    if (request.fHelp || request.params.size() > 4)
        throw runtime_error(
-            "listunspent ( minconf maxconf  [\"addresses\",...] )\n"
+            "listunspent ( minconf maxconf  [\"addresses\",...] [include_unsafe] )\n"


Prefer to add an options Object for this.

Interesting. I wasn't following the named argument discussion in detail, but I thought named arguments would be preferable to options because they are easier to specify on the command line. But maybe there are advantages to options objects that I'm not aware of.

@luke-jr

Fix inaccuracy in include_unsafe description pointed out by @luke-jr in bitcoin#8456 (comment)

@jonasschnelli

This command allows a user to increase the fee on a wallet transaction T, creating a "bumper" transaction B. T must signal that it is BIP-125 replaceable. T's change output is decremented to pay the additional fee. (B will not add inputs to T.) T cannot have any descendant transactions. Once B bumps T, neither T nor B's outputs can be spent until either T or (more likely) B is mined. Includes code by @jonasschnelli and @ryanofsky

ryanofsky · 2017-01-19T16:31:46Z

Squashed 4beb7c9 -> cc0243a

morcos · 2017-01-19T18:13:16Z

Still ACK cc0243a

instagibbs

tACK cc0243a

no need to change nit pre-merge, very minor

instagibbs · 2017-01-19T18:34:56Z

src/wallet/rpcwallet.cpp

+    }
+
+    if (wtx.mapValue.count("replaced_by_txid")) {
+        throw JSONRPCError(RPC_INVALID_REQUEST, strprintf("Cannot bump transaction %s which was already bumped by transaction %s", hash.ToString(), wtx.mapValue.at("replaced_by_txid")));


ultra-nit: "bumped" is a bit confusing since it's already used in another sense(bumped fee), "replace" is likely better.

cc0243a [RPC] bumpfee (mrbandrews) 52dde66 [wallet] Add include_unsafe argument to listunspent RPC (Russell Yanofsky) 766e8a4 [wallet] Add IsAllFromMe: true if all inputs are from wallet (Suhas Daftuar)

jameshilliard · 2017-01-20T02:10:26Z

src/wallet/rpcwallet.cpp

+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction has been mined, or is conflicted with a mined transaction");
+    }
+
+    if (!SignalsOptInRBF(wtx)) {


Should probably have an option to override this.

An option to signal RBF in the original transaction (which already exists, see -walletrbf option, and #9592, #9527), or an option to create a bumpfee transaction even if the original doesn't signal RBF?

An option to create a bumpfee transaction regardless of if the original signaled RBF since some nodes may accept it.

There was some discussion about this idea in IRC https://botbot.me/freenode/bitcoin-core-dev/msg/79836432/ (unclear what the outcome was)

Suggested by Gregory Maxwell <[email protected]> in bitcoin#8456 (comment)

28b112e Get rid of BindWallet (Russell Yanofsky) d002f9d Disable CWalletTx copy constructor (Russell Yanofsky) 65b9d8f Avoid copying CWalletTx in LoadToWallet (Russell Yanofsky) bd2fbc7 Get rid of unneeded CWalletTx::Init parameter (Russell Yanofsky) 2b9cba2 Remove CWalletTx merging logic from AddToWallet (Russell Yanofsky) Pull request description: This is a pure refactoring, no behavior is changing. Instead of AddToWallet taking a temporary CWalletTx object and then potentially merging it with a pre-existing CWalletTx, have it take a callback so callers can update the pre-existing CWalletTx directly. This makes AddToWallet simpler because now it is only has to be concerned with saving CWalletTx objects and not merging them. This makes AddToWallet calls clearer because they can now make direct updates to CWalletTx entries without having to make temporary objects and then worry about how they will be merged. Motivation for this change came from the bumpfee PR #8456 where we wanted to be able to call AddToWallet to make a simple update to an existing transaction, but were reluctant to, because the existing CWalletTx merging logic did not apply and seemed dangerous try to update as part of that PR. After this refactoring, the bumpfee PR could call AddToWallet safely instead of implementing a duplicate AddToWallet function. This also allows getting rid of the CWalletTx copy constructor to prevent unintentional copying. ACKs for top commit: MarcoFalke: Anyway, re-ACK 28b112e Tree-SHA512: 528dd088714472a237500b200f4433db850bdb7fc29c5e5d81cae48072061dfb967f7c37edd90b33f24901239f9be982988547c1f8c80abc25fb243fbf7330ef

28b112e Get rid of BindWallet (Russell Yanofsky) d002f9d Disable CWalletTx copy constructor (Russell Yanofsky) 65b9d8f Avoid copying CWalletTx in LoadToWallet (Russell Yanofsky) bd2fbc7 Get rid of unneeded CWalletTx::Init parameter (Russell Yanofsky) 2b9cba2 Remove CWalletTx merging logic from AddToWallet (Russell Yanofsky) Pull request description: This is a pure refactoring, no behavior is changing. Instead of AddToWallet taking a temporary CWalletTx object and then potentially merging it with a pre-existing CWalletTx, have it take a callback so callers can update the pre-existing CWalletTx directly. This makes AddToWallet simpler because now it is only has to be concerned with saving CWalletTx objects and not merging them. This makes AddToWallet calls clearer because they can now make direct updates to CWalletTx entries without having to make temporary objects and then worry about how they will be merged. Motivation for this change came from the bumpfee PR bitcoin#8456 where we wanted to be able to call AddToWallet to make a simple update to an existing transaction, but were reluctant to, because the existing CWalletTx merging logic did not apply and seemed dangerous try to update as part of that PR. After this refactoring, the bumpfee PR could call AddToWallet safely instead of implementing a duplicate AddToWallet function. This also allows getting rid of the CWalletTx copy constructor to prevent unintentional copying. ACKs for top commit: MarcoFalke: Anyway, re-ACK 28b112e Tree-SHA512: 528dd088714472a237500b200f4433db850bdb7fc29c5e5d81cae48072061dfb967f7c37edd90b33f24901239f9be982988547c1f8c80abc25fb243fbf7330ef

laanwj added the RPC/REST/ZMQ label Aug 4, 2016

jonasschnelli mentioned this pull request Aug 5, 2016

[RPC] Add bumpfee command. #7865

Closed

NicolasDorier reviewed Aug 5, 2016
View reviewed changes

maflcko added the Wallet label Aug 7, 2016

maflcko mentioned this pull request Aug 7, 2016

zombie coins - an issue with my old unconfirmed transaction #8368

Closed

jonasschnelli mentioned this pull request Aug 19, 2016

[Qt] attribute replaceable (RBF) transactions #7817

Closed

jonasschnelli added this to the 0.14.0 milestone Aug 19, 2016

jonasschnelli mentioned this pull request Sep 15, 2016

[Qt] Add simple opt-in-RBF support #8182

Closed

mrbandrews force-pushed the ba-rpcbumpfee branch from 41572b1 to a8f28c2 Compare October 18, 2016 17:49

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

Fixed nits

f012520

Github-Pull: bitcoin#8456 Rebased-From: a8f28c25144f07035f73d90515e8135a933fa723

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016

fixed comments

e73a5f1

Github-Pull: bitcoin#8456 Rebased-From: a5ab6421af5de2255a359bcb3d758990cca279e6

mrbandrews force-pushed the ba-rpcbumpfee branch from a5ab642 to 8e969e3 Compare October 21, 2016 15:35

luke-jr reviewed Oct 22, 2016

View reviewed changes

src/wallet/rpcwallet.cpp Outdated

Copy link

Member

luke-jr Oct 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is outdated

JeremyRubin reviewed Oct 26, 2016

View reviewed changes

ryanofsky force-pushed the ba-rpcbumpfee branch 3 times, most recently from 28fd457 to ce3a363 Compare January 17, 2017 17:19

luke-jr reviewed Jan 18, 2017

View reviewed changes

luke-jr approved these changes Jan 18, 2017

View reviewed changes

ryanofsky added a commit to mrbandrews/bitcoin that referenced this pull request Jan 18, 2017

fixup! [wallet] Add include_unsafe argument to listunspent RPC

4beb7c9

Fix inaccuracy in include_unsafe description pointed out by @luke-jr in bitcoin#8456 (comment)

ryanofsky and others added 2 commits January 19, 2017 11:29

[wallet] Add include_unsafe argument to listunspent RPC

52dde66

ryanofsky force-pushed the ba-rpcbumpfee branch from 4beb7c9 to cc0243a Compare January 19, 2017 16:30

instagibbs approved these changes Jan 19, 2017

View reviewed changes

laanwj merged commit cc0243a into bitcoin:master Jan 19, 2017

maflcko mentioned this pull request Jan 19, 2017

TODO for release notes 0.14.0 #8455

Closed

18 tasks

jameshilliard reviewed Jan 20, 2017

View reviewed changes

ryanofsky mentioned this pull request Jan 25, 2017

Document CWalletTx::mapValue entries and remove erase of nonexistent "version" entry. #9333

Merged

morcos mentioned this pull request Jan 26, 2017

Fixes for bumpfee and minimum fees (use incrementalRelayFee for BIP 125 (RBF) replacement) #9589

Merged

ryanofsky mentioned this pull request Feb 6, 2017

Make bumpfee tests less fragile #9701

Merged

JeremyRubin mentioned this pull request Mar 28, 2017

Refactor Bumpfee, move core functionality to CWallet #9681

Merged

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 23, 2017

Enable RBF transactions in wallet by default

02b059d

Suggested by Gregory Maxwell <[email protected]> in bitcoin#8456 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 2, 2017

Enable RBF transactions in wallet by default

ea282da

Suggested by Gregory Maxwell <[email protected]> in bitcoin#8456 (comment)

codablock mentioned this pull request Feb 7, 2018

Collection of PRs and single commits missed in previous backports dashpay/dash#1908

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[RPC] Simplified bumpfee command. #8456

[RPC] Simplified bumpfee command. #8456

Uh oh!

Conversation

mrbandrews commented Aug 4, 2016

Uh oh!

jonasschnelli commented Aug 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 19, 2016

Uh oh!

mrbandrews commented Oct 18, 2016

Uh oh!

jonasschnelli commented Oct 20, 2016

Uh oh!

mrbandrews commented Oct 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RHavar commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RHavar commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin 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

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!

mrbandrews commented Oct 27, 2016

Uh oh!

Victorsueca commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jan 17, 2017

Uh oh!

TheBlueMatt commented Jan 17, 2017

Uh oh!

morcos commented Jan 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Jan 18, 2017

Uh oh!

luke-jr 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!

ryanofsky commented Jan 19, 2017

Uh oh!

morcos commented Jan 19, 2017

RHavar commented Oct 26, 2016 •

edited

Loading

RHavar commented Oct 26, 2016 •

edited

Loading

Victorsueca commented Nov 11, 2016 •

edited

Loading

instagibbs left a comment •

edited

Loading