From f5d33d8676bd8dffeb93da9f35cc53ba114310cd 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 | 22 ++++++++++++++++++---- src/validation.h | 2 +- 3 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1123d66638..6c0d395683 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1423,7 +1423,8 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve CValidationState state; CBlockHeader first_invalid_header; - if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast, &first_invalid_header)) { + bool all_duplicate = false; + if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast, &first_invalid_header, &all_duplicate)) { int nDoS; if (state.IsInvalid(nDoS)) { LOCK(cs_main); @@ -1489,10 +1490,11 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve nodestate->m_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); connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::GETHEADERS, chainActive.GetLocator(pindexLast), uint256())); } diff --git a/src/validation.cpp b/src/validation.cpp index 42763927ca..1b4b2a4883 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -169,7 +169,7 @@ public: * If a block header hasn't already been seen, call CheckBlockHeader on it, ensure * that it doesn't descend from an invalid block, and then add it to mapBlockIndex. */ - bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool* duplicate = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main); bool AcceptBlock(const std::shared_ptr& pblock, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool fRequested, const CDiskBlockPos* dbp, bool* fNewBlock) EXCLUSIVE_LOCKS_REQUIRED(cs_main); // Block (dis)connection on a given view: @@ -3765,16 +3765,22 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c return true; } -bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex) +bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool* duplicate) { AssertLockHeld(cs_main); // Check for duplicate uint256 hash = block.GetHash(); BlockMap::iterator miSelf = mapBlockIndex.find(hash); CBlockIndex *pindex = nullptr; + if (duplicate) { + *duplicate = false; + } if (hash != chainparams.GetConsensus().hashGenesisBlock) { if (miSelf != mapBlockIndex.end()) { // Block header is already known. + if (duplicate) { + *duplicate = true; + } pindex = miSelf->second; if (ppindex) *ppindex = pindex; @@ -3847,14 +3853,22 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState& } // Exposed wrapper for AcceptBlockHeader -bool ProcessNewBlockHeaders(const std::vector& headers, CValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex, CBlockHeader *first_invalid) +bool ProcessNewBlockHeaders(const std::vector& headers, CValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex, CBlockHeader *first_invalid, bool* all_duplicate) { if (first_invalid != nullptr) first_invalid->SetNull(); { 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 - if (!g_chainstate.AcceptBlockHeader(header, state, chainparams, &pindex)) { + bool accepted = g_chainstate.AcceptBlockHeader(header, state, chainparams, &pindex, &duplicate); + if (all_duplicate) { + (*all_duplicate) &= duplicate; // False if any are false + } + if (!accepted) { if (first_invalid) *first_invalid = header; return false; } diff --git a/src/validation.h b/src/validation.h index 7d578c73ad..f079d2cbf8 100644 --- a/src/validation.h +++ b/src/validation.h @@ -255,7 +255,7 @@ bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr& block, CValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex = nullptr, CBlockHeader* first_invalid = nullptr) LOCKS_EXCLUDED(cs_main); +bool ProcessNewBlockHeaders(const std::vector& block, CValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex = nullptr, CBlockHeader* first_invalid = nullptr, bool* all_duplicate = nullptr) LOCKS_EXCLUDED(cs_main); /** Check whether enough disk space is available for an incoming block */ bool CheckDiskSpace(uint64_t nAdditionalBytes = 0, bool blocks_dir = false);