Wallet incremental fee #9615

morcos · 2017-01-23T18:45:56Z

This is another couple of minor tweaks to bumpfee that I'd hope to merge for 0.14.
Built on top of #9589

Introduce WALLET_INCREMENTAL_RELAY_FEE
Have wallet's default bump value be higher than the default incrementalRelayFee to future proof against changes to incremental relay fee. Only applies when not setting the fee rate directly.
If we ever decide that we'd like to raise the default incrementalRelayFee or nodes on the network start doing it, it would be a shame if old software was generating replacements that didn't relay. 5000 satoshis/KB is a compromise between being sufficiently future proof and not losing too much precision in our ability to bump fees.

Incidentally I do think the incrementalRelayFee is too low and that the "cost" to the network of relaying a transaction around is above 1000 satoshi/KB and there is insufficient benefit on having that much precision to bumpfee and mempool limiting. I don't feel like getting in a big argument about changing the default though. But my recommendation would be to change the default to 5000 satoshis.

Change bumpfee result value from 'oldfee' to 'origfee'.
The result value indicates the actual fee on the transaction that was replaced. But there is an error message which uses the description 'oldfee' to refer to the original fee rate applied to the new transaction's estimated max size. It was confusing that two different uses of 'oldfee' had two different numeric values.

ryanofsky

utACK ec692cca0b5225d37c81b1d4fb7f450b20d10ce0 with two minor suggestions.

Also there an extra line at the end of the "Change bumpfee result value from 'oldfee' to 'origfee." (ec692cca0b5225d37c81b1d4fb7f450b20d10ce0) commit message which I think doesn't belong.

ryanofsky · 2017-01-23T22:19:55Z

src/wallet/rpcwallet.cpp

@@ -2823,8 +2827,17 @@ UniValue bumpfee(const JSONRPCRequest& request)
        // new fee rate must be at least old rate + minimum incremental relay rate
        if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK()) {
            nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK());
+            if (specifiedConfirmTarget || payTxFee.GetFeePerK() == 0) {
+                // In the default case where the user is depending on the wallet
+                // to smartly pick the fee, the wallet should have its own


Maybe s/should have its own minimum incremental fee/uses a conservative WALLET_INCREMENTAL_RELAY_FEE value/. Saying "should" could suggest that the wallet doesn't already do this.

ryanofsky · 2017-01-23T22:47:48Z

src/wallet/rpcwallet.cpp


    if (totalFee > 0) {
        CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + ::incrementalRelayFee.GetFee(maxNewTxSize);
        if (totalFee < minTotalFee) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee, must be at least %s (oldFee %s + relayFee %s)", FormatMoney(minTotalFee), nOldFeeRate.GetFee(maxNewTxSize), ::incrementalRelayFee.GetFee(maxNewTxSize)));
+            CAmount recommendedTotalFee =  nOldFeeRate.GetFee(maxNewTxSize) + walletIncrementalRelayFee.GetFee(maxNewTxSize);
+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee, must be at least %s (oldFee %s + incrementalFee %s), recommended %s (incrementalFee %s)",


Another another way to avoid the ambiguity of having different "oldfee" values might be to phrase error message to in terms of fee rates, rather than fees:

nNewFee = totalFee; nNewFeeRate = CFeeRate(totalFee, maxNewTxSize); CFeeRate minFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK()); if (nNewFeeRate < minFeeRate) { CAmount recommendedTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + walletIncrementalRelayFee.GetFee(maxNewTxSize); throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Insufficient totalFee rate %s %s/kB, must be at least %s %s/kB (old rate %s %s/kB + incremental rate %s %s/kB), recommended %s %s (incremental fee %s %s)", FormatMoney(nNewFeeRate.GetFeePerK()), CURRENCY_UNIT, FormatMoney(minFeeRate.GetFeePerK()), CURRENCY_UNIT, FormatMoney(nOldFeeRate.GetFeePerK()), CURRENCY_UNIT, FormatMoney(::incrementalRelayFee.GetFeePerK()), CURRENCY_UNIT, FormatMoney(recommendedTotalFee), CURRENCY_UNIT, walletIncrementalRelayFee.GetFee(maxNewTxSize))); }

I think this would make more sense anyway, because whatever client code is calling this will probably be deriving totalFee values based on fee rates, and this error message is more explicit about the minimum fee rate required.

(Also think saying insufficient fee is better than saying invalid fee, to be more specific).

hmm... I feel like if the option is total fee (not rate) then we should give the error message in fee (not rate). I don't feel strongly about that.., but its not really relevant to the gist of this PR anyway, so I'll leave it alone for now. BTW, CFeeRate has a ToString().

I will make the change to Insufficient totalFee though..

jonasschnelli · 2017-01-24T08:07:02Z

Concept ACK.

morcos · 2017-01-24T18:03:04Z

Please tag 0.14

sdaftuar · 2017-01-25T18:47:44Z

src/wallet/rpcwallet.cpp

@@ -2823,8 +2827,17 @@ UniValue bumpfee(const JSONRPCRequest& request)
        // new fee rate must be at least old rate + minimum incremental relay rate
        if (nNewFeeRate.GetFeePerK() < nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK()) {
            nNewFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK());
+            if (specifiedConfirmTarget || payTxFee.GetFeePerK() == 0) {


~~I think we want this logic to apply even if the user didn't specify a confirmation target -- just as long as paytxfee is 0.~~ EDIT: I misread the code above, this logic does correctly do what is intended here, though I still think this is complication is unnecessary.

Actually, on further thought, we might as well have this apply even if payTxFee is set... There's no philosophical difference in my mind between being willing to exceed it by incrementalRelayFee vs. walletIncrementalRelayFee.

Ok took this advice

morcos · 2017-01-26T03:46:07Z

Rebased on new #9589 and simplified the logic per @sdaftuar's comment

Use the wallet's fee calculation logic to properly clamp fee against minimums and maximums when calculating the fee for a bumpfee transaction. Unless totalFee is explictly given, in which case, manually check against min, but do nothing to adjust given fee. In all cases do a final check against maxTxFee (after adding any incremental amount).

morcos · 2017-01-26T15:46:23Z

Still just the last 2 commits are part of this PR, updated because of #9589 again and added check that walletIncrementalRelayFee is always at least as big as global policy incrementalRelayFee

sdaftuar · 2017-01-26T16:22:28Z

utACK de798cb

TheBlueMatt · 2017-01-26T16:56:25Z

src/wallet/rpcwallet.cpp


    if (totalFee > 0) {
        CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + ::incrementalRelayFee.GetFee(maxNewTxSize);
        if (totalFee < minTotalFee) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee, must be at least %s (oldFee %s + relayFee %s)", FormatMoney(minTotalFee), nOldFeeRate.GetFee(maxNewTxSize), ::incrementalRelayFee.GetFee(maxNewTxSize)));
+            CAmount recommendedTotalFee =  nOldFeeRate.GetFee(maxNewTxSize) + walletIncrementalRelayFee.GetFee(maxNewTxSize);
+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s), recommended %s (incrementalFee %s)",


I believe this is undefined behavior (though I'm no expert on tinyformat) - you're printing CAmounts with %s. I also find it super strange to print both Bitcoin and Satoshis side-by-side with no unit information.

Have wallet's default bump value be higher than the default incrementalRelayFee to future proof against changes to incremental relay fee. Only applies when not setting the fee rate directly.

The result value indicates the actual fee on the transaction that was replaced. But there is an error message which uses the description 'oldfee' to refer to the original fee rate applied to the new transaction's estimated max size. It was confusing that two different uses of 'oldfee' had two different numeric values.

morcos · 2017-01-26T17:07:54Z

ok fixed and squashed 51c67f4 -> 4b189c1

(removed the unnecessary recommended amount from the error message as well)

TheBlueMatt · 2017-01-26T17:12:00Z

utACK 4b189c1

sdaftuar · 2017-01-26T21:31:37Z

utACK 4b189c1

4b189c1 Change bumpfee result value from 'oldfee' to 'origfee'. (Alex Morcos) 0c0c63f Introduce WALLET_INCREMENTAL_RELAY_FEE (Alex Morcos) e8021ec Use CWallet::GetMinimumFee in bumpfee (Alex Morcos) ae9719a Refactor GetMinimumFee to give option of providing targetFee (Alex Morcos) fe8e8ef [rpc] Add incremental relay fee to getnetworkinfo (Alex Morcos) 6b331e6 Fix to have miner test aware of new separate block min tx fee (Alex Morcos) de6400d Fix missing use of dustRelayFee (Alex Morcos) 5b15870 Use incrementalRelayFee for BIP 125 replacement (Alex Morcos)

morcos added 4 commits January 19, 2017 20:31

Use incrementalRelayFee for BIP 125 replacement

5b15870

Fix missing use of dustRelayFee

de6400d

Fix to have miner test aware of new separate block min tx fee

6b331e6

[rpc] Add incremental relay fee to getnetworkinfo

fe8e8ef

ryanofsky approved these changes Jan 23, 2017

View reviewed changes

morcos force-pushed the walletincremental branch from ec692cc to e0661d0 Compare January 24, 2017 01:30

jonasschnelli added Mempool TX fees and policy labels Jan 24, 2017

sdaftuar reviewed Jan 25, 2017

View reviewed changes

Refactor GetMinimumFee to give option of providing targetFee

ae9719a

morcos force-pushed the walletincremental branch from e0661d0 to 1b06748 Compare January 26, 2017 03:45

morcos force-pushed the walletincremental branch from 1b06748 to de798cb Compare January 26, 2017 15:44

TheBlueMatt reviewed Jan 26, 2017

View reviewed changes

morcos added 2 commits January 26, 2017 12:03

Introduce WALLET_INCREMENTAL_RELAY_FEE

0c0c63f

Have wallet's default bump value be higher than the default incrementalRelayFee to future proof against changes to incremental relay fee. Only applies when not setting the fee rate directly.

morcos force-pushed the walletincremental branch from 51c67f4 to 4b189c1 Compare January 26, 2017 17:07

laanwj added this to the 0.14.0 milestone Jan 26, 2017

laanwj merged commit 4b189c1 into bitcoin:master Jan 30, 2017

laanwj mentioned this pull request Jan 30, 2017

Bumpfee: bugfixes for error handling and feerate calculation #9640

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wallet incremental fee #9615

Wallet incremental fee #9615

morcos commented Jan 23, 2017

ryanofsky left a comment

ryanofsky Jan 23, 2017

morcos Jan 24, 2017

ryanofsky Jan 23, 2017

morcos Jan 24, 2017

jonasschnelli commented Jan 24, 2017

morcos commented Jan 24, 2017

sdaftuar Jan 25, 2017 •

edited

Loading

sdaftuar Jan 25, 2017

morcos Jan 26, 2017

morcos commented Jan 26, 2017

morcos commented Jan 26, 2017

sdaftuar commented Jan 26, 2017

TheBlueMatt Jan 26, 2017

morcos commented Jan 26, 2017 •

edited

Loading

TheBlueMatt commented Jan 26, 2017

sdaftuar commented Jan 26, 2017

Wallet incremental fee #9615

Wallet incremental fee #9615

Conversation

morcos commented Jan 23, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Jan 23, 2017

Choose a reason for hiding this comment

morcos Jan 24, 2017

Choose a reason for hiding this comment

ryanofsky Jan 23, 2017

Choose a reason for hiding this comment

morcos Jan 24, 2017

Choose a reason for hiding this comment

jonasschnelli commented Jan 24, 2017

morcos commented Jan 24, 2017

sdaftuar Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

sdaftuar Jan 25, 2017

Choose a reason for hiding this comment

morcos Jan 26, 2017

Choose a reason for hiding this comment

morcos commented Jan 26, 2017

morcos commented Jan 26, 2017

sdaftuar commented Jan 26, 2017

TheBlueMatt Jan 26, 2017

Choose a reason for hiding this comment

morcos commented Jan 26, 2017 • edited Loading

TheBlueMatt commented Jan 26, 2017

sdaftuar commented Jan 26, 2017

sdaftuar Jan 25, 2017 •

edited

Loading

morcos commented Jan 26, 2017 •

edited

Loading