[wallet] Move min_depth and max_depth to coin control #15906

It's ugly if code calling this with nMinDepth (eg, other PRs or out-of-tree patches) can magically have it become nMaxDepth instead.

Maybe nMaxDepth ought to be moved to CCoinControl also? (Or removed? Is there a use case for this?)

What is the rationale for making CCoinControl a required parameter?

If CCoinControl is made required, it should probably be in a separate commit, or at least the rationale documented in the commit message.

It seems fragile to rely on the fact that the fallback defaults are the same if no cc is passed or a default constructed cc is passed.

It's ugly if code calling this with nMinDepth (eg, other PRs or out-of-tree patches) can magically have it become nMaxDepth instead.

agreed.

Maybe nMaxDepth ought to be moved to CCoinControl also?

was thinking the same. any suggestions on how to go about answering that? or should I just go for it and pull nMaxDepth out?

Or removed? Is there a use case for this?

looks like the listunspent rpc accepts a nMaxDepth & passes that through here

hm, so these are the tradeoffs I see-

optional: closer to current function signature, less duplicated code constructing default cc, don't have this problem with callers

required: expectations are more explicit from the POV of a caller, not a big of deal to make the change since this function isn't invoked frequently.

I don't feel familiar enough with the codebase / C++ to have much of an opinion here. Marco, how strongly do you hold the preference for required? Seems like luke-jr & empact both prefer keeping it optional.

Oh sure, go for what luke and empact prefer. I consider it a stylistic preference.

It's ugly if code calling this with nMinDepth (eg, other PRs or out-of-tree patches) can magically have it become nMaxDepth instead.

Could solve this by swapping the arguments bool fOnlySafe and CCoinControl of the function, so that all out-of-tree patches have a (silent) merge conflict and won't compile anymore?

Alternative is to also pass nMaxDepth through with the coincontrol.

pulled onto coincontrol

This in invalid because the var is out of scope when you dereference it below, suspect that's behind the AddressSanitizer failure on CI.

That said, I would keep coinControl optional, as the default value of 0 for nMinDepth is equivalent to the absent / null condition for coinControl, in that it never fails the check. Would simplify diff overall.

style-nit: Add those {}

        if (nDepth < min_depth || nDepth > nMaxDepth) {

Instead of running this once per loop, could move it to the beginning of the function (out of the loop). That would also avoid hiding the default value 0 deep in the function body.

const int min_depth{coinControl?coinControl->m_min_depth:0};

nit, could drop { }?

I'd rather drop the =. Note that the {} enable the -Wnarrowing compiler check

Or keep it as is, since the compiler checks seems to work even with the =

hm. In a simple example c++ program, I get a compiler error when I drop the =. so I added it.

Huh? Both pass for me

$ echo 'int main(){ const int min_depth = { true ? 1 : 0 };}' | g++ -x c++ -
$ echo 'int main(){ const int min_depth   { true ? 1 : 0 };}' | g++ -x c++ -

FYI, even without optimizations these two are equivalent https://godbolt.org/z/Q4sjcx

Perhaps for another PR but could be nice to document the "why" behind the value of 9999999 and hoist it to a static constant:

src/wallet/coincontrol.h:44:   int m_max_depth{9999999};
src/wallet/rpcwallet.cpp:2822: int nMaxDepth = 9999999;
src/wallet/wallet.cpp:2436:    const int max_depth = { coinControl ? coinControl->m_max_depth : 9999999 };