Merge #18962: net processing: Only send a getheaders for one block in an INV

746736639e6d05acdb85c866d4c605c947d4c500 [net processing] Only send a getheaders for one block in an INV (John Newbery)

Pull request description:

  Headers-first is the primary method of announcement on the network. If a node fell back sending blocks by inv, it's probably for a re-org. The final block hash provided should be the highest, so send a getheaders and then fetch the blocks we need to catch up.

  Sending many GETHEADERS messages to the peer would cause them to send a large number of potentially large HEADERS messages with redundant data, which is a waste of bandwidth.

ACKs for top commit:
  sipa:
    utACK 746736639e6d05acdb85c866d4c605c947d4c500
  mzumsande:
    utACK 746736639e6d05acdb85c866d4c605c947d4c500 as per ajtowns' reasoning.
  naumenkogs:
    utACK 7467366
  ajtowns:
    ACK 746736639e6d05acdb85c866d4c605c947d4c500
  jonatack:
    ACK 746736639e6d05acdb85c866d4c605c947d4c500

Tree-SHA512: 59e243b80d3f0873709dfacb2e4ffba34689aad7de31ec7f69a64e0e3a0756235a0150e4082ff5de823949ba4411ee1aed2344b4749b62e0eb1ea906e41f5ea9

1 files changed, 14 insertions, 10 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1df1fab59d..7e9bb2f27c 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2420,6 +2420,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
 
         uint32_t nFetchFlags = GetFetchFlags(pfrom);
         const auto current_time = GetTime<std::chrono::microseconds>();
+        uint256* best_block{nullptr};
 
         for (CInv &inv : vInv)
         {
@@ -2436,17 +2437,14 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
             if (inv.type == MSG_BLOCK) {
                 UpdateBlockAvailability(pfrom->GetId(), inv.hash);
                 if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) {
-                    // We used to request the full block here, but since headers-announcements are now the
-                    // primary method of announcement on the network, and since, in the case that a node
-                    // fell back to inv we probably have a reorg which we should get the headers for first,
-                    // we now only provide a getheaders response here. When we receive the headers, we will
-                    // then ask for the blocks we need.
-                    connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexBestHeader), inv.hash));
-                    LogPrint(BCLog::NET, "getheaders (%d) %s to peer=%d\n", pindexBestHeader->nHeight, inv.hash.ToString(), pfrom->GetId());
+                    // Headers-first is the primary method of announcement on
+                    // the network. If a node fell back to sending blocks by inv,
+                    // it's probably for a re-org. The final block hash
+                    // provided should be the highest, so send a getheaders and
+                    // then fetch the blocks we need to catch up.
+                    best_block = &inv.hash;
                 }
-            }
-            else
-            {
+            } else {
                 pfrom->AddInventoryKnown(inv);
                 if (fBlocksOnly) {
                     LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom->GetId());
@@ -2457,6 +2455,12 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
                 }
             }
         }
+
+        if (best_block != nullptr) {
+            connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::GETHEADERS, ::ChainActive().GetLocator(pindexBestHeader), *best_block));
+            LogPrint(BCLog::NET, "getheaders (%d) %s to peer=%d\n", pindexBestHeader->nHeight, best_block->ToString(), pfrom->GetId());
+        }
+
         return true;
     }