Skip to content

Fix elements multiple-header-download issue. #1069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stevenroose merged 1 commit into from
Nov 26, 2021
Merged

Fix elements multiple-header-download issue. #1069

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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()));
}
Expand Down
19 changes: 16 additions & 3 deletions src/validation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

@stevenroose stevenroose Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a minute to realize that if (duplicate) was a null-check on the pointer, not using the bool 😅

*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;
Expand Down Expand Up @@ -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;
}
Expand Down
5 changes: 3 additions & 2 deletions src/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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<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);
Expand Down