Skip to content

Conversation

@fanquake
Copy link
Member

Rather than including validation.h, which ultimately means needing boost via txmempool.h, include primitives/block.h for CBlock, and remove validation.h, as we can get cs_main from node/blockstorage.h.

Rather than including validation.h, which ultimately means needing boost
via txmempool.h, include primitives/block.h for CBlock, and remove
validation.h, as we can get cs_main from node/blockstorage.h.
@theuni
Copy link
Member

theuni commented Sep 14, 2022

Nice. ACK a10df7c.

As a follow-up, I wonder if we should just suck it up and move cs_main to its own minimal header/cpp rather than copy/paste the declaration of it all over the place and having it defined in an arbitrary file. That's very c-like :(

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a10df7c, tested on Linux x86_64 using theuni's patch with depends.

#include <zmq.h>

#include <validation.h>
#include <primitives/block.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iwuy reports:

zmq/zmqnotificationinterface.cpp should add these lines:
#include <assert.h>                   // for assert
#include <map>                        // for map, operator==, _Rb_tree_iterator
#include <string>                     // for string, char_traits, operator+
#include <utility>                    // for move, pair
#include <vector>                     // for vector
#include "logging.h"                  // for LogPrintf_, ZMQ, LogPrint
#include "primitives/block.h"         // for CBlock
#include "zmq/zmqabstractnotifier.h"  // for CZMQAbstractNotifier, CZMQNotif...

zmq/zmqnotificationinterface.cpp should remove these lines:
- #include <validation.h>  // lines 11-11

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to leave this as-is for now.

#include <rpc/server.h>
#include <streams.h>
#include <util/system.h>
#include <validation.h> // For cs_main
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This change will confuse iwuy, as now it reports:

The full include-list for zmq/zmqpublishnotifier.cpp:
...
#include <validation.h>              // for cs_main

@fanquake fanquake merged commit a688ff9 into bitcoin:master Sep 16, 2022
@fanquake fanquake deleted the libbitcoin_zmq_prune_boost_cppflags branch September 16, 2022 13:54
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 20, 2022
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jan 16, 2023
282019c refactor: add kernel/cs_main.* (fanquake)

Pull request description:

  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 bitcoin#26087 for another example of why that is useful.

ACKs for top commit:
  ajtowns:
    ACK 282019c

Tree-SHA512: 142835b794873e7a09c3246d6101843ae81ec0c6295e6873130c98a2abfa5f7282748d0f1a37237a779cc71c3bc0a75d03b20313ef5398c83d4814215cbc8287
@bitcoin bitcoin locked and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants