Add Binds, WhiteBinds, Whitelistedrange to CConnman::Options #10496

Minor nit, but what about switching to C++11 strongly typed enumerations (enum class) while we are at it?

Great idea, but unfortunately, bitwise operations don't work with those (unless you manually add those functions to the class) :(

See http://blog.bitwigglers.org/using-enum-classes-as-type-safe-bitmasks/

Ah, of course. Sorry!

This looks like a change in behavior when setting "-bind=foo -listen=0". Granted, that combination makes no sense.

I think we should probably just check for that combination, and exit with an error in that case.

Done.

See other comment. It doesn't make sense to have binds/whitebinds set at all if !fListen.

Done.

Mm, this moves the logic away from the caller and into CConnman, which is what we're trying to avoid by adding the heaps of options.

I think I'd prefer to see flags passed in along with CServices, so that CConnman can just do what it's told. So either a tiny struct, or just a pair.

Then, CConnman will try to bind the BF_EXPLICIT addresses, and if it fails, tries to bind the rest.

Also, in that case, the last failed bind wouldn't need to have BF_REPORT_ERROR set, we'd just notice the false return and act on it accordingly.

Sound reasonable?

Mm, this moves the logic away from the caller and into CConnman, which is what we're trying to avoid by adding the heaps of options. I think I'd prefer to see flags passed in along with CServices, so that CConnman can just do what it's told. So either a tiny struct, or just a pair.

The logic would be in CConnman all the same, just the data structure you pass the info with is changed from two vectors to one vector of pairs, which is equivalent. The code looks more complicated from what I can tell. Or did I misunderstand your request?

I added this in a separate commit to see the diff, I drop it again if needed.

Then, CConnman will try to bind the BF_EXPLICIT addresses, and if it fails, tries to bind the rest.

At the moment, the rest is only attempted if no explicit binds are specified. If they fail, all fails, which makes sense to me.

Also, in that case, the last failed bind wouldn't need to have BF_REPORT_ERROR set, we'd just notice the false return and act on it accordingly.

Act how, apart from reporting the error (which this flag does)?

Mm, this moves the logic away from the caller and into CConnman, which is what we're trying to avoid by adding the heaps of options. I think I'd prefer to see flags passed in along with CServices, so that CConnman can just do what it's told. So either a tiny struct, or just a pair.

The logic would be in CConnman all the same, just the data structure you pass the info with is changed from two vectors to one vector of pairs, which is equivalent. The code looks more complicated from what I can tell. Or did I misunderstand your request?

The logic would still be in CConnman, but the caller would have the ability to dictate exactly how the binds are setup. For example, there's no way to specify "listen on this address as well as the localhost fallbacks".

I think this is ok for now, though. We can add options as things come up.

For example, there's no way to specify "listen on this address as well as the localhost fallbacks".

But that wasn't possible before either.

Do you prefer the commit which passes the whitelist flag in the pair (https://github.com/bitcoin/bitcoin/pull/10496/commits/768c7f72d9fac542a45ad788713faa1a3f46897c), or should I drop it? I don't mind either way, but the new code seems less readable than the code before, but it is semantically exactly the same.

But that wasn't possible before either.

Sure, but we've been slowly moving these config options out to the caller so that less is hard-coded.

In the near-future, I'm looking forward to creating 2 CConnman instances and running them against each-other for testing. The more we can control via setup options, the better. I have no problem with adding those options incrementally, I'd just like to make sure we're not making future changes tougher.

As for the whitelist flag, either way is fine for this PR. Whichever you think is more readable. We'll expand on the options when needed.

:+1: I reverted it as I find the previous version more readable.

Well done! Thanks for using the local instance.

Looks like there's no way to run without listening now.

Sorry for not being more clear here: #10496 (review). I was suggesting that we shouldn't get into InitBinds in the first place if binding isn't allowed.

We'll either need to change this so that its caller uses the ugly global as before:

    if(fListen && !InitBinds(...)) {
        ...
    }

or add a flag for fallback connections, which InitBinds() only uses if nothing else is already bound.

Oops, was a bit too eager here. Re-added the fListen check as you suggested.

This strError param is clunky (my fault!). We should do away with it and just use clientInterface->InitError() directly. That can be done as a follow-up though.

Done.

s/GetBoolArg/gArgs.GetBoolArg/

GetBoolArg() is simply wrapping gArgs.GetBoolArg(), and both variants are already used in the function. Could make that consistent in a separate PR.

gArgs is the new (as of very recently) way of handling these args, it'd be helpful if new code didn't reintroduce the former usage as I assume the wrappers are intended to be temporary.