From df8d57896cdf172462be69e347a5b7c2cf4b96d1 Mon Sep 17 00:00:00 2001 From: frstrtr Date: Fri, 19 Jun 2026 12:06:20 +0000 Subject: [PATCH 1/2] bch(M5): drop evicted-then-arrived block from the download queue expire() requeues a timed-out in-flight block to the FRONT of the pending queue and records it in m_evicted. If the (merely slow) peer then delivers that block, on_block_received bumped false_evict_count but left the hash in the queue -- so the next next_requests() drain re-getdata'd a block already in hand. Remove the hash from the queue on the false-evict path (linear scan; eviction is rare, healthy sync keeps false_evict at 0). A hash already re-issued sits in m_in_flight, not the queue, so the scan is a no-op there. Adds an evicted-then-arrived test pinning both legs: false_evict_count flags the premature eviction (distinct from a clean arrival) AND queued()==0 / a following drain is empty -- no re-download. Refreshes the stale header note that still claimed eviction was deferred (it is implemented and NodeP2P-driven). PURE block-download window plumbing; p2pool-merged-v36 surface NONE; per-coin isolation src/impl/bch/ only; header + test build-inert (bch skip-green). --- src/impl/bch/coin/block_download.hpp | 22 ++++++++++++++---- src/impl/bch/test/block_download_test.cpp | 28 +++++++++++++++++++++++ 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/impl/bch/coin/block_download.hpp b/src/impl/bch/coin/block_download.hpp index 49e1277f0..16b5703d3 100644 --- a/src/impl/bch/coin/block_download.hpp +++ b/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 @@ -109,7 +110,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 diff --git a/src/impl/bch/test/block_download_test.cpp b/src/impl/bch/test/block_download_test.cpp index 0d5567626..b27308bbc 100644 --- a/src/impl/bch/test/block_download_test.cpp +++ b/src/impl/bch/test/block_download_test.cpp @@ -201,6 +201,34 @@ 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()); + } + std::cout << "bch block_download window: ALL PASS\n"; return 0; } From f31183c6ff501be6b3b2a70c75a8e4b978ee86a0 Mon Sep 17 00:00:00 2001 From: frstrtr Date: Fri, 19 Jun 2026 12:42:03 +0000 Subject: [PATCH 2/2] bch(M5): re-download a block whose body fails local validation A full block whose body failed the merkle-root check in emit_full_block was dropped, yet the block handler still called on_block_received -- which keeps the hash in m_known permanently. enqueue() dedupes on m_known, so the height could never be re-requested: a single corrupt or malicious body silently and permanently blackholed that height, starving the ABLA size feed and the block-connector there forever. Add BlockDownloadWindow::on_block_rejected: free the window slot and re-queue the hash to the front for another download attempt, bounded by DEFAULT_MAX_BLOCK_REJECTS so a peer that persistently serves a bad body cannot drive an unbounded getdata loop (after the cap the hash is abandoned, still deduped). Route the merkle-drop path through it; the compact-block reconstruction paths are unaffected (data is local). Pure SPV/IBD plumbing -- no PoW, share format, coinbase commitment, or PPLNS surface. Per-coin isolation: src/impl/bch/ only. --- src/impl/bch/coin/block_download.hpp | 44 ++++++++++++++++++ src/impl/bch/coin/p2p_node.hpp | 19 +++++--- src/impl/bch/test/block_download_test.cpp | 54 +++++++++++++++++++++++ 3 files changed, 112 insertions(+), 5 deletions(-) diff --git a/src/impl/bch/coin/block_download.hpp b/src/impl/bch/coin/block_download.hpp index 16b5703d3..ca2eff2c5 100644 --- a/src/impl/bch/coin/block_download.hpp +++ b/src/impl/bch/coin/block_download.hpp @@ -55,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 @@ -133,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 @@ -183,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 m_queue; // pending, chain order @@ -191,6 +233,8 @@ class BlockDownloadWindow { std::size_t m_reissue_count = 0; // lifetime re-issues (stall-driven) std::unordered_set 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 m_rejects; // hash -> reject attempts }; } // namespace bch::coin::block_download diff --git a/src/impl/bch/coin/p2p_node.hpp b/src/impl/bch/coin/p2p_node.hpp index 7dfdb1d1d..c6135e906 100644 --- a/src/impl/bch/coin/p2p_node.hpp +++ b/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 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(); } diff --git a/src/impl/bch/test/block_download_test.cpp b/src/impl/bch/test/block_download_test.cpp index b27308bbc..c20cf5176 100644 --- a/src/impl/bch/test/block_download_test.cpp +++ b/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)); } @@ -229,6 +230,59 @@ int main() 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; }