/bitcoin

Add change output if necessary to reduce excess fee #10712

Merged
merged 2 commits into from Jul 11, 2017
+74 −46

Conversation

Reviewers

@sipa sipa

@TheBlueMatt TheBlueMatt

@promag promag

@instagibbs instagibbs

@ryanofsky ryanofsky

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
  0.15.0
9 participants
@morcos @promag @instagibbs @laanwj @sipa @TheBlueMatt @ryanofsky @fanquake @jonasschnelli
@morcos
Contributor

morcos commented Jun 30, 2017

This is an alternative to #10333

See commit messages.

The first commit is mostly code move, it just moves the change creation code out of the loop.

@instagibbs
@instagibbs

instagibbs reviewed Jun 30, 2017
View changes

imo this is superior in its simplicity and targeting the exact situation better over #10333 .

A bit worried about the failure case triggering anytime fee estimations change. I know you don't want to in this PR, but pulling out the estimation and getting a feerate that is reapplied in the main loop seems worth it, is easy to review, and will have to be done anyways for effective value work.

src/wallet/wallet.cpp
- // to be addressed so we avoid creating too small an output.
+ CAmount max_excess_fee = GetMinimumFee(change_prototype_size, currentConfirmationTarget, ::mempool, ::feeEstimator, nullptr) +
+ GetDustThreshold(change_prototype_txout, ::dustRelayFee);
+ // If we have no change and a big enough excess fee, then
@instagibbs

instagibbs Jun 30, 2017

Member

please move first explanation a few lines before for the "increase change" to appropriate spot

@ryanofsky

ryanofsky Jul 10, 2017

Contributor

please move first explanation a few lines before for the "increase change" to appropriate spot

I think order of the comments does makes sense. First comment describes the normal case, second comment describes unhandled special case, and third comment describes handled special case accompanied by implementation for that case.

I might move the code for the normal case above the code & comments for the two special cases, though this would increase the size of the diff. Just a thought, though.

@instagibbs

instagibbs Jul 11, 2017

Member

He moved it, the comment wasn't marked as resolved. Happy with current arrangement.

src/wallet/wallet.cpp
+ // change input. Only try this once.
+ if (nFeeRet > nFeeNeeded + max_excess_fee && nChangePosInOut == -1 && nSubtractFeeFromAmount == 0 && pick_new_inputs) {
+ pick_new_inputs = false;
+ nFeeRet = nFeeNeeded;
@instagibbs

instagibbs Jun 30, 2017

Member

this needs to be nFeeRet = nFeeNeeded + GetMinimumFee(change_prototype_size, currentConfirmationTarget, ::mempool, ::feeEstimator, nullptr) ; at a minimum, right?

nFeeNeeded at this point will not pay for the new change output.

@morcos

morcos Jun 30, 2017

Contributor

oops, correct

src/wallet/wallet.cpp
@@ -2754,6 +2771,11 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
}
break; // Done, enough fee included.
}
+ else if (!pick_new_inputs) {
+ // This shouldn't happen
@instagibbs

instagibbs Jun 30, 2017

Member

nit: slightly longer explanation for reader?

"This shouldn't happen, we should have had enough excess fee to pay for the newly created change output."

With fix on the nFeeRet, a very slight increase in fee between estimation calls(can that happen?) will result in this triggering.

@morcos

morcos Jun 30, 2017

Contributor

yes, i don't think that can happen (a change in fee estimation) but belts and suspenders.
In any case will be cleaned up post 0.15 with effective value logic and only calculating the needed fee rate once.

src/wallet/wallet.cpp
@@ -2774,6 +2773,8 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
}
}
+ if (nChangePosInOut == -1) reserveKey.ReturnKey(); // Return any reserved key if we don't have change
@instagibbs

instagibbs Jun 30, 2017

Member

looks like a capital "K" snuck in here.

@morcos

morcos Jun 30, 2017

Contributor

weird, i fixed that but must have not committed before push

@instagibbs

instagibbs Jun 30, 2017

Member

sorry to both you again on this but it appears to still be there

@morcos
Contributor

morcos commented Jun 30, 2017

Fixed bug and nits and squashed

@instagibbs I'm fine with doing the GetMinimumFeeRate refactor for 0.15, but I already did a lot of other work on GetMinimumFee in #10706 which I would really like to get in for 0.15, so I'm wary of piling on too many changes that touch the same lines of code and not being able to get them all in.

fanquake added the Wallet label Jul 1, 2017

@promag
Contributor

promag commented Jul 3, 2017

All return points should call reservekey.ReturnKey()?
@promag
Contributor

promag commented Jul 3, 2017

Reference #10247.
@promag

promag reviewed Jul 3, 2017
View changes

utACK.

IMO going for one last iteration and to have one more flag is a clear sign that a more thoughtful refactor should be done. Not that I'm against this change, but reading this code should be plain and easy.

@instagibbs
Member

instagibbs commented Jul 3, 2017

@promag this is a bugfix with limited scope. Switching to effective value calculation fixes this in a more natural fashion. See: #10360 and #10637
@promag

promag reviewed Jul 4, 2017 • edited
View changes

Nit, should variables be in camel or snake case? Also could you fix brace styles for the moved code?

morcos referenced this pull request Jul 5, 2017

Closed

[wallet] fee fixes: always create change, adjust value, and p… #10333

@morcos
Contributor

morcos commented Jul 5, 2017 • edited

@promag
New variables are supposed to be snake_case, did I miss one?
If there are any other changes, I can fix the braces, but I didn't touch the moved code.

As for calling ReturnKey, I suppose that does make sense, but the existing code did not do that, and I haven't materially changed that behavior. I'm happy to add it though if I should?
(Clarification: My only hesitation is that it looks to me like it's fine to call ReturnKey even if you haven't reserved one, but I'm just not that familiar with the code or it's design, so was hesitant to put in extra calls to ReturnKey where one might not have actually been reserved)
@promag
Contributor

promag commented Jul 6, 2017

@morcos another PR is fine. As @instagibbs said, this is a bugfix.

laanwj added to Blockers in High-priority for review Jul 6, 2017

@laanwj
Owner

laanwj commented Jul 6, 2017 • edited

Replaced #10333 with this one in "High priority for review" (reviewing a closed PR with high priority makes little sense).

jonasschnelli added this to the 0.15.0 milestone Jul 6, 2017

@ryanofsky

ryanofsky reviewed Jul 10, 2017
View changes

Wow, this code is ridiculously complicated. Pretty clean fix, though. Would be nice if we could mock SelectCoins and easily write unit tests for these cases.

utACK d64a4b9

src/wallet/wallet.cpp
- // to be addressed so we avoid creating too small an output.
+ CAmount max_excess_fee = GetMinimumFee(change_prototype_size, currentConfirmationTarget, ::mempool, ::feeEstimator, nullptr) +
+ GetDustThreshold(change_prototype_txout, ::dustRelayFee);
+ // If we have no change and a big enough excess fee, then
@ryanofsky

ryanofsky Jul 10, 2017

Contributor

please move first explanation a few lines before for the "increase change" to appropriate spot

I think order of the comments does makes sense. First comment describes the normal case, second comment describes unhandled special case, and third comment describes handled special case accompanied by implementation for that case.

I might move the code for the normal case above the code & comments for the two special cases, though this would increase the size of the diff. Just a thought, though.

src/wallet/wallet.cpp
+ // try to construct transaction again only without picking
+ // new inputs. We now know we only need the smaller fee
+ // (because of reduced tx size) and so we should add a
+ // change input. Only try this once.
@ryanofsky

ryanofsky Jul 10, 2017

Contributor

change output

src/wallet/wallet.cpp
@@ -2773,7 +2800,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
}
}
- if (nChangePosInOut == -1) reserveKey.ReturnKey(); // Return any reserved key if we don't have change
+ if (nChangePosInOut == -1) reservekey.ReturnKey(); // Return any reserved key if we don't have change
@ryanofsky

ryanofsky Jul 10, 2017

Contributor

In commit "Fix rare edge case of paying too many fees"

This needs to be moved to the previous commit for it to compile.

@morcos
Contributor

morcos commented Jul 11, 2017

Fixed commit history and fixed comment nit

git diff d64a4b9 533b3f0

-                    // change input. Only try this once.
+                    // change output. Only try this once.
src/wallet/wallet.cpp
@@ -2754,6 +2775,12 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
}
break; // Done, enough fee included.
}
+ else if (!pick_new_inputs) {
@sipa

sipa Jul 11, 2017

Owner

Nit: else on the same line as }

@instagibbs
Member

instagibbs commented Jul 11, 2017

utACK 533b3f0
@ryanofsky

ryanofsky reviewed Jul 11, 2017
View changes

utACK 533b3f0. Changes since last review were what Alex mentioned.

morcos added some commits Jun 30, 2017

@morcos morcos Only reserve key for scriptChange once in CreateTransaction 
This does not affect behavior but allows us to have access to an output to
scriptChange even if we currently do not have change in the transaction.
 253cd7e
@morcos morcos Fix rare edge case of paying too many fees when transaction has no ch… 
…ange.

Due to the iterative process of selecting new coins in each loop a new fee is
calculated that needs to be met each time.  In the typical case if the most
recent iteration of the loop produced a much smaller transaction and we have now
gathered inputs with too many fees, we can just reduce the change.  However in
the case where there is no change output, it is possible to end up with a
transaction which drastically overpays fees.  This commit addresses that case,
by creating a change output if the overpayment is large enough to support it,
this is accomplished by rerunning the transaction creation loop without
selecting new coins.

Thanks to instagibbs for working on this as well
0f402b9
@morcos
Contributor

morcos commented Jul 11, 2017 • edited

This was rebased to add 2 new arguments to GetMinimumFee line which were introduced in #10589

Will change again as it conflicts with #10706 on that same point (depending on which is merged first, but I think this is ready)

@laanwj laanwj merged commit 0f402b9 into bitcoin:master Jul 11, 2017
1 check was pending

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

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

@laanwj laanwj Merge #10712: Add change output if necessary to reduce excess fee 
0f402b9 Fix rare edge case of paying too many fees when transaction has no change. (Alex Morcos)
253cd7e Only reserve key for scriptChange once in CreateTransaction (Alex Morcos)

Pull request description:

  This is an alternative to #10333

  See commit messages.

  The first commit is mostly code move, it just moves the change creation code out of the loop.

  @instagibbs

Tree-SHA512: f16287ae0f0c6f09cf8b1f0db5880bb567ffa74a50898e3d1ef549ba592c6309ae1a9b251739f63a8bb622d48f03ce2dff9e7a57a6bac4afb4b95b0a86613ea8
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
e8b9523
@ryanofsky

ryanofsky reviewed Jul 11, 2017
View changes

utACK 0f402b9. Only difference is rebase.

morcos referenced this pull request Jul 11, 2017

Merged

Improve wallet fee logic and fix GUI bugs #10706

@sipa
Owner

sipa commented Jul 11, 2017

Posthumous utACK 0f402b9

laanwj removed from Blockers in High-priority for review Jul 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment