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:None 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:None 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:None 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. MarcoFalke cross-referenced this on Jan 7, 2016 from issue [0.12] Update release-notes.md by MarcoFalke
19. 
    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).

20. MarcoFalke cross-referenced this on Jan 8, 2016 from issue Always respect GetRequiredFee for wallet txs by MarcoFalke
21. 
    btcdrak commented at 2:55 PM on January 8, 2016: contributor

    Concept ACK will test later today.

22. 
    sdaftuar commented at 3:00 PM on January 12, 2016: member

    Tested, ACK bebe58b748532157958f9055e4517e280684006c

23. 
    MarcoFalke commented at 4:29 PM on January 12, 2016: member

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

    utACK

25. laanwj merged this on Jan 13, 2016
26. laanwj closed this on Jan 13, 2016

    ---

27. laanwj referenced this in commit c49551886a on Jan 13, 2016
28. laanwj referenced this in commit a36d79bfe2 on Jan 13, 2016
29. 
    laanwj commented at 10:06 AM on January 13, 2016: member

    Cherry-picked to 0.12 as a36d79bfe247e7fc5c6296fd8603f5094edfe558

30. laanwj removed the label Needs backport on Feb 4, 2016
31. dagurval cross-referenced this on Dec 27, 2017 from issue Add fundrawtransaction by dagurval
32. random-zebra cross-referenced this on Jun 4, 2020 from issue [Wallet][RPC] FundTransaction - fundrawtransaction by random-zebra
33. furszy referenced this in commit 0724bbbad2 on Jun 28, 2020
34. bitcoin locked this on Sep 8, 2021

Contributors

morcosMarcoFalkewumpuslaanwjdcousensbtcdraksdaftuar

Milestone
0.12.0