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

benma · 2017-06-01T10:43:54Z

Subset of #9897, with the goal to decouple command line arg parsing and make AppInitMain() smaller.

benma · 2017-06-01T10:45:31Z

practicalswift · 2017-06-01T10:48:25Z

src/net.cpp

@@ -64,6 +64,14 @@
 #endif
 #endif

+/** Used to pass flags to the Bind() function */
+enum BindFlags {


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!

ryanofsky

utACK 7313bfec5354f44571929249ee3672b8c38675a3. This seems like an improvement, though I also think it might be a little better if the connman options were vectors of strings, so AppInit would only be dealing with translating arguments (and the loops could be eliminated), and connman would deal with everything network related (lookups).

benma · 2017-06-01T15:36:05Z

@ryanofsky thanks. I actually did it like this in a first iteration, but changed it because I figured that the arguments should be evaluated as much as possible before entering ConnMan. After all, ConnMan doesn't particularly care about the command line string representation.

It was a bit weird having the error messages that relate to -whitelist, -bind, -whitebind still in ConnMan as opposed to where the arguments are actually parsed.

Edit: I see the point though that Lookup() etc. are network functions and shouldn't be used in init.cpp. But refactoring all of that sounds like a separate issue, so I propose to defer this to another PR.

theuni · 2017-06-05T20:09:26Z

I believe this is OK. I have a PR coming up which (after lots of thought) moves resolving into its own class, of which CConnman is one user. This way other layers are able to use the resolver without necessarily having to go through CConnman.

So, concept ACK. Reviewing.

theuni

Concept ACK. Thanks for working on this!

theuni · 2017-06-05T20:17:56Z

src/init.cpp

@@ -1644,6 +1594,28 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
    connOptions.nMaxOutboundTimeframe = nMaxOutboundTimeframe;
    connOptions.nMaxOutboundLimit = nMaxOutboundLimit;

+    if (gArgs.IsArgSet("-bind")) {


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.

theuni · 2017-06-05T20:19:09Z

src/net.cpp

+}
+
+bool CConnman::InitBinds(const std::vector<CService>& binds, const std::vector<CService>& whiteBinds) {
+    if (fListen) {


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

theuni · 2017-06-05T20:30:59Z

src/net.cpp

+bool CConnman::InitBinds(const std::vector<CService>& binds, const std::vector<CService>& whiteBinds) {
+    if (fListen) {
+        bool fBound = false;
+        for (const auto& addrBind : binds) {


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 (768c7f7), 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.

👍 I reverted it as I find the previous version more readable.

sipa · 2017-06-12T23:22:50Z

@theuni Can you check whether your requested changes are addressed?

theuni · 2017-06-13T18:31:10Z

src/net.cpp

+        return false;
+    std::string strError;
+    if (!BindListenPort(addr, strError, (flags & BF_WHITELIST) != 0)) {
+        if ((flags & BF_REPORT_ERROR) && clientInterface) {


Well done! Thanks for using the local instance.

theuni · 2017-06-13T18:41:20Z

src/net.cpp

+            flags |= BF_WHITELIST;
+        fBound |= Bind(addrBind.first, flags);
+    }
+    if (binds.empty()) {


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

Sorry for not being more clear here: #10496 (comment). 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.

theuni · 2017-06-13T18:46:30Z

src/net.cpp

+    clientInterface = connOptions.uiInterface;
+
+    if (!InitBinds(connOptions.vBinds)) {
+        strError = _("Failed to listen on any port. Use -listen=0 if you want this.");


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.

theuni · 2017-06-13T18:57:20Z

src/init.cpp

+    size_t nUserBind =
+        (gArgs.IsArgSet("-bind") ? gArgs.GetArgs("-bind").size() : 0) +
+        (gArgs.IsArgSet("-whitebind") ? gArgs.GetArgs("-whitebind").size() : 0);
+    if (nUserBind != 0 && !GetBoolArg("-listen", DEFAULT_LISTEN)) {


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.

theuni · 2017-06-13T19:05:35Z

looks good aside from those few issues.

benma · 2017-06-13T22:39:56Z

@theuni thanks for the feedback. I added a new commit for easier review, I will squash in the end.

benma · 2017-06-13T23:30:38Z

@theuni addressed the GetBoolArgs issue and squashed.

Part of a series of changes to clean up the instantiation of connman by decoupling the command line arguments.

Part of a series of changes to clean up the instantiation of connman by decoupling the command line arguments. We also now abort with an error when explicit binds are set with -listen=0.

benma · 2017-06-15T21:11:17Z

Rebased.

theuni · 2017-06-20T22:09:17Z

utACK 07b2afe

ryanofsky

utACK 07b2afe. There were many changes since last review, all discussed above.

laanwj · 2017-06-26T13:03:33Z

utACK 07b2afe

…tions 07b2afe add Binds, WhiteBinds to CConnman::Options (Marko Bencun) ce79f32 add WhitelistedRange to CConnman::Options (Marko Bencun) Tree-SHA512: c23a6f317c955338af531fa3e53e3c42e995f88c6e1939bbc2ad119fa5b786c54b3dad3d2e9b3f830b7292c0c63a02fcff66a89907d0fa8d7c83aefade01af45

…man::Options 07b2afe add Binds, WhiteBinds to CConnman::Options (Marko Bencun) ce79f32 add WhitelistedRange to CConnman::Options (Marko Bencun) Tree-SHA512: c23a6f317c955338af531fa3e53e3c42e995f88c6e1939bbc2ad119fa5b786c54b3dad3d2e9b3f830b7292c0c63a02fcff66a89907d0fa8d7c83aefade01af45

fanquake added the Refactoring label Jun 1, 2017

practicalswift reviewed Jun 1, 2017

View reviewed changes

fanquake requested a review from theuni June 1, 2017 15:00

ryanofsky reviewed Jun 1, 2017

View reviewed changes

ryanofsky mentioned this pull request Jun 1, 2017

AppInitMain: split initialization of Connman into a new function #9897

Closed

theuni suggested changes Jun 5, 2017

View reviewed changes

benma force-pushed the connman_options branch from 7313bfe to 521bfa7 Compare June 10, 2017 14:13

theuni reviewed Jun 13, 2017

View reviewed changes

benma force-pushed the connman_options branch from 521bfa7 to 2f4e787 Compare June 13, 2017 22:39

benma force-pushed the connman_options branch from 2f4e787 to 8065316 Compare June 13, 2017 23:29

Marko Bencun added 2 commits June 15, 2017 23:06

add WhitelistedRange to CConnman::Options

ce79f32

Part of a series of changes to clean up the instantiation of connman by decoupling the command line arguments.

add Binds, WhiteBinds to CConnman::Options

07b2afe

Part of a series of changes to clean up the instantiation of connman by decoupling the command line arguments. We also now abort with an error when explicit binds are set with -listen=0.

benma force-pushed the connman_options branch from 8065316 to 07b2afe Compare June 15, 2017 21:11

theuni approved these changes Jun 20, 2017

View reviewed changes

ryanofsky reviewed Jun 21, 2017

View reviewed changes

laanwj merged commit 07b2afe into bitcoin:master Jun 26, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

benma commented Jun 1, 2017

Uh oh!

benma commented Jun 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

benma commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Jun 5, 2017

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Jun 13, 2017

Uh oh!

benma commented Jun 13, 2017

Uh oh!

benma commented Jun 13, 2017

Uh oh!

benma commented Jun 15, 2017

Uh oh!

theuni commented Jun 20, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 26, 2017

Uh oh!

Uh oh!

benma commented Jun 1, 2017 •

edited

Loading