[RPC] Add bumpfee command. #7865

jonasschnelli · 2016-04-12T11:45:41Z

Refactor

CreateTransaction() now accepts a input-value over the nFee parameter (currently only used to return the calculated fee). This "base fee" will be the delta to the calculated fee. This allows to create transaction replacements with respecting the previous fee of the replaced transaction.

bumpfee RPC command

Syntax: bumpfee <txid>

Bumpfee will directly sign&send the new transaction and reports the old/new fee and the new <txid>.

How it works: the Bumpfee command removes all previous signatures, removes change outputs and re-funds the transaction (if necessary).

Currently, bumpfee will increase the fees by using the <old transaction fee> + <new transaction fee estimate>.
Increasing the fees can involve adding n inputs (which can again increase the require minimum fee).

sendtoaddress / sandmany

Added another parameter to sendtoaddress and sendmany. The boolean parameter defines if the transaction will signal opt-in-RBF over the nSequence number after BIP125.

Bitcoin-Tx / CreateRawTransaction

Extended bitcoin-tx to accept a sequence number when adding inputs
Extended createrawtransaction to accept a sequence number when defining inputs

Possible next steps

Add a bumprawtransaction command
Refactor transaction signing
Optimize the fee-estimation for replacements

instagibbs · 2016-04-12T15:03:37Z

src/wallet/wallet.h

@@ -533,6 +533,11 @@ class CAccountingEntry
    std::vector<char> _ssExtra;
 };

+enum CreateTransactionFlags {
+    CREATE_TX_DEFAULT     = 0,


It seems to me default is simply "not bip125". Not sure the name "default" really fits, especially longer-term.

I think the CREATE_TX_DEFAULT is okay. Passing just a 0(int) would require casting. "not bip125" is also not true because in future you could combine flags (Don't SIGN & RBF)?

jonasschnelli · 2016-04-12T20:11:22Z

Force push fixed @instagibbs nits.

instagibbs · 2016-04-13T20:34:51Z

utACK post-nits

Although it might make sense to split this into ~3 PRs to get through a couple faster, and each of which would still be useful on their own:

bitcoin-tx stuff
createrawtransaction stuff
rest of stuff

NicolasDorier · 2016-04-14T07:41:13Z

src/wallet/rpcwallet.cpp

+    CReserveKey reservekey(pwalletMain);
+    if(!pwalletMain->FundTransaction(tx, reservekey, nFee, nChangePos, strFailReason, false))
+        throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason);
+


Why not ensuring (nFee - nOldFee) > minTxRelayFee, and if not, exit early with a descriptive JSONRpcError which say the fees are high enough and can't be bumped ?

Yes. I have though about that. But you also would like to have a check that makes sure, it would be accepted as replacement.
This would require to factor out the RBF check from main.cpp to rbf.cpp (which probably would sense in a follow up PR).

Yes, but CommitTransaction later is already doing that under the hood if I understand.
The most likely problem is that the fees can't be bumped, for the 0.0001% chance that it still get rejected for whatever other issue during the CommitTransaction, a generic error message would be enough.

paveljanik · 2016-04-26T07:06:35Z

@jonasschnelli Can you please separate "Bitcoin-Tx / CreateRawTransaction" changes into new PR, so this can get merged?

NicolasDorier · 2016-04-26T08:45:52Z

@jonasschnelli maybe unrelated to this PR, or I should bring that up in a meeting (sadly it is 4am in japan when there is core meeting), but I think a better (or alternative) bumpfee would be one which take the number of block to confirm. As a developer using the RPC API, I'd like my backend to bump fees periodically to reach confirmation target in case of fee increase. So that bumpfee 3 would mean "put whatever fee at current rate that can confirm into 3 blocks". If the calculated fee are smaller than current one, then it would do be a no op.

It might not belong to this PR though.

It would also be used if I initially said "confirm into 20 blocks", then changed my mind and say "now I want to confirm in the next block".

jonasschnelli · 2016-04-27T13:54:03Z

@NicolasDorier: I agree that a confirm target for the bumpfee command would make sense.
The variable confirm target for CreateTransaction would require some refactoring.

This PR is already relatively large, we should probably add this feature in another PR.

NicolasDorier · 2016-04-27T14:04:45Z

right, utACK

jonasschnelli · 2016-04-27T14:13:58Z

Rebased (non trivial after #7518).

jonasschnelli · 2016-04-28T08:41:49Z

Removed createrawtransaction and bitcoin-tx commits because they are now in #7957.

jonasschnelli · 2016-05-06T09:21:15Z

Needed rebase.

maflcko · 2016-05-06T09:32:42Z

qa/rpc-tests/bumpfee.py

@@ -0,0 +1,85 @@
+#!/usr/bin/env python2


Mind to change this to py3?

maflcko · 2016-05-06T10:23:39Z

qa/rpc-tests/bumpfee.py

+            replacedTx= self.nodes[1].getrawtransaction(rbftx)
+            assert(False)
+        except JSONRPCException as e:
+            assert_equal(len(replacedTx), 0)


Is this doing anything different that assert_raises(JSONRPCException, ...?

Right. Fixed.

jonasschnelli · 2016-05-31T08:51:44Z

Rebased. Fixed nit reported by @MarcoFalke.

sipa · 2016-06-01T17:05:47Z

src/wallet/wallet.h

@@ -745,8 +749,13 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
     * selected by SelectCoins(); Also create the change output, when needed
     * @note passing nChangePosInOut as -1 will result in setting a random position
     */
+<<<<<<< 08b37c5e06bf1698c1d0e5905806382cbfa8cefd


Unresolved conflict

~~Oh. Messed up the rebase. Will fix.~~
Wait: you commented and old commit.

@jonasschnelli This commit is still in the history.

You didn't solve the conflict in this commit but in a later one. Looks weird if someone digs through it.

It also breaks bisections. In general, the state of the tree should be valid after every commit (compile and test fine).

This will allow to add flags for RBF and other features

…lculation

…ge key

jonasschnelli · 2016-06-08T19:58:49Z

Rebased and improved.
Made usage of the new feeRate option in fundrawtransaction.

Now the bumpfee command takes an existing wtx (identified by txid), increases the fee by using at the estimating mempool feeRate for the default confirmation target. If the new estimate feeRate is < old feeRate , it will use oldFeeRate+minRelayTxFee instead.

maflcko · 2016-06-09T12:17:48Z

qa/rpc-tests/bumpfee.py

+        nonrbftx = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1, "", "", True, False)
+
+        try:
+            bumpedrawtx = self.nodes[0].bumpfee(nonrbftx)


Replace by

assert_raises(JSONRPCException, self.nodes[0].bumpfee, nonrbftx)

maflcko · 2016-06-09T12:18:17Z

Mind to squash the history to aid review?

rubensayshi · 2016-06-14T11:43:12Z

hmm, "Currently, bumpfee will increase the fees by using the <old transaction fee> + <new transaction fee estimate>."

not sure if I'm missing something but when newFeeRate.GetFeePerK() >= oldFeeRate.GetFeePerK()+::minRelayTxFee.GetFeePerK() it just uses newFeeRate no?

I think I read the code right and you just need to update your initial description of the PR, in which case; utACK
if I'm wrong then it doesn't make sense to me to bump the fee past the , asuming nTxConfirmTarget is set to my desired target and we trust the fee estimation, why would I want to bump the fee higher than what the estimate is?

as said before by others, I think estimateFoundTarget should just be a param (default to 1),
it would make more sense that my configured nTxConfirmTarget when doing the initial TX wasn't 1 and instead I know want to bump it to 1, so you'd specify estimateFoundTarget (default to 1).
though even without that this PR is already very useful!

also nits; remove //CFeeRate newFeeRate = CFeeRate(oldFeeRate.GetFeePerK()*2); and // double fee rate ;-)

sipa · 2016-06-14T14:17:39Z

src/wallet/rpcwallet.cpp

+            it = tx.vout.erase(it);
+        }
+        else {
+            ++it;


I think this increment needs to happen always, even if the condition above is true.

doesn't tx.vout.erase(it); result in pointing to the next element in the iteration?

erase returns the new position of the next element after the one that is erased, so it = tx.vout.erase(it) does the right thing

gmaxwell · 2016-06-15T10:08:31Z

Concept ACK (for future reference, I think this would have potentially been better as two PRs, with the bumpfee separate-- built on top of the first), will review.

fanquake · 2016-06-22T03:21:29Z

Is this still going into 0.13.0 ?

laanwj · 2016-06-22T06:54:43Z

@fanquake No, not as-is. Unfortunately this missed the feature freeze. At last Thursday's meeting @jonasschnelli mentioned working on much simplified minimum RPC functionality for 0.13 to serve as a stop-gap.

jonasschnelli · 2016-08-05T10:55:53Z

superseded by #8456. Closing.

jonasschnelli added the Wallet label Apr 12, 2016

jonasschnelli added this to the 0.13.0 milestone Apr 12, 2016

This was referenced Apr 12, 2016

[RPC] Add RBF opt-in possibilities to rawtx functions #7159

Closed

Add uncontroversial RBF base features #7824

Closed

jonasschnelli force-pushed the 2016/04/rbf_combined branch 3 times, most recently from d1429d6 to f68447a Compare April 12, 2016 12:41

instagibbs reviewed Apr 12, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/04/rbf_combined branch from f68447a to 45b6ec5 Compare April 12, 2016 20:08

NicolasDorier reviewed Apr 14, 2016
View reviewed changes

jonasschnelli mentioned this pull request Apr 27, 2016

[RPC][Bitcoin-TX] Add support for sequence number #7957

Merged

jonasschnelli force-pushed the 2016/04/rbf_combined branch from 45b6ec5 to 32d3666 Compare April 27, 2016 14:12

jonasschnelli force-pushed the 2016/04/rbf_combined branch 2 times, most recently from 7dc957d to 395571b Compare April 28, 2016 08:41

jonasschnelli force-pushed the 2016/04/rbf_combined branch from 395571b to 5a4cb5e Compare May 6, 2016 09:21

maflcko reviewed May 6, 2016
View reviewed changes

qa/rpc-tests/bumpfee.py

@@ -0,0 +1,85 @@

#!/usr/bin/env python2

Copy link

Member

maflcko May 6, 2016

Choose a reason for hiding this comment

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

Mind to change this to py3?

maflcko reviewed May 6, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/04/rbf_combined branch from 84085d1 to 9569bb0 Compare May 6, 2016 10:34

jonasschnelli force-pushed the 2016/04/rbf_combined branch from 9569bb0 to e9931f5 Compare May 31, 2016 08:51

sipa reviewed Jun 1, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/04/rbf_combined branch from e9931f5 to c3b60c8 Compare June 1, 2016 18:54

jonasschnelli added 7 commits June 8, 2016 14:57

[Refactor] CreateTransaction(): use bit flags

2980a7a

This will allow to add flags for RBF and other features

[Wallet] Allow to opt-into RBF when creating a transaction

89a671a

[Wallet] allow to set an initial fee amount to be added to the fee ca…

291e782

…lculation

[Wallet] FundTransaction: return reserveKey to allow keeping the chan…

a501878

…ge key

[Wallet] add bumpfee RPC command

3f2d374

[RPC] Fix createrawtx sequence number unsigned int parsing

72812f7

[RPC] use fundrawtransactions feerate option for the bumpfee command

4bbf2cf

jonasschnelli force-pushed the 2016/04/rbf_combined branch from c3b60c8 to 4bbf2cf Compare June 8, 2016 16:18

jonasschnelli added 2 commits June 8, 2016 18:26

[SquashMe] remove initial fee for createrawtransaction 291e782

ce39786

[SquashMe] overhaul bumpfee

1a2339c

maflcko reviewed Jun 9, 2016
View reviewed changes

sipa reviewed Jun 14, 2016
View reviewed changes

laanwj removed this from the 0.13.0 milestone Jun 22, 2016

mrbandrews mentioned this pull request Aug 4, 2016

[RPC] Simplified bumpfee command. #8456

Merged

jonasschnelli closed this Aug 5, 2016

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RPC] Add bumpfee command. #7865

[RPC] Add bumpfee command. #7865

jonasschnelli commented Apr 12, 2016

instagibbs Apr 12, 2016

jonasschnelli Apr 12, 2016

jonasschnelli commented Apr 12, 2016

instagibbs commented Apr 13, 2016

NicolasDorier Apr 14, 2016

jonasschnelli Apr 14, 2016

NicolasDorier Apr 14, 2016

paveljanik commented Apr 26, 2016

NicolasDorier commented Apr 26, 2016 •

edited

jonasschnelli commented Apr 27, 2016

NicolasDorier commented Apr 27, 2016

jonasschnelli commented Apr 27, 2016

jonasschnelli commented Apr 28, 2016

jonasschnelli commented May 6, 2016

maflcko May 6, 2016

maflcko May 6, 2016

jonasschnelli May 6, 2016

jonasschnelli commented May 31, 2016

sipa Jun 1, 2016

jonasschnelli Jun 1, 2016 •

edited

maflcko Jun 20, 2016

sipa Jun 20, 2016 via email

jonasschnelli commented Jun 8, 2016

maflcko Jun 9, 2016

maflcko commented Jun 9, 2016

rubensayshi commented Jun 14, 2016 •

edited

sipa Jun 14, 2016

jonasschnelli Jun 14, 2016

laanwj Jun 14, 2016

gmaxwell commented Jun 15, 2016

fanquake commented Jun 22, 2016

laanwj commented Jun 22, 2016

jonasschnelli commented Aug 5, 2016

[RPC] Add bumpfee command. #7865

[RPC] Add bumpfee command. #7865

Conversation

jonasschnelli commented Apr 12, 2016

Refactor

bumpfee RPC command

sendtoaddress / sandmany

Bitcoin-Tx / CreateRawTransaction

Possible next steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasschnelli commented Apr 12, 2016

instagibbs commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paveljanik commented Apr 26, 2016

NicolasDorier commented Apr 26, 2016 • edited

jonasschnelli commented Apr 27, 2016

NicolasDorier commented Apr 27, 2016

jonasschnelli commented Apr 27, 2016

jonasschnelli commented Apr 28, 2016

jonasschnelli commented May 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasschnelli commented May 31, 2016

Choose a reason for hiding this comment

jonasschnelli Jun 1, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa Jun 20, 2016 via email

Choose a reason for hiding this comment

jonasschnelli commented Jun 8, 2016

Choose a reason for hiding this comment

maflcko commented Jun 9, 2016

rubensayshi commented Jun 14, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmaxwell commented Jun 15, 2016

fanquake commented Jun 22, 2016

laanwj commented Jun 22, 2016

jonasschnelli commented Aug 5, 2016

NicolasDorier commented Apr 26, 2016 •

edited

jonasschnelli Jun 1, 2016 •

edited

rubensayshi commented Jun 14, 2016 •

edited