From 2e20434e563b09c5209d0a1f6784d96ed8b18851 Mon Sep 17 00:00:00 2001 From: bitflash-maintainer Date: Wed, 2 Sep 2026 16:00:59 +0000 Subject: 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. --- src/main.cpp | 50 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 45 insertions(+), 5 deletions(-) (limited to 'src/main.cpp') 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, -- cgit v1.2.3