/bitcoin

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

Closed
wants to merge 2 commits into
from
+102 −85

Conversation

Reviewers

@gmaxwell gmaxwell

@achow101 achow101

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
  0.15.0
6 participants
@instagibbs @morcos @gmaxwell @achow101 @jonasschnelli @laanwj
@instagibbs
Member

instagibbs commented May 3, 2017 • edited

…rune later

This PR is an attempt to solve some of #10247 , first and foremost the case:

// TODO: The case where there is no change output remains
// to be addressed so we avoid creating too small an output

This is resolved by always assuming a change output exists, and rebalancing that value by the fee excess, and deleting when necessary. The logic is also simpler to me.

instagibbs changed the title from CreateTransction fee fixes: always create change, adjust value, and p… to [wallet] fee fixes: always create change, adjust value, and p… May 3, 2017

jonasschnelli added the Wallet label May 4, 2017

morcos referenced this pull request May 4, 2017

Open

cost too many fees? #10247

@instagibbs
Member

instagibbs commented May 8, 2017

Note: This is a "regression" due to MIN_FINAL_CHANGE being taken out of the calculation: https://github.com/bitcoin/bitcoin/pull/10333/files#diff-b2bb174788c7409b671c46ccc86034bdL2647

A bit confusing since the change is dropped when IsDust as before... this would be a good time to sync this behavior.

instagibbs referenced this pull request May 8, 2017

Open

[WIP] [Wallet] Target effective value during transaction creation #10360

laanwj added this to the 0.15.0 milestone May 11, 2017

@instagibbs
Member

instagibbs commented May 12, 2017

rebased, and included logic to attempt change larger than MIN_FINAL_CHANGE when possible. In the end some cases cannot be avoided, and just-over-dust change may be kept.
@instagibbs
Member

instagibbs commented Jun 21, 2017

suggested new strategy: instead of "trying one more time" when change is in middle-ground, cache the current transaction, increase the amount you're attempting to send, and try again. If you then run out of available coins, you simply return the cached version of the transaction.

laanwj added to Blockers in High-priority for review Jun 22, 2017

@instagibbs
Member

instagibbs commented Jun 23, 2017

Implemented the previously mentioned idea. Instead of doing "one more try" to get acceptable change, it caches the successful yet small change transaction and slowly grows the amount of coins it grabs until it creates an acceptable transaction, given reasonable chosen inputs this will result in a transaction with change larger than MIN_FINAL_CHANGE. If it fails and runs out of coins it returns the cached transaction.
src/wallet/wallet.cpp
+ // change dust or change larger than MIN_FINAL_CHANGE, it will
+ // return this transaction.
+ CMutableTransaction tx_cached;
+ bool have_cached_txn;
@achow101

achow101 Jun 23, 2017

Contributor

Shouldn't you assign have_cached_txn to false here as it is not assiged before it is used down below?

src/wallet/wallet.cpp
+ // Insert change txn at random position:
+ nChangePosInOut = GetRandInt(txNew.vout.size()+1);
+ }
+ else if ((unsigned int)nChangePosInOut > txNew.vout.size())
@achow101

achow101 Jun 23, 2017

Contributor

I don't see how this case matters. Wouldn't nChangePosInOut never be set to an out of range index?

@instagibbs

instagibbs Jun 23, 2017

Member

Caller may request a too-high change position. This is also a simple code move, not going to change this logic.

src/wallet/wallet.cpp
+ } else if (txNew.vout[nChangePosInOut].nValue < MIN_FINAL_CHANGE) {
+ // Save this transaction, use if we cannot get large-enough change
+ if (!have_cached_txn) {
+ tx_cached = txNew;
@achow101

achow101 Jun 23, 2017

Contributor

have_cached_txn should probably be set to true here as it is never actually set anywhere

@instagibbs instagibbs cache the first transaction that has change smaller than MIN_FINAL_CH… 
…ANGE

Raise the target fee, then try again. If the wallet has insufficient funds
to reach MIN_FINAL_CHANGE then use the cached transaction.
f8415fc
@instagibbs
Member

instagibbs commented Jun 24, 2017

fixed issues, thanks @achow101

morcos referenced this pull request Jun 30, 2017

Merged

Add change output if necessary to reduce excess fee #10712

@morcos
Contributor

morcos commented Jul 5, 2017

I think #10712 is preferable to this PR
@instagibbs
Member

instagibbs commented Jul 5, 2017

closing in favor of #10712

instagibbs closed this Jul 5, 2017

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

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment