From cdfb4c9c6cd8909efeb59c8c47cdcef7c1822b26 Mon Sep 17 00:00:00 2001 From: Glenn Willen Date: Tue, 9 Nov 2021 21:06:42 -0800 Subject: [PATCH] Fix elements multiple-header-download issue. This fixes an issue which causes Elements to download the blockchain headers multiple times during initial block download. In particular: each time we receive an INV P2P message with a new block (about once a minute), we start downloading the headers, again, in parallel with any existing download(s) in progress. With this change, after we receive each batch of headers, we check whether any of the headers in it were new to us. If not (they were all duplicates), we stop there, and do not ask the peer for another batch. This reduces the maximum amount of duplication to about 2x, which is not ideal, but a HUGE improvement. --- src/net_processing.cpp | 6 ++++-- src/validation.cpp | 19 ++++++++++++++++--- src/validation.h | 5 +++-- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c649cf7757c..042b58474a8 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1843,7 +1843,8 @@ void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vectorm_last_block_announcement = GetTime(); } - if (nCount == MAX_HEADERS_RESULTS) { + if (nCount == MAX_HEADERS_RESULTS && !all_duplicate) { // Headers message had its maximum size; the peer may have more headers. // TODO: optimize: if pindexLast is an ancestor of ::ChainActive().Tip or pindexBestHeader, continue // from there instead. + // HOWEVER, if all headers we got this time were duplicates that we already had, don't ask for any more. LogPrint(BCLog::NET, "more getheaders (%d) to end to peer=%d (startheight:%d)\n", pindexLast->nHeight, pfrom.GetId(), pfrom.nStartingHeight); m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexLast), uint256())); } diff --git a/src/validation.cpp b/src/validation.cpp index 24605b7bb6f..ff2f81c5d85 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4036,16 +4036,22 @@ static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& stat return true; } -bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex) +bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool* duplicate) { AssertLockHeld(cs_main); // Check for duplicate uint256 hash = block.GetHash(); BlockMap::iterator miSelf = m_block_index.find(hash); CBlockIndex *pindex = nullptr; + if (duplicate) { + *duplicate = false; + } if (hash != chainparams.GetConsensus().hashGenesisBlock) { if (miSelf != m_block_index.end()) { // Block header is already known. + if (duplicate) { + *duplicate = true; + } pindex = miSelf->second; if (ppindex) *ppindex = pindex; @@ -4125,17 +4131,24 @@ bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationS } // Exposed wrapper for AcceptBlockHeader -bool ChainstateManager::ProcessNewBlockHeaders(const std::vector& headers, BlockValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex) +bool ChainstateManager::ProcessNewBlockHeaders(const std::vector& headers, BlockValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex, bool* all_duplicate) { AssertLockNotHeld(cs_main); { LOCK(cs_main); + if (all_duplicate) { + *all_duplicate = true; + } + bool duplicate = false; for (const CBlockHeader& header : headers) { CBlockIndex *pindex = nullptr; // Use a temp pindex instead of ppindex to avoid a const_cast bool accepted = m_blockman.AcceptBlockHeader( - header, state, chainparams, &pindex); + header, state, chainparams, &pindex, &duplicate); ::ChainstateActive().CheckBlockIndex(chainparams.GetConsensus()); + if (all_duplicate) { + (*all_duplicate) &= duplicate; // False if any are false + } if (!accepted) { return false; } diff --git a/src/validation.h b/src/validation.h index f1891be4cbd..174a19eac77 100644 --- a/src/validation.h +++ b/src/validation.h @@ -440,7 +440,8 @@ class BlockManager const CBlockHeader& block, BlockValidationState& state, const CChainParams& chainparams, - CBlockIndex** ppindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + CBlockIndex** ppindex, + bool* duplicate = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); ~BlockManager() { Unload(); @@ -926,7 +927,7 @@ class ChainstateManager * @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, BlockValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main); + bool ProcessNewBlockHeaders(const std::vector& block, BlockValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex = nullptr, bool* all_duplicate = nullptr) LOCKS_EXCLUDED(cs_main); //! Load the block tree and coins database from disk, initializing state if we're running with -reindex bool LoadBlockIndex(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);