-
Notifications
You must be signed in to change notification settings - Fork 36.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
More economical fee estimates for RBF and RPC options to control #10589
Conversation
Concept ACK, this is really great and something that's been needed for a long time. It'll simplify some of the locking and But it'd be nice if it used an But I guess that's something that can be done in a different commit, with |
@RHavar I had the same exact reaction fwiw. |
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. |
Oh yeah, you're right. Guess I should learn how to use named arguments. |
Needs rebase. |
f8e58d5
to
8971c39
Compare
Rebased due to conflict with #10422 |
src/wallet/rpcwallet.cpp
Outdated
"\nResult:\n" | ||
"6. opt_in_rbf (boolean, optional) Allow this transaction to be replaced by a transaction with higher fees\n" | ||
"7. conf_target (numeric, optional) Confirmation target (in blocks)\n" | ||
"8. conservative_estimate (boolean, optional) Use conservative (potentially higher) fee estimation\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Concept ACK |
8971c39
to
1bc6a4a
Compare
Switched to an estimate_mode string and squashed |
1bc6a4a
to
ab07a10
Compare
Changed argument name from opt_in_rbf to replaceable for conformance with bumpfee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK ab07a10
src/wallet/wallet.cpp
Outdated
@@ -4145,3 +4148,14 @@ bool CMerkleTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& | |||
{ | |||
return ::AcceptToMemoryPool(mempool, state, tx, true, NULL, NULL, false, nAbsurdFee); | |||
} | |||
|
|||
bool CalculateEstimateType(FeeEstimateMode mode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is the best possible name. ConservativeEstimateRequested
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok we agree it should be a mode being returned ideally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see #10706
CalculateEstimateType is now only used one place, in GetMinimumFee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/wallet/wallet.cpp
Outdated
@@ -4145,3 +4148,14 @@ bool CMerkleTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& | |||
{ | |||
return ::AcceptToMemoryPool(mempool, state, tx, true, NULL, NULL, false, nAbsurdFee); | |||
} | |||
|
|||
bool CalculateEstimateType(FeeEstimateMode mode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
src/wallet/rpcwallet.cpp
Outdated
if (boost::optional<FeeEstimateMode> fee_mode = FeeModeForString(request.params[7].get_str())) { | ||
coin_control.m_fee_mode = *fee_mode; | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: no need for the \n here (and a few other similar lines in this file.).
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.
Fee estimates will default to be non-conservative if the transaction in question is opt-in-RBF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Definitely want #10706 to go in with this.
@@ -166,6 +166,8 @@ void SendCoinsDialog::setModel(WalletModel *_model) | |||
connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(updateFeeSectionControls())); | |||
connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(updateGlobalFeeVariables())); | |||
connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(coinControlUpdateLabels())); | |||
connect(ui->optInRBF, SIGNAL(stateChanged(int)), this, SLOT(updateSmartFeeLabel())); | |||
connect(ui->optInRBF, SIGNAL(stateChanged(int)), this, SLOT(coinControlUpdateLabels())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, what about make an explicit call to coinControlUpdateLabels()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary. It's analogous to setting the default sliderSmartFee done in the same section of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this would be a cleaner implementation
bool FeeEstimateModeFromString(const std::string& string, FeeEstimateMode& fee_estimate_mode)
{
// lookup string
return false;
}
...
if (!FeeEstimateModeFromString(request.params[7].get_str(), coin_control.m_fee_mode)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, ...);
}
@@ -416,6 +416,12 @@ UniValue sendtoaddress(const JSONRPCRequest& request) | |||
" transaction, just kept in your wallet.\n" | |||
"5. subtractfeefromamount (boolean, optional, default=false) The fee will be deducted from the amount being sent.\n" | |||
" The recipient will receive less bitcoins than you enter in the amount field.\n" | |||
"6. replaceable (boolean, optional) Allow this transaction to be replaced by a transaction with higher fees via BIP 125\n" | |||
"7. conf_target (numeric, optional) Confirmation target (in blocks)\n" | |||
"8. \"estimate_mode\" (string, optional, default=UNSET) The fee estimate mode, must be one of:\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fee_estimate_mode
, fee_mode
or estimate_mode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to point out the inconsistencies between arguments, variable names and types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -166,6 +166,8 @@ void SendCoinsDialog::setModel(WalletModel *_model) | |||
connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(updateFeeSectionControls())); | |||
connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(updateGlobalFeeVariables())); | |||
connect(ui->checkBoxMinimumFee, SIGNAL(stateChanged(int)), this, SLOT(coinControlUpdateLabels())); | |||
connect(ui->optInRBF, SIGNAL(stateChanged(int)), this, SLOT(updateSmartFeeLabel())); | |||
connect(ui->optInRBF, SIGNAL(stateChanged(int)), this, SLOT(coinControlUpdateLabels())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, what about make an explicit call to coinControlUpdateLabels()
?
@@ -26,6 +27,8 @@ class CCoinControl | |||
int nConfirmTarget; | |||
//! Signal BIP-125 replace by fee. | |||
bool signalRbf; | |||
//! Fee estimation mode to control arguments to estimateSmartFee | |||
FeeEstimateMode m_fee_mode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_fee_estimate_mode
?
src/wallet/rpcwallet.cpp
Outdated
@@ -401,7 +401,7 @@ UniValue sendtoaddress(const JSONRPCRequest& request) | |||
return NullUniValue; | |||
} | |||
|
|||
if (request.fHelp || request.params.size() < 2 || request.params.size() > 5) | |||
if (request.fHelp || request.params.size() < 2 || request.params.size() > 8) | |||
throw std::runtime_error( | |||
"sendtoaddress \"address\" amount ( \"comment\" \"comment_to\" subtractfeefromamount )\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing new arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
@@ -910,7 +935,13 @@ UniValue sendmany(const JSONRPCRequest& request) | |||
" \"address\" (string) Subtract fee from this address\n" | |||
" ,...\n" | |||
" ]\n" | |||
"\nResult:\n" | |||
"6. replaceable (boolean, optional) Allow this transaction to be replaced by a transaction with higher fees via BIP 125\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add options
instead of 3 more optional arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw the above comment about named arguments. IMO from the client side feels better as an option.
@@ -416,6 +416,12 @@ UniValue sendtoaddress(const JSONRPCRequest& request) | |||
" transaction, just kept in your wallet.\n" | |||
"5. subtractfeefromamount (boolean, optional, default=false) The fee will be deducted from the amount being sent.\n" | |||
" The recipient will receive less bitcoins than you enter in the amount field.\n" | |||
"6. replaceable (boolean, optional) Allow this transaction to be replaced by a transaction with higher fees via BIP 125\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add options
instead of 3 more optional arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe named arguments are preferable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, but for those that use indexed arguments, they have to pass the middle arguments.
@@ -4154,3 +4157,15 @@ bool CMerkleTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& | |||
{ | |||
return ::AcceptToMemoryPool(mempool, state, tx, true, NULL, NULL, false, nAbsurdFee); | |||
} | |||
|
|||
bool CalculateEstimateType(FeeEstimateMode mode, bool opt_in_rbf) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsFeeEstimateConservative
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function removed in #10706 (or will be shortly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, name not clear don't you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah but who cares, its going away completely in a couple of commits
…timation. 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).
0697317
to
f135923
Compare
Took some of the nits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK f135923
utACK f135923 |
…o control f135923 Add RPC options for RBF, confirmation target, and conservative fee estimation. (Alex Morcos) f0bf33d Change default fee estimation mode. (Alex Morcos) e0738e3 remove default argument from estimateSmartFee (Alex Morcos) d507c30 Introduce a fee estimate mode. (Alex Morcos) cfaef69 remove default argument from GetMinimumFee (Alex Morcos) Tree-SHA512: 49c3a49a6893790a7e8b4e93a48f123dd5307af26c2017800683b76b4df8fc904ba73402917878676242c7440e3e04288d0c1ff3c2c907418724efc03cedab50
11590d3 Properly bound check conf_target in wallet RPC calls (Alex Morcos) fd29d3d Remove checking of mempool min fee from estimateSmartFee. (Alex Morcos) 2fffaa9 Make QT fee displays use GetMinimumFee instead of estimateSmartFee (Alex Morcos) 1983ca6 Use CoinControl to pass custom fee setting from QT. (Alex Morcos) 03ee701 Refactor to use CoinControl in GetMinimumFee and FeeBumper (Alex Morcos) ecd81df Make CoinControl a required argument to CreateTransaction (Alex Morcos) Pull request description: This builds on #10589 (first 5 commits from that PR, last 5 commits are new) The first couple commits refactor to use the CCoinControl class to pass fee calculation parameters around. This allows for fixing the buggy interaction in QT between the global payTxFee which can be modified by the RPC call settxfee or temporarily modified by the QT custom fee settings. Before these changes the GUI could sometimes send a transaction with a recently set payTxFee and not respect the settings displayed in the GUI. After these changes, using the GUI does not involve the global transaction confirm target or payTxFee. The prospective fee displays in the smart fee slider and the coin control dialog are changed to use the fee calculation from GetMinimumFee, this simplifies the code and makes them slightly more correct in edge cases. Maxing the fee calculation with the mempool min fee is move from estimateSmartFee to GetMinimumFee. This fixes a long standing bug, and should be tagged for 0.15 as it is holding up finalizing the estimatesmartfee RPC API before release. Tree-SHA512: 4d36a1bd5934aa62f3806d380fcafbef73e9fe5bdf190fc5259a3e3a13349e5ce796e50e7068c46dc630ccf56d061bce5804f0bfe2e082bb01ca725b63efd4c1
…tions to control f135923 Add RPC options for RBF, confirmation target, and conservative fee estimation. (Alex Morcos) f0bf33d Change default fee estimation mode. (Alex Morcos) e0738e3 remove default argument from estimateSmartFee (Alex Morcos) d507c30 Introduce a fee estimate mode. (Alex Morcos) cfaef69 remove default argument from GetMinimumFee (Alex Morcos) Tree-SHA512: 49c3a49a6893790a7e8b4e93a48f123dd5307af26c2017800683b76b4df8fc904ba73402917878676242c7440e3e04288d0c1ff3c2c907418724efc03cedab50
…tions to control f135923 Add RPC options for RBF, confirmation target, and conservative fee estimation. (Alex Morcos) f0bf33d Change default fee estimation mode. (Alex Morcos) e0738e3 remove default argument from estimateSmartFee (Alex Morcos) d507c30 Introduce a fee estimate mode. (Alex Morcos) cfaef69 remove default argument from GetMinimumFee (Alex Morcos) Tree-SHA512: 49c3a49a6893790a7e8b4e93a48f123dd5307af26c2017800683b76b4df8fc904ba73402917878676242c7440e3e04288d0c1ff3c2c907418724efc03cedab50
…tions to control f135923 Add RPC options for RBF, confirmation target, and conservative fee estimation. (Alex Morcos) f0bf33d Change default fee estimation mode. (Alex Morcos) e0738e3 remove default argument from estimateSmartFee (Alex Morcos) d507c30 Introduce a fee estimate mode. (Alex Morcos) cfaef69 remove default argument from GetMinimumFee (Alex Morcos) Tree-SHA512: 49c3a49a6893790a7e8b4e93a48f123dd5307af26c2017800683b76b4df8fc904ba73402917878676242c7440e3e04288d0c1ff3c2c907418724efc03cedab50
…tions to control f135923 Add RPC options for RBF, confirmation target, and conservative fee estimation. (Alex Morcos) f0bf33d Change default fee estimation mode. (Alex Morcos) e0738e3 remove default argument from estimateSmartFee (Alex Morcos) d507c30 Introduce a fee estimate mode. (Alex Morcos) cfaef69 remove default argument from GetMinimumFee (Alex Morcos) Tree-SHA512: 49c3a49a6893790a7e8b4e93a48f123dd5307af26c2017800683b76b4df8fc904ba73402917878676242c7440e3e04288d0c1ff3c2c907418724efc03cedab50
Contains RBF stuff to be removed in a later commit 11590d3 Properly bound check conf_target in wallet RPC calls (Alex Morcos) fd29d3d Remove checking of mempool min fee from estimateSmartFee. (Alex Morcos) 2fffaa9 Make QT fee displays use GetMinimumFee instead of estimateSmartFee (Alex Morcos) 1983ca6 Use CoinControl to pass custom fee setting from QT. (Alex Morcos) 03ee701 Refactor to use CoinControl in GetMinimumFee and FeeBumper (Alex Morcos) ecd81df Make CoinControl a required argument to CreateTransaction (Alex Morcos) Pull request description: This builds on bitcoin#10589 (first 5 commits from that PR, last 5 commits are new) The first couple commits refactor to use the CCoinControl class to pass fee calculation parameters around. This allows for fixing the buggy interaction in QT between the global payTxFee which can be modified by the RPC call settxfee or temporarily modified by the QT custom fee settings. Before these changes the GUI could sometimes send a transaction with a recently set payTxFee and not respect the settings displayed in the GUI. After these changes, using the GUI does not involve the global transaction confirm target or payTxFee. The prospective fee displays in the smart fee slider and the coin control dialog are changed to use the fee calculation from GetMinimumFee, this simplifies the code and makes them slightly more correct in edge cases. Maxing the fee calculation with the mempool min fee is move from estimateSmartFee to GetMinimumFee. This fixes a long standing bug, and should be tagged for 0.15 as it is holding up finalizing the estimatesmartfee RPC API before release. Tree-SHA512: 4d36a1bd5934aa62f3806d380fcafbef73e9fe5bdf190fc5259a3e3a13349e5ce796e50e7068c46dc630ccf56d061bce5804f0bfe2e082bb01ca725b63efd4c1
…tions to control f135923 Add RPC options for RBF, confirmation target, and conservative fee estimation. (Alex Morcos) f0bf33d Change default fee estimation mode. (Alex Morcos) e0738e3 remove default argument from estimateSmartFee (Alex Morcos) d507c30 Introduce a fee estimate mode. (Alex Morcos) cfaef69 remove default argument from GetMinimumFee (Alex Morcos) Tree-SHA512: 49c3a49a6893790a7e8b4e93a48f123dd5307af26c2017800683b76b4df8fc904ba73402917878676242c7440e3e04288d0c1ff3c2c907418724efc03cedab50
Contains RBF stuff to be removed in a later commit 11590d3 Properly bound check conf_target in wallet RPC calls (Alex Morcos) fd29d3d Remove checking of mempool min fee from estimateSmartFee. (Alex Morcos) 2fffaa9 Make QT fee displays use GetMinimumFee instead of estimateSmartFee (Alex Morcos) 1983ca6 Use CoinControl to pass custom fee setting from QT. (Alex Morcos) 03ee701 Refactor to use CoinControl in GetMinimumFee and FeeBumper (Alex Morcos) ecd81df Make CoinControl a required argument to CreateTransaction (Alex Morcos) Pull request description: This builds on bitcoin#10589 (first 5 commits from that PR, last 5 commits are new) The first couple commits refactor to use the CCoinControl class to pass fee calculation parameters around. This allows for fixing the buggy interaction in QT between the global payTxFee which can be modified by the RPC call settxfee or temporarily modified by the QT custom fee settings. Before these changes the GUI could sometimes send a transaction with a recently set payTxFee and not respect the settings displayed in the GUI. After these changes, using the GUI does not involve the global transaction confirm target or payTxFee. The prospective fee displays in the smart fee slider and the coin control dialog are changed to use the fee calculation from GetMinimumFee, this simplifies the code and makes them slightly more correct in edge cases. Maxing the fee calculation with the mempool min fee is move from estimateSmartFee to GetMinimumFee. This fixes a long standing bug, and should be tagged for 0.15 as it is holding up finalizing the estimatesmartfee RPC API before release. Tree-SHA512: 4d36a1bd5934aa62f3806d380fcafbef73e9fe5bdf190fc5259a3e3a13349e5ce796e50e7068c46dc630ccf56d061bce5804f0bfe2e082bb01ca725b63efd4c1
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.