…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.
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.
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.
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.
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.
2757 | + reservekey.ReturnKey(); 2758 | + // If larger than dust, but still small, increase fee target and try again 2759 | + } else if (txNew.vout[nChangePosInOut].nValue < MIN_FINAL_CHANGE) { 2760 | + // Save this transaction, use if we cannot get large-enough change 2761 | + if (!have_cached_txn) { 2762 | + tx_cached = txNew;
have_cached_txn should probably be set to true here as it is never actually set anywhere
2505 | + // falling between uneconomical dust and MIN_FINAL_CHANGE. If the wallet 2506 | + // runs out of funds trying to find a transaction that has pruneable 2507 | + // change dust or change larger than MIN_FINAL_CHANGE, it will 2508 | + // return this transaction. 2509 | + CMutableTransaction tx_cached; 2510 | + 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?
2702 | + if (nChangePosInOut == -1) 2703 | + { 2704 | + // Insert change txn at random position: 2705 | + nChangePosInOut = GetRandInt(txNew.vout.size()+1); 2706 | + } 2707 | + 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.
Raise the target fee, then try again. If the wallet has insufficient funds
to reach MIN_FINAL_CHANGE then use the cached transaction.
fixed issues, thanks @achow101
Concept ACK
closing in favor of #10712