net: keep a block tracked until its body actually validates

The mapBlockFetch entry was erased before ProcessBlock ran, on the
reasoning that the body had arrived so the fetch was over either way.
That reasoning does not survive the fact that GetHash() covers the
80-byte header and nothing else.

Any peer that saw X announced holds X's header. It can answer our
getdata with that header and a transaction list of its own invention;
the reply still hashes to X, so the entry was erased, and CheckBlock
then rejected the body on hashMerkleRoot and stored nothing. AlreadyHave
stayed false, the mapAskFor entry had already been consumed when the
getdata went out, and nothing recreated the tracking -- so X was never
requested from anybody again unless some peer happened to re-announce
it. That is the #33 stall, reachable on demand for one message.

The erase now sits inside the success branch, beside the
mapAlreadyAskedFor erase it belongs with. ProcessBlock returns true in
exactly two cases, block accepted and block parked as an orphan, and
both store it where AlreadyHave will find it, so one erase covers both
and neither needs different treatment. Of the false cases, the two
already-have ones are cleared by the sweep's own AlreadyHave check on
its next pass, and the rest are rejections that must keep the entry.

A mismatched merkle root is also now grounds to drop the peer. Nothing
honest produces it: peer traffic rides the .btf channel, which is
XChaCha20-Poly1305, so a body that arrives at all arrived intact, and
the mismatch is the peer's own construction. Checking it in the handler
also rejects a forged body before CheckBlock spends a RandomX hash on
it. The unconditional log line fires once per connection, because
ProcessMessages keeps draining the receive buffer after fDisconnect is
set and a peer that packs many forged bodies into one buffer must not
get a line for each.

1 files changed, 45 insertions, 5 deletions

diff --git a/src/main.cpp b/src/main.cpp
index d9fe713..8926d57 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -3293,16 +3293,56 @@ bool ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv)
         CInv inv(MSG_BLOCK, pblock->GetHash());
         pfrom->AddInventoryKnown(inv);
 
-        // The body is here, so the fetch is over whatever validation says
-        // next -- unlike the mapAlreadyAskedFor erase below, which stays
-        // conditional on acceptance so a peer feeding us garbage does not
-        // cancel the scheduled retries for the real thing.
-        mapBlockFetch.erase(inv.hash);
+        // GetHash() covers the 80-byte header and nothing else, so the hash
+        // coming out right does not make this the block we asked for. Every
+        // peer that saw X announced holds X's header, and can answer our
+        // getdata with that header bolted onto a transaction list of its own
+        // invention -- the reply still hashes to X. hashMerkleRoot is the only
+        // thing binding the header to the body, so check it here, before the
+        // mapBlockFetch entry is touched and before CheckBlock spends a
+        // RandomX hash on a body already known to be forged.
+        //
+        // There is no honest way to arrive here. Peer traffic rides the .btf
+        // channel, which is XChaCha20-Poly1305, so a body that arrives at all
+        // arrived intact: a mismatch is the peer's own construction, not line
+        // noise. Drop the peer and leave the fetch outstanding so the failover
+        // sweep asks somebody else for the real X, rather than recording X as
+        // delivered on the strength of a header anyone could copy.
+        if (pblock->hashMerkleRoot != pblock->BuildMerkleTree())
+        {
+            if (LogAcceptsCategory("net"))
+                printf("peer %s served a forged body for %s\n",
+                       pfrom->addr.ToString().c_str(), inv.ToString().c_str());
+            // One line per connection: ProcessMessages keeps draining the
+            // receive buffer after fDisconnect is set, so a peer that packs
+            // many forged bodies into one buffer must not get one log line
+            // for each of them.
+            if (!pfrom->fDisconnect)
+                printf("block fetch: a peer answered the request for %s with a body that is not "
+                       "that block -- dropping it\n", inv.hash.ToString().substr(0,14).c_str());
+            pfrom->fDisconnect = true;
+            return error("ProcessMessage() : forged block body for %s",
+                         inv.hash.ToString().substr(0,14).c_str());
+        }
 
         if (ProcessBlock(pfrom, pblock.release()))
         {
             mapAlreadyAskedFor.erase(inv);
 
+            // The body arrived and validated, so the fetch really is over:
+            // the block is now in mapBlockIndex, or parked in mapOrphanBlocks,
+            // and either way AlreadyHave answers for it from here on.
+            //
+            // This has to stay inside the success branch, next to the erase
+            // above. When validation rejects a body nothing is stored,
+            // AlreadyHave stays false, and the mapAskFor entry was already
+            // consumed when the getdata went out -- so erasing the tracking
+            // here would leave the block requested from nobody at all until
+            // some peer happened to re-announce it. That is exactly the stall
+            // of #33, and erasing before ProcessBlock made it reachable on
+            // demand.
+            mapBlockFetch.erase(inv.hash);
+
             // A peer that just handed us a block is demonstrably at least at
             // that block's height. Without this, nStartingHeight is a snapshot
             // taken once during the version handshake and never touched again,