From 050c345e6e9dec05d15e89486e13656d80887f4d Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 1 Aug 2023 15:32:26 +0200 Subject: [PATCH] validation: log which peer sent us a header Since 2c3a90f663a61ee147d785167a2902494d81d34d we log received headers. For compact blocks we also log which peer sent it (e5ce8576349d404c466b2f4cab1ca7bf920904b2), but not for regular headers. That required an additional refactor, which this commit provides, to move the logging from validation to net_processing. This also reduces the number of log entries (under default configuration) per compact block header from 3 to 2: one for the header and one for the connected tip. --- src/net_processing.cpp | 36 +++++++++++++++++++++++++++++++----- src/validation.cpp | 17 ----------------- src/validation.h | 1 + 3 files changed, 32 insertions(+), 22 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8189d6c9f3b7a..e73a8b650dc95 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1021,6 +1021,8 @@ class PeerManagerImpl final : public PeerManager void AddAddressKnown(Peer& peer, const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); void PushAddress(Peer& peer, const CAddress& addr, FastRandomContext& insecure_rand) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); + + void LogBlockHeader(const CBlockIndex& index, const NodeId peer_id, const bool via_compact_block); }; const CNodeState* PeerManagerImpl::State(NodeId pnode) const EXCLUSIVE_LOCKS_REQUIRED(cs_main) @@ -2883,7 +2885,8 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, // Now process all the headers. BlockValidationState state; - if (!m_chainman.ProcessNewBlockHeaders(headers, /*min_pow_checked=*/true, state, &pindexLast)) { + const bool processed = m_chainman.ProcessNewBlockHeaders(headers, /*min_pow_checked=*/true, state, &pindexLast); + if (!processed) { if (state.IsInvalid()) { MaybePunishNodeForBlock(pfrom.GetId(), state, via_compact_block, "invalid header received"); return; @@ -2891,6 +2894,10 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, } assert(pindexLast); + if (processed && received_new_header) { + LogBlockHeader(*pindexLast, pfrom.GetId(), /*via_compact_block=*/false); + } + // Consider fetching more headers if we are not using our headers-sync mechanism. if (nCount == MAX_HEADERS_RESULTS && !have_headers_sync) { // Headers message had its maximum size; the peer may have more headers. @@ -3250,6 +3257,26 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl return; } +void PeerManagerImpl::LogBlockHeader(const CBlockIndex& index, const NodeId peer_id, const bool via_compact_block) { + // To prevent log spam, this function should only be called after it was determined that a + // header is both new and valid. + // + // These messages are valuable for detecting potential selfish mining behavior; + // if multiple displacing headers are seen near simultaneously across many + // nodes in the network, this might be an indication of selfish mining. + // In addition it can be used to identify peers which send us a header, but + // don't followup with a complete and valid (compact) block. + // Having this log by default when not in IBD ensures broad availability of + // this data in case investigation is merited. + const auto msg = strprintf( + "Saw new %sheader hash=%s height=%d peer=%d", via_compact_block ? "cmpctblock " : "", index.GetBlockHash().ToString(), index.nHeight, peer_id); + if (m_chainman.IsInitialBlockDownload()) { + LogPrintLevel(BCLog::VALIDATION, BCLog::Level::Debug, "%s\n", msg); + } else { + LogPrintf("%s\n", msg); + } +} + void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) @@ -4308,9 +4335,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, } } + // If AcceptBlockHeader returned true, it set pindex + assert(pindex); if (received_new_header) { - LogPrintfCategory(BCLog::NET, "Saw new cmpctblock header hash=%s peer=%d\n", - blockhash.ToString(), pfrom.GetId()); + LogBlockHeader(*pindex, pfrom.GetId(), /*via_compact_block=*/true); } bool fProcessBLOCKTXN = false; @@ -4326,8 +4354,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, { LOCK(cs_main); - // If AcceptBlockHeader returned true, it set pindex - assert(pindex); UpdateBlockAvailability(pfrom.GetId(), pindex->GetBlockHash()); CNodeState *nodestate = State(pfrom.GetId()); diff --git a/src/validation.cpp b/src/validation.cpp index ed9889d9dd368..6cfe10d373382 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3867,23 +3867,6 @@ bool ChainstateManager::AcceptBlockHeader(const CBlockHeader& block, BlockValida if (ppindex) *ppindex = pindex; - // Since this is the earliest point at which we have determined that a - // header is both new and valid, log here. - // - // These messages are valuable for detecting potential selfish mining behavior; - // if multiple displacing headers are seen near simultaneously across many - // nodes in the network, this might be an indication of selfish mining. Having - // this log by default when not in IBD ensures broad availability of this data - // in case investigation is merited. - const auto msg = strprintf( - "Saw new header hash=%s height=%d", hash.ToString(), pindex->nHeight); - - if (IsInitialBlockDownload()) { - LogPrintLevel(BCLog::VALIDATION, BCLog::Level::Debug, "%s\n", msg); - } else { - LogPrintf("%s\n", msg); - } - return true; } diff --git a/src/validation.h b/src/validation.h index ba427afc64fa6..672529f937c83 100644 --- a/src/validation.h +++ b/src/validation.h @@ -1135,6 +1135,7 @@ class ChainstateManager * @param[in] min_pow_checked True if proof-of-work anti-DoS checks have been done by caller for headers chain * @param[out] state This may be set to an Error state if any error occurred processing them * @param[out] ppindex If set, the pointer will be set to point to the last new block index object for the given headers + * @returns false if AcceptBlockHeader fails on any of the headers, true otherwise (including if headers were already known) */ bool ProcessNewBlockHeaders(const std::vector& block, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main);