Refactor CWallet::AvailableCoins method signature

[furszy]

Purely an aesthetic improvement, no functional changes. Combining parameters that should had been coded into CoinControl wrapper class in the first place.

[random-zebra]

This does not compile.
The signature of CreateTransaction needs to be updated in WalletModel::prepareTransaction too.

[furszy]

yeah, doing it. I forgot that my environment had qt disabled.

[furszy]

updated.

[random-zebra]

Aside from a couple bugs, I am not sure about the concept.
It seems to make the code more complex, because these flags don't really belong to the coinControl object.
Most of the times these are used when CC is null.
And when coinControl is not null, and we have coins selected, then we don't care about specific flags because we simply want to skip everything except the selection, at this line:

if (fCoinsSelected && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(COutPoint((*it).first, i)))
    continue;

[furszy]

Aside from a couple bugs, I am not sure about the concept.
It seems to make the code more complex, because these flags don't really belong to the coinControl object.

Essentially made this to be able to add more features and don't continue extending the method signature. It has so many arguments with default values that it's easy to call this method wrongly without noticing it.

The two options to solve this: (1) use the CoinControl wrapper, (2) create a new struct for the extra arguments that we are passing one by one to AvailableCoins (and similar methods).

I'm more inclined to the first one because the CoinControl was essentially built for that reason. Add certain coins filtering capabilities in a single object and not pass them as arguments (making the AvailableCoins and CreateTransaction method signatures unreadable as well).
But.. as a middle step, could move it to the second option if this change adds an unnecessary complexity for now. This PR is a bridge to get other things implemented, not trying to change much things here as there are going to be a lot of changes in the area already.

[random-zebra]

CoinControl was essentially built for that reason

Well, the purpose of CCoinControl is to provide an interface to manage a set of outpoints (setSelected). Using it to store the arguments for AvailableCoins is a stretch in my opinion.
In fact, thinking about it (idea for future refactoring), we shouldn't even call AvailableCoins when there are coins selected in coinControl. We should just get the COutputs from the COutpoints directly.

Anyway the added complexity of (1) is kinda highlighted by these sneaky bugs on something that was supposed to be "purely an aesthetic improvement, no functional changes".

So, yeah, I would definitely prefer (2) over (1).
Or, as alternative solution for the time being, simply don't use default arguments and explicitely add them at every call.

[furszy]

In fact, thinking about it (idea for future refactoring), we shouldn't even call AvailableCoins when there are coins selected in coinControl. We should just get the COutputs from the COutpoints directly.

Indeed, good idea. That will make a nice speed difference in the tx creation process for users with lot of txs that manually selected the utxo to spend.

#-

Going with (2) for now then. It's more organized and readable.

[furszy]

Closing this one, made a complete overhaul of the area in #1977