From ff57eb4bbdfc96c5a25f78c8f37d5edabf1ceb76 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Thu, 29 Dec 2016 13:44:21 -0600 Subject: [PATCH 1/4] Turn mapBlocksInFlight into a multimap --- src/net_processing.cpp | 149 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 97 insertions(+), 52 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a743f04dd15..c4262636f16 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -105,7 +105,8 @@ namespace { bool fValidatedHeaders; //!< Whether this block has validated headers at the time of request. std::unique_ptr partialBlock; //!< Optional, used for CMPCTBLOCK downloads }; - std::map::iterator> > mapBlocksInFlight; + typedef std::multimap::iterator>> BlockDownloadMap; + BlockDownloadMap mmapBlocksInFlight; /** Stack of nodes which we have set to announce using compact blocks */ std::list lNodesAnnouncingHeaderAndIDs; @@ -233,7 +234,6 @@ CNodeState *State(NodeId pnode) { return NULL; return &it->second; } - void UpdatePreferredDownload(CNode* node, CNodeState* state) { nPreferredDownload -= state->fPreferredDownload; @@ -277,6 +277,44 @@ void InitializeNode(CNode *pnode, CConnman& connman) { PushNodeVersion(pnode, connman, GetTime()); } +// Requires cs_main +// Helper function for MarkBlockAsReceived and MarkBlockAsNotInFlight +static void ClearDownloadState(BlockDownloadMap::iterator itInFlight) { + AssertLockHeld(cs_main); + + CNodeState *state = State(itInFlight->second.first); + state->nBlocksInFlightValidHeaders -= itInFlight->second.second->fValidatedHeaders; + if (state->nBlocksInFlightValidHeaders == 0 && itInFlight->second.second->fValidatedHeaders) { + // Last validated block on the queue was received. + nPeersWithValidatedDownloads--; + } + if (state->vBlocksInFlight.begin() == itInFlight->second.second) { + // First block on the queue was received, update the start download time for the next one + state->nDownloadingSince = std::max(state->nDownloadingSince, GetTimeMicros()); + } + state->vBlocksInFlight.erase(itInFlight->second.second); + state->nBlocksInFlight--; + state->nStallingSince = 0; +} + +// Requires cs_main. +// Used to remove block from mmapBlocksInFlight and clear the download state for +// a block if for some reason block was not received. Download state clearing is +// skipped as an optimization in FinalizeNode. +static void MarkBlockAsNotInFlight(const uint256& hash, NodeId nodeid, bool clearState = true) { + AssertLockHeld(cs_main); + + std::pair range = mmapBlocksInFlight.equal_range(hash); + while (range.first != range.second) { + BlockDownloadMap::iterator itInFlight = range.first; + range.first++; + if (itInFlight->second.first == nodeid) { + if (clearState) ClearDownloadState(itInFlight); + mmapBlocksInFlight.erase(itInFlight); + } + } +} + void FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) { fUpdateConnectionTime = false; LOCK(cs_main); @@ -290,7 +328,7 @@ void FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) { } for (const QueuedBlock& entry : state->vBlocksInFlight) { - mapBlocksInFlight.erase(entry.hash); + MarkBlockAsNotInFlight(entry.hash, nodeid, false); } EraseOrphansFor(nodeid); nPreferredDownload -= state->fPreferredDownload; @@ -301,57 +339,48 @@ void FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) { if (mapNodeState.empty()) { // Do a consistency check after the last peer is removed. - assert(mapBlocksInFlight.empty()); + assert(mmapBlocksInFlight.empty()); assert(nPreferredDownload == 0); assert(nPeersWithValidatedDownloads == 0); } LogPrint(BCLog::NET, "Cleared nodestate for peer=%d\n", nodeid); } + // Requires cs_main. // Returns a bool indicating whether we requested this block. -// Also used if a block was /not/ received and timed out or started with another peer bool MarkBlockAsReceived(const uint256& hash) { - std::map::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash); - if (itInFlight != mapBlocksInFlight.end()) { - CNodeState *state = State(itInFlight->second.first); - state->nBlocksInFlightValidHeaders -= itInFlight->second.second->fValidatedHeaders; - if (state->nBlocksInFlightValidHeaders == 0 && itInFlight->second.second->fValidatedHeaders) { - // Last validated block on the queue was received. - nPeersWithValidatedDownloads--; - } - if (state->vBlocksInFlight.begin() == itInFlight->second.second) { - // First block on the queue was received, update the start download time for the next one - state->nDownloadingSince = std::max(state->nDownloadingSince, GetTimeMicros()); - } - state->vBlocksInFlight.erase(itInFlight->second.second); - state->nBlocksInFlight--; - state->nStallingSince = 0; - mapBlocksInFlight.erase(itInFlight); - return true; - } - return false; + bool found = false; + std::pair range = mmapBlocksInFlight.equal_range(hash); + while (range.first != range.second) { + found = true; + ClearDownloadState(range.first); + range.first = mmapBlocksInFlight.erase(range.first); + } + return found; } // Requires cs_main. // returns false, still setting pit, if the block was already in flight from the same peer // pit will only be valid as long as the same cs_main lock is being held bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex = NULL, std::list::iterator** pit = NULL) { + AssertLockHeld(cs_main); CNodeState *state = State(nodeid); assert(state != NULL); // Short-circuit most stuff in case its from the same node - std::map::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash); - if (itInFlight != mapBlocksInFlight.end() && itInFlight->second.first == nodeid) { - if (pit) { - *pit = &itInFlight->second.second; + std::pair range = mmapBlocksInFlight.equal_range(hash); + while (range.first != range.second) { + BlockDownloadMap::iterator itInFlight = range.first; + range.first++; + if (itInFlight->second.first == nodeid) { + if (pit) { + *pit = &itInFlight->second.second; + } + return false; } - return false; } - // Make sure it's not listed somewhere already. - MarkBlockAsReceived(hash); - std::list::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(), {hash, pindex, pindex != NULL, std::unique_ptr(pit ? new PartiallyDownloadedBlock(&mempool) : NULL)}); state->nBlocksInFlight++; @@ -363,7 +392,7 @@ bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* if (state->nBlocksInFlightValidHeaders == 1 && pindex != NULL) { nPeersWithValidatedDownloads++; } - itInFlight = mapBlocksInFlight.insert(std::make_pair(hash, std::make_pair(nodeid, it))).first; + BlockDownloadMap::iterator itInFlight = mmapBlocksInFlight.insert(std::make_pair(hash, std::make_pair(nodeid, it))); if (pit) *pit = &itInFlight->second.second; return true; @@ -519,7 +548,7 @@ void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vectornStatus & BLOCK_HAVE_DATA || chainActive.Contains(pindex)) { if (pindex->nChainTx) state->pindexLastCommonBlock = pindex; - } else if (mapBlocksInFlight.count(pindex->GetBlockHash()) == 0) { + } else if (mmapBlocksInFlight.count(pindex->GetBlockHash()) == 0) { // The block is not already downloaded, and not yet in flight. if (pindex->nHeight > nWindowEnd) { // We reached the end of the window. @@ -535,7 +564,7 @@ void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vectorGetBlockHash()].first; + waitingfor = mmapBlocksInFlight.lower_bound(pindex->GetBlockHash())->second.first; } } } @@ -863,7 +892,7 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta // just check that there are currently no other blocks in flight. else if (state.IsValid() && !IsInitialBlockDownload() && - mapBlocksInFlight.count(hash) == mapBlocksInFlight.size()) { + mmapBlocksInFlight.count(hash) == mmapBlocksInFlight.size()) { if (it != mapBlockSource.end()) { MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first, *connman); } @@ -1551,7 +1580,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr if (inv.type == MSG_BLOCK) { UpdateBlockAvailability(pfrom->GetId(), inv.hash); - if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) { + if (!fAlreadyHave && !fImporting && !fReindex && !mmapBlocksInFlight.count(inv.hash)) { // We used to request the full block here, but since headers-announcements are now the // primary method of announcement on the network, and since, in the case that a node // fell back to inv we probably have a reorg which we should get the headers for first, @@ -2006,12 +2035,18 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr assert(pindex); UpdateBlockAvailability(pfrom->GetId(), pindex->GetBlockHash()); - std::map::iterator> >::iterator blockInFlightIt = mapBlocksInFlight.find(pindex->GetBlockHash()); - bool fAlreadyInFlight = blockInFlightIt != mapBlocksInFlight.end(); - if (pindex->nStatus & BLOCK_HAVE_DATA) // Nothing to do here return true; + std::pair rangeInFlight = mmapBlocksInFlight.equal_range(pindex->GetBlockHash()); + bool fAlreadyInFlight = rangeInFlight.first != rangeInFlight.second; + bool fInFlightFromSamePeer = false; + while (rangeInFlight.first != rangeInFlight.second) { + if (rangeInFlight.first->second.first == pfrom->GetId()) + fInFlightFromSamePeer = true; + rangeInFlight.first++; + } + if (pindex->nChainWork <= chainActive.Tip()->nChainWork || // We know something better pindex->nTx != 0) { // We had this block at some point, but pruned it if (fAlreadyInFlight) { @@ -2040,8 +2075,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // possibilities in compact block processing... if (pindex->nHeight <= chainActive.Height() + 2) { if ((!fAlreadyInFlight && nodestate->nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) || - (fAlreadyInFlight && blockInFlightIt->second.first == pfrom->GetId())) { - std::list::iterator* queuedBlockIt = NULL; + fInFlightFromSamePeer) { + std::list::iterator *queuedBlockIt = NULL; if (!MarkBlockAsInFlight(pfrom->GetId(), pindex->GetBlockHash(), pindex, &queuedBlockIt)) { if (!(*queuedBlockIt)->partialBlock) (*queuedBlockIt)->partialBlock.reset(new PartiallyDownloadedBlock(&mempool)); @@ -2055,7 +2090,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr PartiallyDownloadedBlock& partialBlock = *(*queuedBlockIt)->partialBlock; ReadStatus status = partialBlock.InitData(cmpctblock, vExtraTxnForCompact); if (status == READ_STATUS_INVALID) { - MarkBlockAsReceived(pindex->GetBlockHash()); // Reset in-flight state in case of whitelist + MarkBlockAsNotInFlight(pindex->GetBlockHash(), pfrom->GetId()); // Reset in-flight state in case of whitelist Misbehaving(pfrom->GetId(), 100); LogPrintf("Peer %d sent us invalid compact block\n", pfrom->GetId()); return true; @@ -2159,17 +2194,27 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr { LOCK(cs_main); - std::map::iterator> >::iterator it = mapBlocksInFlight.find(resp.blockhash); - if (it == mapBlocksInFlight.end() || !it->second.second->partialBlock || - it->second.first != pfrom->GetId()) { + bool expected_BLOCKTXN = false; + std::pair rangeInFlight = mmapBlocksInFlight.equal_range(resp.blockhash); + while (rangeInFlight.first != rangeInFlight.second) { + if (rangeInFlight.first->second.first == pfrom->GetId()) { + if (rangeInFlight.first->second.second->partialBlock) { + expected_BLOCKTXN = true; + } + break; + } + rangeInFlight.first++; + } + + if (!expected_BLOCKTXN) { LogPrint(BCLog::NET, "Peer %d sent us block transactions for block we weren't expecting\n", pfrom->GetId()); return true; } - PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock; + PartiallyDownloadedBlock& partialBlock = *rangeInFlight.first->second.second->partialBlock; ReadStatus status = partialBlock.FillBlock(*pblock, resp.txn); if (status == READ_STATUS_INVALID) { - MarkBlockAsReceived(resp.blockhash); // Reset in-flight state in case of whitelist + MarkBlockAsNotInFlight(resp.blockhash, pfrom->GetId()); // Reset in-flight state in case of whitelist Misbehaving(pfrom->GetId(), 100); LogPrintf("Peer %d sent us invalid compact block/non-matching block transactions\n", pfrom->GetId()); return true; @@ -2196,7 +2241,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // though the block was successfully read, and rely on the // handling in ProcessNewBlock to ensure the block index is // updated, reject messages go out, etc. - MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer + MarkBlockAsNotInFlight(resp.blockhash, pfrom->GetId()); // 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. @@ -2208,7 +2253,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr } // Don't hold cs_main when we call into ProcessNewBlock if (fBlockRead) { bool fNewBlock = false; - // Since we requested this block (it was in mapBlocksInFlight), force it to be processed, + // Since we requested this block (it was in mmapBlocksInFlight), force it to be processed, // even if it would not be a candidate for new tip (missing previous block, chain not long enough, etc) ProcessNewBlock(chainparams, pblock, true, &fNewBlock); if (fNewBlock) @@ -2321,7 +2366,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // Calculate all the blocks we'd need to switch to pindexLast, up to a limit. while (pindexWalk && !chainActive.Contains(pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { if (!(pindexWalk->nStatus & BLOCK_HAVE_DATA) && - !mapBlocksInFlight.count(pindexWalk->GetBlockHash()) && + !mmapBlocksInFlight.count(pindexWalk->GetBlockHash()) && (!IsWitnessEnabled(pindexWalk->pprev, chainparams.GetConsensus()) || State(pfrom->GetId())->fHaveWitness)) { // We don't have this block, and it's not yet in flight. vToFetch.push_back(pindexWalk); @@ -2355,7 +2400,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr pindexLast->GetBlockHash().ToString(), pindexLast->nHeight); } if (vGetData.size() > 0) { - if (nodestate->fSupportsDesiredCmpctVersion && vGetData.size() == 1 && mapBlocksInFlight.size() == 1 && pindexLast->pprev->IsValid(BLOCK_VALID_CHAIN)) { + if (nodestate->fSupportsDesiredCmpctVersion && vGetData.size() == 1 && mmapBlocksInFlight.size() == 1 && pindexLast->pprev->IsValid(BLOCK_VALID_CHAIN)) { // In any case, we want to download using a compact block, not a regular one vGetData[0] = CInv(MSG_CMPCT_BLOCK, vGetData[0].hash); } From b4592659b960384f09e26168a0de331a4c334752 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 6 Jun 2017 16:13:55 -0400 Subject: [PATCH 2/4] Call NewPoWValidBlock callbacks for all new blocks, not just !IBD This pushes some "is this callback useful" logic down into net_processing, which is useful for later changes as it allows for more notifications to be used. --- src/net_processing.cpp | 5 ++++- src/net_processing.h | 2 +- src/validation.cpp | 4 +--- src/validationinterface.cpp | 10 +++++----- src/validationinterface.h | 11 ++++++++--- 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c4262636f16..8cd5f587f62 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -796,7 +796,10 @@ static std::shared_ptr most_recent_compact_bloc static uint256 most_recent_block_hash; static bool fWitnessesPresentInMostRecentCompactBlock; -void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) { +void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock, bool fNewCandidateTip) { + if (!fNewCandidateTip || IsInitialBlockDownload()) + return; + std::shared_ptr pcmpctblock = std::make_shared (*pblock, true); const CNetMsgMaker msgMaker(PROTOCOL_VERSION); diff --git a/src/net_processing.h b/src/net_processing.h index db6d81e6b67..8940e45522a 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -37,7 +37,7 @@ class PeerLogicValidation : public CValidationInterface { void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted) override; void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override; void BlockChecked(const CBlock& block, const CValidationState& state) override; - void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) override; + void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock, bool fNewCandidateTip) override; }; struct CNodeStateStats { diff --git a/src/validation.cpp b/src/validation.cpp index 09288be1ca4..99f21deb423 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3141,9 +3141,7 @@ static bool AcceptBlock(const std::shared_ptr& pblock, CValidation } // Header is valid/has work, merkle tree and segwit merkle tree are good...RELAY NOW - // (but if it does not build on our best tip, let the SendMessages loop relay it) - if (!IsInitialBlockDownload() && chainActive.Tip() == pindex->pprev) - GetMainSignals().NewPoWValidBlock(pindex, pblock); + GetMainSignals().NewPoWValidBlock(pindex, pblock, chainActive.Tip() == pindex->pprev); int nHeight = pindex->nHeight; diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index bf20d606f83..a40bb0d887c 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -23,7 +23,7 @@ struct MainSignalsInstance { boost::signals2::signal Inventory; boost::signals2::signal Broadcast; boost::signals2::signal BlockChecked; - boost::signals2::signal&)> NewPoWValidBlock; + boost::signals2::signal&, bool)> NewPoWValidBlock; // We are not allowed to assume the scheduler only runs in one thread, // but must ensure all callbacks happen in-order, so we end up creating @@ -62,7 +62,7 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.m_internals->Inventory.connect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); g_signals.m_internals->Broadcast.connect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1, _2)); g_signals.m_internals->BlockChecked.connect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); - g_signals.m_internals->NewPoWValidBlock.connect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2)); + g_signals.m_internals->NewPoWValidBlock.connect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2, _3)); } void UnregisterValidationInterface(CValidationInterface* pwalletIn) { @@ -74,7 +74,7 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.m_internals->BlockConnected.disconnect(boost::bind(&CValidationInterface::BlockConnected, pwalletIn, _1, _2, _3)); g_signals.m_internals->BlockDisconnected.disconnect(boost::bind(&CValidationInterface::BlockDisconnected, pwalletIn, _1)); g_signals.m_internals->UpdatedBlockTip.disconnect(boost::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, _1, _2, _3)); - g_signals.m_internals->NewPoWValidBlock.disconnect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2)); + g_signals.m_internals->NewPoWValidBlock.disconnect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2, _3)); } void UnregisterAllValidationInterfaces() { @@ -121,6 +121,6 @@ void CMainSignals::BlockChecked(const CBlock& block, const CValidationState& sta m_internals->BlockChecked(block, state); } -void CMainSignals::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr &block) { - m_internals->NewPoWValidBlock(pindex, block); +void CMainSignals::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr &block, bool fNewCandidateTip) { + m_internals->NewPoWValidBlock(pindex, block, fNewCandidateTip); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 568da66df28..c3d30391dcf 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -58,8 +58,13 @@ class CValidationInterface { virtual void BlockChecked(const CBlock&, const CValidationState&) {} /** * Notifies listeners that a block which builds directly on our current tip - * has been received and connected to the headers tree, though not validated yet */ - virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& block) {}; + * has been received and connected to the headers tree, though not validated yet. + * If the new block builds on the current best tip, the final bool argument is + * set to true, otherwise it is false. + * Consider if you need an IsInitialBlockDownload check in your client (and note + * that any such calls will be racy wrt the state when the callback was generated) + * */ + virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& block, bool fNewCandidateTip) {}; friend void ::RegisterValidationInterface(CValidationInterface*); friend void ::UnregisterValidationInterface(CValidationInterface*); friend void ::UnregisterAllValidationInterfaces(); @@ -91,7 +96,7 @@ class CMainSignals { void Inventory(const uint256 &); void Broadcast(int64_t nBestBlockTime, CConnman* connman); void BlockChecked(const CBlock&, const CValidationState&); - void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr&); + void NewPoWValidBlock(const CBlockIndex *, const std::shared_ptr&, bool); }; CMainSignals& GetMainSignals(); From 61c9b81df0e8da31c7c51f4041b8f57155b49c86 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 6 Jun 2017 16:16:58 -0400 Subject: [PATCH 3/4] MarkBlockAsReceived on NewPoWValidBlock at receive. The received block could be malleated, so this is both simpler, and supports parallel downloads. --- src/net_processing.cpp | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8cd5f587f62..e21377f2892 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -347,9 +347,9 @@ void FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) { } -// Requires cs_main. // Returns a bool indicating whether we requested this block. bool MarkBlockAsReceived(const uint256& hash) { + LOCK(cs_main); bool found = false; std::pair range = mmapBlocksInFlight.equal_range(hash); while (range.first != range.second) { @@ -797,6 +797,9 @@ static uint256 most_recent_block_hash; static bool fWitnessesPresentInMostRecentCompactBlock; void PeerLogicValidation::NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock, bool fNewCandidateTip) { + // The block was received in non-malleated form (and is/will be stored on disk). + // We can consider all in-flight requests completed + MarkBlockAsReceived(pindex->GetBlockHash()); if (!fNewCandidateTip || IsInitialBlockDownload()) return; @@ -2174,17 +2177,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr ProcessNewBlock(chainparams, pblock, true, &fNewBlock); if (fNewBlock) pfrom->nLastBlockTime = GetTime(); - - LOCK(cs_main); // hold cs_main for CBlockIndex::IsValid() - if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS)) { - // Clear download state for this block, which is in - // process from some other peer. We do this after calling - // ProcessNewBlock so that a malleated cmpctblock announcement - // can't be used to interfere with block relay. - MarkBlockAsReceived(pblock->GetHash()); - } } - } else if (strCommand == NetMsgType::BLOCKTXN && !fImporting && !fReindex) // Ignore blocks received while importing @@ -2431,7 +2424,10 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr 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(hash); + // TODO: Only process if requested from this peer? + forceProcessing |= mmapBlocksInFlight.count(hash); + // Block is no longer in flight from this peer + MarkBlockAsNotInFlight(hash, pfrom->GetId()); // 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)); From 8689df48cede961a12afa66431fe8c05fc511b46 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Thu, 29 Dec 2016 16:23:07 -0600 Subject: [PATCH 4/4] Only request full blocks from the peer we thought had the block in-flight This is a change in behavior so that if for some reason we request a block from a peer, we don't allow an unsolicited CMPCT_BLOCK announcement for that same block to cause a request for a full block from the uninvited peer (as some type of request is already outstanding from the original peer) --- src/net_processing.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e21377f2892..687834300ce 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2055,7 +2055,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr if (pindex->nChainWork <= chainActive.Tip()->nChainWork || // We know something better pindex->nTx != 0) { // We had this block at some point, but pruned it - if (fAlreadyInFlight) { + if (fInFlightFromSamePeer) { // We requested this block for some reason, but our mempool will probably be useless // so we just grab the block via normal getdata std::vector vInv(1); @@ -2066,7 +2066,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr } // If we're not close to tip yet, give up and let parallel block fetch work its magic - if (!fAlreadyInFlight && !CanDirectFetch(chainparams.GetConsensus())) + if (!fInFlightFromSamePeer && !CanDirectFetch(chainparams.GetConsensus())) return true; CNodeState *nodestate = State(pfrom->GetId()); @@ -2142,7 +2142,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr } } } else { - if (fAlreadyInFlight) { + if (fInFlightFromSamePeer) { // We requested this block, but its far into the future, so our // mempool will probably be useless - request the block normally std::vector vInv(1);