From 4a6b1f36b773001f6c5a8e1d3d196833e4fb872d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 2 Oct 2016 12:22:05 -0400 Subject: [PATCH 1/5] Expose AcceptBlockHeader through main.h --- src/main.cpp | 17 ++++++++++++++++- src/main.h | 11 +++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index 05442057e..4a2682bc7 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3649,7 +3649,7 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, const Co return true; } -static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex=NULL) +static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex) { AssertLockHeld(cs_main); // Check for duplicate @@ -3698,6 +3698,21 @@ static bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state return true; } +// Exposed wrapper for AcceptBlockHeader +bool ProcessNewBlockHeaders(const std::vector& headers, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex) +{ + { + LOCK(cs_main); + for (const CBlockHeader& header : headers) { + if (!AcceptBlockHeader(header, state, chainparams, ppindex)) { + return false; + } + } + } + NotifyHeaderTip(); + return true; +} + /** Store block on disk. If dbp is non-NULL, the file is known to already reside on disk */ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool fRequested, const CDiskBlockPos* dbp, bool* fNewBlock) { diff --git a/src/main.h b/src/main.h index 43c62f6de..9fb6b298c 100644 --- a/src/main.h +++ b/src/main.h @@ -230,6 +230,17 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024; * @return True if state.IsValid() */ bool ProcessNewBlock(const CChainParams& chainparams, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool* fNewBlock); + +/** + * Process incoming block headers. + * + * @param[in] block The block headers themselves + * @param[out] state This may be set to an Error state if any error occurred processing them + * @param[in] chainparams The params for the chain we want to connect to + * @param[out] ppindex If set, the pointer will be set to point to the last new block index object for the given headers + */ +bool ProcessNewBlockHeaders(const std::vector& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex=NULL); + /** Check whether enough disk space is available for an incoming block */ bool CheckDiskSpace(uint64_t nAdditionalBytes = 0); /** Open a block file (blk?????.dat) */ From 63fd101c529cf6e2bc5ecab244fd56e2c5be521f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 3 Nov 2016 16:34:31 -0400 Subject: [PATCH 2/5] Split ::HEADERS processing into two separate cs_main locks This will allow NotifyHeaderTip to be called from an AcceptBlockHeader wrapper function without holding cs_main. --- src/main.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 4a2682bc7..4cd5bebb7 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -5982,14 +5982,14 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, ReadCompactSize(vRecv); // ignore tx count; assume it is 0. } - { - LOCK(cs_main); - if (nCount == 0) { // Nothing interesting. Stop asking this peers for more headers. return true; } + CBlockIndex *pindexLast = NULL; + { + LOCK(cs_main); CNodeState *nodestate = State(pfrom->GetId()); // If this looks like it could be a block announcement (nCount < @@ -6019,7 +6019,6 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, return true; } - CBlockIndex *pindexLast = NULL; BOOST_FOREACH(const CBlockHeader& header, headers) { CValidationState state; if (pindexLast != NULL && header.hashPrevBlock != pindexLast->GetBlockHash()) { @@ -6035,7 +6034,11 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, } } } + } + { + LOCK(cs_main); + CNodeState *nodestate = State(pfrom->GetId()); if (nodestate->nUnconnectingHeaders > 0) { LogPrint("net", "peer=%d: resetting nUnconnectingHeaders (%d -> 0)\n", pfrom->id, nodestate->nUnconnectingHeaders); } From a8b936df2053e68208d106cddec06227ef563e41 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 3 Nov 2016 16:56:33 -0400 Subject: [PATCH 3/5] Use exposed ProcessNewBlockHeaders from ProcessMessages --- src/main.cpp | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 4cd5bebb7..f88efe7b5 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -6019,22 +6019,27 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, return true; } - BOOST_FOREACH(const CBlockHeader& header, headers) { - CValidationState state; - if (pindexLast != NULL && header.hashPrevBlock != pindexLast->GetBlockHash()) { + uint256 hashLastBlock; + for (const CBlockHeader& header : headers) { + if (!hashLastBlock.IsNull() && header.hashPrevBlock != hashLastBlock) { Misbehaving(pfrom->GetId(), 20); return error("non-continuous headers sequence"); } - if (!AcceptBlockHeader(header, state, chainparams, &pindexLast)) { - int nDoS; - if (state.IsInvalid(nDoS)) { - if (nDoS > 0) - Misbehaving(pfrom->GetId(), nDoS); - return error("invalid header received"); + hashLastBlock = header.GetHash(); + } + } + + CValidationState state; + if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast)) { + int nDoS; + if (state.IsInvalid(nDoS)) { + if (nDoS > 0) { + LOCK(cs_main); + Misbehaving(pfrom->GetId(), nDoS); } + return error("invalid header received"); } } - } { LOCK(cs_main); @@ -6110,8 +6115,6 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, } } } - - NotifyHeaderTip(); } else if (strCommand == NetMsgType::BLOCK && !fImporting && !fReindex) // Ignore blocks received while importing From 58a215ce8c13b900cf982c39f8ee4879290d1a95 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 3 Nov 2016 17:01:39 -0400 Subject: [PATCH 4/5] Use ProcessNewBlockHeaders in CMPCTBLOCK processing --- src/main.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index f88efe7b5..04a55b3dc 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -5768,6 +5768,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, CBlockHeaderAndShortTxIDs cmpctblock; vRecv >> cmpctblock; + { LOCK(cs_main); if (mapBlockIndex.find(cmpctblock.header.hashPrevBlock) == mapBlockIndex.end()) { @@ -5776,19 +5777,23 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, connman.PushMessage(pfrom, NetMsgType::GETHEADERS, chainActive.GetLocator(pindexBestHeader), uint256()); return true; } + } CBlockIndex *pindex = NULL; CValidationState state; - if (!AcceptBlockHeader(cmpctblock.header, state, chainparams, &pindex)) { + if (!ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) { int nDoS; if (state.IsInvalid(nDoS)) { - if (nDoS > 0) + if (nDoS > 0) { + LOCK(cs_main); Misbehaving(pfrom->GetId(), nDoS); + } LogPrintf("Peer %d sent us invalid header via cmpctblock\n", pfrom->id); return true; } } + LOCK(cs_main); // If AcceptBlockHeader returned true, it set pindex assert(pindex); UpdateBlockAvailability(pfrom->GetId(), pindex->GetBlockHash()); From 2c8c57e72fe32cac909278312955145632da6d82 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 1 Dec 2016 11:02:23 -0800 Subject: [PATCH 5/5] Document cs_main status when calling into PNB or PNBH --- src/main.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main.h b/src/main.h index 9fb6b298c..8f51e3324 100644 --- a/src/main.h +++ b/src/main.h @@ -223,6 +223,8 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024; * Note that we guarantee that either the proof-of-work is valid on pblock, or * (and possibly also) BlockChecked will have been called. * + * Call without cs_main held. + * * @param[in] pblock The block we want to process. * @param[in] fForceProcessing Process this block even if unrequested; used for non-network block sources and whitelisted peers. * @param[out] dbp The already known disk position of pblock, or NULL if not yet stored. @@ -234,6 +236,8 @@ bool ProcessNewBlock(const CChainParams& chainparams, const CBlock* pblock, bool /** * Process incoming block headers. * + * Call without cs_main held. + * * @param[in] block The block headers themselves * @param[out] state This may be set to an Error state if any error occurred processing them * @param[in] chainparams The params for the chain we want to connect to