One place to find / include cs_main.
No more:
// Actually declared in validation.cpp; can’t include because of circular dependency. extern RecursiveMutex cs_main;
Ultimately, no more need to include validation.h (which also includes (heavy/boost filled) txmempool.h) everywhere for cs_main. See #26087 for another example of why that is useful.
15+ * AcceptToMemoryPool. See CTxMemPool::cs comment for details.
16+ *
17+ * The transaction pool has a separate lock to allow reading from it and the
18+ * chainstate at the same time.
19+ */
20+static RecursiveMutex cs_main;
In commit “refactor: add cs_main.h” (7e2a68204ecbc15a6f661075a34300fe5c101232)
I think this still needs to be extern not static. Otherwise there would be a different instance of cs_main variable in each translation unit and locking one of the cs_main variables would not lock all the other ones.
extern causes link errors on multiple platforms, will take another look.
Making this
externcauses link errors on multiple platforms, will take another look.
Right you would also need to add RecursiveMutex cs_main; without extern to a .cpp file like cs_main.cpp, or undelete it from validation.cpp to actually instantiate the variable and create the linker symbol. Declaring a variable without static or extern will creates a linker symbol. Declaring a variable extern just tells the compiler the type of the variable and makes it look for an exisiting symbol to import. Declaring a variable static makes the compiler create a private variable in the current translation unit without importing or exporting any symbol.
extern RecursiveMutex cs_main;?
How is a one-line include different/better than a simple one-line extern RecursiveMutex cs_main;?
It’s probably neither much different or better, just one way of acheiving the same thing. The point is to remove unecessary/heavy includes, which aside from improving compilation time / # of units to rebuilt due to source changes, also enables cleanup like #26087. If we’d rather use extern RecursiveMutex cs_main; everywhere, and then drop unused validation includes when possible, that could also be fine, I’m not tied to a specific approach.
that should later-on be removed anyway.
If it’s going to be removed later on, having a single file/include to delete is at least somewhat convenient. Although similarly so to removing a number of extern RecursiveMutex....
The point is to remove unecessary/heavy includes
I don’t see how this pull achieves any of that or would help there. You are adding the cs_main.h include to 40 files, all of which either have validation already included or had their extern cs_main replaced.
How is a one-line include different/better than a simple one-line
extern RecursiveMutex cs_main;?It’s probably neither much different or better, just one way of acheiving the same thing. The point is to remove unecessary/heavy includes, which aside from improving compilation time / # of units to rebuilt due to source changes
Agree with the goal of cleaning up includes, and also agree neither extern RecursiveMutex cs_main; nor #include <cs_main.h> seem much better or worse. Would probably favor the extern over the include for a smaller diff and a little less complexity, unless there are other reasons for wanting the include.
Concept ACK. For files that need it, I think it makes sense to be able to include just cs_main without having to know its signature or pull in a header that thinks it knows it.
And giving it a home in its own compilation unit means that we no longer have to pull in the arbitrary object where it resides just to get it linked. So I think this is a nice cleanup.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
CCheckQueue RAII-styled by hebasto)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
#include seems preferable to redeclaring it directly; not really seeing the need to add the #include to cpp files that already #include <validation.h> though. Adding #include <cs_main.h> // IWYU pragma: export in validation.h should satisfy IWYU, I think?
int main() {std::regex{std::string{}};}, let alone on our code base as a whole. So I don’t think IWYU should be used as a reason for this change.
So I don’t think IWYU should be used as a reason for this change.
I was assuming silencing IWYU complaints was the only reason for adding for adding cs_main.h where validation.h was already included.
17@@ -18,6 +18,7 @@
18 #include <chainparams.h>
19 #include <consensus/validation.h>
20 #include <core_io.h>
21+#include <cs_main.h>
I think it makes more sense to have #include <validation.h> imply that cs_main is available – in the short term it reduces the diff here, but even in the long term, I think we can expect cs_main to become a validation specific lock.
Here’s what that would look like: https://github.com/ajtowns/bitcoin/commit/f4d29cf1fe2edeaf2892fe69502fd7dcab9f5a5d – it just takes the current patch, drops the redundant cs_main includes and adds an IWYU pragma to prevent iwyu from suggesting they get added back.
#include things than re-declare them, and since we use cs_main in places where we don’t need all of validation, it makes sense to pull that out into a separate module.
@fanquake did you check if any #include <validation.h> could be removed?
EDIT: irrelevant after #26251 (review)
638@@ -638,6 +639,7 @@ libbitcoin_common_a_SOURCES = \
639 compressor.cpp \
640 core_read.cpp \
641 core_write.cpp \
642+ cs_main.cpp \
Yes, bitcoin_kernel is linked into bitcoin_node according to https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md#dependencies
cs_main shouldn’t be put into any other binary or library
Yes, bitcoin_kernel is linked into bitcoin_node
Yes, but we don’t build the kernel by default. So if you drop cs_main out of the util lib, and try and ./autogen && ./configure && make this PR, it wont link.
kernel/cs_main.cpp in the same section that holds validation.cpp. This should compile and link, no?
kernel/cs_main.*, and have that exist in the node lib. I have moved things around now.
Co-authored-by: Anthony Towns <aj@erisian.com.au>