Skip to content

Commit

Permalink
Merge #18698: Make g_chainman internal to validation
Browse files Browse the repository at this point in the history 
fab6b9d validation: Mark g_chainman DEPRECATED (MarcoFalke)
fa1d97b validation: Make ProcessNewBlock*() members of ChainstateManager (MarcoFalke)
fa24d49 validation: Make PruneOneBlockFile() a member of ChainstateManager (MarcoFalke)
fa84b1c validation: Make LoadBlockIndex() a member of ChainstateManager (MarcoFalke)
fa05fdf net: Pass chainman into PeerLogicValidation (MarcoFalke)
fa7b626 node: Add chainman alias for g_chainman (MarcoFalke)

Pull request description:

  The global `g_chainman` has recently been introduced in #17737. The chainstate manager is primarily needed for the assumeutxo feature, but it can also simplify testing in the future.

  The goal of this pull is to make the global chainstate manager internal to validation, so that all external code does not depend on globals and that unit or fuzz tests can pass in their (potentially mocked) chainstate manager.

  I suggest reviewing the pull request commit-by-commit. It should be relatively straightforward refactoring that does not change behavior at all.

ACKs for top commit:
  ryanofsky:
    Code review ACK fab6b9d. Had to be rebased but still looks good

Tree-SHA512: dcbf114aeef4f8320d466369769f22ce4dd8f46a846870354df176c3de9ff17c64630fbd777e7121d7470d7a8564ed8d37b77168746e8df7489c6877e55d7b4f
  • Loading branch information
MarcoFalke committed May 23, 2020
2 parents 492cdc5 + fab6b9d commit 793e0ff
Show file tree
Hide file tree
Showing 17 changed files with 161 additions and 134 deletions.
36 changes: 20 additions & 16 deletions src/init.cpp
View file
Expand Up @@ -244,9 +244,9 @@ void Shutdown(NodeContext& node)
}

// FlushStateToDisk generates a ChainStateFlushed callback, which we should avoid missing
{
if (node.chainman) {
LOCK(cs_main);
for (CChainState* chainstate : g_chainman.GetAll()) {
for (CChainState* chainstate : node.chainman->GetAll()) {
if (chainstate->CanFlushToDisk()) {
chainstate->ForceFlushStateToDisk();
}
Expand All @@ -271,9 +271,9 @@ void Shutdown(NodeContext& node)
// up with our current chain to avoid any strange pruning edge cases and make
// next startup faster by avoiding rescan.

{
if (node.chainman) {
LOCK(cs_main);
for (CChainState* chainstate : g_chainman.GetAll()) {
for (CChainState* chainstate : node.chainman->GetAll()) {
if (chainstate->CanFlushToDisk()) {
chainstate->ForceFlushStateToDisk();
chainstate->ResetCoinsViews();
Expand All @@ -299,7 +299,8 @@ void Shutdown(NodeContext& node)
globalVerifyHandle.reset();
ECC_Stop();
node.args = nullptr;
if (node.mempool) node.mempool = nullptr;
node.mempool = nullptr;
node.chainman = nullptr;
node.scheduler.reset();

try {
Expand Down Expand Up @@ -689,7 +690,7 @@ static void CleanupBlockRevFiles()
}
}

static void ThreadImport(std::vector<fs::path> vImportFiles)
static void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFiles)
{
const CChainParams& chainparams = Params();
util::ThreadRename("loadblk");
Expand Down Expand Up @@ -741,9 +742,9 @@ static void ThreadImport(std::vector<fs::path> vImportFiles)
// scan for better chains in the block chain database, that are not yet connected in the active best chain

// We can't hold cs_main during ActivateBestChain even though we're accessing
// the g_chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
// the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
// the relevant pointers before the ABC call.
for (CChainState* chainstate : WITH_LOCK(::cs_main, return g_chainman.GetAll())) {
for (CChainState* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
BlockValidationState state;
if (!chainstate->ActivateBestChain(state, chainparams, nullptr)) {
LogPrintf("Failed to connect best block (%s)\n", state.ToString());
Expand Down Expand Up @@ -1377,8 +1378,11 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
// which are all started after this, may use it from the node context.
assert(!node.mempool);
node.mempool = &::mempool;
assert(!node.chainman);
node.chainman = &g_chainman;
ChainstateManager& chainman = EnsureChainman(node);

node.peer_logic.reset(new PeerLogicValidation(node.connman.get(), node.banman.get(), *node.scheduler, *node.mempool));
node.peer_logic.reset(new PeerLogicValidation(node.connman.get(), node.banman.get(), *node.scheduler, *node.chainman, *node.mempool));
RegisterValidationInterface(node.peer_logic.get());

// sanitize comments per BIP-0014, format user agent and check total size
Expand Down Expand Up @@ -1557,7 +1561,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
const int64_t load_block_index_start_time = GetTimeMillis();
try {
LOCK(cs_main);
g_chainman.InitializeChainstate();
chainman.InitializeChainstate();
UnloadBlockIndex();

// new CBlockTreeDB tries to delete the existing file, which
Expand All @@ -1578,7 +1582,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
// block file from disk.
// Note that it also sets fReindex based on the disk flag!
// From here on out fReindex and fReset mean something different!
if (!LoadBlockIndex(chainparams)) {
if (!chainman.LoadBlockIndex(chainparams)) {
if (ShutdownRequested()) break;
strLoadError = _("Error loading block database");
break;
Expand Down Expand Up @@ -1612,7 +1616,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)

bool failed_chainstate_init = false;

for (CChainState* chainstate : g_chainman.GetAll()) {
for (CChainState* chainstate : chainman.GetAll()) {
LogPrintf("Initializing chainstate %s\n", chainstate->ToString());
chainstate->InitCoinsDB(
/* cache_size_bytes */ nCoinDBCache,
Expand Down Expand Up @@ -1667,7 +1671,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
bool failed_rewind{false};
// Can't hold cs_main while calling RewindBlockIndex, so retrieve the relevant
// chainstates beforehand.
for (CChainState* chainstate : WITH_LOCK(::cs_main, return g_chainman.GetAll())) {
for (CChainState* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
if (!fReset) {
// Note that RewindBlockIndex MUST run even if we're about to -reindex-chainstate.
// It both disconnects blocks based on the chainstate, and drops block data in
Expand All @@ -1692,7 +1696,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
try {
LOCK(cs_main);

for (CChainState* chainstate : g_chainman.GetAll()) {
for (CChainState* chainstate : chainman.GetAll()) {
if (!is_coinsview_empty(chainstate)) {
uiInterface.InitMessage(_("Verifying blocks...").translated);
if (fHavePruned && gArgs.GetArg("-checkblocks", DEFAULT_CHECKBLOCKS) > MIN_BLOCKS_TO_KEEP) {
Expand Down Expand Up @@ -1798,7 +1802,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
nLocalServices = ServiceFlags(nLocalServices & ~NODE_NETWORK);
if (!fReindex) {
LOCK(cs_main);
for (CChainState* chainstate : g_chainman.GetAll()) {
for (CChainState* chainstate : chainman.GetAll()) {
uiInterface.InitMessage(_("Pruning blockstore...").translated);
chainstate->PruneAndFlush();
}
Expand Down Expand Up @@ -1841,7 +1845,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
vImportFiles.push_back(strFile);
}

threadGroup.create_thread(std::bind(&ThreadImport, vImportFiles));
threadGroup.create_thread([=, &chainman] { ThreadImport(chainman, vImportFiles); });

// Wait for genesis block to be processed
{
Expand Down
25 changes: 13 additions & 12 deletions src/net_processing.cpp
View file
Expand Up @@ -1153,9 +1153,10 @@ static bool BlockRequestAllowed(const CBlockIndex* pindex, const Consensus::Para
(GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, consensusParams) < STALE_RELAY_AGE_LIMIT);
}

PeerLogicValidation::PeerLogicValidation(CConnman* connmanIn, BanMan* banman, CScheduler& scheduler, CTxMemPool& pool)
PeerLogicValidation::PeerLogicValidation(CConnman* connmanIn, BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool)
: connman(connmanIn),
m_banman(banman),
m_chainman(chainman),
m_mempool(pool),
m_stale_tip_check_time(0)
{
Expand Down Expand Up @@ -1738,7 +1739,7 @@ inline void static SendBlockTransactions(const CBlock& block, const BlockTransac
connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCKTXN, resp));
}

bool static ProcessHeadersMessage(CNode* pfrom, CConnman* connman, CTxMemPool& mempool, const std::vector<CBlockHeader>& headers, const CChainParams& chainparams, bool via_compact_block)
bool static ProcessHeadersMessage(CNode* pfrom, CConnman* connman, ChainstateManager& chainman, CTxMemPool& mempool, const std::vector<CBlockHeader>& headers, const CChainParams& chainparams, bool via_compact_block)
{
const CNetMsgMaker msgMaker(pfrom->GetSendVersion());
size_t nCount = headers.size();
Expand Down Expand Up @@ -1798,7 +1799,7 @@ bool static ProcessHeadersMessage(CNode* pfrom, CConnman* connman, CTxMemPool& m
}

BlockValidationState state;
if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast)) {
if (!chainman.ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast)) {
if (state.IsInvalid()) {
MaybePunishNodeForBlock(pfrom->GetId(), state, via_compact_block, "invalid header received");
return false;
Expand Down Expand Up @@ -2081,7 +2082,7 @@ static void ProcessGetCFCheckPt(CNode* pfrom, CDataStream& vRecv, const CChainPa
connman->PushMessage(pfrom, std::move(msg));
}

bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, CTxMemPool& mempool, CConnman* connman, BanMan* banman, const std::atomic<bool>& interruptMsgProc)
bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, ChainstateManager& chainman, CTxMemPool& mempool, CConnman* connman, BanMan* banman, const std::atomic<bool>& interruptMsgProc)
{
LogPrint(BCLog::NET, "received: %s (%u bytes) peer=%d\n", SanitizeString(msg_type), vRecv.size(), pfrom->GetId());
if (gArgs.IsArgSet("-dropmessagestest") && GetRand(gArgs.GetArg("-dropmessagestest", 0)) == 0)
Expand Down Expand Up @@ -2848,7 +2849,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec

const CBlockIndex *pindex = nullptr;
BlockValidationState state;
if (!ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) {
if (!chainman.ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) {
if (state.IsInvalid()) {
MaybePunishNodeForBlock(pfrom->GetId(), state, /*via_compact_block*/ true, "invalid header via cmpctblock");
return true;
Expand Down Expand Up @@ -2992,15 +2993,15 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
} // cs_main

if (fProcessBLOCKTXN)
return ProcessMessage(pfrom, NetMsgType::BLOCKTXN, blockTxnMsg, nTimeReceived, chainparams, mempool, connman, banman, interruptMsgProc);
return ProcessMessage(pfrom, NetMsgType::BLOCKTXN, blockTxnMsg, nTimeReceived, chainparams, chainman, mempool, connman, banman, interruptMsgProc);

if (fRevertToHeaderProcessing) {
// Headers received from HB compact block peers are permitted to be
// relayed before full validation (see BIP 152), so we don't want to disconnect
// the peer if the header turns out to be for an invalid block.
// Note that if a peer tries to build on an invalid chain, that
// will be detected and the peer will be banned.
return ProcessHeadersMessage(pfrom, connman, mempool, {cmpctblock.header}, chainparams, /*via_compact_block=*/true);
return ProcessHeadersMessage(pfrom, connman, chainman, mempool, {cmpctblock.header}, chainparams, /*via_compact_block=*/true);
}

if (fBlockReconstructed) {
Expand All @@ -3020,7 +3021,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
// we have a chain with at least nMinimumChainWork), and we ignore
// compact blocks with less work than our tip, it is safe to treat
// reconstructed compact blocks as having been requested.
ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock);
chainman.ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock);
if (fNewBlock) {
pfrom->nLastBlockTime = GetTime();
} else {
Expand Down Expand Up @@ -3110,7 +3111,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
// disk-space attacks), but this should be safe due to the
// protections in the compact block handler -- see related comment
// in compact block optimistic reconstruction handling.
ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock);
chainman.ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock);
if (fNewBlock) {
pfrom->nLastBlockTime = GetTime();
} else {
Expand Down Expand Up @@ -3144,7 +3145,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
ReadCompactSize(vRecv); // ignore tx count; assume it is 0.
}

return ProcessHeadersMessage(pfrom, connman, mempool, headers, chainparams, /*via_compact_block=*/false);
return ProcessHeadersMessage(pfrom, connman, chainman, mempool, headers, chainparams, /*via_compact_block=*/false);
}

if (msg_type == NetMsgType::BLOCK)
Expand Down Expand Up @@ -3173,7 +3174,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
mapBlockSource.emplace(hash, std::make_pair(pfrom->GetId(), true));
}
bool fNewBlock = false;
ProcessNewBlock(chainparams, pblock, forceProcessing, &fNewBlock);
chainman.ProcessNewBlock(chainparams, pblock, forceProcessing, &fNewBlock);
if (fNewBlock) {
pfrom->nLastBlockTime = GetTime();
} else {
Expand Down Expand Up @@ -3534,7 +3535,7 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& inter
bool fRet = false;
try
{
fRet = ProcessMessage(pfrom, msg_type, vRecv, msg.m_time, chainparams, m_mempool, connman, m_banman, interruptMsgProc);
fRet = ProcessMessage(pfrom, msg_type, vRecv, msg.m_time, chainparams, m_chainman, m_mempool, connman, m_banman, interruptMsgProc);
if (interruptMsgProc)
return false;
if (!pfrom->vRecvGetData.empty())
Expand Down
4 changes: 3 additions & 1 deletion src/net_processing.h
View file
Expand Up @@ -12,6 +12,7 @@
#include <validationinterface.h>

class CTxMemPool;
class ChainstateManager;

extern RecursiveMutex cs_main;
extern RecursiveMutex g_cs_orphans;
Expand All @@ -27,12 +28,13 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI
private:
CConnman* const connman;
BanMan* const m_banman;
ChainstateManager& m_chainman;
CTxMemPool& m_mempool;

bool CheckIfBanned(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

public:
PeerLogicValidation(CConnman* connman, BanMan* banman, CScheduler& scheduler, CTxMemPool& pool);
PeerLogicValidation(CConnman* connman, BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool);

/**
* Overridden from CValidationInterface.
Expand Down
9 changes: 9 additions & 0 deletions src/node/context.h
View file
Expand Up @@ -5,6 +5,7 @@
#ifndef BITCOIN_NODE_CONTEXT_H
#define BITCOIN_NODE_CONTEXT_H

#include <cassert>
#include <memory>
#include <vector>

Expand All @@ -13,6 +14,7 @@ class BanMan;
class CConnman;
class CScheduler;
class CTxMemPool;
class ChainstateManager;
class PeerLogicValidation;
namespace interfaces {
class Chain;
Expand All @@ -33,6 +35,7 @@ struct NodeContext {
std::unique_ptr<CConnman> connman;
CTxMemPool* mempool{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
std::unique_ptr<PeerLogicValidation> peer_logic;
ChainstateManager* chainman{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
std::unique_ptr<BanMan> banman;
ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
std::unique_ptr<interfaces::Chain> chain;
Expand All @@ -46,4 +49,10 @@ struct NodeContext {
~NodeContext();
};

inline ChainstateManager& EnsureChainman(const NodeContext& node)
{
assert(node.chainman);
return *node.chainman;
}

#endif // BITCOIN_NODE_CONTEXT_H
6 changes: 6 additions & 0 deletions src/rpc/blockchain.cpp
View file
Expand Up @@ -71,6 +71,12 @@ CTxMemPool& EnsureMemPool(const util::Ref& context)
return *node.mempool;
}

ChainstateManager& EnsureChainman(const util::Ref& context)
{
NodeContext& node = EnsureNodeContext(context);
return EnsureChainman(node);
}

/* Calculate the difficulty for a given block index.
*/
double GetDifficulty(const CBlockIndex* blockindex)
Expand Down
2 changes: 2 additions & 0 deletions src/rpc/blockchain.h
View file
Expand Up @@ -16,6 +16,7 @@ extern RecursiveMutex cs_main;
class CBlock;
class CBlockIndex;
class CTxMemPool;
class ChainstateManager;
class UniValue;
struct NodeContext;
namespace util {
Expand Down Expand Up @@ -52,5 +53,6 @@ void CalculatePercentilesByWeight(CAmount result[NUM_GETBLOCKSTATS_PERCENTILES],

NodeContext& EnsureNodeContext(const util::Ref& context);
CTxMemPool& EnsureMemPool(const util::Ref& context);
ChainstateManager& EnsureChainman(const util::Ref& context);

#endif

0 comments on commit 793e0ff

Please sign in to comment.