[RPC] Add bumpfee command.

jonasschnelli commented at 11:45 am on April 12, 2016: contributor

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

jonasschnelli added the label Wallet on Apr 12, 2016

jonasschnelli added this to the milestone 0.13.0 on Apr 12, 2016

jonasschnelli force-pushed on Apr 12, 2016

in src/wallet/wallet.h: in f68447ad7c outdated

532@@ -533,6 +533,11 @@ class CAccountingEntry
533     std::vector<char> _ssExtra;
534 };
535 
536+enum CreateTransactionFlags {
537+    CREATE_TX_DEFAULT     = 0,

instagibbs commented at 3:03 pm on April 12, 2016:

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

jonasschnelli commented at 8:01 pm on April 12, 2016:

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)?

in src/wallet/wallet.cpp: in f68447ad7c outdated

2178@@ -2179,7 +2179,7 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2179                 // nLockTime set above actually works.

instagibbs commented at 3:07 pm on April 12, 2016:

update the comment?

jonasschnelli commented at 3:08 pm on April 12, 2016:

Good catch! Will update.

in src/wallet/rpcwallet.cpp: in f68447ad7c outdated

2559+    }
2560+
2561+    // remove "old" change outputs
2562+    for (std::vector<CTxOut>::iterator it(tx.vout.begin()); it != tx.vout.end();)
2563+    {
2564+        if (pwalletMain->IsMine(*it))

instagibbs commented at 3:47 pm on April 12, 2016:

I think this deletes outputs going to watch-only wallet addresses. Perhaps check for isminetype we care about.

jonasschnelli force-pushed on Apr 12, 2016

jonasschnelli commented at 8:11 pm on April 12, 2016: contributor

Force push fixed @instagibbs nits.

instagibbs commented at 8:34 pm on April 13, 2016: member

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

in src/wallet/rpcwallet.cpp: in 45b6ec5d22 outdated

2572+
2573+    // re-fund the transaction, a new change output will be added
2574+    CReserveKey reservekey(pwalletMain);
2575+    if(!pwalletMain->FundTransaction(tx, reservekey, nFee, nChangePos, strFailReason, false))
2576+        throw JSONRPCError(RPC_INTERNAL_ERROR, strFailReason);
2577+

NicolasDorier commented at 7:41 am on April 14, 2016:

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 ?

jonasschnelli commented at 9:31 am on April 14, 2016:

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

NicolasDorier commented at 9:58 am on April 14, 2016:

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.

in src/wallet/rpcwallet.cpp: in 45b6ec5d22 outdated

2600+        throw JSONRPCError(RPC_WALLET_ERROR, "Can't sign transaction.");
2601+
2602+    // commit/broadcast the transaction
2603+    CWalletTx wtxBumped(pwalletMain,tx);
2604+    if (!pwalletMain->CommitTransaction(wtxBumped, reservekey))
2605+        throw JSONRPCError(RPC_WALLET_ERROR, "Error: The transaction was rejected! Did you already bump the fee for this transaction?");

NicolasDorier commented at 7:42 am on April 14, 2016:

Transaction can be rejected even if the fee are not already bumped if the estimated fee rate decreased compared to the previous version of the transaction.

jonasschnelli commented at 9:32 am on April 14, 2016:

Yes. True. This is why the second sentence is a question. :) I guess it will be a common mistake to bump the same txid again. But I’m open for suggestions.

NicolasDorier commented at 9:59 am on April 14, 2016:

If you detect (nFee - nOldFee) > minTxRelayFee condition above, then it is not a question anymore. The early exit message would be “Fee are already bumped to the actual fee rate”.

The error on CommitTransaction can be “Something went wrong when commiting the transaction in the wallet”. Generic and not descriptive, but it should not happen very often, and if it happens, you are sure it is not because of Fees already bumped as you already checked that above.

jonasschnelli commented at 10:05 am on April 14, 2016:

IMO if you do (nFee - nOldFee) > minTxRelayFee you can’t be sure if you are using enough fees. The new transaction also needs to pay the bandwith-costs of the all replaced transaction descendants. Not?

sipa commented at 10:07 am on April 14, 2016:

nOldFee already is enough for all previously replaced ones, so you only need to add minTxRelayFee to account for everything. That’s the logic used by the replacement code as well.

NicolasDorier commented at 11:25 am on April 14, 2016:

@jonasschnelli I dont think so given the condition, just doing the same check should be enough.

jonasschnelli commented at 12:26 pm on April 14, 2016:

@NicolasDorier: the problem is, it could be the case that you don’t have a change output, or, that increasing the fee requires another <n> input(s). In theory – it could be that you need to add 10 new inputs (you don’t know the fee situation before/during bump and the value of your unspent outputs).

So, this results in the knapsack problem. You don’t know how big your transaction – and therefore what the minimum relay fee – needs to be.

During writing this PR, I though easiest solution is it, to just re-run the CreateTransaction logic (where it deals with min-fee, etc.).

But correct. Adding the old fee to the new fee is probably inefficient and “to much in fees”. But this is just a small change and can be optimized later.

NicolasDorier commented at 1:37 pm on April 14, 2016:

@jonasschnelli I don’t mean to remove the call to CreateTransaction. I mean just adding the fee check after the signature success check. (using the size of the fully signed new transaction replacement)

jonasschnelli commented at 2:10 pm on April 27, 2016:

Nicolas and I have sorted out that problem. The current bumpfee command takes the old fee as base fee, runs the coin selection ( new inputs might be required to bump the fee) which results in: newFee = oldFee + feeOfNewStandaloneTransaction

At the moment, this results in ~doubling the fee.

paveljanik commented at 7:06 am on April 26, 2016: contributor

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

NicolasDorier commented at 8:45 am on April 26, 2016: contributor

@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 commented at 1:54 pm on April 27, 2016: contributor

@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 commented at 2:04 pm on April 27, 2016: contributor

right, utACK

jonasschnelli force-pushed on Apr 27, 2016

jonasschnelli commented at 2:13 pm on April 27, 2016: contributor

Rebased (non trivial after #7518).

jonasschnelli force-pushed on Apr 27, 2016

jonasschnelli force-pushed on Apr 28, 2016

jonasschnelli commented at 8:41 am on April 28, 2016: contributor

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

jonasschnelli force-pushed on May 6, 2016

jonasschnelli commented at 9:21 am on May 6, 2016: contributor

Needed rebase.

in qa/rpc-tests/bumpfee.py: in 5a4cb5eec3 outdated

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

MarcoFalke commented at 9:32 am on May 6, 2016:

Mind to change this to py3?

in qa/rpc-tests/bumpfee.py: in 5a4cb5eec3 outdated

53+        replacedTx = ""
54+        try:
55+            replacedTx= self.nodes[1].getrawtransaction(rbftx)
56+        except JSONRPCException as e:
57+            pass
58+        assert_equal(len(replacedTx), 0)

MarcoFalke commented at 9:33 am on May 6, 2016:

Can you move the assert into the try block? (Keeps the scope clear)

in qa/rpc-tests/bumpfee.py: in 5a4cb5eec3 outdated

77+        errorString = ""
78+        try:
79+            bumpedrawtx = self.nodes[0].bumpfee(nonrbftx)
80+        except JSONRPCException as e:
81+            errorString = e.error['message']
82+        assert(len(errorString) > 0)

MarcoFalke commented at 9:35 am on May 6, 2016:

I don’t like this syntax: #7801

Mind to move the assert into the except block?

jonasschnelli force-pushed on May 6, 2016

jonasschnelli commented at 10:14 am on May 6, 2016: contributor

Migrated test to python3, fixed @MarcoFalke nits.

in qa/rpc-tests/bumpfee.py: in 84085d19a3 outdated

78+        try:
79+            bumpedrawtx = self.nodes[0].bumpfee(nonrbftx)
80+            assert(False)
81+        except JSONRPCException as e:
82+            errorString = e.error['message']
83+            assert(len(errorString) > 0)

MarcoFalke commented at 10:23 am on May 6, 2016:

You don’t need the errorString, just assert(len(e.error['message']) > 0) or assert_raises(JSONRPCException, ...

in qa/rpc-tests/bumpfee.py: in 84085d19a3 outdated

53+        replacedTx = ""
54+        try:
55+            replacedTx= self.nodes[1].getrawtransaction(rbftx)
56+            assert(False)
57+        except JSONRPCException as e:
58+            assert_equal(len(replacedTx), 0)

MarcoFalke commented at 10:23 am on May 6, 2016:

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

jonasschnelli commented at 10:34 am on May 6, 2016:

Right. Fixed.

jonasschnelli force-pushed on May 6, 2016

jonasschnelli force-pushed on May 31, 2016

jonasschnelli commented at 8:51 am on May 31, 2016: contributor

Rebased. Fixed nit reported by @MarcoFalke.

in src/wallet/wallet.h: in fde7a2c7f0 outdated

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

sipa commented at 5:05 pm on June 1, 2016:

Unresolved conflict

jonasschnelli commented at 6:07 pm on June 1, 2016:

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

MarcoFalke commented at 1:07 pm on June 20, 2016:

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

sipa commented at 1:14 pm on June 20, 2016:

It also breaks bisections.

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

in qa/rpc-tests/bumpfee.py: in e9931f56e6 outdated

 8+
 9+class BumpFeeTest (BitcoinTestFramework):
10+
11+    def setup_chain(self):
12+        print("Initializing test directory "+self.options.tmpdir)
13+        initialize_chain_clean(self.options.tmpdir, 4)

MarcoFalke commented at 6:11 pm on June 1, 2016:

Nit: I think you only need 3 nodes?

Also, you can use __init__ now. (Instead of overwriting setup_chain):

0    def __init__(self):
1        super().__init__()
2        self.num_nodes = 3
3        self.setup_clean_chain = True # not needed (default should already be true)

jonasschnelli commented at 6:54 pm on June 1, 2016:

Thanks @MarcoFalke. Test fixed.

jonasschnelli force-pushed on Jun 1, 2016

[Refactor] CreateTransaction(): use bit flags

This will allow to add flags for RBF and other features

2980a7a5ff

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

[Wallet] allow to set an initial fee amount to be added to the fee calculation 291e7823bd

[Wallet] FundTransaction: return reserveKey to allow keeping the change key a5018782fb

[Wallet] add bumpfee RPC command 3f2d37410e

[RPC] Fix createrawtx sequence number unsigned int parsing 72812f709c

[RPC] use fundrawtransactions feerate option for the bumpfee command 4bbf2cf0b3

jonasschnelli force-pushed on Jun 8, 2016

[SquashMe] remove initial fee for createrawtransaction 291e7823bd912a5e83316d9194f98e76f78690d1 ce3978628e

[SquashMe] overhaul bumpfee 1a2339c2e5

jonasschnelli commented at 7:58 pm on June 8, 2016: contributor

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.

in qa/rpc-tests/bumpfee.py: in 1a2339c2e5

69+
70+        # try to replace a non RBF transaction
71+        nonrbftx = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1, "", "", True, False)
72+
73+        try:
74+            bumpedrawtx = self.nodes[0].bumpfee(nonrbftx)

MarcoFalke commented at 12:17 pm on June 9, 2016:

Replace by

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

MarcoFalke commented at 12:18 pm on June 9, 2016: member

Mind to squash the history to aid review?

rubensayshi commented at 11:43 am on June 14, 2016: contributor

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 ;-)

in src/wallet/rpcwallet.cpp: in 1a2339c2e5

2527+        CTxDestination address;
2528+        if (pwalletMain->IsMine(*it) == ISMINE_SPENDABLE && (!ExtractDestination((*it).scriptPubKey, address) || !pwalletMain->mapAddressBook.count(address))) {
2529+            it = tx.vout.erase(it);
2530+        }
2531+        else {
2532+            ++it;

sipa commented at 2:17 pm on June 14, 2016:

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

jonasschnelli commented at 2:19 pm on June 14, 2016:

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

laanwj commented at 2:24 pm on June 14, 2016:

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 commented at 10:08 am on June 15, 2016: contributor

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 commented at 3:21 am on June 22, 2016: member

Is this still going into 0.13.0 ?

laanwj removed this from the milestone 0.13.0 on Jun 22, 2016

laanwj commented at 6:54 am on June 22, 2016: member

@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 commented at 10:55 am on August 5, 2016: contributor

superseded by #8456. Closing.

jonasschnelli closed this on Aug 5, 2016

MarcoFalke locked this on Sep 8, 2021

[RPC] Add bumpfee command. #7865

Refactor

bumpfee RPC command

sendtoaddress / sandmany

Bitcoin-Tx / CreateRawTransaction

Possible next steps