txauthor: Only get change address when needed.#2638
Conversation
Previously a call to NewUnsignedTransaction would derive a new change address immediately. This would unnecessarily increment the last used address index in cases where the transaction could not be adequately funded, and even in cases where a change address is not required or used. Now the change address is only derived when it is definitely needed.
There was a problem hiding this comment.
Nice. I know that I've encountered this (or something very similar) many times.
I'm not super familiar with this code, but the change looks right. The primary concern would be hitting it multiple times since it's in a for loop, but there are no remaining calls to continue after that if statement until it returns, so it can only be hit once.
I see that it can still theoretically burn an address if the error check after that (aka if len(changeScript)... ) were to fail, but that seems implausible unless there is a bug in the script generation itself.
While it isn't something that would fit in this PR at all, I have long suspected that wallet would generally be more robust if it were to make all address generation transactional (by transaction here, I mean in the sense of atomicity as opposed to Decred transactions). By that I mean having something like a pool of unused addresses that is drawn from and each address is marked as "under consideration" while a transaction over it is active, much in the same way dcrd works with utxos marking them spent in a view until that view is either discarded or committed.
Then, code can request an address and do whatever it needs to do to based on a script with the actual address (or anything else involving an address for that matter), but if there is an error, the transaction would fail and remove the "under consideration" flag leaving the address in place for the next call. When the transaction succeeds, it would be removed from the pool, latest address index would be updated (if > current index), and a new one takes its place (under the same lock, of course).
Previously a call to NewUnsignedTransaction would derive a new change address immediately. This would unnecessarily increment the last used address index in cases where the transaction could not be adequately funded, and even in cases where a change address is not required or used.
Now the change address is only derived when it is definitely needed.