Concept ACK.

Nice.
Concept ACK

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?

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

utACK 14e3df2

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.

utACK 14e3df21e067f41b6f624ea7ee25601f87267faa

@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.

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.

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.

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 e878220 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 31f576b ), 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.

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.

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.

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.

re zmq: I understand you considered two different "problems":

1. 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 e878220#diff-ae0c52c7aa40615ed94ef5603b768010R45

2. 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: Testchains: Introduce custom chain whose constructor...  #8994 Consensus: Policy: Move CFeeRate out of consensus module and create CPolicy interface #7820 Encapsulate options for mempool policy #7557 (comment) Policy: Create CPolicy interface and CStandardPolicy class implementing it #6068 Policy: Create CPolicy interface and CStandardPolicy class implementing it #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).

Needs rebase.

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?

ACK.

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.

reACK 62b8ba5f0a3d22e0dfcef9a3e1e7fa5c3f48b5ce

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).

Fixed @dcousens' comments from #9419 (comment)

utACK c2f61be, and thanks for linking me @TheBlueMatt

utACK c2f61be