Merge bitcoin/bitcoin#26355: p2p: Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started
7ad15d1100[net processing] Handle IsContinuationOfLowWorkHeadersSync return value correctly when new headers sync is started (dergoegge) Pull request description: This PR fixes a bug in the headers sync logic that enables submitting headers to a nodes block index that don't lead to a chain that surpasses our DoS limit. The issue is that we ignore the return value on [the first `IsContinuationOfLowWorkHeadersSync` call after a new headers sync is started](https://github.com/bitcoin/bitcoin/blob/fabc0310480b49e159a15d494525c5aa15072cba/src/net_processing.cpp#L2553-L2568), which leads to us passing headers to [`ProcessNewBlockHeaders`](https://github.com/bitcoin/bitcoin/blob/fabc0310480b49e159a15d494525c5aa15072cba/src/net_processing.cpp#L2856) when that initial `IsContinuationOfLowWorkHeadersSync` call returns `false`. One easy way (maybe the only?) to trigger this is by sending 2000 headers where the last header has a different `nBits` value than the prior headers (which fails the pre-sync logic [here](https://github.com/bitcoin/bitcoin/blob/fabc0310480b49e159a15d494525c5aa15072cba/src/headerssync.cpp#L189)). Those 2000 headers will be passed to `ProcessNewBlockHeaders`. I haven't included a test here so far because we can't test this without changing the default value for `CRegTestParams::consensus.fPowAllowMinDifficultyBlocks` or doing some more involved refactoring. ACKs for top commit: sipa: ACK7ad15d1100glozow: ACK7ad15d1100Tree-SHA512: 9aabb8bf3700401e79863d0accda0befd2a83c4d469a53f97d827e51139e2f826aee08cdfbc8866b311b153f61fdac9b7aa515fcfa2a21c5e2812c2bf3c03664
This commit is contained in:
1 file changed
+12
-4
+12
-4
@@ -2565,14 +2565,22 @@ bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlo
|
|||||||
|
|
||||||
// Now a HeadersSyncState object for tracking this synchronization is created,
|
// Now a HeadersSyncState object for tracking this synchronization is created,
|
||||||
// process the headers using it as normal.
|
// process the headers using it as normal.
|
||||||
return IsContinuationOfLowWorkHeadersSync(peer, pfrom, headers);
|
if (!IsContinuationOfLowWorkHeadersSync(peer, pfrom, headers)) {
|
||||||
|
// Something went wrong, reset the headers sync.
|
||||||
|
peer.m_headers_sync.reset(nullptr);
|
||||||
|
LOCK(m_headers_presync_mutex);
|
||||||
|
m_headers_presync_stats.erase(peer.m_id);
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
LogPrint(BCLog::NET, "Ignoring low-work chain (height=%u) from peer=%d\n", chain_start_header->nHeight + headers.size(), pfrom.GetId());
|
LogPrint(BCLog::NET, "Ignoring low-work chain (height=%u) from peer=%d\n", chain_start_header->nHeight + headers.size(), pfrom.GetId());
|
||||||
// Since this is a low-work headers chain, no further processing is required.
|
|
||||||
headers = {};
|
|
||||||
return true;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The peer has not yet given us a chain that meets our work threshold,
|
||||||
|
// so we want to prevent further processing of the headers in any case.
|
||||||
|
headers = {};
|
||||||
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user