Could return true here, return false if the loop exhausts, to avoid duplicating the condition.

Done

Could collapse this to boolean fallback at this point.

Done

eligibilty_filter typo

Fixed

non_input_fees? It's not "not fees", rather "non-input". Easier for me to read.

Done

I think this new logic means that non-BnB will be tried more often? Instead of trying all variants of BnB(6 confirms, 1 confirm, small chain, etc), we seem to be trying 6 confirms for BnB, then 6 confirms for non-BnB

Hmm, yes, that appears to be what is happening. I guess this depends on whether we want to select an exact match for coins with less confirmatoins, or whether we want confirmations over exact match.

This is a good point. Perhaps a solution is to add a use_bnb argument to SelectCoinsMinConf (but not as part CoinSelectionParams), and then have a sequence of attempts with and without use_bnb in SelectCoins?

I prefer the behavior in master due to privacy reasons of change-less transactions.

I have implemented this change.

maybe rename to first_output

Done

parenthesis please

Done

nit: Better to document in the header, and use doxygen comment /**

Done

_bnb probably no longer relevant

Removed here and elsewhere

Same

nit: explicit precedence would be nice here: (out.nInputBytes < 0 || !use_effective)

I thought I did this earlier. Must have gotten lost in a rebase. Fixed.

Doesn't look changed fyi.

How about now?

nit: whitespace

Done

nit: Better to squash this change into the commit that calls for it

Done

Two comments:

• Requesting new entropy for each element being sorted may be slow (each call to GetRandInt invokes OpenSSL for randomness) (this also applies to the code you copied, but you're going to remove that anyway).
• It's overkill to permute all elements, as you're likely only going to use a few.

For the first, create a FastRandomContext and use that in std::shuffle (rather than random_shuffle):

std::shuffle(utxo_pool.begin(), utxo_pool.end(), FastRandomContext());`

For the second, you can permute on the fly; for example:

FastRandomContext ctx;
for (size_t i = 0; i < utxo_pool.size; ++i) {
    size_t pos = i + ctx.randrange(utxo_pool.size() - i); // randomly pick one of the remaining elements
    std::swap(utxo_pool[i], utxo_pool[pos]);
    const CInputCoin& utxo = utxo_pool[i];
    ...
}

Done

This is a good point. Perhaps a solution is to add a use_bnb argument to SelectCoinsMinConf (but not as part CoinSelectionParams), and then have a sequence of attempts with and without use_bnb in SelectCoins?

Typo: decrase

Fixed

Travis fails because of size (instead of size()).

Oops.