Clean up mapArgs and mapMultiArgs Usage

TheBlueMatt commented at 3:16 am on November 30, 2016: member

This fixes several concurrency issues in mapArgs and mapMultiArgs by making mapMultiArgs const-access-only and hiding mapArgs behind the accessors we already have (+IsArgSet).

fanquake added the label Refactoring on Nov 30, 2016

in src/util.h: in 98468faedf outdated

40@@ -41,8 +41,8 @@ class CTranslationInterface
41     boost::signals2::signal<std::string (const char* psz)> Translate;
42 };
43 
44-extern std::map<std::string, std::string> mapArgs;
45-extern std::map<std::string, std::vector<std::string> > mapMultiArgs;
46+extern std::map<std::string, std::vector<std::string> > _mapMultiArgs;
47+static const std::map<std::string, std::vector<std::string> >& mapMultiArgs = _mapMultiArgs;

sipa commented at 3:24 am on November 30, 2016:

I believe this instantiates a mapMultiArgs variable into every .cpp file (all initialized as the same reference).

This will work, I think:

 0util.cpp:
 1 map<string, vector<string> > _mapMultiArgs;
 2+static std::map<std::string, std::vector<std::string> > mapMultiArgs_;
 3+const std::map<std::string, std::vector<std::string> >& mapMultiArgs = mapMultiArgs;
 4 bool fDebug = false;
 5
 6util.h:
 7-extern std::map<std::string, std::vector<std::string> > _mapMultiArgs;
 8-static const std::map<std::string, std::vector<std::string> >& mapMultiArgs = _mapMultiArgs;
 9+extern const std::map<std::string, std::vector<std::string> >& mapMultiArgs;
10 extern bool fDebug;

TheBlueMatt force-pushed on Nov 30, 2016

gmaxwell commented at 6:11 am on November 30, 2016: contributor

Concept ACK.

jonasschnelli commented at 7:41 am on November 30, 2016: contributor

Nice. Concept ACK

sipa commented at 0:32 am on December 1, 2016: member

utACK 69337dcab02e2e972711781ae1fba7f541d67b17, but needs rebase.

Also, maybe the “Fix calculation of number of bound sockets to use” commit should be moved to a separate PR; maybe we want to backport it?

TheBlueMatt force-pushed on Dec 1, 2016

TheBlueMatt commented at 2:57 am on December 1, 2016: member

Rebased and opened #9253, which this is now dependant on (the socket count fix).

instagibbs commented at 4:57 pm on December 1, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/9243/commits/14e3df21e067f41b6f624ea7ee25601f87267faa

jtimon commented at 9:27 am on December 2, 2016: contributor

This seems incompatible with #8994 , if there’s any suggestions there, specifically regarding 5cc59df0dce56c9393b2bd1af773d708bb76a3cb (aka “Globals: Allow to parse arguments without implicitly using the argMap globa"l), that would be welcomed.

in src/init.cpp: in 14e3df21e0 outdated

858@@ -859,7 +859,9 @@ bool AppInitParameterInteraction()
859     }
860 
861     // Make sure enough file descriptors are available
862-    int nBind = std::max((int)mapArgs.count("-bind") + (int)mapArgs.count("-whitebind"), 1);
863+    int nBind = std::max(
864+                (mapMultiArgs.count("-bind") ? mapMultiArgs.at("-bind").size() : 0) +
865+                (mapMultiArgs.count("-whitebind") ? mapMultiArgs.at("-whitebind").size() : 0), size_t(1));

MarcoFalke commented at 4:27 pm on December 2, 2016:

github-web-view-Nit: Needs this patch removed after c1a52276848d8caa9a9789dff176408c1aa6b1ed

(I think you can push an empty commit and then remove it by force push, to get this removed from the web view)

MarcoFalke commented at 5:01 pm on December 2, 2016: member

utACK 14e3df21e067f41b6f624ea7ee25601f87267faa

TheBlueMatt commented at 6:17 pm on December 2, 2016: member

@jtimon I think for things which are their own classes, they should either have the arguments they care about passed into the constructor, or passed in when they need them (eg the way mempool limiting is done now). The concept of a map from arguments to values is inherintly Bitcoin Core-specific, so inherintly global.

jtimon commented at 10:05 pm on December 2, 2016: contributor

I think for things which are their own classes, they should either have the arguments they care about passed into the constructor, or passed in when they need them (eg the way mempool limiting is done now)

I think it is better to discuss the different options for Chainparams in #8994 (I’ve been trying to do something similar for the never created policy class for very long too), I will answer to this there.

The concept of a map from arguments to values is inherently Bitcoin Core-specific, so inherently global.

Huh? Anything that is specific to Bitcoin Core is “inherently global”? Globals are widely recognized in software engineering as a bad practice, and are never strictly necessary. They may be the cleanest or more convenient solution to a specific problem in a given code base, so it may make sense to argue in favor of them in terms of convenience or, in this case maybe, also in terms of encapsulation. But saying any variable in any system is “inherently global” makes no sense to me at all.

TheBlueMatt commented at 10:19 pm on December 2, 2016: member

By “inherently global”, I mean that eg mapArgs probably should not exist in anything like libbitcoinconsensus, and, generally, can just be a global that is queries when things are initialized and then the parameters fed into the object which cares about them.

jtimon commented at 10:39 pm on December 2, 2016: contributor

Right, I agree it should not exist in libbitcoinconsensus. And agree mapArgs and mapMultiArgs should be initialized once while initializing and then never edit them again. I don’t see the problem with passing mapArgs and/or mapMultiArgs as const to a class constructor for future extensibility, but if you think that passing the only arguments they care about is cleaner, that’s not what you are doing in https://github.com/bitcoin/bitcoin/pull/9243/commits/e87822043d4118d1b02d066ee213d341af3ff4e9 you’re making the zmq code rely more on globals for no good reason that I can see.

If mapArgs and mapMultiArgs are initialized once and afterwards only accessed as const (perhaps similarly to what chainparams.o is doing with InitParams and const CChainParams& Params() or what is being done in https://github.com/bitcoin/bitcoin/pull/9243/commits/31f576bd8ac35a912ef5f5315d39c0faeadb1f4b ), I don’t see how could there be any concurrency issues or why a new lock would be needed.

I also don’t understand why this PR isn’t consistent in its treatment of mapArgs and mapMultiArgs (one is hidden, the other can be accessed as const).

Although I like many of the changes in this PR, until I understand some of the decisions, concept NACK.

TheBlueMatt commented at 10:49 pm on December 2, 2016: member

The contents of e87822043d4118d1b02d066ee213d341af3ff4e9 are less about passing around a map (which is, itself, kinda insane) and more about cleaning up the insane argument-parsing monstrosity in the zmq stuff.

As for generally, I dont get your crusade against globals. Some things, like arguments, simply /are/ globals. In places where we want isolated chunks for testing, those isolated chunks shouldn’t be given a list of all arguments passed to the program, they should be given the arguments that they care about.

Note that mapArgs stuff isnt exclusively accessed in a clean order…theres a bunch of Qt initialization that I dont know when its called (and I believe there is some incorrect behavior in init as well), nor do I care - adding a lock has almost 0 overhead (we’re already inexing into a map anyway) when there is no contention and its much more obviously correct and harder to fuck up in the future.

As for mapArgs/mapMultiArgs - I could add accessors to mapMultiArgs, but it has to return the vector of arguments anyway, so I see little point. Feel free to clean it up in a separate PR.

jtimon commented at 11:35 pm on December 2, 2016: contributor

The contents of e878220 are less about passing around a map (which is, itself, kinda insane) and more about cleaning up the insane argument-parsing monstrosity in the zmq stuff.

I don’t see how using something implicitly is any more sane than passing it explicitly.

As for generally, I dont get your crusade against globals. Some things, like arguments, simply /are/ globals.

No, global variables are never strictly necessary, they are always a design choice (which doesn’t mean is always a bad choice, but it usually is in my experience).

In places where we want isolated chunks for testing, those isolated chunks shouldn’t be given a list of all arguments passed to the program, they should be given the arguments that they care about.

If you want to test the old CZMQNotificationInterface::Create(&args) you can create your own map and only setting the values that it cares about. If you want to impede it from using other args that it should not care about, you are not achieving that with the hidden global accessible from everywhere.

Note that mapArgs stuff isn’t exclusively accessed in a clean order […]

Ok, that’s a good reason to use the lock, at least as a short term clean solution. I was forgetting qt setting args after initialization, thanks. Concept ACK.

TheBlueMatt commented at 11:45 pm on December 2, 2016: member

The problem with the ZMQ stuff is less that its being passed the list of all arguments in the world, and more that it bends over backwards to use map operations to look up 4 elements…

Note that this PR isnt about creating an “ideal design” it is about fixing actual race conditions (ok, admittedly that arent a big deal) in mapArgs accessing as used in the codebase today. Eventually mapArgs will only exist in init.cpp and the relevant args passed around to different submodules as neccessary, but for now, this makes it obviously safe.

jtimon commented at 5:10 am on December 3, 2016: contributor

re zmq: I understand you considered two different “problems”:

Not considering the code for retrieving the values elegant. I guess of the ugly use of const_iterator just as the simplest way to comply with the parameter map being const instead of using the global (not the implementor, see below). Or maybe the “hackish” way in which the keys for the args are constructed using string manipulation, but you’re maintaining that in https://github.com/bitcoin/bitcoin/pull/9243/commits/e87822043d4118d1b02d066ee213d341af3ff4e9#diff-ae0c52c7aa40615ed94ef5603b768010R45
You also consider that passing all the args to the constructor was a bad idea in the first place. In defense of the implementor, IIRC it was me who asked for this. Because I think it’s the way forwards for zmq, chainparams, “policy” and wallet not to depend on util globals (which is not a priority since none of these parts should be used in the consensus module anyway). In this reward I’ve been looking for feedback for very long and very much welcome it in any of these PRs: #8994 #7820 #7557 (comment) #6068 #5595 But if I understood your ideal design described here, functions should get all parameters they care about explicitly. Without necessarily disagreeing (I think it depends on the case), in the case of CZMQNotificationInterface::Create that would imply to take 4 values for “std::string address” in the loop explicitly, which is something you’re not doing in this PR (not asking you to do so, just pointing it out). What I would be very thankful of if you could change in that commit is replacing “garbage” for something more specific about what you don’t like about it, if not for the shake of clarity, just out of respect for other contributors. Don’t want to sound patronizing, I believe I make this same mistake very often when finding things I don’t like and I don’t know the history about.

Note that this PR isnt about creating an “ideal design” it is about fixing actual race conditions in mapArgs accessing as used in the codebase today.

I’m very sorry for overreacting by misinterpreting this as an “ideal design” without joining what I understand as an ongoing discussion. For example, @MarcoFalke seems to be ok with passing the argsMap explicitly where we can, as long as the 2 maps were hidden behind a class first, which I wasn’t very fond of at first, but I think makes sense. Also, somewhere in #5595 had an idea for making a dynamic qt form for configuring policy from an argMap and something like my std::vector<std::pair<std::string, std::string> > CDefaultPolicy::GetOptionsHelp() (something like in https://github.com/bitcoin/bitcoin/pull/7820/files#diff-d22bc3e058f8982972e2eb381a1df668R261 but with a single thing looks ridiculous). I loved that idea, and thought it was extensible for other things (and I’m all fine with these new options requiring restart). Anyway, as said, I’m happy to keep discussing this but this is not the place and I already disrupted the thread of this PR enough.

(ok, admittedly that arent a big deal)

Just as globals, “known” race conditions make reasoning about how the system works harder. I agree that removing those, no matter how “big deal” they are, should take preference over reducing the use of globals. I just wasn’t convinced we couldn’t do it without the new lock with a few more lines, but now I’m convinced adding the lock is the most straightforward way to solve the issue.

Eventually mapArgs will only exist in init.cpp and the relevant args passed around to different submodules as neccessary, but for now, this makes it obviously safe.

I agree that in many cases this is what makes the most sense, but we shouldn’t disregard “shortcuts” like Consensus::Params, chainparams, policy…(again, probably not to discuss in this PR, I’m making too much noise in this PR already).

gmaxwell commented at 9:16 am on December 3, 2016: contributor

Needs rebase.

TheBlueMatt force-pushed on Dec 3, 2016

TheBlueMatt commented at 10:06 pm on December 3, 2016: member

Rebased. @jtimon I suppose we dont really strongly disagree on the goal, though maybe disagree on how to get there. Can we leave that for another PR and take this as-is to fix the issues that we have now?

gmaxwell commented at 7:19 am on December 5, 2016: contributor

ACK.

laanwj commented at 7:33 am on December 5, 2016: member

ACK on wrapping the accesses.

I’m surprised that a lock is needed here, I assumed the arguments were read-only after initialization.

Anyhow there shouldn’t be any argument accesses in an inner loop where overhead matters so adding the lock just in case makes sense, I guess.

The problem with the ZMQ stuff is less that its being passed the list of all arguments in the world, and more that it bends over backwards to use map operations to look up 4 elements…

Yes the ZMQ argument parsing loop is silly. I fixed that once, as part of the mempool notifications, but it never got merged.

MarcoFalke commented at 9:33 pm on December 6, 2016: member

reACK 62b8ba5f0a3d22e0dfcef9a3e1e7fa5c3f48b5ce

laanwj commented at 12:41 pm on December 8, 2016: member

Needs a rebase in DoS_tests.

Remove arguments to ParseConfigFile c8042a48f0

Fix non-const mapMultiArgs[] access after init.

Swap mapMultiArgs for a const-reference to a _mapMultiArgs which is
only accessed in util.cpp

2b5f085ad1

Introduce (and use) an IsArgSet accessor method 0cf86a6678

TheBlueMatt force-pushed on Dec 24, 2016

TheBlueMatt commented at 2:36 am on December 24, 2016: member

Rebased.

in src/test/util_tests.cpp: in c69c9a7f33 outdated

18@@ -19,6 +19,8 @@
19 
20 using namespace std;
21 
22+extern map<string, string> mapArgs;

jtimon commented at 9:02 am on December 24, 2016:

If we’re going to still have it in the tests, why not keep having it declared in one place (util.h) instead of at least 4 places?

TheBlueMatt commented at 4:11 pm on December 24, 2016:

Removed it from three of the four by adding a ForceSetArg method for testing.

jtimon commented at 9:03 am on December 24, 2016: contributor

Yes the ZMQ argument parsing loop is silly

Note that silly loop hasn’t been removed here. I still don’t see what “garbage” is being cleaned up in https://github.com/bitcoin/bitcoin/pull/9243/commits/61444322f36fdbf9ff5ba1a3d8382f9c892da8d8

utACK 87a6f1d (although as previously said I still don’t like some commit messages and I left another comment).

Get rid of mapArgs direct access in ZMQ construction 71fde5563b

TheBlueMatt force-pushed on Dec 24, 2016

TheBlueMatt commented at 4:12 pm on December 24, 2016: member

OK, retitled that commit to make you happy, but at some point someone needs to go back and clean up that stuff.

Un-expose mapArgs from utils.h 4cd373aea8

Lock mapArgs/mapMultiArgs access in util 4e048142a5

TheBlueMatt force-pushed on Dec 24, 2016

jtimon commented at 8:14 pm on December 24, 2016: contributor

I would have been happy with just renaming the commit message, or explaining further what garbage were you cleaning, but thanks. No opposition on removing getArg from the loop, but you maintained the loop and everything which was supposedly wrong with it (except from passing mapArgs as arguments, which is the art you wanted to remove I think).

Add a ForceSetArg method for testing c2f61bebb1

TheBlueMatt force-pushed on Dec 27, 2016

TheBlueMatt commented at 12:53 pm on December 27, 2016: member

Fixed @dcousens’ comments from #9419 (review)

dcousens commented at 1:15 pm on December 27, 2016: contributor

utACK c2f61be, and thanks for linking me @TheBlueMatt

dcousens approved

sipa commented at 6:07 pm on December 27, 2016: member

utACK c2f61bebb190258753714b29ab2041e80651cec9

sipa merged this on Dec 27, 2016

sipa closed this on Dec 27, 2016

sipa referenced this in commit 7aa700424c on Dec 27, 2016

jtimon commented at 11:21 pm on December 27, 2016: contributor

Thank you, this will hirt less the faster it is imo.

On 27 Dec 2016 19:17, “Pieter Wuille” notifications@github.com wrote:

Merged #9243 https://github.com/bitcoin/bitcoin/pull/9243.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9243#event-905908891, or mute the thread https://github.com/notifications/unsubscribe-auth/AA9jSu-ji0J3ZdPwz4-8SGdcbYpBK71Lks5rMVZVgaJpZM4K_wjb .

codablock referenced this in commit aff4b6d2c5 on Jan 18, 2018

andvgal referenced this in commit 3573d3fd31 on Jan 6, 2019

CryptoCentric referenced this in commit 06474e04c6 on Feb 26, 2019

furszy referenced this in commit de5b240881 on Oct 28, 2020

LarryRuane referenced this in commit 13d73ad9c2 on Feb 24, 2021

LarryRuane referenced this in commit f60964c374 on Apr 1, 2021

zkbot referenced this in commit 1b5f17c900 on Apr 1, 2021

zkbot referenced this in commit 80e66e7daa on Apr 2, 2021

DrahtBot locked this on Sep 8, 2021

Clean up mapArgs and mapMultiArgs Usage #9243