From b4b343145878b92211e4c1e1c9497369f025800c Mon Sep 17 00:00:00 2001 From: Oleg Girko Date: Wed, 2 Aug 2017 19:35:04 +0100 Subject: [PATCH] Backport Bitcoin PR#9075: Decouple peer-processing-logic from block-connection-logic (#3) (#1560) * Remove pfrom parameter from ProcessNewBlock This further decouples ProcessNewBlock from networking/peer logic. * Replace CValidationState param in ProcessNewBlock with BlockChecked --- src/main.cpp | 39 +++++++++++++++++---------------------- src/main.h | 12 +++++++++--- src/miner.cpp | 3 +-- src/rpc/mining.cpp | 16 +++++----------- src/test/miner_tests.cpp | 4 +--- src/test/test_dash.cpp | 3 +-- 6 files changed, 34 insertions(+), 43 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 9d5ae414dc558..d3360d3cc4c45 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3971,26 +3971,26 @@ static bool IsSuperMajority(int minVersion, const CBlockIndex* pstart, unsigned } -bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp) +bool ProcessNewBlock(const CChainParams& chainparams, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool *fNewBlock) { { LOCK(cs_main); // Store to disk CBlockIndex *pindex = NULL; - bool fNewBlock = false; - bool ret = AcceptBlock(*pblock, state, chainparams, &pindex, fForceProcessing, dbp, &fNewBlock); - if (pindex && pfrom) { - mapBlockSource[pindex->GetBlockHash()] = pfrom->GetId(); - if (fNewBlock) pfrom->nLastBlockTime = GetTime(); - } + if (fNewBlock) *fNewBlock = false; + CValidationState state; + bool ret = AcceptBlock(*pblock, state, chainparams, &pindex, fForceProcessing, dbp, fNewBlock); CheckBlockIndex(chainparams.GetConsensus()); - if (!ret) + if (!ret) { + GetMainSignals().BlockChecked(*pblock, state); return error("%s: AcceptBlock FAILED", __func__); + } } NotifyHeaderTip(); + CValidationState state; // Only used to report errors, not invalidity - ignore it if (!ActivateBestChain(state, chainparams, pblock)) return error("%s: ActivateBestChain failed", __func__); @@ -6118,30 +6118,25 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, pfrom->AddInventoryKnown(inv); - CValidationState state; // Process all blocks from whitelisted peers, even if not requested, // unless we're still syncing with the network. // Such an unrequested block may still be processed, subject to the // conditions in AcceptBlock(). bool forceProcessing = pfrom->fWhitelisted && !IsInitialBlockDownload(); + const uint256 hash(block.GetHash()); { LOCK(cs_main); // Also always process if we requested the block explicitly, as we may // need it even though it is not a candidate for a new best tip. - forceProcessing |= MarkBlockAsReceived(block.GetHash()); - } - ProcessNewBlock(state, chainparams, pfrom, &block, forceProcessing, NULL); - int nDoS; - if (state.IsInvalid(nDoS)) { - assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes - connman.PushMessage(pfrom, NetMsgType::REJECT, strCommand, (unsigned char)state.GetRejectCode(), - state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), inv.hash); - if (nDoS > 0) { - LOCK(cs_main); - Misbehaving(pfrom->GetId(), nDoS); - } + forceProcessing |= MarkBlockAsReceived(hash); + // mapBlockSource is only used for sending reject messages and DoS scores, + // so the race between here and cs_main in ProcessNewBlock is fine. + mapBlockSource.emplace(hash, pfrom->GetId()); } - + bool fNewBlock = false; + ProcessNewBlock(chainparams, &block, forceProcessing, NULL, &fNewBlock); + if (fNewBlock) + pfrom->nLastBlockTime = GetTime(); } diff --git a/src/main.h b/src/main.h index b2259f9a61742..db49095ef0a88 100644 --- a/src/main.h +++ b/src/main.h @@ -193,15 +193,21 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024; * Process an incoming block. This only returns after the best known valid * block is made active. Note that it does not, however, guarantee that the * specific block passed to it has been checked for validity! + * + * If you want to *possibly* get feedback on whether pblock is valid, you must + * install a CValidationInterface (see validationinterface.h) - this will have + * its BlockChecked method called whenever *any* block completes validation. + * + * Note that we guarantee that either the proof-of-work is valid on pblock, or + * (and possibly also) BlockChecked will have been called. * - * @param[out] state This may be set to an Error state if any error occurred processing it, including during validation/connection/etc of otherwise unrelated blocks during reorganisation; or it may be set to an Invalid state if pblock is itself invalid (but this is not guaranteed even when the block is checked). If you want to *possibly* get feedback on whether pblock is valid, you must also install a CValidationInterface (see validationinterface.h) - this will have its BlockChecked method called whenever *any* block completes validation. - * @param[in] pfrom The node which we are receiving the block from; it is added to mapBlockSource and may be penalised if the block is invalid. * @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. + * @param[out] fNewBlock A boolean which is set to indicate if the block was first received via this call * @return True if state.IsValid() */ -bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp); +bool ProcessNewBlock(const CChainParams& chainparams, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool* fNewBlock); /** Check whether enough disk space is available for an incoming block */ bool CheckDiskSpace(uint64_t nAdditionalBytes = 0); /** Open a block file (blk?????.dat) */ diff --git a/src/miner.cpp b/src/miner.cpp index 9dbc8c3f28262..c1a53bc6a6e54 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -392,8 +392,7 @@ static bool ProcessBlockFound(const CBlock* pblock, const CChainParams& chainpar GetMainSignals().BlockFound(pblock->GetHash()); // Process this block the same as if we had received it from another node - CValidationState state; - if (!ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL)) + if (!ProcessNewBlock(chainparams, pblock, true, NULL, NULL)) return error("ProcessBlockFound -- ProcessNewBlock() failed, block not accepted"); return true; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index d00c8366c98a0..6264324f1397b 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -177,8 +177,7 @@ UniValue generate(const UniValue& params, bool fHelp) // target -- 1 in 2^(2^32). That ain't gonna happen. ++pblock->nNonce; } - CValidationState state; - if (!ProcessNewBlock(state, Params(), NULL, pblock, true, NULL)) + if (!ProcessNewBlock(Params(), pblock, true, NULL, NULL)) throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted"); ++nHeight; blockHashes.push_back(pblock->GetHash().GetHex()); @@ -794,10 +793,9 @@ UniValue submitblock(const UniValue& params, bool fHelp) } } - CValidationState state; submitblock_StateCatcher sc(block.GetHash()); RegisterValidationInterface(&sc); - bool fAccepted = ProcessNewBlock(state, Params(), NULL, &block, true, NULL); + bool fAccepted = ProcessNewBlock(Params(), &block, true, NULL, NULL); UnregisterValidationInterface(&sc); if (fBlockPresent) { @@ -805,13 +803,9 @@ UniValue submitblock(const UniValue& params, bool fHelp) return "duplicate-inconclusive"; return "duplicate"; } - if (fAccepted) - { - if (!sc.found) - return "inconclusive"; - state = sc.state; - } - return BIP22ValidationResult(state); + if (!sc.found) + return "inconclusive"; + return BIP22ValidationResult(sc.state); } UniValue estimatefee(const UniValue& params, bool fHelp) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 4ccde505cab5a..58995ab413761 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -117,9 +117,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) txFirst.push_back(new CTransaction(pblock->vtx[0])); pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); pblock->nNonce = blockinfo[i].nonce; - CValidationState state; - BOOST_CHECK(ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL)); - BOOST_CHECK(state.IsValid()); + BOOST_CHECK(ProcessNewBlock(chainparams, pblock, true, NULL, NULL)); pblock->hashPrevBlock = pblock->GetHash(); } delete pblocktemplate; diff --git a/src/test/test_dash.cpp b/src/test/test_dash.cpp index af06f8485cf3b..0b03f1c585f9d 100644 --- a/src/test/test_dash.cpp +++ b/src/test/test_dash.cpp @@ -135,8 +135,7 @@ TestChain100Setup::CreateAndProcessBlock(const std::vector& while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce; - CValidationState state; - ProcessNewBlock(state, chainparams, NULL, &block, true, NULL); + ProcessNewBlock(chainparams, &block, true, NULL, NULL); CBlock result = block; delete pblocktemplate;