More economical fee estimates for RBF and RPC options to control #10589

pull morcos wants to merge 5 commits into bitcoin:master from morcos:rpcestimatechoice changing 13 files +157 −28
  1. morcos commented at 7:24 pm on June 14, 2017: member

    This PR takes advantage of the new fee estimation feature that will potentially give lower estimates if recent market conditions warrant it. The logic used here is that any time a transaction signals opt-in-RBF and uses automatic fee estimation then it will use the non-conservative estimate. Transactions which do not signal opt-in-RBF will still use the default conservative estimate.

    In a nutshell conservative estimates require that your fee rate would meet the necessary confirmation threshold for double your target at longer time horizons as well. This reduces the likelihood that you place a transaction just as the market is starting to get busy again that ends up being stuck for a very long time.

    This PR also allows the specification of transaction confirmation target and whether the estimate should be conservative or not on a per-RPC call basis for sendtoaddress, sendmany, bumpfee, and fundrawtransaction using optional named arguments.

    Left as an exercise to the reader is adding this to GUI functions to send transactions and bump fee.

  2. RHavar commented at 7:43 pm on June 14, 2017: contributor

    Concept ACK, this is really great and something that’s been needed for a long time. It’ll simplify some of the locking and settxfee stuff I’ve needed to do.

    But it’d be nice if it used an options object or something. Otherwise if I want to specify the conservative_estimate I’ll need to fill in all the other fields, which I might not want to know?

    But I guess that’s something that can be done in a different commit, with if (arguments.length == 3 && typeof arguments == 'object') style

  3. instagibbs commented at 7:46 pm on June 14, 2017: member
    @RHavar I had the same exact reaction fwiw. fee_options or something.
  4. morcos commented at 8:01 pm on June 14, 2017: member
    I thought the whole point of named arguments is we didn’t have to worry about that any more. I think I made sure these work correctly with holes so we can use them with named arguments.
  5. RHavar commented at 8:03 pm on June 14, 2017: contributor
    Oh yeah, you’re right. Guess I should learn how to use named arguments.
  6. fanquake added the label RPC/REST/ZMQ on Jun 15, 2017
  7. fanquake added the label Wallet on Jun 15, 2017
  8. sipa commented at 11:27 pm on June 16, 2017: member
    Needs rebase.
  9. morcos force-pushed on Jun 18, 2017
  10. morcos commented at 12:21 pm on June 18, 2017: member
    Rebased due to conflict with #10422
  11. in src/wallet/rpcwallet.cpp:932 in 8971c39dec outdated 
    926@@ -909,7 +927,10 @@ UniValue sendmany(const JSONRPCRequest& request)
927             "      \"address\"          (string) Subtract fee from this address\n"
928             "      ,...\n"
929             "    ]\n"
930-            "\nResult:\n"
931+            "6. opt_in_rbf             (boolean, optional) Allow this transaction to be replaced by a transaction with higher fees\n"
932+            "7. conf_target            (numeric, optional) Confirmation target (in blocks)\n"
933+            "8. conservative_estimate  (boolean, optional) Use conservative (potentially higher) fee estimation\n"
    laanwj commented at 1:08 pm on June 22, 2017:
    I’d prefer to use a string here (which maps to FeeEstimateMode) instead of a boolean. This would also allow adding additional fee estimation strategies in the future without changing the interface. (if we keep it this way, it should be documented that this is really a tri-value boolean, where null/unset is auto)
  12. laanwj commented at 1:09 pm on June 22, 2017: member
    Concept ACK
  13. morcos renamed this:
    Add RPC options for RBF, confirmation target and conservative fee estimates
    More economical fee estimates for RBF and RPC options to control
    on Jun 27, 2017
  14. morcos force-pushed on Jun 28, 2017
  15. morcos commented at 11:48 am on June 28, 2017: member
    Switched to an estimate_mode string and squashed 5cbc1f6 (rpcestimatechoice.ver2) -> 1bc6a4a (rpcestimatechoices.ver2.squash)
  16. morcos force-pushed on Jun 28, 2017
  17. morcos commented at 12:17 pm on June 28, 2017: member
    Changed argument name from opt_in_rbf to replaceable for conformance with bumpfee
  18. laanwj added this to the "Blockers" column in a project
  19. in src/wallet/wallet.cpp:4152 in df7ea7a9f0 outdated 
    4147@@ -4145,3 +4148,14 @@ bool CMerkleTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState&
4148 {
4149     return ::AcceptToMemoryPool(mempool, state, tx, true, NULL, NULL, false, nAbsurdFee);
4150 }
4151+
4152+bool CalculateEstimateType(FeeEstimateMode mode) {
    instagibbs commented at 1:17 pm on July 3, 2017:
    Not sure this is the best possible name. ConservativeEstimateRequested ?
    morcos commented at 5:26 pm on July 3, 2017:
    #10586 (review)
    instagibbs commented at 5:34 pm on July 3, 2017:
    Ok we agree it should be a mode being returned ideally.
    TheBlueMatt commented at 11:43 pm on July 5, 2017:

    I think it makes more sense to let it be generic and ideally estimatesmartfee would take a FeeEstimateMode and this functions purpose would be to take any requested mode and other wallet parameters and use wallet logic to determine a final mode. Rather than make all those changes now while not yet needed though, I think I’ll just change the RPC interface to conservative estimates to take a string as per #10589 (review)

    I stand entirely unconvinced that you shouldn’t just do this now - just add more args (or add a struct arg) to GetMinimumFee instead of adding some new function that returns some magic value that you then immediately go and pass into GetMinimumFee everywhere?

    morcos commented at 1:21 pm on July 6, 2017:
    Please see #10706 CalculateEstimateType is now only used one place, in GetMinimumFee.
  21. in src/wallet/rpcwallet.cpp:469 in ab07a1063b outdated 
    465+
466+    if (request.params.size() > 7 && !request.params[7].isNull()) {
467+        if (boost::optional<FeeEstimateMode> fee_mode = FeeModeForString(request.params[7].get_str())) {
468+            coin_control.m_fee_mode = *fee_mode;
469+        }
470+        else {
    TheBlueMatt commented at 11:47 pm on July 5, 2017:
    Nit: no need for the \n here (and a few other similar lines in this file.).
  22. TheBlueMatt commented at 0:11 am on July 6, 2017: member
    I still dont really feel comfortable ACKing the Qt changes until I get a chance to test, but this is generally a very welcome improvement.
  23. jonasschnelli added this to the milestone 0.15.0 on Jul 6, 2017
  24. remove default argument from GetMinimumFee cfaef69ace
  25. Introduce a fee estimate mode. 
    GetMinimumFee now passes the conservative argument into estimateSmartFee.
Call CalculateEstimateType(mode) before calling GetMinimumFee or estimateSmartFee to determine the value of this argument.
CCoinControl can now be used to control this mode.
    d507c301bc
  26. remove default argument from estimateSmartFee e0738e3d31
  27. Change default fee estimation mode. 
    Fee estimates will default to be non-conservative if the transaction in question is opt-in-RBF.
    f0bf33da83
  28. morcos force-pushed on Jul 7, 2017
  29. morcos commented at 2:27 am on July 7, 2017: member

    rebased for adjacent line change in #10698 and removed a couple extra \n’s

    no other changes

    I believe we’ve settled on removing the CalculateEstimateType function entirely at the end of #10706 since it just becomes a helper for GetMinimumFee at that point.

  30. in src/qt/sendcoinsdialog.cpp:170 in 0697317c36 outdated 
    165@@ -166,6 +166,8 @@ void SendCoinsDialog::setModel(WalletModel *_model)
166         connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(updateFeeSectionControls()));
167         connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(updateGlobalFeeVariables()));
168         connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(coinControlUpdateLabels()));
169+        connect(ui->optInRBF, SIGNAL(stateChanged(int)), this, SLOT(updateSmartFeeLabel()));
170+        connect(ui->optInRBF, SIGNAL(stateChanged(int)), this, SLOT(coinControlUpdateLabels()));
    TheBlueMatt commented at 9:01 pm on July 9, 2017:
    Cant comment there, but I believe you also need to move the default setting of optInRBF up a bunch here (about 10 lines below this (set default rbf checkbox state, or does that setting automagically result in calling these registrations?).
    morcos commented at 12:48 pm on July 10, 2017:
    Not sure if I understand your question (or how QT works) but I’m pretty sure it’s correct as is. Calling setCheckState below will trigger the signal if the checkbox changed from its initial value.
    promag commented at 1:15 pm on July 10, 2017:
    Either way, what about make an explicit call to coinControlUpdateLabels()?
    morcos commented at 3:56 pm on July 10, 2017:
    I don’t think this is necessary. It’s analogous to setting the default sliderSmartFee done in the same section of code.
  31. TheBlueMatt commented at 11:10 pm on July 9, 2017: member
    Looks good. Definitely want #10706 to go in with this.
  32. in src/wallet/rpcwallet.cpp:421 in 0697317c36 outdated 
    415@@ -416,6 +416,12 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
416             "                             transaction, just kept in your wallet.\n"
417             "5. subtractfeefromamount  (boolean, optional, default=false) The fee will be deducted from the amount being sent.\n"
418             "                             The recipient will receive less bitcoins than you enter in the amount field.\n"
419+            "6. replaceable            (boolean, optional) Allow this transaction to be replaced by a transaction with higher fees via BIP 125\n"
420+            "7. conf_target            (numeric, optional) Confirmation target (in blocks)\n"
421+            "8. \"estimate_mode\"      (string, optional, default=UNSET) The fee estimate mode, must be one of:\n"
    promag commented at 1:12 pm on July 10, 2017:
    fee_estimate_mode, fee_mode or estimate_mode?
    morcos commented at 3:58 pm on July 10, 2017:
    Hmm.. Yes, I think it might make sense to change this to fee_mode, but leave it estimate_mode for estimateSmartFee (done in another PR). Even though they take the same values now, I could imagine they would differ in the future. Any other thoughts?
    promag commented at 4:03 pm on July 10, 2017:
    I just wanted to point out the inconsistencies between arguments, variable names and types.
    morcos commented at 4:21 pm on July 10, 2017:
    I decided leaving it as estimate_mode in both RPC’s is preferable. The inconsistency between variable and parameter name is not worth the churn imo.
  33. in src/wallet/coincontrol.h:31 in 0697317c36 outdated 
    26@@ -26,6 +27,8 @@ class CCoinControl
27     int nConfirmTarget;
28     //! Signal BIP-125 replace by fee.
29     bool signalRbf;
30+    //! Fee estimation mode to control arguments to estimateSmartFee
31+    FeeEstimateMode m_fee_mode;
    promag commented at 1:16 pm on July 10, 2017:
    m_fee_estimate_mode?
  34. in src/wallet/rpcwallet.cpp:406 in 0697317c36 outdated 
    400@@ -401,7 +401,7 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
401         return NullUniValue;
402     }
403 
404-    if (request.fHelp || request.params.size() < 2 || request.params.size() > 5)
405+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 8)
406         throw std::runtime_error(
407             "sendtoaddress \"address\" amount ( \"comment\" \"comment_to\" subtractfeefromamount )\n"
    promag commented at 1:17 pm on July 10, 2017:
    Missing new arguments.
    morcos commented at 3:56 pm on July 10, 2017:
    will do
  35. in src/wallet/rpcwallet.cpp:936 in 0697317c36 outdated 
    934@@ -910,7 +935,13 @@ UniValue sendmany(const JSONRPCRequest& request)
935             "      \"address\"          (string) Subtract fee from this address\n"
936             "      ,...\n"
937             "    ]\n"
938-            "\nResult:\n"
939+            "6. replaceable            (boolean, optional) Allow this transaction to be replaced by a transaction with higher fees via BIP 125\n"
    promag commented at 1:23 pm on July 10, 2017:
    Add options instead of 3 more optional arguments?
    promag commented at 1:45 pm on July 10, 2017:
    I saw the above comment about named arguments. IMO from the client side feels better as an option.
  36. in src/wallet/rpcwallet.cpp:419 in 0697317c36 outdated 
    415@@ -416,6 +416,12 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
416             "                             transaction, just kept in your wallet.\n"
417             "5. subtractfeefromamount  (boolean, optional, default=false) The fee will be deducted from the amount being sent.\n"
418             "                             The recipient will receive less bitcoins than you enter in the amount field.\n"
419+            "6. replaceable            (boolean, optional) Allow this transaction to be replaced by a transaction with higher fees via BIP 125\n"
    promag commented at 1:24 pm on July 10, 2017:
    Add options instead of 3 more optional arguments?
    morcos commented at 3:57 pm on July 10, 2017:
    I believe named arguments are preferable
    promag commented at 8:06 am on July 11, 2017:
    I understand, but for those that use indexed arguments, they have to pass the middle arguments.
  37. in src/wallet/wallet.cpp:4161 in 0697317c36 outdated 
    4156@@ -4154,3 +4157,15 @@ bool CMerkleTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState&
4157 {
4158     return ::AcceptToMemoryPool(mempool, state, tx, true, NULL, NULL, false, nAbsurdFee);
4159 }
4160+
4161+bool CalculateEstimateType(FeeEstimateMode mode, bool opt_in_rbf) {
    promag commented at 1:38 pm on July 10, 2017:
    IsFeeEstimateConservative?
    morcos commented at 4:00 pm on July 10, 2017:
    Function removed in #10706 (or will be shortly)
    promag commented at 4:05 pm on July 10, 2017:
    Still, name not clear don’t you think?
    morcos commented at 4:13 pm on July 10, 2017:
    yeah but who cares, its going away completely in a couple of commits
  38. promag commented at 1:39 pm on July 10, 2017: member

    IMO this would be a cleaner implementation

     0bool FeeEstimateModeFromString(const std::string& string, FeeEstimateMode& fee_estimate_mode)
 1{
 2    // lookup string
 3    return false;
 4}
 5
 6...
 7
 8
 9if (!FeeEstimateModeFromString(request.params[7].get_str(), coin_control.m_fee_mode)) {
10    throw JSONRPCError(RPC_INVALID_PARAMETER, ...);
11}
  39. Add RPC options for RBF, confirmation target, and conservative fee estimation. 
    Add support for setting each of these attributes on a per RPC call basis to sendtoaddress, sendmany, fundrawtransaction (already had RBF), and bumpfee (already had RBF and conf target).
    f135923ee2
  40. morcos force-pushed on Jul 10, 2017
  41. morcos commented at 4:41 pm on July 10, 2017: member
    Took some of the nits (rpcestimatechoice.ver3) -> f135923 (rpcestimatechoices.ver3.squash)
  42. ryanofsky commented at 8:55 pm on July 10, 2017: member
    utACK f135923ee2cf1a1a9a436626dc5b9219f8ad97da
  43. laanwj commented at 9:57 am on July 11, 2017: member
    utACK f135923
  44. laanwj merged this on Jul 11, 2017
  45. laanwj closed this on Jul 11, 2017
  46. laanwj referenced this in commit 104f5f21dc on Jul 11, 2017
  47. laanwj removed this from the "Blockers" column in a project
  48. laanwj referenced this in commit 6859ad2936 on Jul 17, 2017
  49. PastaPastaPasta referenced this in commit 5b06d5eb1d on Aug 8, 2019
  50. PastaPastaPasta referenced this in commit 5bd0926e0c on Aug 9, 2019
  51. UdjinM6 referenced this in commit 7c35fee2de on Aug 10, 2019
  52. PastaPastaPasta referenced this in commit c9784838a7 on Aug 12, 2019
  53. PastaPastaPasta referenced this in commit c849708996 on Sep 3, 2019
  54. barrystyle referenced this in commit f4073e2f5c on Jan 22, 2020
  55. barrystyle referenced this in commit 7013c8b341 on Jan 22, 2020
  56. MarcoFalke locked this on Sep 8, 2021


morcos RHavar instagibbs sipa laanwj TheBlueMatt promag ryanofsky

Labels
Wallet RPC/REST/ZMQ

Milestone
0.15.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 19:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me