This modifies the CreateTransaction loop in a way not remotely worth the complexity,
and includes an absurdly fragile test where I had to add a bunch of trace statements
and tweak pretty-much every single hardcoded number. Not to name names, but it was
Sjors. (In fairness, the PR is a pure simplification of the CreateTransaction logic,
and it wasn't hard to merge even. It was just the test that caused my grief.)
Adapting the "use a dummy CTxDestination in the case that we cannot retrieve one from
the wallet" logic to our `mapScriptChange` map was not trivial. On my first attempt I
incorrectly assigned a positive vout index to the dummy script, which caused us to
call `ReturnDestination` later on the (unused) dummy destination. This is harmless now,
but when descriptor wallets are introduced in #16528, they introduce an edge case where
returning a null destination can incorrectly mark the 0th key of a BIP32 range as
unused. This triggered a test failure much later, in #19504, which uses descriptor
wallets in fundrawtransaction. The bug was that we'd import a descriptor, mark the
first key as being used, lock the wallet, call `fundrawtransaction` on a transaction
that did not require change (incorrectly marking the first key as unused but leaving
it in the descriptor ScriptPubKeyMan's cache), then call `fundrawtransaction` again
on a transaction that *did* require change. The wallet would then incorrectly retrieve
the "unused" key from cache and use it for change, rather than correctly failing and
advising the user that it could not produce change with a locked wallet and empty
keypool. This was not a fun bug to track down.
Another interesting observation is that branch-and-bound uses the CT size-overestimate
for change when trying to create changeless outputs, while our normal dust detection
uses Core's unchanged "an output is 133 bytes" logic. So when BnB is used we're willing
to delete a far bigger change output than we are when we don't use BnB.
Lest you think this works in Core, they're also inconsistent because BnB uses a
normal fee estimate for gauging change cost, while non-BnB uses the discardfee rate.
My advice is to hold your nose, pull stuff in from Core as it comes in, and thanks
to Andy's efforts things are getting better. Don't bother reviewing this too closely.
