From f25b8712c368ecfa250a6878af66f3fea3a4bcb0 Mon Sep 17 00:00:00 2001
From: Sjors Provoost <sjors@sprovoost.nl>
Date: Mon, 5 Jun 2023 13:45:55 +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 | 28 ++++++++++++++++++++++++++--
 src/validation.cpp     | 17 -----------------
 src/validation.h       |  1 +
 3 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 51bcaf6d73f96d..1fe4f3dab19e2b 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1032,6 +1032,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)
@@ -2928,6 +2930,8 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
     }
     assert(pindexLast);
 
+    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.
@@ -3200,6 +3204,26 @@ void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr<const CBlo
     }
 }
 
+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.ActiveChainstate().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<bool>& interruptMsgProc)
@@ -4268,8 +4292,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         }
 
         if (received_new_header) {
-            LogPrintfCategory(BCLog::NET, "Saw new cmpctblock header hash=%s peer=%d\n",
-                blockhash.ToString(), pfrom.GetId());
+            assert(pindex);
+            LogBlockHeader(*pindex, pfrom.GetId(), /*via_compact_block=*/true);
         }
 
         // When we succeed in decoding a block's txids from a cmpctblock
diff --git a/src/validation.cpp b/src/validation.cpp
index 5fd2d05447e000..04b99b02c42c93 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3821,23 +3821,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 (ActiveChainstate().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 444fe72db4ea4c..bd987cfcd2a5d7 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -1114,6 +1114,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<CBlockHeader>& block, bool min_pow_checked, BlockValidationState& state, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main);