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

instagibbs · 2017-05-03T21:13:48Z

…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 · 2017-05-08T17:33:37Z

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 · 2017-05-12T14:18:50Z

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 · 2017-06-21T13:57:38Z

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.

…rune later

instagibbs · 2017-06-23T04:48:13Z

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.

achow101 · 2017-06-23T19:28:07Z

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;


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

achow101 · 2017-06-23T19:28:35Z

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;


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

achow101 · 2017-06-23T19:30:38Z

src/wallet/wallet.cpp

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


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

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

…ANGE Raise the target fee, then try again. If the wallet has insufficient funds to reach MIN_FINAL_CHANGE then use the cached transaction.

instagibbs · 2017-06-24T13:15:02Z

fixed issues, thanks @achow101

gmaxwell

Concept ACK

morcos · 2017-07-05T15:25:59Z

I think #10712 is preferable to this PR

instagibbs · 2017-07-05T15:31:46Z

closing in favor of #10712

@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

@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 changed the title ~~CreateTransction fee fixes: always create change, adjust value, and p…~~ [wallet] fee fixes: always create change, adjust value, and p… May 3, 2017

jonasschnelli added the Wallet label May 4, 2017

morcos mentioned this pull request May 4, 2017

cost too many fees? #10247

Closed

instagibbs mentioned this pull request May 8, 2017

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

Closed

laanwj added this to the 0.15.0 milestone May 11, 2017

instagibbs force-pushed the fixfeefinal branch from 10b6776 to a19220a Compare May 12, 2017 14:16

instagibbs force-pushed the fixfeefinal branch from a19220a to 364241d Compare June 22, 2017 21:34

CreateTransction fee fixes: always create change, adjust value, and p…

fd20e0b

…rune later

instagibbs force-pushed the fixfeefinal branch from 364241d to 5cd5609 Compare June 22, 2017 21:40

achow101 reviewed Jun 23, 2017

View reviewed changes

instagibbs force-pushed the fixfeefinal branch from 405cf10 to 01b2071 Compare June 24, 2017 02:47

cache the first transaction that has change smaller than MIN_FINAL_CH…

f8415fc

…ANGE Raise the target fee, then try again. If the wallet has insufficient funds to reach MIN_FINAL_CHANGE then use the cached transaction.

instagibbs force-pushed the fixfeefinal branch from 01b2071 to f8415fc Compare June 24, 2017 13:12

gmaxwell reviewed Jun 29, 2017

View reviewed changes

morcos mentioned this pull request Jun 30, 2017

Add change output if necessary to reduce excess fee #10712

Merged

instagibbs closed this Jul 5, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

instagibbs commented May 3, 2017 •

edited

Loading

Uh oh!

instagibbs commented May 8, 2017

Uh oh!

instagibbs commented May 12, 2017

Uh oh!

instagibbs commented Jun 21, 2017

Uh oh!

instagibbs commented Jun 23, 2017

Uh oh!

achow101 Jun 23, 2017

Uh oh!

achow101 Jun 23, 2017

Uh oh!

achow101 Jun 23, 2017

Uh oh!

instagibbs Jun 23, 2017

Uh oh!

instagibbs commented Jun 24, 2017

Uh oh!

gmaxwell left a comment

Uh oh!

morcos commented Jul 5, 2017

Uh oh!

instagibbs commented Jul 5, 2017

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

instagibbs commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented May 8, 2017

Uh oh!

instagibbs commented May 12, 2017

Uh oh!

instagibbs commented Jun 21, 2017

Uh oh!

instagibbs commented Jun 23, 2017

Uh oh!

achow101 Jun 23, 2017

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 23, 2017

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 23, 2017

Choose a reason for hiding this comment

Uh oh!

instagibbs Jun 23, 2017

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jun 24, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 5, 2017

Uh oh!

instagibbs commented Jul 5, 2017

Uh oh!

Uh oh!

instagibbs commented May 3, 2017 •

edited

Loading