[RPC] Simplified bumpfee command.

mrbandrews commented at 1:52 pm on August 4, 2016: contributor

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:

0confirmTarget - perhaps the reason the user feels the need to bump the fee is because the original confirmTarget was too slow
1totalFee - 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.

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

jonasschnelli commented at 10:56 am on August 5, 2016: contributor

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

in src/wallet/rpcwallet.cpp: in 41572b183d outdated

2652+        }
2653+    }
2654+
2655+    // calculate the old fee and fee-rate
2656+    CAmount nDebit = wtx.GetDebit(ISMINE_SPENDABLE);
2657+    CAmount nOldFee = -(wtx.IsFromMe(ISMINE_SPENDABLE) ? wtx.GetValueOut() - nDebit : 0);

NicolasDorier commented at 10:39 pm on August 5, 2016:

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

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

mrbandrews commented at 6:13 pm on October 18, 2016:

Fixed.

in src/wallet/rpcwallet.cpp: in 41572b183d outdated

2692+    // check that fee rate is higher than mempool's mininum fee
2693+    // (no point in bumping fee if we know that the new tx won't be accepted to the mempool)
2694+    CFeeRate minMempoolFeeRate = mempool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000);
2695+    if (nNewFeeRate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {
2696+        strError = strprintf("New fee rate (%s) is too low to get into the mempool (min rate: %s)", FormatMoney(nNewFeeRate.GetFeePerK()), FormatMoney(minMempoolFeeRate.GetFeePerK()));
2697+        throw JSONRPCError(RPC_MISC_ERROR, strError);

NicolasDorier commented at 10:47 pm on August 5, 2016:

I’m not sure about this. The problem is that such case will rarely happen so applications risk to break during sudden peak usage, amplifying bad news.

I suggest to silently bump fees to the minMempoolFeeRate.

mrbandrews commented at 6:13 pm on October 18, 2016:

I edited the comment to explain why I’m reporting an error. Especially if the user set totalFee (or had in the recent past used paytxfee to set an explicit fee rate) I think silently bumping could surprise the user.

in src/wallet/rpcwallet.cpp: in 41572b183d outdated

2715+            // situation than bumping the fee to pay the minimum relay fee. Here, we can't be sure whether the user
2716+            // really wants to pay full price for all of the child transactions.  If so, the user can set payTxFee
2717+            // and run the command again.
2718+            //
2719+            CAmount nFeesWithDescendants = it->GetModFeesWithDescendants();
2720+            if (nNewFee < nFeesWithDescendants + ::minRelayTxFee.GetFee(txSize)) {

NicolasDorier commented at 10:50 pm on August 5, 2016:

nit:

0 nFeesWithDescendants + ::minRelayTxFee.GetFee(txSize)

Is repeated. Suggest using variable.

mrbandrews commented at 6:11 pm on October 18, 2016:

Fixed.

in src/wallet/rpcwallet.cpp: in 41572b183d outdated

2743+
2744+    // Now modify the output to increase the fee.
2745+    // Output must be able to pay the increased fee, without being reduced to dust
2746+    CMutableTransaction tx(wtx);
2747+    CTxOut* poutput = &(tx.vout[nOutput]);
2748+    if (poutput->nValue >= nDelta + poutput->GetDustThreshold(::minRelayTxFee)) {

NicolasDorier commented at 10:55 pm on August 5, 2016:

additional check should be done:

0nDelta >= poutput->GetDustThreshold(::minRelayTxFee);

Or it won’t get propagated.

mrbandrews commented at 6:11 pm on October 18, 2016:

I checked earlier that the bumped fee pays for the new relay fee; I’m not following as to the relationship between nDelta and dust.

luke-jr commented at 7:33 am on November 23, 2016:

@NicolasDorier I also don’t see a need for that…

in src/wallet/rpcwallet.cpp: in 41572b183d outdated

2753+    }
2754+
2755+    // sign the new tx
2756+    CTransaction txNewConst(tx);
2757+    int nIn = 0;
2758+    for (auto it(tx.vin.begin()); it != tx.vin.end(); ++it, nIn++) {

NicolasDorier commented at 10:56 pm on August 5, 2016:

can you use C++11 loop style ?

mrbandrews commented at 6:07 pm on October 18, 2016:

Good idea, fixed.

in src/wallet/rpcwallet.cpp: in 41572b183d outdated

2758+    for (auto it(tx.vin.begin()); it != tx.vin.end(); ++it, nIn++) {
2759+        std::map<uint256, CWalletTx>::const_iterator mi = pwalletMain->mapWallet.find((*it).prevout.hash);
2760+        if (mi != pwalletMain->mapWallet.end() && (nIn < (int)(*mi).second.vout.size())) {
2761+            const CScript& scriptPubKey = (*mi).second.vout[(*it).prevout.n].scriptPubKey;
2762+            SignatureData sigdata;
2763+            if (!ProduceSignature(TransactionSignatureCreator(pwalletMain, &txNewConst, nIn, SIGHASH_ALL), scriptPubKey, sigdata))

NicolasDorier commented at 10:58 pm on August 5, 2016:

You should use the same SIGHASH than the previous one. (might be done separate PR ?)

mrbandrews commented at 6:07 pm on October 18, 2016:

Let’s punt to separate PR.

in src/wallet/rpcwallet.cpp: in 41572b183d outdated

2755+    // sign the new tx
2756+    CTransaction txNewConst(tx);
2757+    int nIn = 0;
2758+    for (auto it(tx.vin.begin()); it != tx.vin.end(); ++it, nIn++) {
2759+        std::map<uint256, CWalletTx>::const_iterator mi = pwalletMain->mapWallet.find((*it).prevout.hash);
2760+        if (mi != pwalletMain->mapWallet.end() && (nIn < (int)(*mi).second.vout.size())) {

NicolasDorier commented at 11:02 pm on August 5, 2016:

(nIn < (int)(*mi).second.vout.size())

I think this is a bug, why does the index of the input to sign should be below the number of vout of the parent ? Does not make sense.

mrbandrews commented at 6:09 pm on October 18, 2016:

Good catch, edited to bounds-check the prevout.

MarcoFalke added the label Wallet on Aug 7, 2016

jonasschnelli commented at 4:50 pm on August 19, 2016: contributor

Needs rebase and reviewers… setting 0.14 milestone.

jonasschnelli added this to the milestone 0.14.0 on Aug 19, 2016

mrbandrews force-pushed on Oct 18, 2016

mrbandrews commented at 6:07 pm on October 18, 2016: contributor

Rebased and addressed feedback.

luke-jr referenced this in commit 75ed0b32a8 on Oct 20, 2016

luke-jr referenced this in commit f012520116 on Oct 20, 2016

luke-jr referenced this in commit e73a5f1dfb on Oct 20, 2016

jonasschnelli commented at 7:55 pm on October 20, 2016: contributor

Needs rebase again…

mrbandrews force-pushed on Oct 21, 2016

mrbandrews commented at 3:44 pm on October 21, 2016: contributor

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

in src/wallet/rpcwallet.cpp: in 8e969e3fc4 outdated

2582+    if (!EnsureWalletIsAvailable(request.fHelp))
2583+        return NullUniValue;
2584+
2585+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 3)
2586+        throw runtime_error(
2587+                            "bumpfee \"txid\"\n"

luke-jr commented at 2:35 pm on October 22, 2016:

This line is outdated

RHavar commented at 0:30 am on October 26, 2016: contributor

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 commented at 0:34 am on October 26, 2016: contributor

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?

in src/wallet/rpcwallet.cpp: in 8e969e3fc4 outdated

2647+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Output out of bounds");
2648+
2649+    // optional parameters
2650+    int newConfirmTarget = nTxConfirmTarget;
2651+    CAmount totalFee = 0;
2652+    if (request.params.size() > 2) {

JeremyRubin commented at 7:38 pm on October 26, 2016:

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.

in src/wallet/rpcwallet.cpp: in 8e969e3fc4 outdated

2666+        }
2667+        if (options.exists("totalFee")) {
2668+            totalFee = options["totalFee"].get_int();
2669+            if (totalFee <= 0)
2670+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid totalFee (cannot be <= 0)");
2671+            else if (totalFee > maxTxFee)

JeremyRubin commented at 7:48 pm on October 26, 2016:

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.

in src/wallet/rpcwallet.cpp: in 8e969e3fc4 outdated

2719+    if (nNewFeeRate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) {
2720+        strError = strprintf("New fee rate (%s) is too low to get into the mempool (min rate: %s)", FormatMoney(nNewFeeRate.GetFeePerK()), FormatMoney(minMempoolFeeRate.GetFeePerK()));
2721+        throw JSONRPCError(RPC_MISC_ERROR, strError);
2722+    }
2723+
2724+    CAmount nDelta = nNewFee - nOldFee;

JeremyRubin commented at 8:12 pm on October 26, 2016:

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.

mrbandrews commented at 9:03 pm on October 27, 2016:

Done.

morcos commented at 9:28 pm on November 21, 2016:

@JeremyRubin how could nDelta be negative here?

in src/wallet/rpcwallet.cpp: in 8e969e3fc4 outdated

2678+    CAmount nOldFee = (wtx.IsFromMe(ISMINE_SPENDABLE) ? nDebit - wtx.GetValueOut() : 0);
2679+    int txSize = (int)GetVirtualTransactionSize((CTransaction)wtx);
2680+    CFeeRate nOldFeeRate(nOldFee, txSize);
2681+
2682+    // use the user-defined payTxFee if possible, otherwise use smartfee / fallbackfee
2683+    CAmount nNewFee = 0;

JeremyRubin commented at 8:13 pm on October 26, 2016:

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.

in src/wallet/rpcwallet.cpp: in 8e969e3fc4 outdated

2739+            // situation than bumping the fee to pay the minimum relay fee. Here, we can't be sure whether the user
2740+            // really wants to pay full price for all of the child transactions.  If so, the user can set payTxFee
2741+            // and run the command again.
2742+            //
2743+            CAmount nFeesWithDescendantsPlusRelay = it->GetModFeesWithDescendants() + ::minRelayTxFee.GetFee(txSize);
2744+            if (nNewFee < nFeesWithDescendantsPlusRelay) {

JeremyRubin commented at 8:21 pm on October 26, 2016:

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?

JeremyRubin commented at 8:48 pm on October 26, 2016:

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 commented at 9:05 pm on October 27, 2016:

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 commented at 8:52 pm on October 26, 2016: contributor

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.

mrbandrews commented at 9:10 pm on October 27, 2016: contributor

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 commented at 1:13 pm on November 11, 2016: none

ACK f3833f4 Tested on Windows x64

0bumpfee 8c56b13830405a55ec4bc58b26b531f1b187d2349ee19bd0dd01aa835972929a 1
1
2{
3  "txid": "67d6af1a3e29e246eaef0b7ce272f745e2ae6178050ccb78fca515b13c0f9e92",
4  "oldfee": 0.00000260,
5  "fee": 0.00000520
6}

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

2592+                            "Fee must be high enough to pay a new relay fee.\n"
2593+                            "If tx has child transactions in mempool, the new fee must pay for them as well.\n"
2594+                            "This command will fail if fee is not high enough or output is not large enough.\n"
2595+                            "User can specify totalFee, or use RPC setpaytxfee to set a higher fee rate.\n"
2596+                            "\nArguments:\n"
2597+                            "1. \"transactionid\"     (string, required) The txid to be bumped\n"

jonasschnelli commented at 1:18 pm on November 11, 2016:

nit: Maybe use txid to correspond with the call header at L2587 or use transactionid there?

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

2582+    if (!EnsureWalletIsAvailable(request.fHelp))
2583+        return NullUniValue;
2584+
2585+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 3)
2586+        throw runtime_error(
2587+                            "bumpfee \"txid\" \"output\" \n"

jonasschnelli commented at 1:19 pm on November 11, 2016:

add third parameter ("options")?

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

2597+                            "1. \"transactionid\"     (string, required) The txid to be bumped\n"
2598+                            "2. \"output\"            (int, required) The output to be decremented\n"
2599+                            "3. options               (object, optional)\n"
2600+                            "   {\n"
2601+                            "     \"confTarget\"      (int, optional) Confirmation target (in blocks)\n"
2602+                            "     \"totalFee\"        (numeric, optional) Total fee to pay, in satoshis (not btc)\n"

jonasschnelli commented at 1:22 pm on November 11, 2016:

I’m not sure if we want absolute fee values here. The user can’t be sure how many inputs are getting added when setting this value, probably resulting in an uncontrollable feerate.

What about switching this to a feerate?

Victorsueca commented at 1:26 pm on November 11, 2016:

Another possibility is to leave the RPC with an absolute value and use Fee/KB on the GUI. Some software may want to use it’s own relative fee rate.

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

2627+
2628+    // check that we are not trying to bump a tx that has already been mined
2629+    vector<COutput> vecOutputs;
2630+    pwalletMain->AvailableCoins(vecOutputs, false, NULL, true);
2631+    BOOST_FOREACH(const COutput& out, vecOutputs) {
2632+        if (out.tx->GetHash().GetHex() == hash.GetHex() && out.nDepth > 0)

jonasschnelli commented at 1:25 pm on November 11, 2016:

This check looks really expensive for large wallets. Why not calling mapWallet.find(hash) and check nDepth (and maybe call GetConflicts() to ensure its not conflicted with a already mined tx)?

jonasschnelli commented at 1:31 pm on November 11, 2016: contributor

Oh. I just realized that this PR does not add new inputs (it requires an output index to identify the change-output which then can be reduced).

IMO we should…

1.) not let the user identify which output is change 2.) allow bumping fees including adding new inputs (some transactions do not have a change output, some will not allow a reasonable bumping without adding a new input).

But 1.) & 2.) can also be solved later.

mrbandrews commented at 4:38 pm on November 14, 2016: contributor

Feedback addressed. Good catch on checking whether the tx had already been mined - that code was able to be shortened to a single line. Yes, the approach of this PR is to solve 1) and 2) later. If this looks good I’ll squash the commits.

in src/wallet/rpcwallet.cpp: in 82062cd27d outdated

2636+    // Check that if the original tx has wallet conflicts (meaning there is at least one other tx
2637+    // in the wallet that spends at least one of the same outputs) that it is the most recent one.
2638+    // This prevents us from trying to bump fee on a tx that has already been bumped.  So if the
2639+    // user created txid1, bumped the fee which resulted in txid2, and now wishes to bump the fee
2640+    // again, the user must call bumpfee on txid2.
2641+    const CWalletTx *conflictTx;

morcos commented at 7:49 pm on November 14, 2016:

I don’t think this section makes sense. Transactions which are in newer in the wallet are not a good indication of the most “up to date” spend of the outputs. If anything, conflicts in the mempool would be a good proxy, because at least for those you’d be able to calculate any descendants fees. But it may make the most sense to make this more of a utility function and just assume the user is trying to bump the right txid. I would just eliminate this set of checks entirely.

mrbandrews commented at 4:21 pm on November 17, 2016:

Fixed (removed)

in src/wallet/rpcwallet.cpp: in 82062cd27d outdated

2593+                            "If tx has child transactions in mempool, the new fee must pay for them as well.\n"
2594+                            "This command will fail if fee is not high enough or output is not large enough.\n"
2595+                            "User can specify totalFee, or use RPC setpaytxfee to set a higher fee rate.\n"
2596+                            "\nArguments:\n"
2597+                            "1. \"txid\"              (string, required) The txid to be bumped\n"
2598+                            "2. output                (int, required) The output to be decremented\n"

morcos commented at 7:53 pm on November 14, 2016:

Would be helpful to refer to this as the output index throughout… (comments, help, and error messages)

mrbandrews commented at 4:21 pm on November 17, 2016:

Done.

in src/wallet/rpcwallet.cpp: in 82062cd27d outdated

2691+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid totalFee (cannot be higher than bumpfee's maxFee)");
2692+        }
2693+    }
2694+
2695+    // calculate the old fee and fee-rate
2696+    CAmount nDebit = wtx.GetDebit(ISMINE_SPENDABLE);

morcos commented at 9:29 pm on November 15, 2016:

We need to be sure we are calculating the correct fee here. Either using new IsAllFromMe or getting the fee from the mempool.

mrbandrews commented at 4:24 pm on November 17, 2016:

This should be correct since now we’re checking that the user owns all the inputs.

in src/wallet/rpcwallet.cpp: in 82062cd27d outdated

2703+    CFeeRate nNewFeeRate;
2704+    if (payTxFee.GetFeePerK() > 0) {
2705+        nNewFeeRate = CFeeRate(payTxFee.GetFeePerK());
2706+    }
2707+    else {
2708+        int estimateFoundTarget = newConfirmTarget;

morcos commented at 9:44 pm on November 15, 2016:

you don’t need this, you can just leave it out as an argument

mrbandrews commented at 4:24 pm on November 17, 2016:

Fixed.

in src/wallet/rpcwallet.cpp: in 82062cd27d outdated

2760+            // situation than bumping the fee to pay the minimum relay fee. Here, we can't be sure whether the user
2761+            // really wants to pay full price for all of the child transactions.  If so, the user can set payTxFee
2762+            // and run the command again.
2763+            //
2764+            CAmount nFeesWithDescendantsPlusRelay = it->GetModFeesWithDescendants() + ::minRelayTxFee.GetFee(txSize);
2765+            if (nNewFee < nFeesWithDescendantsPlusRelay) {

morcos commented at 9:52 pm on November 15, 2016:

I think this reduces bumpfee to a two step process in the common case (whenever there are children) and makes it so the second step is going to require setting totalfee. It seems more user friendly to just pay what it takes?

On a separate note, I think it might be nice to check all of the children transactions and make sure their ancestor fee rate isn’t higher than the new feerate you are bumping to. If one of them has a higher ancestor fee rate you are actually make your situation worse.

morcos commented at 3:25 pm on November 17, 2016:

I spoke with @sdaftuar and @mrbandrews about this and withdraw both of the above points. If the transaction has children, it becomes quite complicated to decide whether bumping fee makes sense and by how much and its probably better to just report to the user the minimum fee that would be required to bump and let the user decide whether that makes sense.

mrbandrews commented at 4:26 pm on November 17, 2016: contributor

morcos feedback addressed, including adding the first commit from #9167.

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

2625+    if (!pwalletMain->mapWallet.count(hash))
2626+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid or non-wallet transaction id");
2627+    const CWalletTx& wtx = pwalletMain->mapWallet[hash];
2628+
2629+    // check that we are not trying to bump a tx that has already been mined
2630+    if (wtx.GetDepthInMainChain() > 0)

morcos commented at 9:04 pm on November 21, 2016:

You should probably check that the depth is exactly 0, because otherwise the tx is already mined or conflicted with a mined tx and no point in bumping.

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

2630+    if (wtx.GetDepthInMainChain() > 0)
2631+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "This transaction has already been mined!");
2632+
2633+    // check that original tx signals opt-in-RBF
2634+    if (!SignalsOptInRBF(wtx))
2635+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction is not bip-125 replaceable");

luke-jr commented at 7:18 am on November 23, 2016:

“BIP 125”

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

2582+    if (!EnsureWalletIsAvailable(request.fHelp))
2583+        return NullUniValue;
2584+
2585+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 3)
2586+        throw runtime_error(
2587+                            "bumpfee \"txid\" output ( options ) \n"

luke-jr commented at 7:19 am on November 23, 2016:

Maybe move output into options? Even if it’s required in this version, it should ideally become optional in the future…

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

2688+        }
2689+    }
2690+
2691+    // calculate the old fee and fee-rate
2692+    CAmount nOldFee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.GetValueOut();
2693+    int txSize = (int)GetVirtualTransactionSize((CTransaction)wtx);

luke-jr commented at 7:22 am on November 23, 2016:

Just use int64_t? int is only guaranteed to be 15-bit, so it’s too small anyway.

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

2695+
2696+    // use the user-defined payTxFee if possible, otherwise use smartfee / fallbackfee
2697+    CAmount nNewFee = 0;
2698+    CFeeRate nNewFeeRate;
2699+    if (payTxFee.GetFeePerK() > 0) {
2700+        nNewFeeRate = CFeeRate(payTxFee.GetFeePerK());

luke-jr commented at 7:23 am on November 23, 2016:

nNewFeeRate = payTxFee;?

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

2712+    // signature sizes can vary by a byte, so add 1 for each input when calculating the new fee
2713+    nNewFee = nNewFeeRate.GetFee(txSize + wtx.vin.size());
2714+
2715+    // if user set totalFee, use that instead
2716+    if (totalFee > 0) {
2717+        CAmount minTotalFee = nOldFeeRate.GetFee(txSize) + minRelayTxFee.GetFee(txSize);

luke-jr commented at 7:26 am on November 23, 2016:

+ wtx.vin.size() seems appropriate here too?

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

2760+                uint64_t numDescendants = it->GetCountWithDescendants()-1;
2761+                uint64_t sizeDescendants = it->GetSizeWithDescendants() - it->GetTxSize();
2762+                std::string strError = strprintf("Insufficent fee due to the child transactions.\n");
2763+                strError += strprintf("The bumped fee must be at least: %s.\n", FormatMoney(nFeesWithDescendantsPlusRelay));
2764+                strError += strprintf("Number of child transactions: %u, total size of child transactions: %u\n", numDescendants, sizeDescendants);
2765+                strError += strprintf("Note that the child transactions would be evicted from the mempool and would not be mined.\n");

luke-jr commented at 7:31 am on November 23, 2016:

s/would/may probably/

In the future, it may make sense to combine some children transactions (ie, our own) into this one while bumping the fee.

luke-jr commented at 8:24 am on November 23, 2016:

Also no strprintf here

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

2795+    CTxOut* poutput = &(tx.vout[nOutput]);
2796+    if (poutput->nValue >= nDelta + poutput->GetDustThreshold(::minRelayTxFee)) {
2797+        poutput->nValue = poutput->nValue - nDelta;
2798+    }
2799+    else {
2800+        throw JSONRPCError(RPC_MISC_ERROR, "Output does not have enough money to bump the fee");

luke-jr commented at 7:33 am on November 23, 2016:

Why not just discard the change?

luke-jr changes_requested

luke-jr commented at 7:34 am on November 23, 2016: member

IMO the main logic should probably be split up into non-RPC files so it can be reused in the GUI…

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

2816+    }
2817+
2818+    // commit/broadcast the tx
2819+    CReserveKey reservekey(pwalletMain);
2820+    CWalletTx wtxBumped(pwalletMain, tx);
2821+    if (!pwalletMain->CommitTransaction(wtxBumped, reservekey, g_connman.get()))

luke-jr commented at 7:35 am on November 23, 2016:

This needs to be rebased…

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

2757+            //
2758+            CAmount nFeesWithDescendantsPlusRelay = it->GetModFeesWithDescendants() + ::minRelayTxFee.GetFee(txSize);
2759+            if (nNewFee < nFeesWithDescendantsPlusRelay) {
2760+                uint64_t numDescendants = it->GetCountWithDescendants()-1;
2761+                uint64_t sizeDescendants = it->GetSizeWithDescendants() - it->GetTxSize();
2762+                std::string strError = strprintf("Insufficent fee due to the child transactions.\n");

luke-jr commented at 8:24 am on November 23, 2016:

No need for strprintf here

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

2597+                            "1. \"txid\"              (string, required) The txid to be bumped\n"
2598+                            "2. output index          (numeric, required) The output index to be decremented\n"
2599+                            "3. options               (object, optional)\n"
2600+                            "   {\n"
2601+                            "     \"confTarget\":       \"n\",          (numeric, optional) Confirmation target (in blocks)\n"
2602+                            "     \"totalFee\":         \"n\",          (numeric, optional) Total fee to pay, in satoshis (not btc)\n"

jonasschnelli commented at 9:24 am on November 23, 2016:

I think, because every else use fee-ratres per KB, we should make clear at this point that totalFee and maxFee are absolute fee-values (and not per KB).

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

2761+                uint64_t sizeDescendants = it->GetSizeWithDescendants() - it->GetTxSize();
2762+                std::string strError = strprintf("Insufficent fee due to the child transactions.\n");
2763+                strError += strprintf("The bumped fee must be at least: %s.\n", FormatMoney(nFeesWithDescendantsPlusRelay));
2764+                strError += strprintf("Number of child transactions: %u, total size of child transactions: %u\n", numDescendants, sizeDescendants);
2765+                strError += strprintf("Note that the child transactions would be evicted from the mempool and would not be mined.\n");
2766+                strError += strprintf("To avoid mempool eviction, consider bumping fee on the child transactions (with fee to pay for the ancestors).\n");

luke-jr commented at 10:20 am on November 23, 2016:

del strprintf

mrbandrews force-pushed on Dec 1, 2016

mrbandrews commented at 7:52 pm on December 1, 2016: contributor

OK, rebased and squashed, with edits addressing the most recent feedback in a separate commit.

I didn’t separate code into non-rpc files because I’m not 100% sure which logic should be moved, and thought this decision could be made when using it from the GUI, as moving the code then should be easy enough.

in src/wallet/rpcwallet.cpp: in 7d9f218dab outdated

2716+    // signature sizes can vary by a byte, so add 1 for each input when calculating the new fee
2717+    nNewFee = nNewFeeRate.GetFee(txSize + wtx.vin.size());
2718+
2719+    // if user set totalFee, use that instead
2720+    if (totalFee > 0) {
2721+        CAmount minTotalFee = nOldFeeRate.GetFee(txSize+wtx.vin.size()) + minRelayTxFee.GetFee(txSize+wtx.vin.size());

luke-jr commented at 4:48 am on December 4, 2016:

You should calculate txSize + wtx.vin.size() once and keep it in a const variable for these…

0const int64_t maxNewTxSize = txSize + wtx.vin.size();

gmaxwell commented at 9:55 pm on December 4, 2016: contributor

Asking the user to identify change is unreasonable and dangerous. Within our own wallet we should know which outputs are our own. This also has a problem of creating a mess when the original version of the transaction it spent, but later sends may have spent the replacements output.

I would suggest that a minimal bump-fee would do this:

(1) Only be available on transactions where none of their outputs have been spent (even in mempool). (2) Preserve all the user provided outputs, potentially changing the change amount, potentially adding more inputs if needed to get past dust or just sufficient fees. Obey the opt-in RBF minimum increment (3) Mark its local output(s) as unspendable until confirmed.

1/3 are required so that we don’t end up with a mess when the ‘wrong’ version of the transaction confirms and invalidates the others.

A somewhat more advanced – and perhaps better to do instead– would be a “bumpunconfirmed” which would not act on a single transaction but on all valid unconfirmed sends of the local wallet at once– generating a single replacement transaction which conflicts each of the originals, pays a higher fee, and marks its own output as unspendable until confirmed.

I suggest this might be better instead because it would not need to have the requirement that its local outputs not be unspent, since it would replace all those spends at once. The reduced number of change outputs plus the ability to drop some of the inputs (so long as it still conflicts, and still marks them as unspendable until this transaction confirms) means that it can reduce the total transaction sizes.

mrbandrews force-pushed on Dec 9, 2016

mrbandrews commented at 6:59 pm on December 9, 2016: contributor

Addressed the gmaxwell feedback as follows: now it identifies the change output and guards against spending outputs until the bumped transaction (or perhaps the original transaction) is mined.

I reorganized this into two commits, the latter being the python test, which is updated.

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

2635+
2636+    if (!SignalsOptInRBF(wtx))
2637+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction is not BIP 125 replaceable");
2638+
2639+    if (wtx.mapValue.count("replaced_by_txid"))
2640+      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")));

luke-jr commented at 0:25 am on December 10, 2016:

Any reason not to automatically just switch to the new txid here?

mrbandrews commented at 5:42 pm on December 13, 2016:

For now, I’d rather leave it as is, and let the user resubmit with the correct txid.

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

2645+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction contains inputs that don't belong to this wallet");
2646+
2647+    // figure out which output was change
2648+    // if there was no change output or if there were multiple change outputs, fail
2649+    int nOutput = -1;
2650+    for (int i=0; i < (int) wtx.tx->vout.size(); i++) {

luke-jr commented at 0:25 am on December 10, 2016:

Just use a size_t here, not cast to int… And use ++i, not i++

But more importantly, better to use C++11 iterators in this case:

0for (const CTxOut& txout : wtx.tv->vout) {
1    if (pwalletMain->IsChange(txout)) {
2        if (nOutput != -1) {
3            throw JSONRPCError(RPC_MISC_ERROR, "Transaction has multiple change outputs");
4        }
5        nOutput = i;
6    }
7}

sdaftuar commented at 2:30 pm on December 12, 2016:

@luke-jr That won’t compile sincei is undefined… Is there a better way to loop here where the index is needed? Also I’d suggest that this kind of style nit can be cleaned up in a later pull anyway; this one has been open for long enough.

mrbandrews commented at 5:43 pm on December 13, 2016:

Changed to a size_t (and ++i)

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

2793+        throw JSONRPCError(RPC_WALLET_ERROR, strError);
2794+    }
2795+
2796+    // mark the original tx as bumped
2797+    {
2798+        CWalletDB walletdb(pwalletMain->strWalletFile, "r+");

luke-jr commented at 0:32 am on December 10, 2016:

I think we’re trying to keep CWalletDB only used by CWallet. @pstratem ?

luke-jr commented at 0:39 am on December 10, 2016:

Probably this needs to go through CWallet::AddToWallet

luke-jr commented at 0:41 am on December 10, 2016:

May need to be atomic with adding the new one as well…

mrbandrews commented at 5:48 pm on December 13, 2016:

I moved this code into a new method on the wallet so the CWalletDB is only used from CWallet. I looked at AddToWallet but I would need to have edited it a bit, plus it does some stuff I don’t need so I thought a new but concise method was a better approach.

luke-jr changes_requested

luke-jr commented at 0:51 am on December 10, 2016: member

I wonder if this ought to interact with abandontransaction in some way?

in src/wallet/rpcwallet.cpp: in 64a17d6b95 outdated

2594+                            "bumpfee \"txid\" ( options ) \n"
2595+                            "\nBumps the fee of a opt-in-RBF transaction.\n"
2596+                            "This command requires that the transaction with the given txid is in the wallet.\n"
2597+                            "This command will NOT add new inputs.\n"
2598+                            "Fee must be high enough to pay a new relay fee.\n"
2599+                            "If tx has child transactions in mempool, the new fee must pay for them as well.\n"

sdaftuar commented at 4:34 pm on December 12, 2016:

This line in the help text needs to be updated for the latest semantics (no descendants in mempool or wallet).

in src/wallet/rpcwallet.cpp: in 64a17d6b95 outdated

2593+        throw runtime_error(
2594+                            "bumpfee \"txid\" ( options ) \n"
2595+                            "\nBumps the fee of a opt-in-RBF transaction.\n"
2596+                            "This command requires that the transaction with the given txid is in the wallet.\n"
2597+                            "This command will NOT add new inputs.\n"
2598+                            "Fee must be high enough to pay a new relay fee.\n"

sdaftuar commented at 4:37 pm on December 12, 2016:

I found this sentence, and the ones below (“This command will fail if fee is not high enough…”) to be confusing. Perhaps some explanation of what automatic fee calculation will occur, unless overriden/modified by the options, and that if the resulting fee isn’t high enough then the command will fail?

in src/wallet/rpcwallet.cpp: in 64a17d6b95 outdated

2613+                            "  \"oldfee\":    n,         (numeric) Fee of the replaced transaction\n"
2614+                            "  \"fee\":       n,         (numeric) Fee of the new transaction\n"
2615+                            "}\n"
2616+                            "\nExamples:\n"
2617+                            "\nBump the fee, get the new transaction\'s txid\n"
2618+                            + HelpExampleCli("bumpfee", "<txid> <output>")

sdaftuar commented at 4:38 pm on December 12, 2016:

<output> should not be here anymore.

in src/wallet/rpcwallet.cpp: in 64a17d6b95 outdated

2696+        }
2697+    }
2698+
2699+    // calculate the old fee and fee-rate
2700+    CAmount nOldFee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();
2701+    int64_t txSize = GetVirtualTransactionSize((CTransaction)wtx);

sdaftuar commented at 5:24 pm on December 12, 2016:

I think this cast isn’t correct after #8580 was merged; looks like the correct way to do this is GetVirtualTransactionSize(*wtx->tx).

in src/wallet/rpcwallet.cpp: in 64a17d6b95 outdated

2753+            throw JSONRPCError(RPC_MISC_ERROR, "Transaction has descendants in the mempool");
2754+    }
2755+
2756+    // Now modify the output to increase the fee.
2757+    // If the output is not large enough to pay the fee, fail.
2758+    CMutableTransaction tx(wtx);

sdaftuar commented at 5:35 pm on December 12, 2016:

I don’t think we should rely on the CMerkleTx function

0operator const CTransaction&() const { return *tx; }

that makes this work, as that function has a comment saying it should be removed. *wtx->tx here again I think?

in src/rpc/client.cpp: in 64a17d6b95 outdated

111@@ -112,6 +112,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
112     { "setnetworkactive", 0 },
113     { "getmempoolancestors", 1 },
114     { "getmempooldescendants", 1 },
115+    { "bumpfee", 1 },
116+    { "bumpfee", 2 },

sdaftuar commented at 6:47 pm on December 12, 2016:

I think this line should be removed now that the output is no longer specified?

sdaftuar changes_requested

sdaftuar commented at 6:57 pm on December 12, 2016: member

Looks pretty good to me overall, though there’s one bug here (after #8850) that needs to be fixed, plus some doc fixes to reflect the changes that have been made in this PR from when it was first opened.

Also, I think bumpfee.py should be added to rpc-tests.py.

mrbandrews force-pushed on Dec 13, 2016

mrbandrews commented at 5:49 pm on December 13, 2016: contributor

Pushed a new commit addressing the recent feedback and rebased due to a conflict in rpc-tests.py.

mrbandrews force-pushed on Dec 13, 2016

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

2659+        throw JSONRPCError(RPC_MISC_ERROR, "Transaction does not have a change output");
2660+
2661+    // optional parameters
2662+    int newConfirmTarget = nTxConfirmTarget;
2663+    CAmount totalFee = 0;
2664+    CAmount bumpfeeMaxTxFee = 0.01 * COIN;

ryanofsky commented at 5:51 pm on December 14, 2016:

Why use 0.01 * COIN instead of the global maxTxFee? I think it would be worth explaining in a comment. Also, if 0.01 * COIN is a significant value, maybe it would be good to declare it in validation.h alongside similar sounding constants like HIGH_MAX_TX_FEE.

MarcoFalke commented at 9:47 pm on December 14, 2016:

Regardless, we don’t want to use flops for CAmount.

(I know that in this case the compiler can do static analysis and figure out the exact result, but we should not promote bad practice; It is proved to break on 32 bit platforms when static code analysis does not catch the flop.)

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

2701+    CAmount nOldFee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();
2702+    int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
2703+    CFeeRate nOldFeeRate(nOldFee, txSize);
2704+
2705+    // use the user-defined payTxFee if possible, otherwise use smartfee / fallbackfee
2706+    CAmount nNewFee = 0;

ryanofsky commented at 6:06 pm on December 14, 2016:

I would move this declaration down closer to where the variable is first used (line 2719 nNewFee = nNewFeeRate.GetFee(maxNewTxSize);

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

2717+    // signature sizes can vary by a byte, so add 1 for each input when calculating the new fee
2718+    const int64_t maxNewTxSize = txSize + wtx.tx->vin.size();
2719+    nNewFee = nNewFeeRate.GetFee(maxNewTxSize);
2720+
2721+    // if user set totalFee, use that instead
2722+    if (totalFee > 0) {

ryanofsky commented at 6:37 pm on December 14, 2016:

Seems like if this condition is true, the work done above gets thrown away. Maybe that code could be moved into an else block so the sematics of the totalFee > 0 case are more straightforward.

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

2703+    CFeeRate nOldFeeRate(nOldFee, txSize);
2704+
2705+    // use the user-defined payTxFee if possible, otherwise use smartfee / fallbackfee
2706+    CAmount nNewFee = 0;
2707+    CFeeRate nNewFeeRate = payTxFee;
2708+    if (nNewFeeRate.GetFeePerK() == 0)

ryanofsky commented at 6:48 pm on December 14, 2016:

This block of code seems very similar to existing code in CWallet::GetMinimumFee. If you tweaked that function to return the fee rate in addition to the fee, could that function be reused here?

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

2740+        throw JSONRPCError(RPC_MISC_ERROR, strError);
2741+    }
2742+
2743+    CAmount nDelta = nNewFee - nOldFee;
2744+    if (nDelta <= 0) // it should not be possible to have a negative delta at this point (an attempt to reduce the fee)
2745+        throw JSONRPCError(RPC_INVALID_PARAMETER, "New fee must be higher than old fee");

ryanofsky commented at 7:00 pm on December 14, 2016:

What should the user do when this case happens? Should they report a bug because it should not be possible for this condition to happen? Or should they just try passing in a high(er) totalFee value? I think it’d be good to clarify in the error message.

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

2739+        strError = strprintf("New fee rate (%s) is too low to get into the mempool (min rate: %s)", FormatMoney(nNewFeeRate.GetFeePerK()), FormatMoney(minMempoolFeeRate.GetFeePerK()));
2740+        throw JSONRPCError(RPC_MISC_ERROR, strError);
2741+    }
2742+
2743+    CAmount nDelta = nNewFee - nOldFee;
2744+    if (nDelta <= 0) // it should not be possible to have a negative delta at this point (an attempt to reduce the fee)

ryanofsky commented at 7:01 pm on December 14, 2016:

Would suggest changing error code below to RPC_INTERNAL_ERROR if it’s really not possible for this condition to happen at this point.

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

2743+    CAmount nDelta = nNewFee - nOldFee;
2744+    if (nDelta <= 0) // it should not be possible to have a negative delta at this point (an attempt to reduce the fee)
2745+        throw JSONRPCError(RPC_INVALID_PARAMETER, "New fee must be higher than old fee");
2746+
2747+    // Fail if the tx has any descendants - check both the wallet and the mempool
2748+    if (pwalletMain->HasWalletSpend(hash))

ryanofsky commented at 7:13 pm on December 14, 2016:

I think it would make sense to move this check up towards the top of this function near the pwalletMain / wtx checks, instead of leaving it down here after fee estimation & option parsing. As a user I wouldn’t want to first resolve a bunch of errors about fees and options before finding out that the transaction I am trying to bump can’t be bumped in the first place.

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

2761+    if (poutput->nValue < nDelta)
2762+        throw JSONRPCError(RPC_MISC_ERROR, "Change output is too small to bump the fee");
2763+
2764+    // If the output would become dust, discard it (converting the dust to fee)
2765+    poutput->nValue = poutput->nValue - nDelta;
2766+    if (poutput->nValue < poutput->GetDustThreshold(::minRelayTxFee)) {

ryanofsky commented at 7:18 pm on December 14, 2016:

Think <= would be better than < here. Thinking in a theoretical situation where nValue was 0 and dust threshould was somehow also 0, it would be more consistent to remove the output than keep it.

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

2760+    CTxOut* poutput = &(tx.vout[nOutput]);
2761+    if (poutput->nValue < nDelta)
2762+        throw JSONRPCError(RPC_MISC_ERROR, "Change output is too small to bump the fee");
2763+
2764+    // If the output would become dust, discard it (converting the dust to fee)
2765+    poutput->nValue = poutput->nValue - nDelta;

ryanofsky commented at 7:21 pm on December 14, 2016:

Maybe use poutput->nValue -= nDelta syntax here (and nNewFee += poutput->nValue below). += and -= operators I think would make the code easier to read and less prone to typo errors.

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

2592+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
2593+        throw runtime_error(
2594+                            "bumpfee \"txid\" ( options ) \n"
2595+                            "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
2596+                            "An opt-in RBF transaction with the given txid must be in the wallet.\n"
2597+                            "The command will not add new inputs.\n"

ryanofsky commented at 7:29 pm on December 14, 2016:

Maybe say this will decrease and possibly remove the change output, but leave other outputs alone. Also could say explicitly that this will not change existing inputs.

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

2770+    }
2771+
2772+    // sign the new tx
2773+    CTransaction txNewConst(tx);
2774+    int nIn = 0;
2775+    for (auto &it : tx.vin) {

ryanofsky commented at 7:34 pm on December 14, 2016:

it isn’t really a good name for this variable, since it’s a direct reference to the input, and not an iterator. Maybe in or input would be a better name.

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

2771+
2772+    // sign the new tx
2773+    CTransaction txNewConst(tx);
2774+    int nIn = 0;
2775+    for (auto &it : tx.vin) {
2776+        std::map<uint256, CWalletTx>::const_iterator mi = pwalletMain->mapWallet.find(it.prevout.hash);

ryanofsky commented at 7:36 pm on December 14, 2016:

Maybe use auto here since you’re already using it one line up.

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

2777+        if (mi != pwalletMain->mapWallet.end() && it.prevout.n < (*mi).second.tx->vout.size()) {
2778+            const CScript& scriptPubKey = (*mi).second.tx->vout[it.prevout.n].scriptPubKey;
2779+            SignatureData sigdata;
2780+            if (!ProduceSignature(TransactionSignatureCreator(pwalletMain, &txNewConst, nIn, SIGHASH_ALL), scriptPubKey, sigdata))
2781+                throw JSONRPCError(RPC_WALLET_ERROR, "Can't sign transaction.");
2782+            tx.vin[nIn].scriptSig = sigdata.scriptSig;

ryanofsky commented at 7:38 pm on December 14, 2016:

Would suggest replacing tx.vin[nIn] here with it or replacing the opposite way above to be consistent about how the input is referred to inside the loop.

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

2787+    // commit/broadcast the tx
2788+    CReserveKey reservekey(pwalletMain);
2789+    CWalletTx wtxBumped(pwalletMain, MakeTransactionRef(std::move(tx)));
2790+    wtxBumped.mapValue["replaces_txid"] = hash.ToString();
2791+    CValidationState state;
2792+    if (!pwalletMain->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) {

ryanofsky commented at 7:45 pm on December 14, 2016:

Maybe add || !state.IsValid() to the condition here or assert(state.IsValid()) as a sanity check below.

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

2620+                            );
2621+
2622+    RPCTypeCheck(request.params, boost::assign::list_of(UniValue::VSTR));
2623+    uint256 hash;
2624+    hash.SetHex(request.params[0].get_str());
2625+    std::string strError;

ryanofsky commented at 7:48 pm on December 14, 2016:

Unclear what good having this variable does. Seems like it’s always just set in one line then thrown in the next line, making the code more verbose and causing unnecessary string copies.

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

2772+    // sign the new tx
2773+    CTransaction txNewConst(tx);
2774+    int nIn = 0;
2775+    for (auto &it : tx.vin) {
2776+        std::map<uint256, CWalletTx>::const_iterator mi = pwalletMain->mapWallet.find(it.prevout.hash);
2777+        if (mi != pwalletMain->mapWallet.end() && it.prevout.n < (*mi).second.tx->vout.size()) {

ryanofsky commented at 7:56 pm on December 14, 2016:

Can write mi->second instead of (*mi).second.

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

25+        self.nodes.append(start_node(1, self.options.tmpdir, ["-debug"]))
26+        connect_nodes_bi(self.nodes,0,1)
27+        self.is_network_split=False
28+        self.sync_all()
29+
30+    def create_fund_sign_send(self, node, outputs, feerate=0):

ryanofsky commented at 7:57 pm on December 14, 2016:

self isn’t being used here, would suggest making this a standalone function like submit_block_with_tx below.

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

51+        self.sync_all()
52+        self.nodes[1].generate(1)
53+        self.sync_all()
54+        assert_equal(self.nodes[0].getbalance(), Decimal('0.01'))
55+
56+        # create and bump an RBF transaction

ryanofsky commented at 8:11 pm on December 14, 2016:

This test is long and it’s not clear right away whether the different parts of it depend on each other. If it could be broken up in some way, I think it would be more readable and easier to work with in the future. E.g.

0self.test_simple_bumpfee_succeeds(a1)
1self.test_nonrbf_bumpfee_fails(a1)
2self.test_notmine_bumpfee_fails(a0)
3...

ryanofsky approved

ryanofsky commented at 8:13 pm on December 14, 2016: member

Looks good. Lots of nitpicky comments, nothing major.

mrbandrews force-pushed on Dec 15, 2016

mrbandrews commented at 4:55 pm on December 15, 2016: contributor

Pushed a new commit addressing ryanofsky’s feedback.

I removed the maxFee option. The rationale for this option was to have a more conservative tx fee limit for a bumpfee user, as the default maxTxFee (0.1 btc) is quite high and in particular, if bumping fee on a transaction with lots of descendants a user might end up paying a pretty high fee for the parent transaction alone (since the descendants would be removed). Since the new approach is to not allow bumpfee to be used on a tx with descendants at all, this rationale mostly goes away and I think the maxFee option is not worth it.

I moved the python test above ’nulldummy.py’ in the pull-tester list for the purpose of avoiding a conflict.

mrbandrews force-pushed on Dec 15, 2016

mrbandrews commented at 8:15 pm on December 15, 2016: contributor

I got a travis failure I think due to the default transaction version=2, which caused a failure in the python test. I corrected the test, rebased, and force-pushed.

mrbandrews force-pushed on Dec 15, 2016

in src/wallet/rpcwallet.cpp: in 244319366e outdated

2592+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
2593+        throw runtime_error(
2594+                            "bumpfee \"txid\" ( options ) \n"
2595+                            "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
2596+                            "An opt-in RBF transaction with the given txid must be in the wallet.\n"
2597+                            "The command will not add new inputs or alter existing inputs.\n"

luke-jr commented at 4:38 am on December 20, 2016:

I don’t think we should guarantee this. (sorry for reposting)

ryanofsky commented at 4:08 pm on January 4, 2017:

Thanks, I changed the wording to describe the current behavior but suggest it could be improved in the future.

luke-jr referenced this in commit 844f0e14c4 on Dec 21, 2016

luke-jr referenced this in commit 60d8e8c94d on Dec 21, 2016

luke-jr referenced this in commit 3645224caa on Dec 21, 2016

luke-jr referenced this in commit 9111809b2a on Dec 21, 2016

luke-jr referenced this in commit 8718402327 on Dec 21, 2016

ryanofsky force-pushed on Jan 4, 2017

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

10@@ -11,9 +11,12 @@
11 #include "init.h"
12 #include "validation.h"
13 #include "net.h"
14+#include "policy/policy.h"
15 #include "policy/rbf.h"
16 #include "rpc/server.h"
17+#include "script/sign.h"
18 #include "timedata.h"
19+#include "ui_interface.h"

sdaftuar commented at 5:03 pm on January 5, 2017:

I think this include is not needed.

ryanofsky commented at 3:09 pm on January 6, 2017:

Thanks, removed.

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

2618+                            "\nExamples:\n"
2619+                            "\nBump the fee, get the new transaction\'s txid\n"
2620+                            + HelpExampleCli("bumpfee", "<txid>")
2621+                            );
2622+
2623+    RPCTypeCheck(request.params, boost::assign::list_of(UniValue::VSTR));

sdaftuar commented at 5:31 pm on January 5, 2017:

nit: We’re calling RPCTypeCheck multiple times, which I don’t think should be needed. I believe we can just change this to RPCTypeCheck(request.params, boost::assign::list_of(UniValue::VSTR)(UniValue::VOBJ)) and remove the one at line 2675 below.

ryanofsky commented at 3:09 pm on January 6, 2017:

Fixed.

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

2710+    if (totalFee > 0) {
2711+        CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + minRelayTxFee.GetFee(maxNewTxSize);
2712+        if (totalFee < minTotalFee)
2713+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee, must be at least oldFee + relayFee: %s", FormatMoney(minTotalFee)));
2714+        nNewFee = totalFee;
2715+        nNewFeeRate = CFeeRate(totalFee, txSize);

sdaftuar commented at 6:10 pm on January 5, 2017:

I think this should be CFeeRate(totalFee, maxNewTxSize).

ryanofsky commented at 3:10 pm on January 6, 2017:

Added a comment. I think this was probably intentional since it makes the fee rate check below more conservative.

ryanofsky commented at 3:24 pm on January 6, 2017:

Now fixed (got this backward).

sdaftuar changes_requested

sdaftuar commented at 8:17 pm on January 5, 2017: member

A couple nits that you can take or leave, and one minor bug that should be fixed. Did some light testing (including review of the bumpfee.py test included here) and this otherwise looks good to me.

ryanofsky force-pushed on Jan 6, 2017

sdaftuar commented at 3:32 pm on January 6, 2017: member

~ACK b1a06207ed57425e0e4cccaad4588485d4483b08 (feel free to squash).~ EDIT: ACK pending resolution of #8456 (review)

I believe all of @gmaxwell’s comments in #8456 (comment) have been addressed, with the exception of being able to add new inputs to a transaction to bump the fee. I think that would be a fine improvement for the future, but I think this is still a useful feature even without it, so I don’t think that should be a merge blocker. @gmaxwell – what do you think?

ryanofsky force-pushed on Jan 6, 2017

ryanofsky commented at 3:41 pm on January 6, 2017: member

Squashed b1a06207ed57425e0e4cccaad4588485d4483b08 -> f4fa93b29a59c6d0f0341f442fd1805972b43ca1.

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

2760+        assert(mi != pwalletMain->mapWallet.end() && input.prevout.n < mi->second.tx->vout.size());
2761+        const CScript& scriptPubKey = mi->second.tx->vout[input.prevout.n].scriptPubKey;
2762+        SignatureData sigdata;
2763+        if (!ProduceSignature(TransactionSignatureCreator(pwalletMain, &txNewConst, nIn, SIGHASH_ALL), scriptPubKey, sigdata))
2764+            throw JSONRPCError(RPC_WALLET_ERROR, "Can't sign transaction.");
2765+        input.scriptSig = sigdata.scriptSig;

morcos commented at 4:54 pm on January 9, 2017:

@sdaftuar points out that this won’t correctly update scriptWitness for witness transactions Probably better to use the UpdateTransaction function

ryanofsky commented at 10:48 pm on January 10, 2017:

Added UpdateTransaction in e4a766291a90440d0ee8b8b25619cdd91329ffc2, and after a lot of struggle, eventually got a test to work which creates a segwit transaction and makes sure scriptWitness is set.

The test also uncovered another bug in the TransactionSignatureCreator call above where the SIGHASH_ALL value was incorrectly being interpretted as a CAmount. This is also fixed in e4a766291a90440d0ee8b8b25619cdd91329ffc2.

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

2595+            "An opt-in RBF transaction with the given txid must be in the wallet.\n"
2596+            "The command will pay the additional fee by decreasing (or perhaps removing) its change output.\n"
2597+            "If the change output is not big enough to cover the increased fee, the command will currently fail\n"
2598+            "instead of adding new inputs to compensate. (A future implementation could improve this.)\n"
2599+            "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
2600+            "By default, the new fee will be calculated automatically using estimatefee/fallbackfee.\n"

morcos commented at 5:57 pm on January 9, 2017:

nit: mentioning fallbackfee is too much implementation detail here

ryanofsky commented at 10:49 pm on January 10, 2017:

Removed in e4a766291a90440d0ee8b8b25619cdd91329ffc2.

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

2683+        if (options.exists("confTarget")) {
2684+            newConfirmTarget = options["confTarget"].get_int();
2685+            if (newConfirmTarget <= 0) // upper-bound will be checked by estimatefee/smartfee
2686+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid confTarget (cannot be <= 0)");
2687+        }
2688+        if (options.exists("totalFee")) {

morcos commented at 6:12 pm on January 9, 2017:

Should there be a check against providing both options

ryanofsky commented at 10:49 pm on January 10, 2017:

Added in e4a766291a90440d0ee8b8b25619cdd91329ffc2.

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

172+
173+    # submit a block with the rbf tx to clear the bump transaction out
174+    # of the mempool, then invalidate the block so the rbf transaction will be
175+    # put back in the mempool. this makes it possible to check whether the rbf
176+    # transaction outputs are spendable before the rbf tx is confirmed.
177+    block = submit_block_with_tx(rbf_node, rbftx)

morcos commented at 7:33 pm on January 9, 2017:

i’d abandon bumpid here so it doesn’t accidentally rereplace rbfid if its reaccepted from the wallet

if later abandoning conflicts/replacements frees up the original to spend again then thats a change in behavior that can result in changing the test

ryanofsky commented at 10:59 pm on January 10, 2017:

Added abandontransaction call in b0d4f9323250d81b53668362413134bc7a13d998.

morcos commented at 7:35 pm on January 9, 2017: member

ACK modulo comments above.

[wallet] Add IsAllFromMe: true if all inputs are from wallet 766e8a40b4

jtimon commented at 7:37 pm on January 10, 2017: contributor

Concept ACK, needs rebase.

ryanofsky force-pushed on Jan 10, 2017

ryanofsky commented at 10:44 pm on January 10, 2017: member

Rebased to handle named arguments.

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

212@@ -213,6 +213,10 @@ def test_unconfirmed_not_spendable(rbf_node, rbf_node_address):
213     assert bumpid not in rbf_node.getrawmempool()
214     assert rbfid in rbf_node.getrawmempool()
215 
216+    # call abandontransaction to make sure the wallet does not add the bump
217+    # transaction back into the mempool.
218+    rbf_node.abandontransaction(bumpid)

morcos commented at 2:00 pm on January 11, 2017:

This should take place after submit_block_with_tx and before invalidateblock That is the only time it is guaranteed that bumpid is not in the mempool.

ryanofsky commented at 3:16 pm on January 11, 2017:

Oops, yes, I got confused about what the test was doing with the two transactions. Fixed and updated the comment in a1046e099e4fac8b82d6e268eeb9fb97e4a9c9ef.

ryanofsky force-pushed on Jan 11, 2017

gmaxwell commented at 1:26 am on January 12, 2017: contributor

I believe this should make the new transaction non-RBF replaceable. (1) we currently won’t replace it, (2) if someone made an original payment and the receiver is squaking because it’s replaceable, a mechanism is needed to issue a non-replacable version. Since we won’t replace it, we can do this in one step. Does this make sense?

gmaxwell commented at 1:31 am on January 12, 2017: contributor

Hm. I expected this to change the walletrbf default.

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

2624+    hash.SetHex(request.params[0].get_str());
2625+
2626+    // retrieve the original tx from the wallet
2627+    assert(pwalletMain != NULL);
2628+    LOCK2(cs_main, pwalletMain->cs_wallet);
2629+    if (!pwalletMain->mapWallet.count(hash))

gmaxwell commented at 6:38 am on January 12, 2017:

I can’t see where it checks if the wallet is unlocked. Am I missing it?

morcos commented at 1:40 pm on January 12, 2017:

yikes that seems like a pretty big oversight

ryanofsky commented at 3:29 pm on January 12, 2017:

Added EnsureWalletIsUnlocked call in 59e425aa8e634a939e12594ce7c8824e9764e61c

laanwj commented at 11:09 am on January 12, 2017: member

Hm. I expected this to change the walletrbf default.

I don’t think we should change that last-minute for 0.14 (only mentioning that because this is tagged 0.14).

Edit: Concept ACK otherwise.

ryanofsky referenced this in commit 700af943fe on Jan 12, 2017

ryanofsky commented at 3:33 pm on January 12, 2017: member

Created #9527 for changing the walletrbf default.

ryanofsky commented at 7:22 pm on January 12, 2017: member

Added replaceable option in 8d3cd2844b844cad92d741d848ee0ffebdffef40, to allow users to create replacement transactions that are not replaceable.

I ran into a lot of spurious errors related to fundrawtransaction in the bumpfee tests while trying to add a simple test for this. fundrawtransaction would sometimes chose different sized inputs than the small_output_fails and dust_to_fee tests were expecting, so I rewrote these tests in 13960f870c1ba94f391012f9c9243c59951ea6ab to manually create RBF transactions with the right inputs.

I also ran into problems with fundrawtransaction using up all the right-sized inputs before these tests could use them, so I changed the order of the tests so they would run before any fundrawtransaction calls.

I also ran into problems with fundrawtransaction sometimes chosing a higher fee for the RBF transaction than the test_rebumping test expected, causing the bumpfee command in the test to fail with an “Invalid totalFee, must be at least …” error. I did not figure out what was causing this, but it seemed to somehow be related to the number of peer_node.sendtoaddress(rbf_node_address, 0.001) calls made during the test setup.

sdaftuar commented at 8:56 pm on January 12, 2017: member

re-ACK 8d3cd28

morcos commented at 9:12 pm on January 12, 2017: member

tested ACK 8d3cd28

in src/wallet/rpcwallet.cpp: in 8d3cd2844b outdated

2599+            "instead of adding new inputs to compensate. (A future implementation could improve this.)\n"
2600+            "The command will fail if the wallet or mempool contains a transaction that spends one of T's outputs.\n"
2601+            "By default, the new fee will be calculated automatically using estimatefee.\n"
2602+            "The user can specify a confirmation target for estimatefee.\n"
2603+            "Alternatively, the user can specify totalFee, or use RPC setpaytxfee to set a higher fee rate.\n"
2604+            "At a minimum, the new fee rate must be high enough to pay a new relay fee and to enter the node's mempool.\n"

instagibbs commented at 9:36 pm on January 12, 2017:

nit: maybe name the relay fee constant in case user wants to look it up.

ryanofsky commented at 5:16 pm on January 13, 2017:

Added in 7c9ad69f946f1db623d4243aa4a10fe57296cc4d

in src/wallet/rpcwallet.cpp: in 8d3cd2844b outdated

2604+            "At a minimum, the new fee rate must be high enough to pay a new relay fee and to enter the node's mempool.\n"
2605+            "\nArguments:\n"
2606+            "1. txid                  (string, required) The txid to be bumped\n"
2607+            "2. options               (object, optional)\n"
2608+            "   {\n"
2609+            "     \"confTarget\"        (numeric, optional) Confirmation target (in blocks)\n"

instagibbs commented at 9:36 pm on January 12, 2017:

nit: add n here as well as below

ryanofsky commented at 5:20 pm on January 13, 2017:

Options style here is the same as the style used in fundrawtransaction (and it seems clearer without all the n’s and extra punctuation).

in src/wallet/rpcwallet.cpp: in 8d3cd2844b outdated

2605+            "\nArguments:\n"
2606+            "1. txid                  (string, required) The txid to be bumped\n"
2607+            "2. options               (object, optional)\n"
2608+            "   {\n"
2609+            "     \"confTarget\"        (numeric, optional) Confirmation target (in blocks)\n"
2610+            "     \"totalFee\"          (numeric, optional) Total fee (NOT feerate) to pay, in satoshis\n"

instagibbs commented at 9:37 pm on January 12, 2017:

can you explain why user-chosen feerate isn’t an option, but absolute fee?

ryanofsky commented at 5:28 pm on January 13, 2017:

User chosen feerate is possible with settxfee, and could be added as another option here in the future. Absolute fee is a reasonable thing a user might want to set, probably determining it from the fee of the previous transaction.

in src/wallet/rpcwallet.cpp: in 8d3cd2844b outdated

2611+            "     \"replaceable\"       (boolean, optional, default true) Whether the new transaction should still be\n"
2612+            "                         marked bip-125 replaceable. If true, the sequence numbers in the transaction will\n"
2613+            "                         be left unchanged from the original. If false, any input sequence numbers in the\n"
2614+            "                         original transaction that were less than 0xfffffffe will be increased to 0xfffffffe\n"
2615+            "                         so the new transaction will not be explicitly bip-125 replaceable (though it may\n"
2616+            "                         still be replacable in practice if it has unconfirmed ancestors which are\n"

instagibbs commented at 9:45 pm on January 12, 2017:

nit: or full RBF nodes relay it

ryanofsky commented at 5:32 pm on January 13, 2017:

Referring to nodes that don’t follow BIP-125? Maybe a more general note about these nodes would be helpful, but it seems like an odd thing to mention only in this context.

luke-jr commented at 10:11 pm on January 13, 2017:

“(though it may still be replacable in practice)” seems sufficient.

ryanofsky commented at 10:51 pm on January 15, 2017:

Reworded, adding “for example” in 23c389ab8697bc0040cfd45a32d282906554a84a. Just saying non-replaceable transactions are replaceable sounds mysterious. Maybe I’m less knowledgeable than the target user, but at least I would find it confusing.

in src/wallet/rpcwallet.cpp: in 8d3cd2844b outdated

2629+    RPCTypeCheck(request.params, boost::assign::list_of(UniValue::VSTR)(UniValue::VOBJ));
2630+    uint256 hash;
2631+    hash.SetHex(request.params[0].get_str());
2632+
2633+    // retrieve the original tx from the wallet
2634+    assert(pwalletMain != NULL);

instagibbs commented at 9:47 pm on January 12, 2017:

nit: you already check that the wallet is available above, I believe unneeded.

ryanofsky commented at 5:17 pm on January 13, 2017:

Removed in 7c9ad69f946f1db623d4243aa4a10fe57296cc4d

in src/wallet/rpcwallet.cpp: in 8d3cd2844b outdated

2743+
2744+    // check that fee rate is higher than mempool's minimum fee
2745+    // (no point in bumping fee if we know that the new tx won't be accepted to the mempool)
2746+    // This may occur if the user set TotalFee or paytxfee too low, if fallbackfee is too low, or, perhaps,
2747+    // in a rare situation where the mempool minimum fee increased significantly since the fee estimation just a
2748+    // moment earlier. In this case, we report an error to the user, who may use totalFee to make an adjustment.

instagibbs commented at 10:25 pm on January 12, 2017:

if this is the intended user flow, we might want to inform them via the following error message by including a totalfee guess alongside the minimum required feerate with a suggestion to use that option.

ryanofsky commented at 5:17 pm on January 13, 2017:

Added in 7c9ad69f946f1db623d4243aa4a10fe57296cc4d

ryanofsky force-pushed on Jan 13, 2017

ryanofsky commented at 2:02 pm on January 13, 2017: member

Squashed 8d3cd2844b844cad92d741d848ee0ffebdffef40 -> 1d71fd0dab96430643b818ea993acea2aaa09256

in src/wallet/rpcwallet.cpp: in 1d71fd0dab outdated

2769+    }
2770+
2771+    // Mark new tx not replaceable, if requested.
2772+    if (!replaceable) {
2773+        for (auto& input : tx.vin) {
2774+            if (input.nSequence < 0xfffffffe) input.nSequence = 0xfffffffe;

instagibbs commented at 3:06 pm on January 13, 2017:

just set it

morcos commented at 4:52 pm on January 13, 2017:

I guess I don’t see how it makes a difference, but it seems to me that the whole point of this PR is to change the transaction to the least degree possible. I don’t know why someone would have chosen to set their sequence to 0xffffffff, but if they did, why should we change it for them here?

luke-jr commented at 11:26 pm on January 13, 2017:

Agreed with @morcos, leave this as-is.

ryanofsky commented at 10:22 pm on January 15, 2017:

Ok, keeping as is to change the transaction as little as possible.

instagibbs approved

instagibbs commented at 3:12 pm on January 13, 2017: member

tACK with random nits. If you were to make one change I think printing out the necessary flat fee on rejection(for whatever reason) would be a solid improvement.

in src/wallet/rpcwallet.cpp: in 8d3cd2844b outdated

2591+
2592+    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
2593+        throw runtime_error(
2594+            "bumpfee \"txid\" ( options ) \n"
2595+            "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
2596+            "An opt-in RBF transaction with the given txid must be in the wallet.\n"

morcos commented at 4:36 pm on January 13, 2017:

why does it need to be in the mempool?

instagibbs commented at 4:39 pm on January 13, 2017:

(weird, comments are in wrong order) it doesn’t you’re right.

luke-jr commented at 10:01 pm on January 13, 2017:

It says in the wallet, not the mempool…?

ryanofsky commented at 10:37 pm on January 15, 2017:

morcos was responding to a review comment “painfully obvious, but needs to be in mempool as well :)” that is gone now.

in src/wallet/wallet.cpp: in 1d71fd0dab outdated

847+    assert(wtx.mapValue.count("replaced_by_txid") == 0);
848+
849+    wtx.mapValue["replaced_by_txid"] = newHash.ToString();
850+
851+    CWalletDB walletdb(strWalletFile, "r+");
852+    walletdb.WriteTx(wtx);

TheBlueMatt commented at 6:17 pm on January 13, 2017:

Hum, should we be ignoring the return value here? (We check it in some places, ignore it in others in wallet.cpp, but it seems wrong to ignore it).

ryanofsky commented at 7:59 pm on January 13, 2017:

Will add a check.

sdaftuar commented at 8:13 pm on January 13, 2017:

I think we can just clean this up in a subsequent PR? I agree that the differing behaviors are not good, but perhaps this is a small enough issue to not hold this up…

ryanofsky commented at 8:42 pm on January 13, 2017:

Either way seems fine. Added a check and error print for now in f9c4007977a25d53cc5ad9459db51a831fb8ae89.

TheBlueMatt commented at 8:45 pm on January 13, 2017:

I only brought it up because I’d prefer new code to check it…it seems bad to not do so.

ryanofsky commented at 9:12 pm on January 13, 2017:

Agreed, check is added in f9c4007977a25d53cc5ad9459db51a831fb8ae89.

in src/wallet/wallet.cpp: in 1d71fd0dab outdated

844+    CWalletTx& wtx = (*mi).second;
845+
846+    // Ensure for now that we're not overwriting data
847+    assert(wtx.mapValue.count("replaced_by_txid") == 0);
848+
849+    wtx.mapValue["replaced_by_txid"] = newHash.ToString();

TheBlueMatt commented at 6:21 pm on January 13, 2017:

I’m super not convinced that this is sufficient. See all the places where isAbandoned is checked - we probably need to check for replacement in most, if not all, of the same places.

eg probably want to check in RelayWalletTransaction and possibly isSpent?

ryanofsky commented at 7:16 pm on January 13, 2017:

This is being set specifically to change the behavior of AvailableCoins to avoid creating new transactions which spend outputs of rbf & bumpfee transactions, for reasons described in comments in AvailableCoins.

I’m actually not sure why we don’t mark the transaction that is being replaced to be abandoned. But I don’t think it should make any practical difference right now.

RelayWalletTransaction in the normal case should already skip this transaction because it conflicts with the bumpfee transaction and won’t be in mempool.

IsSpent could perhaps be written to not consider inputs of a transaction that’s been replaced to be spent, though in practice it doesn’t make a difference because the bumpfee replacing transaction always spends the same inputs as the replaced transaction.

sdaftuar commented at 7:54 pm on January 13, 2017:

Fee bumped should not imply abandoned – it’s entirely possible the original version of the transaction will confirm.

Changing IsSpent as you suggest ought to have no visible effect (since bumpfee spends all the same inputs, currently), yet I think it would be incorrect to make your change – if we add smarter fee bumping behavior in the future, and it becomes possible to drop an input from the original tx in a replacement transaction, we would still want to treat the original input as spent until the transaction that spends it becomes conflicted (or the user explicitly abandons it).

In RelayWalletTransaction, I think it’s more debatable whether to attempt relay of the original version, but since it would only succeed if somehow the feebumped version dropped out of your mempool, I think it’s reasonable for the wallet to attempt to relay unless the user explicitly calls abandontransaction on it. We don’t care which version gets mined, and if something is subtly wrong with the bumped transaction then we might as well attempt the original. The effect on tx relay bandwidth should be negligible.

Also – I believe this is no different behavior than if we have conflicting transactions in our wallet that originate via some means other than bumpfee. For instance if someone sends funds to us and then fee bumps it themselves, we’ll wind up with both transactions in our wallet, and we’ll attempt to relay both, which I think is correct behavior.

morcos commented at 8:08 pm on January 13, 2017:

100% agree with sdaftuar. it would be incorrect to auto abandon the bumpee.

TheBlueMatt commented at 8:27 pm on January 13, 2017:

Abandoned doesn’t imply the original version of the transaction won’t confirm, it just means “pretend this isnt here, unless it somehow gets confirmed”?

But, ok, fair point regarding IsSpent - I’m not convinced thats is definitely The Right Behavior to not allow a user to spend something they removed from their transaction, but this is the more conservative option, so have no problem with it for now.

re: RelayWalletTransaction: I think this absolutely should have a replacement check…if a transaction has been replaced it shouldn’t make it back into our mempool without the other transaction also replacing it, sure, but belt-and-suspenders. Plus a user could, theoretically, restart with a higher min relay fee (or upgrade to a new version), and this could break.

TheBlueMatt commented at 9:15 pm on January 13, 2017:

I’ll withdraw my request after IRC discussion. I’ve been convinced that in some use-cases a user might prefer us to accept the old version of a transaction which has been bumped, so no need to change that.

ryanofsky commented at 9:28 pm on January 13, 2017:

Long discussion in IRC about this beginning here: https://botbot.me/freenode/bitcoin-core-dev/msg/79332875/.

I think the conclusion is that it is preferable to keep the current code which attempts to add all replaced and replacing transactions to the mempool , because there are cases when there is a problem with the replacing tx (e.g. output too close to dust limit) that would cause it to get rejected from mempool, and where it would be preferable to relay the original replaced transaction.

Also, the risk of the wallet relaying both replaced and replaced transactions to peers is very low. It will not happen unless a CWallet::ResendWalletTransactions call somehow is made before the initial CWallet::ReacceptWalletTransactions call in CWallet::postInitProcess finishes.

in src/wallet/wallet.cpp: in 1d71fd0dab outdated

849+    wtx.mapValue["replaced_by_txid"] = newHash.ToString();
850+
851+    CWalletDB walletdb(strWalletFile, "r+");
852+    walletdb.WriteTx(wtx);
853+
854+    NotifyTransactionChanged(this, originalHash, CT_UPDATED);

TheBlueMatt commented at 6:26 pm on January 13, 2017:

Do we need/want a MarkDirty here, too? I think we might not need it, but it seems strange to not have it here when we have it in all the other similar things (conflicted, abandon, etc).

ryanofsky commented at 7:58 pm on January 13, 2017:

Will look more, but I would want to find a specific case it is needed before adding MarkDirty here. I have another PR #9381 which eliminates MarkDirty from AddToWallet() and will get rid of this entire MarkReplaced() method, replacing it with an AddToWallet call.

sdaftuar commented at 8:10 pm on January 13, 2017:

I don’t think we should treat this differently from any other unconfirmed, conflicting transaction entering the wallet, so I don’t believe this is necessary (and it seems confusing to me to add calls like that in places where we think that balances shouldn’t be changing).

in src/wallet/wallet.cpp: in 1d71fd0dab outdated

2035+            // transaction B were initially confirmed, conflicting with A and
2036+            // A', we wouldn't want to the user to create a transaction D
2037+            // intending to replace A', but potentially resulting in a scenario
2038+            // where A, A', and D could all be accepted (instead of just B and
2039+            // D, or just A and A' like the user would want).
2040+            if (nDepth == 0 && pcoin->mapValue.count("replaced_by_txid"))

TheBlueMatt commented at 6:31 pm on January 13, 2017:

Obviously prefer to leave this in because belt-and-suspenders, but shouldn’t this check be redundant with the !InMempool() check, two checks above?

ryanofsky commented at 7:42 pm on January 13, 2017:

It’s not completely redundant (see https://github.com/mrbandrews/bitcoin/blob/1d71fd0dab96430643b818ea993acea2aaa09256/qa/rpc-tests/bumpfee.py#L251 for the test which triggers this condition), but this is checking for a really esoteric case. See description above for all the gory details, but in this can happen when the replaced transaction A temporarily becomes part of a block instead of the replacing transaction B, and then gets added back to the mempool when there’s a reorg.

TheBlueMatt commented at 6:31 pm on January 13, 2017: member

Only reviewed parts of the wallet.cpp stuff just yet, but figured I’d post it now.

ryanofsky referenced this in commit f9c4007977 on Jan 13, 2017

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

2679+    int newConfirmTarget = nTxConfirmTarget;
2680+    CAmount totalFee = 0;
2681+    bool replaceable = true;
2682+    if (request.params.size() > 1) {
2683+        UniValue options = request.params[1];
2684+        if (options.size() > 2)

luke-jr commented at 11:13 pm on January 13, 2017:

>3 now… but I don’t know that we need an explicit check here. Seems like asking for bugs.

ryanofsky commented at 10:21 pm on January 15, 2017:

Makes sense, removed in 23c389ab8697bc0040cfd45a32d282906554a84a.

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

2693+
2694+        if (options.exists("confTarget") && options.exists("totalFee")) {
2695+            throw JSONRPCError(RPC_INVALID_PARAMETER, "confTarget and totalFee options should not both be set. Please provide either a confirmation target for fee estimation or an explicit total fee for the transaction.");
2696+        } else if (options.exists("confTarget")) {
2697+            newConfirmTarget = options["confTarget"].get_int();
2698+            if (newConfirmTarget <= 0) // upper-bound will be checked by estimatefee/smartfee

luke-jr commented at 11:14 pm on January 13, 2017:

Should be 1 now (as of #9239). Maybe make a const int in some header for this number…

morcos commented at 0:06 am on January 14, 2017:

nothing bad happens if you call it with 1, i think it makes most sense to allow users to select 1 meaning as fast as possible, and then we just return the fastest estimate we’re comfortable with

luke-jr commented at 0:14 am on January 14, 2017:

Okay

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

2696+        } else if (options.exists("confTarget")) {
2697+            newConfirmTarget = options["confTarget"].get_int();
2698+            if (newConfirmTarget <= 0) // upper-bound will be checked by estimatefee/smartfee
2699+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid confTarget (cannot be <= 0)");
2700+        } else if (options.exists("totalFee")) {
2701+            totalFee = options["totalFee"].get_int();

luke-jr commented at 11:15 pm on January 13, 2017:

get_int64

ryanofsky commented at 10:54 pm on January 15, 2017:

Thanks, fixed in 23c389ab8697bc0040cfd45a32d282906554a84a.

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

2704+            else if (totalFee > maxTxFee)
2705+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid totalFee (cannot be higher than maxTxFee)");
2706+        }
2707+
2708+        if (options.exists("replaceable") && !options["replaceable"].get_bool()) {
2709+            replaceable = false;

luke-jr commented at 11:17 pm on January 13, 2017:

Prefer to just do:

0if (options.exists("replaceable")) {
1    replaceable = options["replaceable"].get_bool();
2}

So that if the default changes, this still works right.

ryanofsky commented at 10:55 pm on January 15, 2017:

Good point, fixed in 23c389ab8697bc0040cfd45a32d282906554a84a.

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

2762+    // If the output would become dust, discard it (converting the dust to fee)
2763+    poutput->nValue -= nDelta;
2764+    if (poutput->nValue <= poutput->GetDustThreshold(::minRelayTxFee)) {
2765+        LogPrint("rpc", "Bumping fee and discarding dust output\n");
2766+        nNewFee += poutput->nValue;
2767+        tx.vout.erase(tx.vout.begin() + nOutput);

luke-jr commented at 11:25 pm on January 13, 2017:

So long as the user is explicitly setting a total fee, we should either fail when discarding dust, or at least document this behaviour in the RPC help.

ryanofsky commented at 11:04 pm on January 15, 2017:

Added note to totalFee documentation in 4e1a04002b0783d53c27eaed8df4ddb2bded6bf7.

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

2801+    if (!pwalletMain->MarkReplaced(wtx.GetHash(), wtxBumped.GetHash())) {
2802+        // TODO: see if JSON-RPC has a standard way of returning a response
2803+        // along with an exception. It would be good to return information about
2804+        // wtxBumped to the caller even if marking the original transaction
2805+        // replaced does not succeed for some reason.
2806+        throw JSONRPCError(RPC_WALLET_ERROR, "Unable to mark the original transaction as replaced.");

luke-jr commented at 11:38 pm on January 13, 2017:

Maybe just add "errors" to the response below?

ryanofsky commented at 10:35 pm on January 15, 2017:

Clarified the error message in 23c389ab8697bc0040cfd45a32d282906554a84a. If there are other examples of returning an “errors” key in a RPC response, I could change this to conform. But my preference would be to keep the exception (since this would be a sign of a serious problem that should not be ignored) and to just return more complete status information with the exception.

luke-jr commented at 6:04 pm on January 18, 2017:

I would worry that a caller might ignore the error and try again, but I guess the harm risk of that is minimal with bumpfee.

luke-jr commented at 11:40 pm on January 13, 2017: member

Almost there… (if you could use braces more, that’d be nice, but not critical)

ryanofsky commented at 11:22 pm on January 15, 2017: member

Implemented suggestions from @luke-jr in 23c389ab8697bc0040cfd45a32d282906554a84a, adding braces to all if statements, and making most of the other requested changes.

TheBlueMatt commented at 0:39 am on January 16, 2017: member

I know other ways of calling getbalance are pretty broken (#8183), but I think we need to fix a bit of how bumpfee interacts with getbalance “*”. In testing I got getbalance “*” to give me a negative number, which seems super shitty.

Additionally, we need documentation on how bumpfee interacts with listunspent. While I dont mind the change outputs from bumpfee not appearing in the results, this is likely to be surprising to users (I believe this only otherwise occurs if the transaction was abandoned), so needs documented.

in src/wallet/rpcwallet.cpp: in 4e1a04002b outdated

2608@@ -2609,7 +2609,10 @@ UniValue bumpfee(const JSONRPCRequest& request)
2609             "2. options               (object, optional)\n"
2610             "   {\n"
2611             "     \"confTarget\"        (numeric, optional) Confirmation target (in blocks)\n"
2612-            "     \"totalFee\"          (numeric, optional) Total fee (NOT feerate) to pay, in satoshis\n"
2613+            "     \"totalFee\"          (numeric, optional) Total fee (NOT feerate) to pay, in satoshis.\n"
2614+            "                         In rare cases, the actual fee paid might be slightly higher than the specified\n"

morcos commented at 2:10 am on January 16, 2017:

I’d vote against making this statement. I think it should be a more general principle of our wallet that you might always lose unexpectedly something on the order of the dust threshold, rather than explicitly mention it every time we think it could happen. But if I’m outvoted, so be it..

morcos commented at 2:30 am on January 16, 2017: member

@TheBlueMatt I don’t see how this PR has any affect on any of the getbalance calls. I think what you are seeing is pre-existing misbehavior for multiple spends of the same outputs being present in the wallet.

I went through all the calls to AvailableCoins (which is the only thing this PR changes) and I think the only open decision is deciding what we want the interaction with listunspent to be and documenting it.

in src/wallet/rpcwallet.cpp: in 4e1a04002b outdated

2742+        nNewFee = totalFee;
2743+        nNewFeeRate = CFeeRate(totalFee, maxNewTxSize);
2744+    } else {
2745+        // use the user-defined payTxFee if possible, otherwise use smartfee / fallbackfee
2746+        nNewFeeRate = payTxFee;
2747+        if (nNewFeeRate.GetFeePerK() == 0) {

TheBlueMatt commented at 1:33 am on January 17, 2017:

I think if you have specified a manual confTarget you probably want to always take this branch.

ryanofsky commented at 4:27 pm on January 17, 2017:

Done in a29b3f557fe8b7bb8e1b28933f3d5a4a3a36d362.

in src/wallet/wallet.cpp: in 4e1a04002b outdated

2030+            // one-block reorg, transactions B' and C might BOTH be accepted,
2031+            // when the user only wanted one of them. Specifically, there could
2032+            // be a 1-block reorg away from the chain where transactions A and C
2033+            // were accepted to another chain where B, B', and C were all
2034+            // accepted.
2035+            if (nDepth == 0 && pcoin->mapValue.count("replaces_txid")) {

TheBlueMatt commented at 2:08 am on January 17, 2017:

I believe this and the check further down should have a fOnlyConfirmed && at the beginning to avoid not showing the tx currently in mempool’s change output in listunspent, which I think would be the correct behavior.

TheBlueMatt commented at 2:10 am on January 17, 2017:

Or maybe only the one below, but in either case should be well-documented.

ryanofsky commented at 3:51 pm on January 17, 2017:

Added in 0f83aa3a1e1b2668a6231a1660ed864e2da3a341.

ryanofsky referenced this in commit 32c01603a9 on Jan 17, 2017

in src/wallet/rpcwallet.cpp: in 759a7cd0b6 outdated

2426     UniValue results(UniValue::VARR);
2427     vector<COutput> vecOutputs;
2428     assert(pwalletMain != NULL);
2429     LOCK2(cs_main, pwalletMain->cs_wallet);
2430-    pwalletMain->AvailableCoins(vecOutputs, false, NULL, true);
2431+    pwalletMain->AvailableCoins(vecOutputs, !include_untrusted, NULL, true);

instagibbs commented at 4:01 pm on January 17, 2017:

note: in the future we should rename and invert the AvailableCoins argument to this.

ryanofsky force-pushed on Jan 17, 2017

ryanofsky referenced this in commit 2fd0475e98 on Jan 17, 2017

ryanofsky referenced this in commit ceeecad175 on Jan 17, 2017

ryanofsky force-pushed on Jan 17, 2017

ryanofsky referenced this in commit 0f83aa3a1e on Jan 17, 2017

ryanofsky referenced this in commit a29b3f557f on Jan 17, 2017

ryanofsky force-pushed on Jan 17, 2017

ryanofsky commented at 5:20 pm on January 17, 2017: member

Squashed 28fd457a73fc6291f72d89d9dd3532ada9ebe35c -> ce3a363049bf8fdd8be926ca3ab8c803c98de6d2.

TheBlueMatt commented at 6:08 pm on January 17, 2017: member

utACK ce3a363049bf8fdd8be926ca3ab8c803c98de6d2

morcos commented at 6:09 pm on January 17, 2017: member

tested previous iteration ACK ce3a363

in src/rpc/server.cpp: in ce3a363049 outdated

89         }
90         i++;
91     }
92 }
93 
94+void RPCTypeCheckArgument(const UniValue& value, UniValue::VType typeExpected)

luke-jr commented at 6:06 pm on January 18, 2017:

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

TheBlueMatt commented at 6:09 pm on January 18, 2017:

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.

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

2365@@ -2364,6 +2366,10 @@ UniValue listunspent(const JSONRPCRequest& request)
2366             "      \"address\"   (string) bitcoin address\n"
2367             "      ,...\n"
2368             "    ]\n"
2369+            "4. include_unsafe (bool, optional, default=true) Include outputs that are not safe to spend\n"
2370+            "                  because they come from unconfirmed untrusted transactions or unconfirmed\n"
2371+            "                  replacement transactions (cases where we can't be sure a conflicting\n"
2372+            "                  transaction won't be mined).\n"

luke-jr commented at 6:07 pm on January 18, 2017:

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

ryanofsky commented at 8:32 pm on January 18, 2017:

Changed wording in 4beb7c92c70b7668042383d37c49545198aa00cd.

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

2351@@ -2350,9 +2352,9 @@ UniValue listunspent(const JSONRPCRequest& request)
2352     if (!EnsureWalletIsAvailable(request.fHelp))
2353         return NullUniValue;
2354 
2355-    if (request.fHelp || request.params.size() > 3)
2356+    if (request.fHelp || request.params.size() > 4)
2357         throw runtime_error(
2358-            "listunspent ( minconf maxconf  [\"addresses\",...] )\n"
2359+            "listunspent ( minconf maxconf  [\"addresses\",...] [include_unsafe] )\n"

luke-jr commented at 6:26 pm on January 18, 2017:

Prefer to add an options Object for this.

ryanofsky commented at 8:34 pm on January 18, 2017:

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.

sdaftuar commented at 7:59 pm on January 18, 2017: member

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

luke-jr approved

luke-jr commented at 8:12 pm on January 18, 2017: member

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

ryanofsky referenced this in commit 4beb7c92c7 on Jan 18, 2017

[wallet] Add include_unsafe argument to listunspent RPC 52dde66770

[RPC] bumpfee

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

cc0243ad32

ryanofsky force-pushed on Jan 19, 2017

ryanofsky commented at 4:31 pm on January 19, 2017: member

Squashed 4beb7c92c70b7668042383d37c49545198aa00cd -> cc0243ad32cee1cc9faab317364b889beaf07647

morcos commented at 6:13 pm on January 19, 2017: member

Still ACK cc0243a

in src/wallet/rpcwallet.cpp: in cc0243ad32

2676+    if (!SignalsOptInRBF(wtx)) {
2677+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction is not BIP 125 replaceable");
2678+    }
2679+
2680+    if (wtx.mapValue.count("replaced_by_txid")) {
2681+        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")));

instagibbs commented at 6:34 pm on January 19, 2017:

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

instagibbs approved

instagibbs commented at 6:36 pm on January 19, 2017: member

tACK cc0243ad32cee1cc9faab317364b889beaf07647

no need to change nit pre-merge, very minor

laanwj merged this on Jan 19, 2017

laanwj closed this on Jan 19, 2017

laanwj referenced this in commit 2ef52d3cf1 on Jan 19, 2017

in src/wallet/rpcwallet.cpp: in cc0243ad32

2671+
2672+    if (wtx.GetDepthInMainChain() != 0) {
2673+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction has been mined, or is conflicted with a mined transaction");
2674+    }
2675+
2676+    if (!SignalsOptInRBF(wtx)) {

jameshilliard commented at 2:10 am on January 20, 2017:

Should probably have an option to override this.

ryanofsky commented at 5:14 am on January 20, 2017:

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?

jameshilliard commented at 5:26 am on January 20, 2017:

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

ryanofsky commented at 6:51 pm on January 24, 2017:

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

ryanofsky referenced this in commit 02b059dd6a on May 23, 2017

ryanofsky referenced this in commit ea282da6da on Jun 2, 2017

meshcollider referenced this in commit 60091d20f9 on May 5, 2020

sidhujag referenced this in commit 04bea9fcac on May 12, 2020

DrahtBot locked this on Sep 8, 2021

[RPC] Simplified bumpfee command. #8456