Add change output if necessary to reduce excess fee #10712

morcos · 2017-06-30T17:29:41Z

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

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.

instagibbs · 2017-06-30T17:42:38Z

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


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

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.

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

instagibbs · 2017-06-30T17:48:51Z

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


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.

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.

instagibbs · 2017-06-30T17:51:02Z

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


looks like a capital "K" snuck in here.

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

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

instagibbs · 2017-06-30T18:02:09Z

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;


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.

oops, correct

morcos · 2017-06-30T19:51:29Z

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.

promag · 2017-07-03T09:06:35Z

All return points should call reservekey.ReturnKey()?

promag · 2017-07-03T09:16:41Z

Reference #10247.

promag

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 · 2017-07-03T12:20:23Z

@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

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

morcos · 2017-07-05T16:36:27Z

@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 · 2017-07-06T00:10:21Z

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

laanwj · 2017-07-06T14:09:56Z

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

ryanofsky

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

ryanofsky · 2017-07-10T21:38:20Z

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


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

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

ryanofsky · 2017-07-10T22:03:14Z

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.


change output

ryanofsky · 2017-07-10T22:24:10Z

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


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.

morcos · 2017-07-11T00:21:01Z

Fixed commit history and fixed comment nit

git diff d64a4b9 533b3f0

-                    // change input. Only try this once.
+                    // change output. Only try this once.

TheBlueMatt

utACK 533b3f0

sipa · 2017-07-11T08:30:32Z

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) {


Nit: else on the same line as }

instagibbs · 2017-07-11T13:57:41Z

utACK 533b3f0

ryanofsky

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

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.

…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

morcos · 2017-07-11T16:28:51Z

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)

@instagibbs

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

ryanofsky

utACK 0f402b9. Only difference is rebase.

sipa · 2017-07-11T18:03:25Z

Posthumous utACK 0f402b9

@instagibbs

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 bitcoin#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 Signed-off-by: Pasta <pasta@dashboost.org>

@instagibbs

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 bitcoin#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 Signed-off-by: Pasta <pasta@dashboost.org>

@instagibbs

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 bitcoin#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 Signed-off-by: Pasta <pasta@dashboost.org>

@instagibbs

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 bitcoin#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 Signed-off-by: Pasta <pasta@dashboost.org>

@instagibbs

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 bitcoin#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 Signed-off-by: Pasta <pasta@dashboost.org>

@instagibbs

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 bitcoin#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 Signed-off-by: Pasta <pasta@dashboost.org>

@instagibbs

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 bitcoin#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 Signed-off-by: Pasta <pasta@dashboost.org>

instagibbs reviewed Jun 30, 2017

View reviewed changes

morcos force-pushed the addchangehwenoverpaying branch from e846170 to d64a4b9 Compare June 30, 2017 19:47

fanquake added the Wallet label Jul 1, 2017

promag reviewed Jul 3, 2017

View reviewed changes

promag reviewed Jul 4, 2017

View reviewed changes

morcos mentioned this pull request Jul 5, 2017

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

Closed

jonasschnelli added this to the 0.15.0 milestone Jul 6, 2017

ryanofsky reviewed Jul 10, 2017

View reviewed changes

morcos force-pushed the addchangehwenoverpaying branch from d64a4b9 to 533b3f0 Compare July 11, 2017 00:17

TheBlueMatt reviewed Jul 11, 2017

View reviewed changes

sipa reviewed Jul 11, 2017

View reviewed changes

ryanofsky reviewed Jul 11, 2017

View reviewed changes

morcos added 2 commits July 11, 2017 12:17

Only reserve key for scriptChange once in CreateTransaction

253cd7e

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.

morcos force-pushed the addchangehwenoverpaying branch from 533b3f0 to 0f402b9 Compare July 11, 2017 16:27

laanwj merged commit 0f402b9 into bitcoin:master Jul 11, 2017

ryanofsky reviewed Jul 11, 2017

View reviewed changes

morcos mentioned this pull request Jul 11, 2017

Improve wallet fee logic and fix GUI bugs #10706

Merged

ryanofsky mentioned this pull request May 12, 2021

Use effective values throughout coin selection #17331

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add change output if necessary to reduce excess fee #10712

Add change output if necessary to reduce excess fee #10712

Uh oh!

Conversation

morcos commented Jun 30, 2017

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jun 30, 2017

Uh oh!

promag commented Jul 3, 2017

Uh oh!

promag commented Jul 3, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 3, 2017

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jul 6, 2017

Uh oh!

laanwj commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 11, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 11, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 11, 2017

promag left a comment •

edited

Loading

morcos commented Jul 5, 2017 •

edited

Loading

laanwj commented Jul 6, 2017 •

edited

Loading

morcos commented Jul 11, 2017 •

edited

Loading