From 7c98ce584ec23bcddcba8cdb33efa6547212f6ef Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 2 Oct 2016 11:04:49 -0400 Subject: [PATCH] Remove pfrom parameter from ProcessNewBlock This further decouples ProcessNewBlock from networking/peer logic. --- src/main.cpp | 35 +++++++++++++++++++++++------------ src/main.h | 4 ++-- src/rpc/mining.cpp | 4 ++-- src/test/miner_tests.cpp | 2 +- src/test/test_bitcoin.cpp | 2 +- 5 files changed, 29 insertions(+), 18 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 7e5b9528b9..7b203cf62b 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -3787,19 +3787,15 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CCha return true; } -bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool fMayBanPeerIfInvalid) +bool ProcessNewBlock(CValidationState& state, 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()] = std::make_pair(pfrom->GetId(), fMayBanPeerIfInvalid); - if (fNewBlock) pfrom->nLastBlockTime = GetTime(); - } + if (fNewBlock) *fNewBlock = false; + bool ret = AcceptBlock(*pblock, state, chainparams, &pindex, fForceProcessing, dbp, fNewBlock); CheckBlockIndex(chainparams.GetConsensus()); if (!ret) return error("%s: AcceptBlock FAILED", __func__); @@ -5914,22 +5910,29 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // updated, reject messages go out, etc. MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer fBlockRead = true; + // 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(resp.blockhash, std::make_pair(pfrom->GetId(), false)); } } // Don't hold cs_main when we call into ProcessNewBlock if (fBlockRead) { CValidationState state; + bool fNewBlock = false; // Since we requested this block (it was in mapBlocksInFlight), force it to be processed, // even if it would not be a candidate for new tip (missing previous block, chain not long enough, etc) // BIP 152 permits peers to relay compact blocks after validating // the header only; we should not punish peers if the block turns // out to be invalid. - ProcessNewBlock(state, chainparams, pfrom, &block, true, NULL, false); + ProcessNewBlock(state, chainparams, &block, true, NULL, &fNewBlock); 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), block.GetHash()); - } + LOCK(cs_main); + mapBlockSource.erase(resp.blockhash); + } else if (fNewBlock) + pfrom->nLastBlockTime = GetTime(); } } @@ -6093,13 +6096,18 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // 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()); + 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, std::make_pair(pfrom->GetId(), true)); } - ProcessNewBlock(state, chainparams, pfrom, &block, forceProcessing, NULL, true); + bool fNewBlock = false; + ProcessNewBlock(state, chainparams, &block, forceProcessing, NULL, &fNewBlock); int nDoS; if (state.IsInvalid(nDoS)) { assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes @@ -6109,7 +6117,10 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, LOCK(cs_main); Misbehaving(pfrom->GetId(), nDoS); } - } + LOCK(cs_main); + mapBlockSource.erase(hash); + } else if (fNewBlock) + pfrom->nLastBlockTime = GetTime(); } diff --git a/src/main.h b/src/main.h index 9343330587..5e5d083efa 100644 --- a/src/main.h +++ b/src/main.h @@ -217,13 +217,13 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024; * specific block passed to it has been checked for validity! * * @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 reorganization; 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 fMayBanPeerIfInvalid); +bool ProcessNewBlock(CValidationState& state, 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/rpc/mining.cpp b/src/rpc/mining.cpp index 3e91a79a64..d9fe9530a1 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -132,7 +132,7 @@ UniValue generateBlocks(boost::shared_ptr coinbaseScript, int nG continue; } CValidationState state; - if (!ProcessNewBlock(state, Params(), NULL, pblock, true, NULL, false)) + if (!ProcessNewBlock(state, Params(), pblock, true, NULL, NULL)) throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted"); ++nHeight; blockHashes.push_back(pblock->GetHash().GetHex()); @@ -757,7 +757,7 @@ UniValue submitblock(const JSONRPCRequest& request) CValidationState state; submitblock_StateCatcher sc(block.GetHash()); RegisterValidationInterface(&sc); - bool fAccepted = ProcessNewBlock(state, Params(), NULL, &block, true, NULL, false); + bool fAccepted = ProcessNewBlock(state, Params(), &block, true, NULL, NULL); UnregisterValidationInterface(&sc); if (fBlockPresent) { diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 2762cafa38..e71a4b1ebc 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -223,7 +223,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); pblock->nNonce = blockinfo[i].nonce; CValidationState state; - BOOST_CHECK(ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL, false)); + BOOST_CHECK(ProcessNewBlock(state, chainparams, pblock, true, NULL, NULL)); BOOST_CHECK(state.IsValid()); pblock->hashPrevBlock = pblock->GetHash(); } diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index 3da0be8ca4..4440585c01 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -127,7 +127,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, false); + ProcessNewBlock(state, chainparams, &block, true, NULL, NULL); CBlock result = block; return result;