Add sane fallback for fee estimation #7296

1. 
   morcos commented at 5:06 pm on January 5, 2016: member

   Make fee estimation logic respect GetRequiredFee at all times. In addition add new commandline option “-fallbackfee” to use when fee estimation does not have sufficient data.
2. 
   MarcoFalke commented at 5:15 pm on January 5, 2016: member

   utACK cc99c41
3. Always respect GetRequiredFee for wallet txs 995b9f385b
4. Add sane fallback for fee estimation

   Add new commandline option "-fallbackfee" to use when fee estimation does not have sufficient data.

   e420a1b15e
5. in src/wallet/wallet.cpp: in cc99c41ec0 outdated

   46@@ -47,6 +47,12 @@ bool fSendFreeTransactions = DEFAULT_SEND_FREE_TRANSACTIONS;
   47  * Override with -mintxfee
   48  */
   49 CFeeRate CWallet::minTxFee = CFeeRate(DEFAULT_TRANSACTION_MINFEE);
   50+/**
   51+ * If fee estimation does not have enough data to provide estimates, the use this fee instead.
   

   ---

   ---

   

   MarcoFalke commented at 5:15 pm on January 5, 2016:

   Nit: , then use …
6. in src/wallet/wallet.cpp: in cc99c41ec0 outdated

   2240+        // ... unless we don't have enough mempool data for estimatefee, then use fallbackFee
   2241+        if (nFeeNeeded == 0)
   2242+            nFeeNeeded = fallbackFee.GetFee(nTxBytes);
   2243+    }
   2244+    // prevent user from paying a fee below minRelayTxFee or minTxFee
   2245+    nFeeNeeded = std::max(nFeeNeeded, GetRequiredFee(nTxBytes));
   

   ---

   ---

   

   MarcoFalke commented at 5:16 pm on January 5, 2016:

   This is important on it’s own. Could you split this diff (line 2236-2237) into a separate commit?

   Edit: See 995b9f3

7. in src/wallet/wallet.h: in cc99c41ec0 outdated

   40@@ -41,6 +41,8 @@ static const unsigned int DEFAULT_KEYPOOL_SIZE = 100;
   41 static const CAmount DEFAULT_TRANSACTION_FEE = 0;
   42 //! -paytxfee will warn if called with a higher fee than this amount (in satoshis) per KB
   43 static const CAmount nHighTransactionFeeWarning = 0.01 * COIN;
   44+//! -fallbackfee default
   45+static const CAmount DEFAULT_FALLBACK_FEE = 20000;
   

   ---

   ---

   

   MarcoFalke commented at 5:31 pm on January 5, 2016:

   I’d say 11000 is enough, we can determine a “better” DEFAULT_ for 0.13
8. morcos force-pushed on Jan 5, 2016
9. 
   morcos commented at 6:24 pm on January 5, 2016: member

   Addressed @MarcoFalke’s comments. Although left fee = 20k satoshis until the bikeshedding is finished. I don’t care want goes there, but I propose using the result of estimatefee(4) with a significantly longer time decay over the last few months. I’ll post the result of that once calculated.

   This is meant for 0.12 backport

10. 
    morcos commented at 9:10 pm on January 5, 2016: member

    @MarcoFalke ugh.. thanks. more than just that one fail. These are probably things that should be fixed b/c the travis tests that fail are implicitly depending on estimatefee returning minrelaytxfee, which isn’t a good assumption. I’m working on it
11. SQUASHME: Fix rpc tests that assumed fallback to minRelayTxFee bebe58b748
12. 
    wumpus commented at 4:38 pm on January 6, 2016: none

    Reminder: @wumpus on github is not a bitcoin person.
13. 
    morcos commented at 4:45 pm on January 6, 2016: member

    Apologies. @laanwj I think we could use this in 0.12
14. laanwj added the label Wallet on Jan 7, 2016
15. laanwj added this to the milestone 0.12.0 on Jan 7, 2016
16. laanwj added the label Needs backport on Jan 7, 2016
17. 
    laanwj commented at 8:04 am on January 7, 2016: member

    Concept ACK.

    One nit: this adds yet another -XXXfee parameter. Although it makes sense, for the wallet we already have -mintxfee, -paytxfee, -maxtxfee this wild growth of options does get a bit confusing for users, I think. But I don’t know a better solution either.

18. 
    dcousens commented at 1:19 am on January 8, 2016: contributor

    @laanwj it’s also not clear whether those parameters are referring to the wallet or to relay policy (until you read the documentation in-depth, anyway).
19. 
    btcdrak commented at 2:55 pm on January 8, 2016: contributor

    Concept ACK will test later today.
20. 
    sdaftuar commented at 3:00 pm on January 12, 2016: member

    Tested, ACK bebe58b748532157958f9055e4517e280684006c
21. 
    MarcoFalke commented at 4:29 pm on January 12, 2016: member

    • utACK 995b9f3
    • utACK bebe58b
22. 
    laanwj commented at 10:03 am on January 13, 2016: member

    utACK
23. laanwj merged this on Jan 13, 2016
24. laanwj closed this on Jan 13, 2016

    ---

25. laanwj referenced this in commit c49551886a on Jan 13, 2016
26. laanwj referenced this in commit a36d79bfe2 on Jan 13, 2016
27. 
    laanwj commented at 10:06 am on January 13, 2016: member

    Cherry-picked to 0.12 as a36d79bfe247e7fc5c6296fd8603f5094edfe558
28. laanwj removed the label Needs backport on Feb 4, 2016
29. furszy referenced this in commit 0724bbbad2 on Jun 28, 2020
30. MarcoFalke locked this on Sep 8, 2021

Contributors
morcos MarcoFalke wumpus laanwj dcousens btcdrak sdaftuar

Labels
Wallet

Milestone
0.12.0