Skip to content
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

Improve wallet fee logic and fix GUI bugs #10706

Merged
merged 6 commits into from Jul 17, 2017

Conversation

@morcos
Copy link
Member

morcos commented Jun 29, 2017

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.

@TheBlueMatt
Copy link
Contributor

TheBlueMatt left a comment

Looks good, definitely want this to go in with #10589 (I'm super not a fan of CalculateEstimateType as an GetMinimumFee helper, but its easy to get rid of after this PR, less so after 10589).

src/wallet/coincontrol.h Outdated
//! Override the default payTxFee if set, 0 = use smart fee estimation
boost::optional<CFeeRate> m_feerate;
//! Override the default confirmation target
boost::optional<unsigned int> m_confirm_target;

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 9, 2017

Contributor

Gah, I'd really rather keep the old magic value of just 0 means default instead of adding boost::optional (and kinda would prefer to skip boost::option in favor of more descriptive names for m_feerate, too).

This comment has been minimized.

@morcos

morcos Jul 10, 2017

Author Member

I hate the magic values of 0. Actually I think I should make the logic even slightly more clear.
There should just be a clear parameter precedence.

  • coin_control.m_feerate
  • coin_control.m_confirm_target
  • ::payTxFee (global)
  • ::nTxConfirmTarget (globa)

It goes down the list until it fines one that is set and all the first 3 should be boost::optional's. Why do you not like boost::optional?
I could skip changing the global payTxFee for now, and still have that work via the 0 magic value.
The code to process this will be slightly longer in GetMininumFee but I think it'll be far more clear no?

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 10, 2017

Contributor

Hmm, maybe I shouldnt argue against boost::optional. I'm not a fan of optional types, but they do make sense here. Any further simplification would be nice, this stuff is gross (thanks for cleaning it up!).

src/wallet/coincontrol.h Outdated
//! Override the default confirmation target, 0 = use default
int nConfirmTarget;
//! Override the default payTxFee if set, 0 = use smart fee estimation
boost::optional<CFeeRate> m_feerate;

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 9, 2017

Contributor

Can you write out more docs here?
"m_feerate unset uses global paytxfee (if set) otherwise uses smart fee estimation. m_feerate set to 0 ignores global paytxfee and uses smart fee estimation. m_feerate set skips fee estimation but applies minTxFee and maxTxFee"
"fOverrideFeeRate overrides all other feerate options (global paytxfee, smart fee estimation, txconfirmtarget, min+maxTxFee, etc) and uses m_feerate"

This comment has been minimized.

@morcos

morcos Jul 10, 2017

Author Member

(see above, will document when we have a final logic)

src/qt/walletmodel.cpp Outdated
LOCK2(cs_main, wallet->cs_wallet);
feeBump.reset(new CFeeBumper(wallet, hash, nTxConfirmTarget, false, 0, true));
feeBump.reset(new CFeeBumper(wallet, hash, no_coin_control, 0));

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 9, 2017

Contributor

This changes the newTxReplaceable option to fWalletRbf instead of true. Maybe leave it the way it was, or was this on purpose?

This comment has been minimized.

@morcos

morcos Jul 10, 2017

Author Member

Good catch. Will fix. I think I just assumed how the code worked and didn't even look at what I was replacing.

@morcos morcos force-pushed the morcos:improveWalletFeeLogic branch Jul 10, 2017

@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Jul 10, 2017

Rebased against the updated #10589 and fixed bug as well as improved parameter precedence logic for Coin Control.

@@ -2567,7 +2567,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
LOCK2(cs_main, cs_wallet);
{
std::vector<COutput> vAvailableCoins;
AvailableCoins(vAvailableCoins, true, coinControl);
AvailableCoins(vAvailableCoins, true, &coin_control);

This comment has been minimized.

@ryanofsky

ryanofsky Jul 10, 2017

Contributor

In commit "Make CoinControl a required argument to CreateTransaction"

Would be nice to make coin control a required argument to AvailableCoins as well. (To get rid of more multiply-defined default values.)

This comment has been minimized.

@morcos

morcos Jul 11, 2017

Author Member

I agree, but decided it would make this PR a bit too big.

Which multiply defined values are you referring to though? (edit: i think you just mean too many arguments with default values defined)

This comment has been minimized.

@ryanofsky

ryanofsky Jul 11, 2017

Contributor

Which multiply defined values are you referring to though?

I was just referring to the fact that functions accepting null coincontrol pointers have to figure out their own default values instead of using the ones in CoinControl::SetNull.

src/wallet/wallet.cpp Outdated
@@ -2617,7 +2617,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
// Choose coins to use
CAmount nValueIn = 0;
setCoins.clear();
if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, coinControl))
if (!SelectCoins(vAvailableCoins, nValueToSelect, setCoins, nValueIn, &coin_control))

This comment has been minimized.

@ryanofsky

ryanofsky Jul 10, 2017

Contributor

In commit "Make CoinControl a required argument to CreateTransaction"

Would be nice if to make coin control a required argument to SelectCoins as well.

src/wallet/coincontrol.h Outdated
@@ -41,9 +43,9 @@ class CCoinControl
fAllowOtherInputs = false;
fAllowWatchOnly = false;
setSelected.clear();
nFeeRate = CFeeRate(0);
m_feerate = boost::none;

This comment has been minimized.

@ryanofsky

ryanofsky Jul 10, 2017

Contributor

In commit "Refactor to use CoinControl in GetMinimumFee and FeeBumper"

Maybe replace = boost::none with .reset() here and other places to remove boost reference and make it a little easier to port to std::optional in the future.

This comment has been minimized.

@morcos

morcos Jul 11, 2017

Author Member

will do

This comment has been minimized.

@morcos

morcos Jul 11, 2017

Author Member

Actually turns out .reset() is deprecated so left it as is.

This comment has been minimized.

@ryanofsky

ryanofsky Jul 11, 2017

Contributor

Actually turns out .reset() is deprecated so left it as is.

.reset is part of c++17 (http://en.cppreference.com/w/cpp/utility/optional/reset) so I still think it would be better to switch to this. (I don't think we'd be using boost::optional at all now if we weren't planning to switch to std::optional in the future.)

@@ -19,12 +21,12 @@ class CCoinControl
bool fAllowOtherInputs;
//! Includes watch only addresses which match the ISMINE_WATCH_SOLVABLE criteria
bool fAllowWatchOnly;
//! Override estimated feerate
//! Override automatic min/max checks on fee, m_feerate must be set if true
bool fOverrideFeeRate;

This comment has been minimized.

@ryanofsky

ryanofsky Jul 10, 2017

Contributor

In commit "Refactor to use CoinControl in GetMinimumFee and FeeBumper"

You should definitely get rid of the fOverrideFeeRate member now that m_feerate is optional.

This comment has been minimized.

@morcos

morcos Jul 11, 2017

Author Member

Well.. almost
fOverrideFeeRate is used from fundrawtransaction and it seems to me that it might make sense that you don't want your specified fee rate subject to clamping by various min fees or the maxTxFee when you are using it from there but you do generally.

In any case it would be a change of functionality to change that, so we should do it separately.

This comment has been minimized.

@ryanofsky

ryanofsky Jul 11, 2017

Contributor

fOverrideFeeRate is used from fundrawtransaction

Sorry, misread fOverrideFeeRate comment. And this is now pretty clear with fOverrideFeeRate handled in GetMinimumFee.

@morcos morcos force-pushed the morcos:improveWalletFeeLogic branch Jul 11, 2017

@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Jul 11, 2017

Rebased cleanly so only new commits are left after #10589 was merged

src/wallet/wallet.cpp Outdated
CAmount nFeeNeeded = GetMinimumFee(nBytes, currentConfirmationTarget, ::mempool, ::feeEstimator, &feeCalc, false /* ignoreGlobalPayTxFee */, conservative_estimate);
if (coinControl && coinControl->fOverrideFeeRate)
nFeeNeeded = coinControl->nFeeRate.GetFee(nBytes);
// Allow to override automatic fee calculation (min/max checks) over coin control instance

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 11, 2017

Contributor

It would be nice to move this check into GetMinimumFee so that everything is ompletely in the same place.

This comment has been minimized.

@morcos

morcos Jul 11, 2017

Author Member

done

@morcos morcos force-pushed the morcos:improveWalletFeeLogic branch 2 times, most recently to fb98c64 Jul 11, 2017

@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Jul 11, 2017

Rebased to accomodate #10712 and moved fOverrideFeeRate inside GetMinimumFee

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK fb98c64

@@ -2567,7 +2567,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
LOCK2(cs_main, cs_wallet);
{
std::vector<COutput> vAvailableCoins;
AvailableCoins(vAvailableCoins, true, coinControl);
AvailableCoins(vAvailableCoins, true, &coin_control);

This comment has been minimized.

@ryanofsky

ryanofsky Jul 11, 2017

Contributor

Which multiply defined values are you referring to though?

I was just referring to the fact that functions accepting null coincontrol pointers have to figure out their own default values instead of using the ones in CoinControl::SetNull.

src/wallet/coincontrol.h Outdated
@@ -41,9 +43,9 @@ class CCoinControl
fAllowOtherInputs = false;
fAllowWatchOnly = false;
setSelected.clear();
nFeeRate = CFeeRate(0);
m_feerate = boost::none;

This comment has been minimized.

@ryanofsky

ryanofsky Jul 11, 2017

Contributor

Actually turns out .reset() is deprecated so left it as is.

.reset is part of c++17 (http://en.cppreference.com/w/cpp/utility/optional/reset) so I still think it would be better to switch to this. (I don't think we'd be using boost::optional at all now if we weren't planning to switch to std::optional in the future.)

newConfirmTarget = options["confTarget"].get_int();
if (newConfirmTarget <= 0) { // upper-bound will be checked by estimatefee/smartfee
int target = options["confTarget"].get_int();
if (target <= 0) { // FIXME: Check upper bound too

This comment has been minimized.

@ryanofsky

ryanofsky Jul 11, 2017

Contributor

In commit "Refactor to use CoinControl in GetMinimumFee and FeeBumper"

Why this fixme? Is the previous comment about upper bound not actually true?

This comment has been minimized.

@morcos

morcos Jul 12, 2017

Author Member

Correct, that comment about upper-bound is not true, I don't know why it was put there.
At the time I wrote these PR's I had a lot of important stuff I wanted to make sure got in, so I didn't want to fix every last little thing, but I think I could easily add a commit now that checks the bounds everywhere necessary.

@@ -19,12 +21,12 @@ class CCoinControl
bool fAllowOtherInputs;
//! Includes watch only addresses which match the ISMINE_WATCH_SOLVABLE criteria
bool fAllowWatchOnly;
//! Override estimated feerate
//! Override automatic min/max checks on fee, m_feerate must be set if true
bool fOverrideFeeRate;

This comment has been minimized.

@ryanofsky

ryanofsky Jul 11, 2017

Contributor

fOverrideFeeRate is used from fundrawtransaction

Sorry, misread fOverrideFeeRate comment. And this is now pretty clear with fOverrideFeeRate handled in GetMinimumFee.

@@ -587,7 +587,7 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
if (payTxFee.GetFeePerK() > 0)
dFeeVary = (double)std::max(CWallet::GetRequiredFee(1000), payTxFee.GetFeePerK()) / 1000;
else {
dFeeVary = (double)std::max(CWallet::GetRequiredFee(1000), ::feeEstimator.estimateSmartFee(coinControl->nConfirmTarget, NULL, ::mempool, conservative_estimate).GetFeePerK()) / 1000;
dFeeVary = (double)std::max(CWallet::GetRequiredFee(1000), ::feeEstimator.estimateSmartFee(*coinControl->m_confirm_target, NULL, ::mempool, conservative_estimate).GetFeePerK()) / 1000;

This comment has been minimized.

@ryanofsky

ryanofsky Jul 11, 2017

Contributor

In commit "Refactor to use CoinControl in GetMinimumFee and FeeBumper"

Maybe assert(coinControl->m_confirm_target) somewhere in this function. The event handling code which sets this seems a little precarious, so it'd be good to know if coinControl hasn't been initialized correctly.

This comment has been minimized.

@morcos

morcos Jul 12, 2017

Author Member

seems reasonable

This comment has been minimized.

@morcos

morcos Jul 12, 2017

Author Member

this one went away, but addressed a similar one in qt/sendcoinsdialog.cpp

std::max(CWallet::fallbackFee.GetFeePerK(), CWallet::GetRequiredFee(1000))) + "/kB");
ui->labelSmartFee->setText(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(), feeRate.GetFeePerK()) + "/kB");

if (feeCalc.reason == FeeReason::FALLBACK) {

This comment has been minimized.

@ryanofsky

ryanofsky Jul 11, 2017

Contributor

In commit "Make QT fee displays use GetMinimumFee instead of estimateSmartFee":

Is this sufficient to know fee estimation succeeded? For example would it be possible for estimate to fail, and then fallbackFee to be less than RequiredFee, so reason would be REQUIRED not FALLBACK?

Also can you expand commit description to say a little bit about why it's better to display minimum fee instead of estimated fee directly here. It seems perfectly reasonable, I'd just like to understand why the min fee wasn't being being used all along, and whether one value is clearly more useful than the other or if this is more intended to be a minor tweak that simplifies the code.

This comment has been minimized.

@morcos

morcos Jul 12, 2017

Author Member

I think the behavior in rare edge cases is maybe not precisely correct, but there is likely no harm in it. The only thing you are missing is a warning that fee estimation is not up to date yet, and before this PR that could already happen if there was a mempool min fee (in which case you wouldn't even get maxed with the fallback fee). After this PR, you only miss the warning if your fallback fee is less than mempool min fee or minRelayTxFee or minTxFee. The first case is a strict improvement over prior behavior. The second two cases can only result from very poor configuration of command line options. In short, I think it could be improved to report even more precise information to the user but it's outside the scope of what I wanted to do here.

I can expand commit description, but the idea is the displays should correspond to the fees that your actually about to put on the transaction if you click send. I think it's essentially a bug that it didn't do that before.

This comment has been minimized.

@ryanofsky

ryanofsky Jul 12, 2017

Contributor

I think the behavior in rare edge cases is maybe not precisely correct, but there is likely no harm in it.

I see. This is less significant than I thought because the fee is shown either way, just the labeling is different.

I can expand commit description, but the idea is the displays should correspond to the fees that your actually about to put on the transaction if you click send. I think it's essentially a bug that it didn't do that before.

Thanks, I didn't know if the motivation was to simplify code or if min fee was actually preferable to display. Fact that min fee is used to send the transaction definitely makes sense as a reason to display it.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Jul 12, 2017

utACK fb98c64

@morcos morcos force-pushed the morcos:improveWalletFeeLogic branch 2 times, most recently to c494da9 Jul 12, 2017

@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Jul 12, 2017

Addressed @ryanofsky feedback and added a commit to do proper bounds checking. boost::none's were just all removed in one commit for simplicity.

(feelogic.ver2) -> c494da9 (feelogic.ver2.squash)

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK c494da9. Changes were adding qt coincontrol asserts, removing uses of boost::none, and adding a new commit with more conf_target checking.

@jnewbery
Copy link
Member

jnewbery left a comment

utACK c494da9 . I'm not very familiar with the qt code, so I can't give you much feedback there, but the changes all look sensible.

A few nits. All could be addressed after merge (or left entirely).

if (feeCalc) feeCalc->reason = FeeReason::PAYTXFEE;
// Allow to override automatic min/max check over coin control instance
if (coin_control.fOverrideFeeRate) return fee_needed;
} else {

This comment has been minimized.

@jnewbery

jnewbery Jul 13, 2017

Member

nit: I think this is clearer as:

if {
    ...
} else if {
    ...
} else {
    ...
}

rather than:

if {
    ...
} else {
    if {
        ...
    } else {
        ...
    }
}

(to match the logical construction of the comment above)

This comment has been minimized.

@ryanofsky

ryanofsky Jul 13, 2017

Contributor

I wondered same thing initially, but I think it was written this way to unify the logic behind cases #2 and #4 which are very similar.

@@ -2469,7 +2469,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC

CReserveKey reservekey(this);
CWalletTx wtx;
if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, &coinControl, false))
if (!CreateTransaction(vecSend, wtx, reservekey, nFeeRet, nChangePosInOut, strFailReason, coinControl, false))

This comment has been minimized.

@jnewbery

jnewbery Jul 13, 2017

Member

supernit: braces


// Create change script that will be used if we need change
// TODO: pass in scriptChange instead of reservekey so
// change transaction isn't always pay-to-bitcoin-address
CScript scriptChange;

// coin control: send change to custom address
if (coinControl && !boost::get<CNoDestination>(&coinControl->destChange))
scriptChange = GetScriptForDestination(coinControl->destChange);
if (!boost::get<CNoDestination>(&coin_control.destChange))

This comment has been minimized.

@jnewbery

jnewbery Jul 13, 2017

Member

nit: braces


fee_needed = estimator.estimateSmartFee(target, feeCalc, conservative_estimate).GetFee(nTxBytes);
if (fee_needed == 0) {
// ... unless we don't have enough mempool data for estimatefee, then use fallbackFee

This comment has been minimized.

@jnewbery

jnewbery Jul 13, 2017

Member

The ... unless comment doesn't make sense now that you've removed the earlier use -txconfirmtarget to estimate... comment. Perhaps just remove the ... unless?

@@ -12,4 +12,7 @@
/** Generate blocks (mine) */
UniValue generateBlocks(std::shared_ptr<CReserveScript> coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript);

/** Check bounds on a command line confirm target */

This comment has been minimized.

@jnewbery

jnewbery Jul 13, 2017

Member

Why is this in rpc/mining.cpp? All of the calls to this function are in wallet/rpcwallet.cpp. Can you just put the function there?

Also the comment is a bit misleading - this function checks confirm target in RPCs, not command line arguments.

This comment has been minimized.

@morcos

morcos Jul 13, 2017

Author Member

#10707 adds it to the RPC calls in rpc/mining.cpp and rpcwallet.cpp already includes rpc/mining.h

@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Jul 13, 2017

@jnewbery See if you like the reorganization of GetMinimumFee and if so I'll squash both commits

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Jul 13, 2017

See if you like the reorganization of GetMinimumFee

I wasn't necessarily advocating for changing the order of the conditionals, just for flattening the nested ifs. I'm happy in either order.

Definitely like the extra comments referring to which of the 4 branches the blocks match up to.

The indentation on LL2942-2943 is incorrect. Fix that and I'm happy!

@morcos morcos force-pushed the morcos:improveWalletFeeLogic branch from 29fd69c to 759db91 Jul 14, 2017

@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Jul 14, 2017

addressed @jnewbery nits and slight refactor of logic in GetMinimumFee (same end result)
(feelogic.ver4) -> 759db91 (feelogic.ver4.squash)

morcos added some commits Jun 28, 2017

Refactor to use CoinControl in GetMinimumFee and FeeBumper
Improve parameter precedence in coin_control
Use CoinControl to pass custom fee setting from QT.
This fixes buggy behavior where we were temporarily setting and unsetting the
global payTxFee when trying to send a transaction with a custom fee from the
GUI. The previous behavior was inconsistent depending on the order of using the
RPC call settxfee and clicking various radio buttons in the sendcoinsdialog.
The new behavior is that transactions sent with the GUI will always use either
the smartfee slider value or the custom fee set on the GUI and they will not
affect the global defaults which are only for RPC and initial GUI values.
Make QT fee displays use GetMinimumFee instead of estimateSmartFee
Remove helper function (CalculateEstimateType) for determining whether
estimates should be conservative or not, now that this is only called
once from GetMinimumFee and incorporate the logic directly there.
Remove checking of mempool min fee from estimateSmartFee.
This check has been moved to the wallet logic GetMinimumFee. The rpc call to
estimatesmartfee will now no longer return a result maxed with the mempool min
fee, but automated fee calculations from the wallet will produce the same result
as before and coincontrol and sendcoins dialogs in the GUI will correctly
display the right prospective fee.

changes to policy/fees.cpp include a big whitespace indentation change.

@morcos morcos force-pushed the morcos:improveWalletFeeLogic branch from 759db91 to 11590d3 Jul 15, 2017

@morcos

This comment has been minimized.

Copy link
Member Author

morcos commented Jul 15, 2017

Unfortunately this required a bit of a rebase due to conflict with #10769 in qt/sendcoinsdialog.cpp
Other files had clean rebase.

@gmaxwell
Copy link
Member

gmaxwell left a comment

concept ACK

@sipa
Copy link
Member

sipa left a comment

Concept ACK. I very much like making everything uniformly use a coin control object for guiding estimation, and making the GUI's coin control independent from global settings.

Code looks good, but I haven't had time to review all logic changes.

@@ -815,7 +815,6 @@ UniValue estimatesmartfee(const JSONRPCRequest& request)
"\n"
"A negative value is returned if not enough transactions and blocks\n"
"have been observed to make an estimate for any number of blocks.\n"
"However it will not return a value below the mempool reject fee.\n"

This comment has been minimized.

@sipa

sipa Jul 15, 2017

Member

Perhaps worth pointing out that the result may not be sufficient to be acceptable for the mempool?

This comment has been minimized.

@morcos

morcos Jul 15, 2017

Author Member

I think we should at least flag the change in the release notes but I don't think that's necessarily something users need to worry about. In the event that the tx isn't accepted to the mempool it's still likely'ish to be confirmed within the target by being resubmitted after mempool min fee decays. Which makes me wonder if we should even be worrying about mempool min fee in the GetMinimumFee logic but enough changes for now.

// Allow to override automatic min/max check over coin control instance
if (coin_control.fOverrideFeeRate) return fee_needed;
}
else if (!coin_control.m_confirm_target && ::payTxFee != CFeeRate(0)) { // 3. TODO: remove magic value of 0 for global payTxFee

This comment has been minimized.

@sipa

sipa Jul 15, 2017

Member

Nit: else on the same line

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Jul 16, 2017

re-utACK 11590d3

@gmaxwell
Copy link
Member

gmaxwell left a comment

ACK

@@ -274,14 +268,10 @@ void SendCoinsDialog::on_sendButton_clicked()
CCoinControl ctrl;
if (model->getOptionsModel()->getCoinControlFeatures())
ctrl = *CoinControlDialog::coinControl;

This comment has been minimized.

@laanwj

laanwj Jul 17, 2017

Member

Thanks. Definitely seems an improvement with regard to CCoinControl handling.
Hopefully in a next PR we can finally get rid of this global object too.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jul 17, 2017

utACK 11590d3

@laanwj laanwj merged commit 11590d3 into bitcoin:master Jul 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 17, 2017

Merge #10706: Improve wallet fee logic and fix GUI bugs
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

sipa added a commit that referenced this pull request Jul 17, 2017

Merge #10707: Better API for estimatesmartfee RPC
06bcdb8 Convert named argument from nblocks to conf_target (Alex Morcos)
439c4e8 Improve api to estimatesmartfee (Alex Morcos)

Pull request description:

  Through 0.14 branch, the estimatesmartfee API was tagged "WARNING: This interface is unstable and may disappear or change!" and this warning is removed for 0.15, so any wanted API updates should happen now.

  The changes here are to make the additional parameter for conservative estimates a more general estimate_mode string , to omit the feerate and include an error string instead of returning -1 on error, and to do better parameter checking initially.

  ~It is only the last 2 commits, but it's built on #10706 and #10543~.

  See #10707 (comment) for renaming of nblocks argument to conf_target.  Will need to be included before string freeze.

  PR description edited for clarity

Tree-SHA512: 6d8ebee8bb410e2950ffd59663eebfed8d1611d995dc935bb91e430d9da7e2f306796f45631458376027d26341c660f09e825e61748103d2f2736ec6dc3df3ae

laanwj added a commit that referenced this pull request Jul 25, 2017

Merge #10799: Prevent user from specifying conflicting parameters to …
…fundrawtx

99c7fc3 Prevent user from specifying conflicting parameters to fundrawtx (Matt Corallo)

Pull request description:

  estimate_mode/conf_target both are overridden by feeRate, so should
  not be specified together with feeRate.

  Based on #10706

Tree-SHA512: 8ccd08575fd1f2a0d45112538ffbbc73983ee172963230b0cc7ac41d13c6f3c740917f82b212c41ded3a64d873452e7f2c7af49f3b47cab897f8e85117f21333

@jnewbery jnewbery referenced this pull request Aug 1, 2017

Closed

TODO for release notes 0.15.0 #9889

12 of 12 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.