Merge ElementsProject/elements#1069: Fix elements multiple-header-download issue.

cdfb4c9c6c Fix elements multiple-header-download issue. (Glenn Willen)

Pull request description:

  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.

  With luck, this may help with #1013. But it's primarily directed at fixing slow initial block download, which I can't immediately find an open issue for on this repo, but has been a long-standing problem.

ACKs for top commit:
  stevenroose:
    looks good, utACK cdfb4c9c6c

Tree-SHA512: 693a8f89cae1819292146b5bc676e34cd9045bf3a771d78959d8f704072020d5ccb073d6d5b89786598129fd9fd4f80d2c207a2db29597984fcae774cbfacadb
This commit is contained in:
Steven Roose 2021-11-26 11:57:36 +00:00
commit b707a6f00f
No known key found for this signature in database
GPG key ID: 2F2A88D7F8D68E87
3 changed files with 23 additions and 7 deletions

View file

@ -1843,7 +1843,8 @@ void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vector<CBlockHe
}
BlockValidationState state;
if (!m_chainman.ProcessNewBlockHeaders(headers, state, m_chainparams, &pindexLast)) {
bool all_duplicate = false;
if (!m_chainman.ProcessNewBlockHeaders(headers, state, m_chainparams, &pindexLast, &all_duplicate)) {
if (state.IsInvalid()) {
MaybePunishNodeForBlock(pfrom.GetId(), state, via_compact_block, "invalid header received");
return;
@ -1869,10 +1870,11 @@ void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vector<CBlockHe
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);
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexLast), uint256()));
}

View file

@ -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<CBlockHeader>& headers, BlockValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex)
bool ChainstateManager::ProcessNewBlockHeaders(const std::vector<CBlockHeader>& 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;
}

View file

@ -440,7 +440,8 @@ public:
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 @@ public:
* @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<CBlockHeader>& block, BlockValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main);
bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& 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);