Wallet incremental fee #9615

1. 
   morcos commented at 6:45 pm on January 23, 2017: member

   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.

2. Use incrementalRelayFee for BIP 125 replacement 5b158707f2
3. Fix missing use of dustRelayFee de6400de5d
4. Fix to have miner test aware of new separate block min tx fee 6b331e6cf9
5. [rpc] Add incremental relay fee to getnetworkinfo fe8e8efcf9
6. in src/wallet/rpcwallet.cpp: in 86e8cca1d2 outdated

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

   ---

   ---

   

   ryanofsky commented at 10:19 pm on January 23, 2017:

   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.

   ---

   

   morcos commented at 1:17 am on January 24, 2017:

   will do
7. in src/wallet/rpcwallet.cpp: in 86e8cca1d2 outdated

   2806     if (totalFee > 0) {
   2807         CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + ::incrementalRelayFee.GetFee(maxNewTxSize);
   2808         if (totalFee < minTotalFee) {
   2809-            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)));
   2810+            CAmount recommendedTotalFee =  nOldFeeRate.GetFee(maxNewTxSize) + walletIncrementalRelayFee.GetFee(maxNewTxSize);
   2811+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee, must be at least %s (oldFee %s + incrementalFee %s), recommended %s (incrementalFee %s)",
   

   ---

   ---

   

   ryanofsky commented at 10:47 pm on January 23, 2017:

   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:

    0nNewFee = totalFee;
    1nNewFeeRate = CFeeRate(totalFee, maxNewTxSize);
    2CFeeRate minFeeRate = CFeeRate(nOldFeeRate.GetFeePerK() + ::incrementalRelayFee.GetFeePerK());
    3if (nNewFeeRate < minFeeRate) {
    4    CAmount recommendedTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + walletIncrementalRelayFee.GetFee(maxNewTxSize);
    5    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)",
    6                                                        FormatMoney(nNewFeeRate.GetFeePerK()), CURRENCY_UNIT,
    7                                                        FormatMoney(minFeeRate.GetFeePerK()), CURRENCY_UNIT,
    8                                                        FormatMoney(nOldFeeRate.GetFeePerK()), CURRENCY_UNIT,
    9                                                        FormatMoney(::incrementalRelayFee.GetFeePerK()), CURRENCY_UNIT,
   10                                                        FormatMoney(recommendedTotalFee), CURRENCY_UNIT,
   11                                                        walletIncrementalRelayFee.GetFee(maxNewTxSize)));
   12}
   

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

   ---

   

   morcos commented at 1:17 am on January 24, 2017:

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

8. ryanofsky approved
9. 
   ryanofsky commented at 10:55 pm on January 23, 2017: member

   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.

10. morcos force-pushed on Jan 24, 2017
11. jonasschnelli added the label Mempool on Jan 24, 2017
12. jonasschnelli added the label TX fees and policy on Jan 24, 2017
13. 
    jonasschnelli commented at 8:07 am on January 24, 2017: contributor

    Concept ACK.
14. 
    morcos commented at 6:03 pm on January 24, 2017: member

    Please tag 0.14
15. Refactor GetMinimumFee to give option of providing targetFee ae9719ab87
16. in src/wallet/rpcwallet.cpp: in ad2e6fb2dc outdated

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

    ---

    ---

    

    sdaftuar commented at 6:47 pm on January 25, 2017:

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

    ---

    

    sdaftuar commented at 7:39 pm on January 25, 2017:

    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.

    ---

    

    morcos commented at 3:45 am on January 26, 2017:

    Ok took this advice
17. morcos force-pushed on Jan 26, 2017
18. 
    morcos commented at 3:46 am on January 26, 2017: member

    Rebased on new #9589 and simplified the logic per @sdaftuar’s comment
19. Use CWallet::GetMinimumFee in bumpfee

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

    e8021ec919
20. morcos force-pushed on Jan 26, 2017
21. 
    morcos commented at 3:46 pm on January 26, 2017: member

    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
22. 
    sdaftuar commented at 4:22 pm on January 26, 2017: member

    utACK de798cb
23. 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.

    0c0c63f70a
24. 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.

    4b189c1340
25. in src/wallet/rpcwallet.cpp: in 633ed03b97 outdated

    2797     if (totalFee > 0) {
    2798         CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + ::incrementalRelayFee.GetFee(maxNewTxSize);
    2799         if (totalFee < minTotalFee) {
    2800-            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)));
    2801+            CAmount recommendedTotalFee =  nOldFeeRate.GetFee(maxNewTxSize) + walletIncrementalRelayFee.GetFee(maxNewTxSize);
    2802+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s), recommended %s (incrementalFee %s)",
    

    ---

    ---

    

    TheBlueMatt commented at 4:56 pm on January 26, 2017:

    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.
26. morcos force-pushed on Jan 26, 2017
27. 
    morcos commented at 5:07 pm on January 26, 2017: member

    ok fixed and squashed https://github.com/bitcoin/bitcoin/commit/51c67f4 -> 4b189c1

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

28. 
    TheBlueMatt commented at 5:12 pm on January 26, 2017: member

    utACK 4b189c13401bcd350c05cf8194beaeb3d18b3ebc
29. laanwj added this to the milestone 0.14.0 on Jan 26, 2017
30. 
    sdaftuar commented at 9:31 pm on January 26, 2017: member

    utACK 4b189c1
31. laanwj merged this on Jan 30, 2017
32. laanwj closed this on Jan 30, 2017

    ---

33. laanwj referenced this in commit d2c9e4d422 on Jan 30, 2017
34. codablock referenced this in commit 4716f36c98 on Jan 19, 2018
35. codablock referenced this in commit daa8582b18 on Jan 23, 2018
36. andvgal referenced this in commit 7888029a23 on Jan 6, 2019
37. CryptoCentric referenced this in commit dcc422a866 on Feb 27, 2019
38. DrahtBot locked this on Sep 8, 2021

Contributors
morcos ryanofsky jonasschnelli sdaftuar TheBlueMatt

Labels
TX fees and policy Mempool

Milestone
0.14.0