bch(M5): re-download a merkle-rejected block body (no height blackhole)

src/impl/bch/coin/block_download.hpp

@@ -27,10 +27,11 @@
 // re-requested, so an overlapping getheaders locator batch or a re-announce
 // cannot double-download.
 //
-// NOT IN THIS SLICE (deferred per integrator 2026-06-18): in-flight timeout /
-// eviction (a block requested but never delivered by a stalling peer). That is
-// robustness hardening worth doing only once blocks are actually flowing; this
-// slice builds the happy-path window first.
+// IN-FLIGHT TIMEOUT / EVICTION (expire()): a block getdata'd but never delivered
+// by a stalling peer is timed out, requeued oldest-first, and re-issued; an
+// evicted block the peer later delivers anyway is flagged via false_evict_count
+// and dropped from the queue so it is not re-downloaded. NodeP2P drives this on
+// a steady-clock timer (see p2p_node.hpp BLOCK_DL_TIMEOUT_SEC).
 //
 // p2pool-merged-v36 SURFACE: NONE -- pure SPV/IBD wire-sync plumbing; no PoW
 // hash, share format, coinbase commitment, AuxPoW, or PPLNS math. PER-COIN
@@ -54,6 +55,13 @@ namespace bch::coin::block_download {
 /// peer's pipe full without unbounded memory on a slow one.
 inline constexpr std::size_t DEFAULT_MAX_BLOCKS_IN_FLIGHT = 16;
 
+/// Default cap on re-download attempts for a block whose BODY failed local
+/// validation (merkle-root mismatch in emit_full_block). A flaky or malicious
+/// peer that repeatedly serves the same bad body must not drive an unbounded
+/// re-getdata loop, so after this many rejects the hash is abandoned (left in
+/// m_known, never re-queued) and the caller should consider peer demotion.
+inline constexpr std::size_t DEFAULT_MAX_BLOCK_REJECTS = 3;
+
 /// Bounded headers-first block-download window. Peer-free + deterministic:
 /// callers feed it learned header hashes (enqueue), drain the requests it wants
 /// issued now (next_requests), and report arrivals (on_block_received) which
@@ -109,7 +117,18 @@ class BlockDownloadWindow {
         // On a healthy peer with a generous timeout NOTHING expires, so this
         // stays 0 -- a non-zero value means the BLOCK_DL_TIMEOUT_SEC fired on a
         // request the peer was merely slow to answer, not genuinely stalled.
-        if (m_evicted.erase(h)) ++m_false_evict_count;
+        if (m_evicted.erase(h)) {
+            ++m_false_evict_count;
+            // expire() requeued this hash to the FRONT when it timed out; the
+            // (merely slow) peer has now delivered it, so drop it from the pending
+            // queue -- otherwise the next drain would re-getdata a block we already
+            // hold. Linear scan is fine: eviction is rare (a healthy sync keeps
+            // false_evict at 0). If it was already re-issued it is in m_in_flight,
+            // not m_queue, and the scan simply finds nothing.
+            for (auto qit = m_queue.begin(); qit != m_queue.end(); ++qit) {
+                if (*qit == h) { m_queue.erase(qit); break; }
+            }
+        }
         auto it = m_in_flight.find(h);
         if (it == m_in_flight.end()) {
             // Unsolicited or already handled: still remember it so a later
@@ -121,6 +140,36 @@ class BlockDownloadWindow {
         return true;
     }
 
+    /// Report an arrived block whose BODY FAILED local validation (a merkle-root
+    /// mismatch in emit_full_block). Contrast on_block_received, which remembers
+    /// the hash permanently and would thus BLACKHOLE the height -- enqueue()
+    /// skips anything in m_known, so a once-rejected block could never be
+    /// re-requested and the ABLA feed / block-connector would silently skip that
+    /// height forever. This instead frees the window slot and RE-QUEUES the hash
+    /// to the FRONT for another download attempt (ideally answered by a healthier
+    /// peer). Bounded: after max_rejects attempts the hash is abandoned (left in
+    /// m_known, no further re-queue) so a peer that persistently lies cannot
+    /// drive an unbounded getdata loop; the caller logs / demotes that peer.
+    /// Returns true iff the block was re-queued for another attempt.
+    bool on_block_rejected(const uint256& h,
+                           std::size_t max_rejects = DEFAULT_MAX_BLOCK_REJECTS)
+    {
+        m_in_flight.erase(h);   // free the window slot (no-op if not in flight)
+        m_evicted.erase(h);     // a rejected body also clears any stale eviction marker
+        ++m_reject_count;
+        if (++m_rejects[h] > max_rejects) {
+            // Give up: leave it in m_known so it is neither re-queued nor
+            // re-counted, and stop hammering the peer.
+            return false;
+        }
+        // Re-download: push to the FRONT (retry before fresh tip blocks), but
+        // only if not already pending -- expire() may have requeued it. m_known
+        // still holds it, so a parallel headers batch won't add a second copy.
+        for (const auto& q : m_queue) if (q == h) return true;
+        m_queue.push_front(h);
+        return true;
+    }
+
     /// Evict in-flight requests that have gone stale -- a block we getdata'd but a
     /// stalling peer never delivered. now_tick and timeout_ticks are in the caller's
     /// monotonic unit (the same unit passed to next_requests()); any request issued
@@ -171,6 +220,11 @@ class BlockDownloadWindow {
     /// Read-only IBD evidence alongside reissue_count().
     std::size_t false_evict_count() const { return m_false_evict_count; }
 
+    /// Cumulative count of arrived block bodies that FAILED local validation
+    /// (merkle mismatch) and were routed through on_block_rejected. 0 on a clean
+    /// sync; non-zero means a peer served a corrupt body. Read-only IBD evidence.
+    std::size_t reject_count() const { return m_reject_count; }
+
 private:
     std::size_t m_max_in_flight;
     std::deque<uint256> m_queue;                            // pending, chain order
@@ -179,6 +233,8 @@ class BlockDownloadWindow {
     std::size_t m_reissue_count = 0;                       // lifetime re-issues (stall-driven)
     std::unordered_set<uint256, HashHasher> m_evicted;     // expired, awaiting (re-)arrival
     std::size_t m_false_evict_count = 0;                   // evicted-then-arrived (premature)
+    std::size_t m_reject_count = 0;                        // arrived-but-invalid (merkle mismatch)
+    std::unordered_map<uint256, std::size_t, HashHasher> m_rejects; // hash -> reject attempts
 };
 
 } // namespace bch::coin::block_download

src/impl/bch/coin/p2p_node.hpp

@@ -721,7 +721,7 @@ class NodeP2P : public core::ICommunicator, public core::INetwork, public core::
     // where the resulting discontinuity would (safely) sink the template budget
     // to the 32 MB floor. Mirrors BCHN CheckMerkleRoot (validation.cpp) at
     // accept time. p2pool-merged-v36 surface: NONE -- local accept gate only.
-    void emit_full_block(const BlockType& block, const uint256& blockhash)
+    bool emit_full_block(const BlockType& block, const uint256& blockhash)
     {
         std::vector<uint256> txids;
         txids.reserve(block.m_txs.size());
@@ -735,9 +735,10 @@ class NodeP2P : public core::ICommunicator, public core::INetwork, public core::
                         << block.m_merkle_root.GetHex().substr(0, 16) << "... vs computed "
                         << computed.GetHex().substr(0, 16) << "...) over "
                         << block.m_txs.size() << " txs -- dropping, full_block NOT emitted";
-            return;
+            return false;
         }
         m_coin->full_block.happened(block);
+        return true;
     }
 
     ADD_P2P_HANDLER(block)
@@ -758,11 +759,19 @@ class NodeP2P : public core::ICommunicator, public core::INetwork, public core::
         LOG_INFO << "[" << m_chain_label << "] Full block received: "
                  << blockhash.GetHex().substr(0, 16) << "..."
                  << " txs=" << block.m_txs.size();
-        emit_full_block(block, blockhash);
+        const bool accepted = emit_full_block(block, blockhash);
 
         // IBD window: this block freed a slot (if it was one we requested) --
-        // top the window back up so the next queued block bodies stream in.
-        m_block_dl.on_block_received(blockhash);
+        // top the window back up so the next queued block bodies stream in. A
+        // body that FAILED local validation (merkle mismatch) must NOT be marked
+        // permanently received -- on_block_received keeps it in m_known forever,
+        // blackholing the height so it is never re-downloaded and the ABLA feed /
+        // block-connector starve there. Route the drop through on_block_rejected
+        // so the slot frees and the height is re-requested (bounded retries).
+        if (accepted)
+            m_block_dl.on_block_received(blockhash);
+        else
+            m_block_dl.on_block_rejected(blockhash);
         drain_block_window();
     }
 

src/impl/bch/test/block_download_test.cpp

@@ -25,6 +25,7 @@
 
 using bch::coin::block_download::BlockDownloadWindow;
 using bch::coin::block_download::DEFAULT_MAX_BLOCKS_IN_FLIGHT;
+using bch::coin::block_download::DEFAULT_MAX_BLOCK_REJECTS;
 
 // Deterministic distinct hashes (no Math.random) -- hash i = uint256(i+1).
 static uint256 H(uint64_t i) { return uint256(base_uint<256>(i + 1)); }
@@ -201,6 +202,87 @@ int main()
         assert(w.reissue_count() == 1);
     }
 
+    // ---- evicted-then-arrived: false_evict accounting + NO re-download ------
+    //      A block we expired() but the (merely slow) peer delivers anyway is a
+    //      PREMATURE eviction. The window must (a) flag it via false_evict_count
+    //      -- distinct from a clean arrival -- and (b) drop it from the pending
+    //      queue so the next drain does NOT re-getdata a block we already hold.
+    {
+        BlockDownloadWindow w(2);
+        w.enqueue(hashes(0, 2));                    // [H0 H1]
+        w.next_requests(/*now=*/100);               // H0,H1 issued @100
+        assert(w.on_block_received(H(1)) == true);  // H1 arrives clean
+        assert(w.false_evict_count() == 0);         // a clean arrival is NOT premature
+
+        // H0 stalls past the timeout -> evicted + requeued to the front.
+        auto evicted = w.expire(/*now=*/200, /*timeout=*/50);
+        assert(evicted.size() == 1 && evicted[0] == H(0));
+        assert(w.reissue_count() == 1);
+        assert(w.queued() == 1 && w.in_flight() == 0);
+
+        // The peer was merely slow: H0 arrives AFTER its eviction.
+        assert(w.on_block_received(H(0)) == false); // unsolicited (no longer in flight)
+        assert(w.false_evict_count() == 1);         // flagged as a premature eviction
+
+        // ...and it must NOT be re-requested -- we already hold the body.
+        assert(w.queued() == 0);
+        assert(w.next_requests(/*now=*/300).empty());
+        assert(w.idle());
+    }
+
+    // ---- arrived-but-INVALID body: re-download, do NOT blackhole the height -
+    //      A block whose body fails local validation (merkle mismatch) must be
+    //      RE-QUEUED rather than marked permanently received -- on_block_received
+    //      keeps the hash in m_known forever, and enqueue() dedupes on m_known,
+    //      so a once-rejected block could never be re-requested and that height
+    //      would starve the ABLA feed / block-connector. on_block_rejected frees
+    //      the slot and requeues to the front; the height recovers.
+    {
+        BlockDownloadWindow w(2);
+        w.enqueue(hashes(0, 2));                       // [H0 H1]
+        auto req = w.next_requests();                  // H0,H1 issued
+        assert(req.size() == 2 && w.in_flight() == 2);
+        assert(w.reject_count() == 0);
+
+        // H0's body arrives but fails validation -> re-queued, slot freed.
+        assert(w.on_block_rejected(H(0)) == true);
+        assert(w.reject_count() == 1);
+        assert(w.in_flight() == 1);                    // slot freed
+        assert(w.queued() == 1);                       // H0 back in the queue
+        // The next drain re-requests H0 (front) -- the height is NOT blackholed.
+        auto re = w.next_requests();
+        assert(re.size() == 1 && re[0] == H(0));
+        assert(w.in_flight() == 2);
+
+        // A rejected body never DOUBLE-queues even if expire() already requeued
+        // it: force-evict everything to the front, then reject H0 again.
+        w.expire(/*now=*/1, /*timeout=*/0);            // H0,H1 -> front of queue
+        assert(w.queued() == 2 && w.in_flight() == 0);
+        assert(w.on_block_rejected(H(0)) == true);     // already pending: no dup
+        assert(w.queued() == 2);                       // still exactly two queued
+        assert(w.reject_count() == 2);
+    }
+
+    // ---- bounded rejects: a persistently-bad peer cannot loop forever -------
+    //      After DEFAULT_MAX_BLOCK_REJECTS re-downloads the hash is abandoned
+    //      (returns false, not re-queued) and stays deduped so a re-announce
+    //      does not resurrect it -- the cap that stops an unbounded getdata loop.
+    {
+        BlockDownloadWindow w(1);
+        w.enqueue(hashes(0, 1));                        // [H0]
+        w.next_requests();                              // H0 in flight
+        for (std::size_t i = 0; i < DEFAULT_MAX_BLOCK_REJECTS; ++i) {
+            assert(w.on_block_rejected(H(0)) == true);  // re-queued...
+            w.next_requests();                          // ...and re-issued
+        }
+        // The next reject exceeds the budget -> abandoned.
+        assert(w.on_block_rejected(H(0)) == false);
+        assert(w.queued() == 0 && w.in_flight() == 0);  // not re-queued
+        // Still deduped: a re-announce does not resurrect an abandoned hash.
+        assert(w.enqueue(hashes(0, 1)) == 0);
+        assert(w.queued() == 0);
+    }
+
     std::cout << "bch block_download window: ALL PASS\n";
     return 0;
 }