From 4bffdf0bea3ce5982a93dfbdf67ecdeeb1ce6a3b Mon Sep 17 00:00:00 2001 From: frstrtr Date: Sun, 26 Jul 2026 11:46:25 +0400 Subject: [PATCH 1/4] btc/dgb/bch: gate share broadcast on tx completeness + mark only what was sent Two reward-critical share-loss fixes, ported to BTC, DGB and BCH from the already-correct DASH implementation (src/impl/dash/node.cpp). F3 -- tx-completeness broadcast gate. send_shares() collected the referenced new-tx bytes with `if (it != m_known_txs.end()) full_txs.emplace_back(...)` and no else branch: a tx we do not hold was silently omitted and the share was written to the peer anyway. Canonical p2pool then drops the connection on "referenced unknown transaction" (p2p.py), which isolates us from the sharechain and orphans our shares. Shares pulled via sharereply arrive without their tx bytes, so this is reachable in normal operation. send_shares now filters the outgoing batch through partition_backable() (gate on the BYTES in m_known_txs, not on the peer's possibly-stale have_tx advert) and the now-unreachable omission path increments a counter that logs a warning. F2 -- mark only what was sent. broadcast_share() inserted each walked hash into m_shared_share_hashes INSIDE the chain-walk loop, before send_shares ran. send_shares returned void and has abandon paths (tracker try_to_lock miss, empty batch, and now the F3 gate), and zero connected peers abandons the batch too. A marked-but-unsent share is lost permanently: the next walk breaks on the first marked hash, so nothing ever re-pushes it -- silent PPLNS-credit loss with no retry path. send_shares now returns the hashes it actually wrote and broadcast_share marks only those, after the peer loop, via broadcast_and_mark(). m_last_broadcast_to likewise records what was written rather than what was offered, so a withheld share is never blamed for a peer disconnect (which would mark it rejected and block all future re-broadcast). Consensus surface: NONE. Both changes decide only WHETHER a share goes on the wire. Share bytes, minting, PPLNS weighting and payout are untouched, and a withheld share stays in the chain and is re-offered once its txs arrive. Primitives follow the per-coin header pattern established for BTC in #868: src/impl/{btc,dgb,bch}/known_txs_retention.hpp. The LTC lane is concurrently hoisting all_txs_backable into src/core/; these four per-coin headers (including dash) should be collapsed onto that hoist once both land. Tests ride existing allowlisted CI targets -- no new add_executable, no build.yml change: btc -> btc_share_test (broadcast_gate_test.cpp) dgb -> dgb_share_test (broadcast_gate_test.cpp, known_txs_retention_test.cpp) bch -> bch_embedded_block_broadcast_test (broadcast_gate_test.cpp; the bch test tree has no GTest harness, so the TU exposes a checks function the host main() calls) Mutation-verified red: deleting the gate reddens 2 btc / 4 bch assertions; restoring mark-before-send reddens 4 btc / 5 bch assertions. --- src/impl/bch/known_txs_retention.hpp | 181 +++++++++++++++ src/impl/bch/node.cpp | 97 +++++++- src/impl/bch/node.hpp | 10 +- src/impl/bch/test/CMakeLists.txt | 6 + src/impl/bch/test/broadcast_gate_test.cpp | 211 ++++++++++++++++++ .../test/embedded_block_broadcast_test.cpp | 8 + src/impl/btc/known_txs_retention.hpp | 72 +++++- src/impl/btc/node.cpp | 97 +++++++- src/impl/btc/node.hpp | 10 +- src/impl/btc/test/CMakeLists.txt | 2 +- src/impl/btc/test/broadcast_gate_test.cpp | 208 +++++++++++++++++ src/impl/dgb/known_txs_retention.hpp | 181 +++++++++++++++ src/impl/dgb/node.cpp | 106 ++++++++- src/impl/dgb/node.hpp | 10 +- src/impl/dgb/test/CMakeLists.txt | 6 +- src/impl/dgb/test/broadcast_gate_test.cpp | 208 +++++++++++++++++ .../dgb/test/known_txs_retention_test.cpp | 101 +++++++++ 17 files changed, 1469 insertions(+), 45 deletions(-) create mode 100644 src/impl/bch/known_txs_retention.hpp create mode 100644 src/impl/bch/test/broadcast_gate_test.cpp create mode 100644 src/impl/btc/test/broadcast_gate_test.cpp create mode 100644 src/impl/dgb/known_txs_retention.hpp create mode 100644 src/impl/dgb/test/broadcast_gate_test.cpp create mode 100644 src/impl/dgb/test/known_txs_retention_test.cpp diff --git a/src/impl/bch/known_txs_retention.hpp b/src/impl/bch/known_txs_retention.hpp new file mode 100644 index 000000000..14d481bfc --- /dev/null +++ b/src/impl/bch/known_txs_retention.hpp @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// +// Pure, unit-testable bookkeeping for the known-transaction pool that backs +// share broadcast (remember_tx forwarding). Ported to the BCH variant from +// src/impl/dash/known_txs_retention.hpp so the retention/eviction and +// broadcast-gate semantics can be exercised without a live node. +// +// Two concerns: +// retain_template_txs() — the rolling-window retention with template-identity +// dedup (code review F1): register_template_txs is invoked per producer-job +// cache MISS, i.e. per (prev_share_hash, payout_script), NOT per template +// rotation. A ~50-payout-script cluster therefore re-registers the SAME +// template ~50 times on one tip; pushing each unconditionally collapses the +// N-slot window to a single template within seconds. Dedup by the template's +// tx-hash SET (its identity): a set already retained refreshes recency and +// consumes NO new slot, so N re-registrations of one template use ONE slot +// and the window holds N DISTINCT templates as intended. +// +// all_txs_backable() — the canonical broadcast gate (F3): a share may be sent +// only when we HOLD the bytes of every referenced new-tx (so remember_tx can +// always forward them). This is what makes the "referenced unknown +// transaction" disconnect unreachable by construction, and — paired with the +// mint-time decline of shares whose txs are not retained — makes "every share +// we mint/broadcast is backable" a by-construction invariant. +// +// partition_backable() — the batch form of the gate, applied by send_shares() +// to the shares it is about to write to one peer. +// +// broadcast_and_mark() — the F2 marking policy: broadcast_share() may mark a +// share "already shared" only AFTER a peer actually accepted it. Marking the +// whole chain-walk up front (the pre-fix pattern) loses every share in a +// batch that send_shares then abandons — the gate skipped it, the tracker +// try_to_lock missed, or there were no peers — because the de-dup set then +// breaks the walk forever and nothing re-pushes it. +// +// NOTE (de-dup): the same primitives now exist per-coin (btc/dgb/bch/dash) while +// the LTC lane hoists all_txs_backable into src/core/. Once both land, these +// per-coin headers should be collapsed onto that core hoist. + +#pragma once + +#include +#include +#include +#include + +#include + +namespace bch { + +// Retain the tx set of a newly-registered template in a rolling window of the +// last `cap` DISTINCT templates, inserting its tx bytes into `known_txs`. +// +// recent_sets : rolling history of distinct template tx-hash sets +// (front = oldest, back = newest). +// known_txs : uint256 -> tx map; needs insert_or_assign(k,v) and erase(k). +// hashes/txs : parallel arrays of the template's tx hashes and bytes. +// cap : number of DISTINCT templates to retain. +// +// A tx is erased from `known_txs` only once it has fallen out of EVERY retained +// set. F1 dedup: if this template's tx set is already retained, its recency is +// refreshed (moved to back) and no new slot is consumed. +template +inline void retain_template_txs(std::deque>& recent_sets, + TxMap& known_txs, + const std::vector& hashes, + const std::vector& txs, + std::size_t cap) +{ + if (cap == 0) + return; + + std::set new_set(hashes.begin(), hashes.end()); + + // F1 dedup by template identity (the tx-hash set). If this template is + // already retained, refresh its recency (move to back) — no new slot, no + // re-eviction. This is what stops ~50 payout-script re-registrations of one + // template from evicting the rest of the window. + for (auto it = recent_sets.begin(); it != recent_sets.end(); ++it) { + if (*it == new_set) { + if (std::next(it) != recent_sets.end()) { + std::set refreshed = std::move(*it); + recent_sets.erase(it); + recent_sets.push_back(std::move(refreshed)); + } + return; + } + } + + // Distinct template: insert its tx bytes and consume a slot. + const std::size_t n = std::min(hashes.size(), txs.size()); + for (std::size_t i = 0; i < n; ++i) + known_txs.insert_or_assign(hashes[i], txs[i]); + recent_sets.push_back(std::move(new_set)); + + // Evict the oldest slots beyond `cap`; a tx is removed from `known_txs` only + // once it is absent from EVERY remaining retained set. + while (recent_sets.size() > cap) { + const std::set evicted = std::move(recent_sets.front()); + recent_sets.pop_front(); + for (const auto& h : evicted) { + bool still_retained = false; + for (const auto& s : recent_sets) { + if (s.count(h)) { still_retained = true; break; } + } + if (!still_retained) + known_txs.erase(h); + } + } +} + +// True iff every hash in `tx_hashes` is present in `held` (the bytes we hold, +// e.g. m_known_txs). The canonical broadcast gate: a share is backable — safe to +// send without risking the peer's "referenced unknown transaction" disconnect — +// only when we can forward every referenced tx. +template +inline bool all_txs_backable(const std::vector& tx_hashes, + const Held& held) +{ + for (const auto& h : tx_hashes) + if (held.find(h) == held.end()) + return false; + return true; +} + +// Batch form of the F3 gate. Keeps in `shares` only the entries every one of +// whose referenced new-tx hashes we hold, and returns how many were dropped. +// `hashes_of(share)` yields that share's referenced new-tx hashes. +// +// Skipping costs no propagation: a share we downloaded is by definition already +// on the network we fetched it from, and it will be re-offered on a later +// broadcast once its tx bytes arrive (F2 leaves it un-marked). +template +inline std::size_t partition_backable(std::vector& shares, + const Held& held, + HashesOf hashes_of) +{ + std::vector sendable; + sendable.reserve(shares.size()); + std::size_t skipped = 0; + for (auto& share : shares) { + if (all_txs_backable(hashes_of(share), held)) + sendable.push_back(std::move(share)); + else + ++skipped; + } + shares.swap(sendable); + return skipped; +} + +// F2 marking policy. Offers the batch to every peer via `send(peer)` — which +// returns the hashes it ACTUALLY wrote to that peer — and marks in `marked` +// only the hashes that reached at least one peer. Returns that count. +// +// The pre-fix code marked the whole chain-walk before calling send_shares, so a +// batch abandoned by every peer (gate skip, try_to_lock miss, empty peer set) +// stayed marked forever: the next broadcast_share walk breaks on the first +// marked hash and the share is never re-pushed. That is silent, permanent share +// loss with no retry path — a PPLNS-credit loss for whoever mined it. +template +inline std::size_t broadcast_and_mark(MarkedSet& marked, Peers& peers, + const std::vector& to_send, + SendFn send) +{ + if (to_send.empty()) + return 0; + + // NOTE: `to_send` is deliberately NOT what gets marked. Marking the offered + // batch here is precisely the pre-fix bug this signature exists to make + // un-writable by accident — only `send`'s report may be marked. + std::set actually_sent; + for (auto& entry : peers) { + const std::vector sent = send(entry.second); + actually_sent.insert(sent.begin(), sent.end()); + } + for (const auto& h : actually_sent) + marked.insert(h); + return actually_sent.size(); +} + +} // namespace bch diff --git a/src/impl/bch/node.cpp b/src/impl/bch/node.cpp index 9e0e29cbd..c231fcb53 100644 --- a/src/impl/bch/node.cpp +++ b/src/impl/bch/node.cpp @@ -1,5 +1,6 @@ // SPDX-License-Identifier: AGPL-3.0-or-later #include "node.hpp" +#include "known_txs_retention.hpp" // bch::all_txs_backable / partition_backable / broadcast_and_mark (F2/F3) #include "share_tx_refs.hpp" // bch::new_tx_hashes -- uniform send-side probe (#905) @@ -684,8 +685,13 @@ std::vector NodeImpl::handle_get_share(std::vector hash return shares; } -void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hashes) +std::vector NodeImpl::send_shares(peer_ptr peer, + const std::vector& share_hashes) { + // Returns the hashes of the shares actually WRITTEN to this peer (F2). The + // caller marks only those as broadcast, so a share skipped here — by the + // try_to_lock miss below or by the F3 tx-completeness gate — is re-offered + // on the next broadcast instead of being silently dropped forever. // try_to_lock per the architectural rule (node.hpp:67) — see freeze // analysis in handle_get_share above. If we can't acquire NOW, skip // this batch. The shares are still in our chain; the next broadcast @@ -697,7 +703,7 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash if (defer_log++ % 50 == 0) LOG_INFO << "[send_shares] tracker busy — skipping send to " << peer->addr().to_string() << " (will retry next cycle)"; - return; + return {}; } // Collect shares that exist in our chain (skip rejected) @@ -717,7 +723,48 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash } if (shares.empty()) - return; + return {}; + + // --- F3 tx-completeness broadcast gate -------------------------------- + // A share whose referenced new-tx BYTES we do not hold cannot be backed by + // the remember_tx forward below, and canonical p2pool drops the connection + // on "referenced unknown transaction" (p2p.py) — which isolates us from the + // sharechain and orphans our shares. Shares pulled via sharereply arrive + // WITHOUT their tx bytes, so this is reachable in normal operation. + // + // Gate on the BYTES in m_known_txs, not on the peer's have_tx advert: that + // advert can be stale (the peer may have dropped the tx) and sending + // hash-only for a tx the peer no longer holds is exactly the disconnect. + // m_remote_txs is used ONLY below to choose the hash-vs-bytes encoding. + // + // CONSENSUS-NEUTRAL: this decides only WHETHER we broadcast. Share bytes, + // minting and payout are untouched; a skipped share stays in our chain and + // is re-offered once its txs arrive (send_shares reports it as unsent, so + // broadcast_share leaves it un-marked). + { + const size_t skipped_incomplete = partition_backable( + shares, m_known_txs, [](ShareType& share) { + std::vector hashes; + share.invoke([&](auto* obj) { + if constexpr (requires { obj->m_new_transaction_hashes; }) + hashes.assign(obj->m_new_transaction_hashes.begin(), + obj->m_new_transaction_hashes.end()); + }); + return hashes; + }); + if (skipped_incomplete > 0) + { + static int skip_log = 0; + if (skip_log++ % 20 == 0) + LOG_INFO << "[send_shares] skipped " << skipped_incomplete + << " share(s) whose new-tx bytes we do not hold " + "(downloaded via sharereply / evicted) to " + << peer->addr().to_string() + << " — avoids canonical 'unknown transaction' disconnect"; + } + if (shares.empty()) + return {}; + } // Collect transactions that the peer doesn't know about std::set needed_txs; @@ -749,6 +796,8 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash std::vector known_hashes; // hashes in peer's remote set std::vector full_txs; // full txs otherwise + size_t missing = 0; + for (const auto& th : needed_txs) { if (peer->m_remote_txs.count(th)) @@ -760,9 +809,20 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash auto it = m_known_txs.find(th); if (it != m_known_txs.end()) full_txs.emplace_back(it->second); + else + ++missing; // unreachable after the gate above — see below } } + // Defence in depth: the gate should already have removed every share + // with an unheld tx, so this must stay at zero. If it ever fires, the + // gate's view of the referenced hashes disagrees with needed_txs and + // the peer may drop these shares — make that loud rather than silent. + if (missing > 0) + LOG_WARNING << "[send_shares] " << missing << " referenced tx(s) not in " + "m_known_txs after the completeness gate — peer may drop " + "these shares (share/tx hash accounting mismatch?)"; + if (!known_hashes.empty() || !full_txs.empty()) { auto rtx_msg = message_remember_tx::make_raw(known_hashes, full_txs); @@ -799,6 +859,13 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash LOG_INFO << "[Pool] Sent " << shares.size() << " shares (+" << needed_txs.size() << " txs) to " << peer->addr().to_string(); + + // F2: report exactly which shares reached this peer. + std::vector sent; + sent.reserve(shares.size()); + for (auto& share : shares) + sent.push_back(share.hash()); + return sent; } void NodeImpl::broadcast_share(const uint256& share_hash) @@ -821,7 +888,14 @@ void NodeImpl::broadcast_share(const uint256& share_hash) return; } - // Walk the chain back from share_hash, collecting un-broadcast shares + // Walk the chain back from share_hash, collecting un-broadcast shares. + // + // F2: do NOT mark them shared here. send_shares can abandon the whole batch + // — the tx-completeness gate skipped it, its tracker try_to_lock missed, or + // there are zero peers — and a share marked before that never gets another + // chance: the next walk breaks on the first marked hash, so the tip is + // permanently withheld from the sharechain and its PPLNS credit is lost. + // We mark only what actually reached >= 1 peer, below. std::vector to_send; int32_t height = m_chain->get_height(share_hash); int32_t walk = std::min(height, 5); @@ -832,7 +906,6 @@ void NodeImpl::broadcast_share(const uint256& share_hash) break; if (m_rejected_share_hashes.count(hash)) continue; // skip shares previously rejected by peers - m_shared_share_hashes.insert(hash); to_send.push_back(hash); } @@ -840,10 +913,16 @@ void NodeImpl::broadcast_share(const uint256& share_hash) return; auto now = std::chrono::steady_clock::now(); - for (auto& [nonce, peer] : m_peers) { - send_shares(peer, to_send); - m_last_broadcast_to[peer->addr()] = {to_send, now}; - } + broadcast_and_mark(m_shared_share_hashes, m_peers, to_send, [&](peer_ptr& peer) { + std::vector sent = send_shares(peer, to_send); + // Record what we ACTUALLY wrote, not what we offered: a peer that drops + // within 10s marks this record's hashes rejected, and a rejected hash is + // never re-broadcast. Blaming a share we withheld would turn a transient + // gate skip into permanent loss. + if (!sent.empty()) + m_last_broadcast_to[peer->addr()] = {sent, now}; + return sent; + }); } void NodeImpl::notify_local_share(const uint256& share_hash) diff --git a/src/impl/bch/node.hpp b/src/impl/bch/node.hpp index da0c3695d..d163cd811 100644 --- a/src/impl/bch/node.hpp +++ b/src/impl/bch/node.hpp @@ -605,9 +605,13 @@ class NodeImpl : public pool::SharechainNode& share_hashes); + /// Send a set of shares (with any needed txs) to a single peer, and return + /// the subset actually written (F2). Shares dropped by the tx-completeness + /// gate, or the whole batch when the tracker lock is unavailable, are + /// reported as NOT sent, so the caller leaves them un-marked and retries + /// them on the next broadcast. + std::vector send_shares(peer_ptr peer, + const std::vector& share_hashes); /// Broadcast a locally-generated (or newly-received) share to all peers. void broadcast_share(const uint256& share_hash); diff --git a/src/impl/bch/test/CMakeLists.txt b/src/impl/bch/test/CMakeLists.txt index 9a343c10b..d36464147 100644 --- a/src/impl/bch/test/CMakeLists.txt +++ b/src/impl/bch/test/CMakeLists.txt @@ -222,6 +222,12 @@ if(BUILD_TESTING) # allowlisted target rather than a new add_executable -- a new one would # silently report "Not Run" (see merged PR #868). Still bch-only, so the # per-coin isolation invariant holds. + # F2/F3 share-broadcast gate KATs ride bch_embedded_block_broadcast_test (an + # already-allowlisted --target). The bch test tree has no GTest harness and a + # standalone target would need a build.yml entry this repo's CI token cannot + # add, which CTest would then report as a silently-passing NOT_BUILT sentinel. + target_sources(bch_embedded_block_broadcast_test PRIVATE broadcast_gate_test.cpp) + add_executable(bch_g2_block_assembly_roundtrip_test g2_block_assembly_roundtrip_test.cpp ${CMAKE_SOURCE_DIR}/src/impl/bch/coin/transaction.cpp) diff --git a/src/impl/bch/test/broadcast_gate_test.cpp b/src/impl/bch/test/broadcast_gate_test.cpp new file mode 100644 index 000000000..cc09e2a75 --- /dev/null +++ b/src/impl/bch/test/broadcast_gate_test.cpp @@ -0,0 +1,211 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// --------------------------------------------------------------------------- +// F3 tx-completeness broadcast gate + F2 mark-only-what-was-sent — BCH KATs. +// +// Both primitives are the ones send_shares()/broadcast_share() in +// src/impl/bch/node.cpp actually call, so these checks pin the shipped policy: +// +// partition_backable() — F3. send_shares must NOT write a share whose +// referenced new-tx BYTES we do not hold: canonical p2pool disconnects on +// "referenced unknown transaction" (p2p.py), which isolates us from the +// sharechain and orphans our shares. Pre-fix, send_shares looked the tx up +// and simply omitted it when absent (`if (it != m_known_txs.end())` with no +// else), sending the share anyway. +// +// broadcast_and_mark() — F2. broadcast_share may add a hash to +// m_shared_share_hashes only AFTER a peer accepted it. Pre-fix it marked the +// whole chain-walk up front, so a batch that send_shares then abandoned (F3 +// skip, tracker try_to_lock miss, zero peers) was withheld FOREVER: the next +// walk breaks on the first marked hash and nothing ever re-pushes it. +// +// Red-able: the walk-stranding block reproduces the pre-fix marking order beside +// the shipped one and asserts they differ; restoring mark-before-send reddens +// the shipped half. The gate block asserts a skip count of 1; deleting the gate +// reddens it. +// +// HARNESS: the bch test tree is plain int main()/assert (no GTest), so this TU +// exposes run_share_broadcast_gate_checks() and rides the already-allowlisted +// bch_embedded_block_broadcast_test executable — no build.yml --target allowlist +// change, no NOT_BUILT sentinel risk. +// Consensus surface: NONE. These primitives decide only WHETHER a share is put +// on the wire; share bytes, minting and payout are untouched. +// --------------------------------------------------------------------------- + +#include +#include +#include +#include +#include + +#include +#include + +namespace { + +int g_failures = 0; + +void check(bool ok, const char* what) +{ + if (!ok) { + ++g_failures; + std::cerr << " FAIL: " << what << "\n"; + } +} + +uint256 h(const char* hex) { uint256 v; v.SetHex(hex); return v; } + +// Stand-in for a share on the broadcast path: its hash plus the new-tx hashes +// it references. The gate is agnostic to everything else in a share. +struct FakeShare { + uint256 hash; + std::vector new_txs; +}; + +std::vector refs_of(FakeShare& s) { return s.new_txs; } + +// A peer that accepts exactly the hashes send_shares would have written to it. +struct FakePeer { std::set accepts; }; + +// The tip-first walk broadcast_share performs, verbatim in its essentials: it +// BREAKS on the first hash already in the de-dup set. +std::vector walk(const std::vector& chain_tip_first, + const std::set& marked) +{ + std::vector to_send; + for (const auto& hash : chain_tip_first) { + if (marked.count(hash)) + break; + to_send.push_back(hash); + } + return to_send; +} + +} // namespace + +int run_share_broadcast_gate_checks() +{ + g_failures = 0; + + const uint256 S1 = h("11"), S2 = h("22"), S3 = h("33"); + const uint256 TA = h("aa"), TB = h("bb"), TC = h("cc"); + + // -- F3: an unbacked share is removed; backable ones survive, in order ---- + { + const std::set held{TA, TB}; // TC bytes NOT held + std::vector batch{{S1, {TA}}, {S2, {TC}}, {S3, {TA, TB}}}; + + const std::size_t skipped = bch::partition_backable(batch, held, refs_of); + check(skipped == 1u, "F3 gate skips exactly the unbacked share"); + check(batch.size() == 2u, "F3 gate keeps the two backable shares"); + if (batch.size() == 2u) { + check(batch[0].hash == S1, "F3 gate preserves batch order (S1)"); + check(batch[1].hash == S3, "F3 gate preserves batch order (S3)"); + } + } + + // -- F3: a share referencing no new txs is trivially backable ------------- + { + const std::set held; // hold nothing at all + std::vector batch{{S1, {}}}; + check(bch::partition_backable(batch, held, refs_of) == 0u, + "F3 gate never withholds a share with no tx refs"); + check(batch.size() == 1u, "F3 gate keeps the no-refs share"); + } + + // -- F3: every share unbacked -> empty batch, so nothing is reported sent - + { + const std::set held; + std::vector batch{{S1, {TA}}, {S2, {TB}}}; + check(bch::partition_backable(batch, held, refs_of) == 2u, + "F3 gate skips a wholly-unbacked batch"); + check(batch.empty(), "F3 gate empties a wholly-unbacked batch"); + } + + // -- F2: a batch no peer accepted leaves the de-dup set untouched --------- + { + std::set marked; + std::map peers{{1, {}}, {2, {}}}; + const std::size_t n = bch::broadcast_and_mark( + marked, peers, std::vector{S1, S2}, + [](FakePeer&) { return std::vector{}; }); + check(n == 0u, "F2 reports nothing sent when no peer accepted"); + check(marked.empty(), "F2 marks nothing when no peer accepted"); + } + + // -- F2: zero peers -> nothing sent, therefore nothing marked ------------- + { + std::set marked; + std::map peers; + check(bch::broadcast_and_mark(marked, peers, std::vector{S1, S2}, + [](FakePeer&) { + return std::vector{}; + }) == 0u, + "F2 reports nothing sent with zero peers"); + check(marked.empty(), "F2 marks nothing with zero peers"); + } + + // -- F2: exactly the union of what peers accepted is marked, no more ------ + { + std::set marked; + std::map peers{{1, {{S1}}}, {2, {{S1, S3}}}}; + const std::size_t n = bch::broadcast_and_mark( + marked, peers, std::vector{S1, S2, S3}, [](FakePeer& p) { + return std::vector(p.accepts.begin(), p.accepts.end()); + }); + check(n == 2u, "F2 counts the union of accepted hashes"); + check(marked.count(S1) == 1u, "F2 marks an accepted hash"); + check(marked.count(S3) == 1u, "F2 marks an accepted hash"); + check(marked.count(S2) == 0u, "F2 leaves a withheld share un-marked"); + } + + // -- The regression: mark-before-send strands the walk permanently -------- + // Round 1: TB is missing so S2 is withheld and no peer accepts anything. + { + const std::vector chain{S2, S1}; // S2 is the tip + std::map one_peer{{1, {}}}; + + // pre-fix ordering: mark the walk, THEN discover nothing was sent + { + std::set marked; + auto to_send = walk(chain, marked); + check(to_send.size() == 2u, "pre-fix walk yields the full batch once"); + for (const auto& hash : to_send) marked.insert(hash); // the bug + bch::broadcast_and_mark(marked, one_peer, to_send, + [](FakePeer&) { return std::vector{}; }); + // Round 2, TB has since arrived — but the tip is marked, so nothing + // is ever re-pushed. Silent, permanent share loss, no retry path. + check(walk(chain, marked).empty(), + "pre-fix ordering strands the walk (documents the bug)"); + } + + // shipped ordering: mark only what a peer accepted + { + std::set marked; + auto offered = walk(chain, marked); + check(offered.size() == 2u, "shipped walk yields the batch"); + bch::broadcast_and_mark(marked, one_peer, offered, + [](FakePeer&) { return std::vector{}; }); + check(marked.empty(), "shipped ordering marks nothing on an abandoned send"); + + auto retry = walk(chain, marked); + check(retry.size() == 2u, "shipped ordering re-offers the batch"); + if (!retry.empty()) + check(retry[0] == S2, "shipped retry still starts at the tip"); + + // ...and once a peer accepts them they are marked exactly once. + std::map live{{1, {{S1, S2}}}}; + bch::broadcast_and_mark(marked, live, retry, [](FakePeer& p) { + return std::vector(p.accepts.begin(), p.accepts.end()); + }); + check(marked.size() == 2u, "shipped ordering marks after a real send"); + check(walk(chain, marked).empty(), "shipped ordering then de-dups"); + } + } + + if (g_failures == 0) + std::cout << "share_broadcast_gate (F2/F3) KATs: ALL PASS\n"; + else + std::cerr << "share_broadcast_gate (F2/F3) KATs: " << g_failures + << " FAILURE(S)\n"; + return g_failures; +} diff --git a/src/impl/bch/test/embedded_block_broadcast_test.cpp b/src/impl/bch/test/embedded_block_broadcast_test.cpp index 3cf38cfe7..0ee13f4ac 100644 --- a/src/impl/bch/test/embedded_block_broadcast_test.cpp +++ b/src/impl/bch/test/embedded_block_broadcast_test.cpp @@ -60,6 +60,11 @@ struct SinkThrew : std::exception { } // namespace +// F2/F3 share-broadcast gate KATs (broadcast_gate_test.cpp). Kept in its own +// TU but run from this already-allowlisted executable, so it is compiled and +// executed in CI without a new --target the token cannot add to build.yml. +int run_share_broadcast_gate_checks(); + int main() { boost::asio::io_context ioc; TestConfig config; @@ -157,6 +162,9 @@ int main() { CHECK(!threw_out); } + // Share-broadcast completeness gate + mark-after-send (separate TU). + failures += run_share_broadcast_gate_checks(); + if (failures == 0) { std::cout << "embedded_block_broadcast_test: ALL PASS\n"; return 0; diff --git a/src/impl/btc/known_txs_retention.hpp b/src/impl/btc/known_txs_retention.hpp index 23a2aee75..615b96eca 100644 --- a/src/impl/btc/known_txs_retention.hpp +++ b/src/impl/btc/known_txs_retention.hpp @@ -23,10 +23,19 @@ // mint-time decline of shares whose txs are not retained — makes "every share // we mint/broadcast is backable" a by-construction invariant. // -// NOTE (BTC wiring): the call-sites that plug these primitives into -// send_shares / create_share_fn depend on how the BTC share model carries its -// referenced new-tx hashes; that wiring is tracked separately. This header is -// the pure primitive + its unit coverage. +// partition_backable() — the batch form of the gate, applied by send_shares() +// to the shares it is about to write to one peer. +// +// broadcast_and_mark() — the F2 marking policy: broadcast_share() may mark a +// share "already shared" only AFTER a peer actually accepted it. Marking the +// whole chain-walk up front (the pre-fix pattern) loses every share in a +// batch that send_shares then abandons — the gate skipped it, the tracker +// try_to_lock missed, or there were no peers — because the de-dup set then +// breaks the walk forever and nothing re-pushes it. +// +// NOTE (de-dup): the same primitives now exist per-coin (btc/dgb/bch/dash) while +// the LTC lane hoists all_txs_backable into src/core/. Once both land, these +// per-coin headers should be collapsed onto that core hoist. #pragma once @@ -133,4 +142,59 @@ inline std::vector select_backable_shares( return out; } +// Batch form of the F3 gate. Keeps in `shares` only the entries every one of +// whose referenced new-tx hashes we hold, and returns how many were dropped. +// `hashes_of(share)` yields that share's referenced new-tx hashes. +// +// Skipping costs no propagation: a share we downloaded is by definition already +// on the network we fetched it from, and it will be re-offered on a later +// broadcast once its tx bytes arrive (F2 leaves it un-marked). +template +inline std::size_t partition_backable(std::vector& shares, + const Held& held, + HashesOf hashes_of) +{ + std::vector sendable; + sendable.reserve(shares.size()); + std::size_t skipped = 0; + for (auto& share : shares) { + if (all_txs_backable(hashes_of(share), held)) + sendable.push_back(std::move(share)); + else + ++skipped; + } + shares.swap(sendable); + return skipped; +} + +// F2 marking policy. Offers the batch to every peer via `send(peer)` — which +// returns the hashes it ACTUALLY wrote to that peer — and marks in `marked` +// only the hashes that reached at least one peer. Returns that count. +// +// The pre-fix code marked the whole chain-walk before calling send_shares, so a +// batch abandoned by every peer (gate skip, try_to_lock miss, empty peer set) +// stayed marked forever: the next broadcast_share walk breaks on the first +// marked hash and the share is never re-pushed. That is silent, permanent share +// loss with no retry path — a PPLNS-credit loss for whoever mined it. +template +inline std::size_t broadcast_and_mark(MarkedSet& marked, Peers& peers, + const std::vector& to_send, + SendFn send) +{ + if (to_send.empty()) + return 0; + + // NOTE: `to_send` is deliberately NOT what gets marked. Marking the offered + // batch here is precisely the pre-fix bug this signature exists to make + // un-writable by accident — only `send`'s report may be marked. + std::set actually_sent; + for (auto& entry : peers) { + const std::vector sent = send(entry.second); + actually_sent.insert(sent.begin(), sent.end()); + } + for (const auto& h : actually_sent) + marked.insert(h); + return actually_sent.size(); +} + } // namespace btc diff --git a/src/impl/btc/node.cpp b/src/impl/btc/node.cpp index 20d4dc55d..30fc798b9 100644 --- a/src/impl/btc/node.cpp +++ b/src/impl/btc/node.cpp @@ -727,8 +727,12 @@ void NodeImpl::register_template_txs(const uint256& share_hash, << m_template_recent_sets.size(); } -void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hashes) +std::vector NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hashes) { + // Returns the hashes of the shares actually WRITTEN to this peer (F2). The + // caller marks only those as broadcast, so a share skipped here — by the + // try_to_lock miss below or by the F3 tx-completeness gate — is re-offered + // on the next broadcast instead of being silently dropped forever. // try_to_lock per the architectural rule (node.hpp:67) — see freeze // analysis in handle_get_share above. If we can't acquire NOW, skip // this batch. The shares are still in our chain; the next broadcast @@ -740,7 +744,7 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash if (defer_log++ % 50 == 0) LOG_INFO << "[send_shares] tracker busy — skipping send to " << peer->addr().to_string() << " (will retry next cycle)"; - return; + return {}; } // Collect shares that exist in our chain (skip rejected) @@ -760,7 +764,48 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash } if (shares.empty()) - return; + return {}; + + // --- F3 tx-completeness broadcast gate -------------------------------- + // A share whose referenced new-tx BYTES we do not hold cannot be backed by + // the remember_tx forward below, and canonical p2pool drops the connection + // on "referenced unknown transaction" (p2p.py) — which isolates us from the + // sharechain and orphans our shares. Shares pulled via sharereply arrive + // WITHOUT their tx bytes, so this is reachable in normal operation. + // + // Gate on the BYTES in m_known_txs, not on the peer's have_tx advert: that + // advert can be stale (the peer may have dropped the tx) and sending + // hash-only for a tx the peer no longer holds is exactly the disconnect. + // m_remote_txs is used ONLY below to choose the hash-vs-bytes encoding. + // + // CONSENSUS-NEUTRAL: this decides only WHETHER we broadcast. Share bytes, + // minting and payout are untouched; a skipped share stays in our chain and + // is re-offered once its txs arrive (send_shares reports it as unsent, so + // broadcast_share leaves it un-marked). + { + const size_t skipped_incomplete = partition_backable( + shares, m_known_txs, [](ShareType& share) { + std::vector hashes; + share.invoke([&](auto* obj) { + if constexpr (requires { obj->m_new_transaction_hashes; }) + hashes.assign(obj->m_new_transaction_hashes.begin(), + obj->m_new_transaction_hashes.end()); + }); + return hashes; + }); + if (skipped_incomplete > 0) + { + static int skip_log = 0; + if (skip_log++ % 20 == 0) + LOG_INFO << "[send_shares] skipped " << skipped_incomplete + << " share(s) whose new-tx bytes we do not hold " + "(downloaded via sharereply / evicted) to " + << peer->addr().to_string() + << " — avoids canonical 'unknown transaction' disconnect"; + } + if (shares.empty()) + return {}; + } // (b) Extract each share's referenced new-tx hashes. BTC shares carry these // inside m_tx_info (btc::ShareTxInfo), NOT as a top-level @@ -806,7 +851,7 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash shares = std::move(kept_shares); per_share_refs = std::move(kept_refs); if (shares.empty()) - return; + return {}; } // Collect transactions the peer doesn't know about, over the surviving @@ -824,6 +869,8 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash std::vector known_hashes; // hashes in peer's remote set std::vector full_txs; // full txs otherwise + size_t missing = 0; + for (const auto& th : needed_txs) { if (peer->m_remote_txs.count(th)) @@ -835,9 +882,20 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash auto it = m_known_txs.find(th); if (it != m_known_txs.end()) full_txs.emplace_back(it->second); + else + ++missing; // unreachable after the gate above — see below } } + // Defence in depth: the gate should already have removed every share + // with an unheld tx, so this must stay at zero. If it ever fires, the + // gate's view of the referenced hashes disagrees with needed_txs and + // the peer may drop these shares — make that loud rather than silent. + if (missing > 0) + LOG_WARNING << "[send_shares] " << missing << " referenced tx(s) not in " + "m_known_txs after the completeness gate — peer may drop " + "these shares (share/tx hash accounting mismatch?)"; + if (!known_hashes.empty() || !full_txs.empty()) { auto rtx_msg = message_remember_tx::make_raw(known_hashes, full_txs); @@ -874,6 +932,13 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash LOG_INFO << "[Pool] Sent " << shares.size() << " shares (+" << needed_txs.size() << " txs) to " << peer->addr().to_string(); + + // F2: report exactly which shares reached this peer. + std::vector sent; + sent.reserve(shares.size()); + for (auto& share : shares) + sent.push_back(share.hash()); + return sent; } void NodeImpl::broadcast_share(const uint256& share_hash) @@ -896,7 +961,14 @@ void NodeImpl::broadcast_share(const uint256& share_hash) return; } - // Walk the chain back from share_hash, collecting un-broadcast shares + // Walk the chain back from share_hash, collecting un-broadcast shares. + // + // F2: do NOT mark them shared here. send_shares can abandon the whole batch + // — the tx-completeness gate skipped it, its tracker try_to_lock missed, or + // there are zero peers — and a share marked before that never gets another + // chance: the next walk breaks on the first marked hash, so the tip is + // permanently withheld from the sharechain and its PPLNS credit is lost. + // We mark only what actually reached >= 1 peer, below. std::vector to_send; int32_t height = m_chain->get_height(share_hash); int32_t walk = std::min(height, 5); @@ -907,7 +979,6 @@ void NodeImpl::broadcast_share(const uint256& share_hash) break; if (m_rejected_share_hashes.count(hash)) continue; // skip shares previously rejected by peers - m_shared_share_hashes.insert(hash); to_send.push_back(hash); } @@ -915,10 +986,16 @@ void NodeImpl::broadcast_share(const uint256& share_hash) return; auto now = std::chrono::steady_clock::now(); - for (auto& [nonce, peer] : m_peers) { - send_shares(peer, to_send); - m_last_broadcast_to[peer->addr()] = {to_send, now}; - } + broadcast_and_mark(m_shared_share_hashes, m_peers, to_send, [&](peer_ptr& peer) { + std::vector sent = send_shares(peer, to_send); + // Record what we ACTUALLY wrote, not what we offered: a peer that drops + // within 10s marks this record's hashes rejected, and a rejected hash is + // never re-broadcast. Blaming a share we withheld would turn a transient + // gate skip into permanent loss. + if (!sent.empty()) + m_last_broadcast_to[peer->addr()] = {sent, now}; + return sent; + }); } void NodeImpl::notify_local_share(const uint256& share_hash) diff --git a/src/impl/btc/node.hpp b/src/impl/btc/node.hpp index 43f73ed42..8981e771a 100644 --- a/src/impl/btc/node.hpp +++ b/src/impl/btc/node.hpp @@ -377,9 +377,13 @@ class NodeImpl : public pool::SharechainNode& share_hashes); + /// Send a set of shares (with any needed txs) to a single peer, and return + /// the subset actually written (F2). Shares dropped by the tx-completeness + /// gate, or the whole batch when the tracker lock is unavailable, are + /// reported as NOT sent, so the caller leaves them un-marked and retries + /// them on the next broadcast. + std::vector send_shares(peer_ptr peer, + const std::vector& share_hashes); /// F3: retain a freshly-minted share's TEMPLATE tx set so every referenced /// new-tx is forwardable (remember_tx) and the share is backable. Called diff --git a/src/impl/btc/test/CMakeLists.txt b/src/impl/btc/test/CMakeLists.txt index 788167148..b70819940 100644 --- a/src/impl/btc/test/CMakeLists.txt +++ b/src/impl/btc/test/CMakeLists.txt @@ -1,7 +1,7 @@ if (BUILD_TESTING AND GTest_FOUND) # btc twin of ltc share_test — uniquely named to avoid the CMP0002 target # collision with src/impl/ltc/test (both subdirs build in the same tree). - add_executable(btc_share_test share_test.cpp f11_donation_invariance_test.cpp f10b_version_punish_removal_test.cpp g01_share_format_parity_test.cpp auto_ratchet_sim_test.cpp block_broadcast_guard_test.cpp block_broadcast_connect_test.cpp regtest_sharechain_isolation_test.cpp stratum_merkle_branch_test.cpp witness_commitment_merkle_test.cpp finder_fee_integer_exact_test.cpp rpc_conf_test.cpp genesis_check_test.cpp won_share_dualpath_test.cpp gentx_unpack_test.cpp block_assembly_test.cpp gentx_coinbase_test.cpp template_capture_test.cpp template_other_txs_test.cpp reconstruct_test.cpp known_txs_retention_test.cpp won_block_mints_share_test.cpp) + add_executable(btc_share_test share_test.cpp f11_donation_invariance_test.cpp f10b_version_punish_removal_test.cpp g01_share_format_parity_test.cpp auto_ratchet_sim_test.cpp block_broadcast_guard_test.cpp block_broadcast_connect_test.cpp regtest_sharechain_isolation_test.cpp stratum_merkle_branch_test.cpp witness_commitment_merkle_test.cpp finder_fee_integer_exact_test.cpp rpc_conf_test.cpp genesis_check_test.cpp won_share_dualpath_test.cpp gentx_unpack_test.cpp block_assembly_test.cpp gentx_coinbase_test.cpp template_capture_test.cpp template_other_txs_test.cpp reconstruct_test.cpp known_txs_retention_test.cpp won_block_mints_share_test.cpp broadcast_gate_test.cpp) target_link_libraries(btc_share_test PRIVATE GTest::gtest_main GTest::gtest core btc diff --git a/src/impl/btc/test/broadcast_gate_test.cpp b/src/impl/btc/test/broadcast_gate_test.cpp new file mode 100644 index 000000000..502dc7946 --- /dev/null +++ b/src/impl/btc/test/broadcast_gate_test.cpp @@ -0,0 +1,208 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// --------------------------------------------------------------------------- +// F3 tx-completeness broadcast gate + F2 mark-only-what-was-sent — BTC KATs. +// +// Both primitives are the ones send_shares()/broadcast_share() in +// src/impl/btc/node.cpp actually call, so these KATs pin the shipped policy: +// +// partition_backable() — F3. send_shares must NOT write a share whose +// referenced new-tx BYTES we do not hold: canonical p2pool disconnects on +// "referenced unknown transaction" (p2p.py), which isolates us from the +// sharechain and orphans our shares. Pre-fix, send_shares looked the tx up +// and simply omitted it when absent (`if (it != m_known_txs.end())` with no +// else), sending the share anyway. +// +// broadcast_and_mark() — F2. broadcast_share may add a hash to +// m_shared_share_hashes only AFTER a peer accepted it. Pre-fix it marked the +// whole chain-walk up front, so a batch that send_shares then abandoned (F3 +// skip, tracker try_to_lock miss, zero peers) was withheld FOREVER: the next +// walk breaks on the first marked hash and nothing ever re-pushes it. +// +// Red-able: WalkStrandedWhenMarkedBeforeSend reproduces the pre-fix marking +// order side by side with the shipped one and asserts they differ — restoring +// mark-before-send makes the shipped half fail. GateWithholdsUnbackedShare +// asserts a skip count of 1; deleting the gate flips it red. +// +// Rides the already-allowlisted btc_share_test executable — no build.yml +// --target allowlist change, no NOT_BUILT sentinel risk. +// Consensus surface: NONE. These primitives decide only WHETHER a share is put +// on the wire; share bytes, minting and payout are untouched. +// --------------------------------------------------------------------------- + +#include + +#include +#include +#include + +#include +#include + +namespace { + +uint256 h(const char* hex) { uint256 v; v.SetHex(hex); return v; } + +// Share hashes S1..S3 and tx hashes TA..TC. +const uint256 S1 = h("11"), S2 = h("22"), S3 = h("33"); +const uint256 TA = h("aa"), TB = h("bb"), TC = h("cc"); + +// Stand-in for a share on the broadcast path: its hash plus the new-tx hashes +// it references. The gate is agnostic to everything else in a share. +struct FakeShare { + uint256 hash; + std::vector new_txs; +}; + +std::vector refs_of(FakeShare& s) { return s.new_txs; } + +// A peer that accepts exactly the hashes send_shares would have written to it. +struct FakePeer { std::set accepts; }; + +} // namespace + +// F3: a share referencing a tx whose bytes we do not hold is removed from the +// outgoing batch; the backable ones survive, in order. +TEST(BtcBroadcastGate, GateWithholdsUnbackedShare) +{ + const std::set held{TA, TB}; // TC bytes NOT held + + std::vector batch{ + {S1, {TA}}, + {S2, {TC}}, // references an unheld tx -> must be withheld + {S3, {TA, TB}}, + }; + + const std::size_t skipped = btc::partition_backable(batch, held, refs_of); + + EXPECT_EQ(skipped, 1u); + ASSERT_EQ(batch.size(), 2u); + EXPECT_EQ(batch[0].hash, S1); + EXPECT_EQ(batch[1].hash, S3); // order preserved +} + +// F3: a share referencing no new txs is trivially backable (the common case for +// an empty-template share) — the gate must not withhold it. +TEST(BtcBroadcastGate, ShareWithNoTxRefsIsAlwaysSendable) +{ + const std::set held; // hold nothing at all + std::vector batch{{S1, {}}}; + + EXPECT_EQ(btc::partition_backable(batch, held, refs_of), 0u); + ASSERT_EQ(batch.size(), 1u); + EXPECT_EQ(batch[0].hash, S1); +} + +// F3: when every share is unbacked the batch empties, and send_shares reports +// nothing sent (which is what keeps F2 from marking them). +TEST(BtcBroadcastGate, WholeBatchUnbackedEmptiesTheSend) +{ + const std::set held; + std::vector batch{{S1, {TA}}, {S2, {TB}}}; + + EXPECT_EQ(btc::partition_backable(batch, held, refs_of), 2u); + EXPECT_TRUE(batch.empty()); +} + +// F2: a batch no peer accepted must leave the de-dup set untouched, so the next +// broadcast walk re-offers it. +TEST(BtcBroadcastMarking, NothingMarkedWhenNoPeerAccepted) +{ + std::set marked; + std::map peers{{1, {}}, {2, {}}}; + + const std::size_t n = btc::broadcast_and_mark( + marked, peers, std::vector{S1, S2}, + [](FakePeer&) { return std::vector{}; }); + + EXPECT_EQ(n, 0u); + EXPECT_TRUE(marked.empty()); +} + +// F2: with zero peers connected nothing is sent, therefore nothing is marked. +TEST(BtcBroadcastMarking, NothingMarkedWithZeroPeers) +{ + std::set marked; + std::map peers; + + EXPECT_EQ(btc::broadcast_and_mark(marked, peers, std::vector{S1, S2}, + [](FakePeer&) { return std::vector{}; }), + 0u); + EXPECT_TRUE(marked.empty()); +} + +// F2: exactly the union of what the peers accepted gets marked — no more. +TEST(BtcBroadcastMarking, OnlyAcceptedHashesAreMarked) +{ + std::set marked; + std::map peers{{1, {{S1}}}, {2, {{S1, S3}}}}; + + const std::size_t n = btc::broadcast_and_mark( + marked, peers, std::vector{S1, S2, S3}, [](FakePeer& p) { + return std::vector(p.accepts.begin(), p.accepts.end()); + }); + + EXPECT_EQ(n, 2u); + EXPECT_TRUE(marked.count(S1)); + EXPECT_TRUE(marked.count(S3)); + EXPECT_FALSE(marked.count(S2)); // withheld by the gate -> retryable +} + +// The regression itself. broadcast_share walks back from the tip and BREAKS on +// the first already-marked hash. Round 1: TB is missing so S2 is withheld and no +// peer accepts anything. Pre-fix (mark first) the walk is stranded forever; +// shipped (mark after send) it re-offers the batch in round 2, when TB arrives. +TEST(BtcBroadcastMarking, WalkStrandedWhenMarkedBeforeSend) +{ + // The tip-first walk broadcast_share performs, verbatim in its essentials. + auto walk = [](const std::vector& chain_tip_first, + const std::set& marked) { + std::vector to_send; + for (const auto& hash : chain_tip_first) { + if (marked.count(hash)) + break; + to_send.push_back(hash); + } + return to_send; + }; + + const std::vector chain{S2, S1}; // S2 is the tip + std::map one_peer{{1, {}}}; + + // ---- pre-fix ordering: mark the walk, THEN discover nothing was sent ---- + { + std::set marked; + auto to_send = walk(chain, marked); + ASSERT_EQ(to_send.size(), 2u); + for (const auto& hash : to_send) marked.insert(hash); // the bug + btc::broadcast_and_mark(marked, one_peer, to_send, + [](FakePeer&) { return std::vector{}; }); + + // Round 2, TB has since arrived and S2 is backable — but the walk is + // dead: the tip is marked, so nothing is ever re-pushed. Silent, + // permanent share loss with no retry path. + EXPECT_TRUE(walk(chain, marked).empty()); + } + + // ---- shipped ordering: mark only what a peer accepted -------------------- + { + std::set marked; + auto to_send = walk(chain, marked); + ASSERT_EQ(to_send.size(), 2u); + btc::broadcast_and_mark(marked, one_peer, to_send, + [](FakePeer&) { return std::vector{}; }); + EXPECT_TRUE(marked.empty()); + + // Round 2: the walk still yields the full batch, so the share is retried. + auto retry = walk(chain, marked); + ASSERT_EQ(retry.size(), 2u); + EXPECT_EQ(retry[0], S2); + + // ...and once a peer accepts them they are marked exactly once. + std::map live{{1, {{S1, S2}}}}; + btc::broadcast_and_mark(marked, live, retry, [](FakePeer& p) { + return std::vector(p.accepts.begin(), p.accepts.end()); + }); + EXPECT_EQ(marked.size(), 2u); + EXPECT_TRUE(walk(chain, marked).empty()); // now correctly de-duped + } +} diff --git a/src/impl/dgb/known_txs_retention.hpp b/src/impl/dgb/known_txs_retention.hpp new file mode 100644 index 000000000..b1004ba9d --- /dev/null +++ b/src/impl/dgb/known_txs_retention.hpp @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// +// Pure, unit-testable bookkeeping for the known-transaction pool that backs +// share broadcast (remember_tx forwarding). Ported to the DGB variant from +// src/impl/dash/known_txs_retention.hpp so the retention/eviction and +// broadcast-gate semantics can be exercised without a live node. +// +// Two concerns: +// retain_template_txs() — the rolling-window retention with template-identity +// dedup (code review F1): register_template_txs is invoked per producer-job +// cache MISS, i.e. per (prev_share_hash, payout_script), NOT per template +// rotation. A ~50-payout-script cluster therefore re-registers the SAME +// template ~50 times on one tip; pushing each unconditionally collapses the +// N-slot window to a single template within seconds. Dedup by the template's +// tx-hash SET (its identity): a set already retained refreshes recency and +// consumes NO new slot, so N re-registrations of one template use ONE slot +// and the window holds N DISTINCT templates as intended. +// +// all_txs_backable() — the canonical broadcast gate (F3): a share may be sent +// only when we HOLD the bytes of every referenced new-tx (so remember_tx can +// always forward them). This is what makes the "referenced unknown +// transaction" disconnect unreachable by construction, and — paired with the +// mint-time decline of shares whose txs are not retained — makes "every share +// we mint/broadcast is backable" a by-construction invariant. +// +// partition_backable() — the batch form of the gate, applied by send_shares() +// to the shares it is about to write to one peer. +// +// broadcast_and_mark() — the F2 marking policy: broadcast_share() may mark a +// share "already shared" only AFTER a peer actually accepted it. Marking the +// whole chain-walk up front (the pre-fix pattern) loses every share in a +// batch that send_shares then abandons — the gate skipped it, the tracker +// try_to_lock missed, or there were no peers — because the de-dup set then +// breaks the walk forever and nothing re-pushes it. +// +// NOTE (de-dup): the same primitives now exist per-coin (btc/dgb/bch/dash) while +// the LTC lane hoists all_txs_backable into src/core/. Once both land, these +// per-coin headers should be collapsed onto that core hoist. + +#pragma once + +#include +#include +#include +#include + +#include + +namespace dgb { + +// Retain the tx set of a newly-registered template in a rolling window of the +// last `cap` DISTINCT templates, inserting its tx bytes into `known_txs`. +// +// recent_sets : rolling history of distinct template tx-hash sets +// (front = oldest, back = newest). +// known_txs : uint256 -> tx map; needs insert_or_assign(k,v) and erase(k). +// hashes/txs : parallel arrays of the template's tx hashes and bytes. +// cap : number of DISTINCT templates to retain. +// +// A tx is erased from `known_txs` only once it has fallen out of EVERY retained +// set. F1 dedup: if this template's tx set is already retained, its recency is +// refreshed (moved to back) and no new slot is consumed. +template +inline void retain_template_txs(std::deque>& recent_sets, + TxMap& known_txs, + const std::vector& hashes, + const std::vector& txs, + std::size_t cap) +{ + if (cap == 0) + return; + + std::set new_set(hashes.begin(), hashes.end()); + + // F1 dedup by template identity (the tx-hash set). If this template is + // already retained, refresh its recency (move to back) — no new slot, no + // re-eviction. This is what stops ~50 payout-script re-registrations of one + // template from evicting the rest of the window. + for (auto it = recent_sets.begin(); it != recent_sets.end(); ++it) { + if (*it == new_set) { + if (std::next(it) != recent_sets.end()) { + std::set refreshed = std::move(*it); + recent_sets.erase(it); + recent_sets.push_back(std::move(refreshed)); + } + return; + } + } + + // Distinct template: insert its tx bytes and consume a slot. + const std::size_t n = std::min(hashes.size(), txs.size()); + for (std::size_t i = 0; i < n; ++i) + known_txs.insert_or_assign(hashes[i], txs[i]); + recent_sets.push_back(std::move(new_set)); + + // Evict the oldest slots beyond `cap`; a tx is removed from `known_txs` only + // once it is absent from EVERY remaining retained set. + while (recent_sets.size() > cap) { + const std::set evicted = std::move(recent_sets.front()); + recent_sets.pop_front(); + for (const auto& h : evicted) { + bool still_retained = false; + for (const auto& s : recent_sets) { + if (s.count(h)) { still_retained = true; break; } + } + if (!still_retained) + known_txs.erase(h); + } + } +} + +// True iff every hash in `tx_hashes` is present in `held` (the bytes we hold, +// e.g. m_known_txs). The canonical broadcast gate: a share is backable — safe to +// send without risking the peer's "referenced unknown transaction" disconnect — +// only when we can forward every referenced tx. +template +inline bool all_txs_backable(const std::vector& tx_hashes, + const Held& held) +{ + for (const auto& h : tx_hashes) + if (held.find(h) == held.end()) + return false; + return true; +} + +// Batch form of the F3 gate. Keeps in `shares` only the entries every one of +// whose referenced new-tx hashes we hold, and returns how many were dropped. +// `hashes_of(share)` yields that share's referenced new-tx hashes. +// +// Skipping costs no propagation: a share we downloaded is by definition already +// on the network we fetched it from, and it will be re-offered on a later +// broadcast once its tx bytes arrive (F2 leaves it un-marked). +template +inline std::size_t partition_backable(std::vector& shares, + const Held& held, + HashesOf hashes_of) +{ + std::vector sendable; + sendable.reserve(shares.size()); + std::size_t skipped = 0; + for (auto& share : shares) { + if (all_txs_backable(hashes_of(share), held)) + sendable.push_back(std::move(share)); + else + ++skipped; + } + shares.swap(sendable); + return skipped; +} + +// F2 marking policy. Offers the batch to every peer via `send(peer)` — which +// returns the hashes it ACTUALLY wrote to that peer — and marks in `marked` +// only the hashes that reached at least one peer. Returns that count. +// +// The pre-fix code marked the whole chain-walk before calling send_shares, so a +// batch abandoned by every peer (gate skip, try_to_lock miss, empty peer set) +// stayed marked forever: the next broadcast_share walk breaks on the first +// marked hash and the share is never re-pushed. That is silent, permanent share +// loss with no retry path — a PPLNS-credit loss for whoever mined it. +template +inline std::size_t broadcast_and_mark(MarkedSet& marked, Peers& peers, + const std::vector& to_send, + SendFn send) +{ + if (to_send.empty()) + return 0; + + // NOTE: `to_send` is deliberately NOT what gets marked. Marking the offered + // batch here is precisely the pre-fix bug this signature exists to make + // un-writable by accident — only `send`'s report may be marked. + std::set actually_sent; + for (auto& entry : peers) { + const std::vector sent = send(entry.second); + actually_sent.insert(sent.begin(), sent.end()); + } + for (const auto& h : actually_sent) + marked.insert(h); + return actually_sent.size(); +} + +} // namespace dgb diff --git a/src/impl/dgb/node.cpp b/src/impl/dgb/node.cpp index 68cef923a..e7fd55c11 100644 --- a/src/impl/dgb/node.cpp +++ b/src/impl/dgb/node.cpp @@ -1,5 +1,6 @@ // SPDX-License-Identifier: AGPL-3.0-or-later #include "node.hpp" +#include "known_txs_retention.hpp" // dgb::all_txs_backable / partition_backable / broadcast_and_mark (F2/F3) #include #include @@ -660,8 +661,13 @@ std::vector NodeImpl::handle_get_share(std::vector hash return shares; } -void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hashes) +std::vector NodeImpl::send_shares(peer_ptr peer, + const std::vector& share_hashes) { + // Returns the hashes of the shares actually WRITTEN to this peer (F2). The + // caller marks only those as broadcast, so a share skipped here — by the + // try_to_lock miss below or by the F3 tx-completeness gate — is re-offered + // on the next broadcast instead of being silently dropped forever. // try_to_lock per the architectural rule (node.hpp:67) — see freeze // analysis in handle_get_share above. If we can't acquire NOW, skip // this batch. The shares are still in our chain; the next broadcast @@ -673,7 +679,7 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash if (defer_log++ % 50 == 0) LOG_INFO << "[send_shares] tracker busy — skipping send to " << peer->addr().to_string() << " (will retry next cycle)"; - return; + return {}; } // Collect shares that exist in our chain (skip rejected) @@ -693,7 +699,48 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash } if (shares.empty()) - return; + return {}; + + // --- F3 tx-completeness broadcast gate -------------------------------- + // A share whose referenced new-tx BYTES we do not hold cannot be backed by + // the remember_tx forward below, and canonical p2pool drops the connection + // on "referenced unknown transaction" (p2p.py) — which isolates us from the + // sharechain and orphans our shares. Shares pulled via sharereply arrive + // WITHOUT their tx bytes, so this is reachable in normal operation. + // + // Gate on the BYTES in m_known_txs, not on the peer's have_tx advert: that + // advert can be stale (the peer may have dropped the tx) and sending + // hash-only for a tx the peer no longer holds is exactly the disconnect. + // m_remote_txs is used ONLY below to choose the hash-vs-bytes encoding. + // + // CONSENSUS-NEUTRAL: this decides only WHETHER we broadcast. Share bytes, + // minting and payout are untouched; a skipped share stays in our chain and + // is re-offered once its txs arrive (send_shares reports it as unsent, so + // broadcast_share leaves it un-marked). + { + const size_t skipped_incomplete = partition_backable( + shares, m_known_txs, [](ShareType& share) { + std::vector hashes; + share.invoke([&](auto* obj) { + if constexpr (requires { obj->m_new_transaction_hashes; }) + hashes.assign(obj->m_new_transaction_hashes.begin(), + obj->m_new_transaction_hashes.end()); + }); + return hashes; + }); + if (skipped_incomplete > 0) + { + static int skip_log = 0; + if (skip_log++ % 20 == 0) + LOG_INFO << "[send_shares] skipped " << skipped_incomplete + << " share(s) whose new-tx bytes we do not hold " + "(downloaded via sharereply / evicted) to " + << peer->addr().to_string() + << " — avoids canonical 'unknown transaction' disconnect"; + } + if (shares.empty()) + return {}; + } // Collect transactions that the peer doesn't know about. A share's // referenced new-tx hashes live INSIDE m_tx_info (dgb::ShareTxInfo) on @@ -722,6 +769,8 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash std::vector known_hashes; // hashes in peer's remote set std::vector full_txs; // full txs otherwise + size_t missing = 0; + for (const auto& th : needed_txs) { if (peer->m_remote_txs.count(th)) @@ -733,9 +782,20 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash auto it = m_known_txs.find(th); if (it != m_known_txs.end()) full_txs.emplace_back(it->second); + else + ++missing; // unreachable after the gate above — see below } } + // Defence in depth: the gate should already have removed every share + // with an unheld tx, so this must stay at zero. If it ever fires, the + // gate's view of the referenced hashes disagrees with needed_txs and + // the peer may drop these shares — make that loud rather than silent. + if (missing > 0) + LOG_WARNING << "[send_shares] " << missing << " referenced tx(s) not in " + "m_known_txs after the completeness gate — peer may drop " + "these shares (share/tx hash accounting mismatch?)"; + if (!known_hashes.empty() || !full_txs.empty()) { auto rtx_msg = message_remember_tx::make_raw(known_hashes, full_txs); @@ -772,6 +832,13 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash LOG_INFO << "[Pool] Sent " << shares.size() << " shares (+" << needed_txs.size() << " txs) to " << peer->addr().to_string(); + + // F2: report exactly which shares reached this peer. + std::vector sent; + sent.reserve(shares.size()); + for (auto& share : shares) + sent.push_back(share.hash()); + return sent; } void NodeImpl::broadcast_share(const uint256& share_hash) @@ -794,7 +861,14 @@ void NodeImpl::broadcast_share(const uint256& share_hash) return; } - // Walk the chain back from share_hash, collecting un-broadcast shares + // Walk the chain back from share_hash, collecting un-broadcast shares. + // + // F2: do NOT mark them shared here. send_shares can abandon the whole batch + // — the tx-completeness gate skipped it, its tracker try_to_lock missed, or + // there are zero peers — and a share marked before that never gets another + // chance: the next walk breaks on the first marked hash, so the tip is + // permanently withheld from the sharechain and its PPLNS credit is lost. + // We mark only what actually reached >= 1 peer, below. std::vector to_send; int32_t height = m_chain->get_height(share_hash); int32_t walk = std::min(height, 5); @@ -805,7 +879,6 @@ void NodeImpl::broadcast_share(const uint256& share_hash) break; if (m_rejected_share_hashes.count(hash)) continue; // skip shares previously rejected by peers - m_shared_share_hashes.insert(hash); to_send.push_back(hash); } @@ -813,10 +886,16 @@ void NodeImpl::broadcast_share(const uint256& share_hash) return; auto now = std::chrono::steady_clock::now(); - for (auto& [nonce, peer] : m_peers) { - send_shares(peer, to_send); - m_last_broadcast_to[peer->addr()] = {to_send, now}; - } + broadcast_and_mark(m_shared_share_hashes, m_peers, to_send, [&](peer_ptr& peer) { + std::vector sent = send_shares(peer, to_send); + // Record what we ACTUALLY wrote, not what we offered: a peer that drops + // within 10s marks this record's hashes rejected, and a rejected hash is + // never re-broadcast. Blaming a share we withheld would turn a transient + // gate skip into permanent loss. + if (!sent.empty()) + m_last_broadcast_to[peer->addr()] = {sent, now}; + return sent; + }); } void NodeImpl::notify_local_share(const uint256& share_hash) @@ -1039,8 +1118,13 @@ void NodeImpl::readvertise_best_share() auto now = std::chrono::steady_clock::now(); for (auto& [nonce, peer] : m_peers) { - send_shares(peer, to_send); - m_last_broadcast_to[peer->addr()] = {to_send, now}; + // Advertise-only path: deliberately ignores the de-dup set, so nothing + // is marked here. Still record only what was ACTUALLY written — a + // gate-skipped share must not be blamed for a later peer drop (that + // would mark it rejected and block every future re-broadcast). + std::vector sent = send_shares(peer, to_send); + if (!sent.empty()) + m_last_broadcast_to[peer->addr()] = {sent, now}; } LOG_INFO << "[readvertise] re-pushed " << to_send.size() << " head share(s) to " << m_peers.size() << " peer(s) (ROOT-2)"; diff --git a/src/impl/dgb/node.hpp b/src/impl/dgb/node.hpp index 86b785d4d..abe14fb26 100644 --- a/src/impl/dgb/node.hpp +++ b/src/impl/dgb/node.hpp @@ -382,9 +382,13 @@ class NodeImpl : public pool::SharechainNode& share_hashes); + /// Send a set of shares (with any needed txs) to a single peer, and return + /// the subset actually written (F2). Shares dropped by the tx-completeness + /// gate, or the whole batch when the tracker lock is unavailable, are + /// reported as NOT sent, so the caller leaves them un-marked and retries + /// them on the next broadcast. + std::vector send_shares(peer_ptr peer, + const std::vector& share_hashes); /// Broadcast a locally-generated (or newly-received) share to all peers. void broadcast_share(const uint256& share_hash); diff --git a/src/impl/dgb/test/CMakeLists.txt b/src/impl/dgb/test/CMakeLists.txt index 3dda7a383..2ebffe1b3 100644 --- a/src/impl/dgb/test/CMakeLists.txt +++ b/src/impl/dgb/test/CMakeLists.txt @@ -3,7 +3,11 @@ # verified into a test executable. Gated on BUILD_TESTING + GTest like the # sibling coins; registered via gtest_add_tests for the CI --target allowlist. if (BUILD_TESTING AND GTest_FOUND) - add_executable(dgb_share_test share_test.cpp) + # Rides dgb_share_test (already in the build.yml --target allowlist) so the + # F1 retention + F2/F3 broadcast-gate KATs are actually compiled and run; + # a fresh target would need a build.yml entry and would otherwise report + # the silently-passing NOT_BUILT sentinel. + add_executable(dgb_share_test share_test.cpp known_txs_retention_test.cpp broadcast_gate_test.cpp) target_link_libraries(dgb_share_test PRIVATE GTest::gtest_main GTest::gtest core dgb diff --git a/src/impl/dgb/test/broadcast_gate_test.cpp b/src/impl/dgb/test/broadcast_gate_test.cpp new file mode 100644 index 000000000..3cd250df7 --- /dev/null +++ b/src/impl/dgb/test/broadcast_gate_test.cpp @@ -0,0 +1,208 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// --------------------------------------------------------------------------- +// F3 tx-completeness broadcast gate + F2 mark-only-what-was-sent — DGB KATs. +// +// Both primitives are the ones send_shares()/broadcast_share() in +// src/impl/dgb/node.cpp actually call, so these KATs pin the shipped policy: +// +// partition_backable() — F3. send_shares must NOT write a share whose +// referenced new-tx BYTES we do not hold: canonical p2pool disconnects on +// "referenced unknown transaction" (p2p.py), which isolates us from the +// sharechain and orphans our shares. Pre-fix, send_shares looked the tx up +// and simply omitted it when absent (`if (it != m_known_txs.end())` with no +// else), sending the share anyway. +// +// broadcast_and_mark() — F2. broadcast_share may add a hash to +// m_shared_share_hashes only AFTER a peer accepted it. Pre-fix it marked the +// whole chain-walk up front, so a batch that send_shares then abandoned (F3 +// skip, tracker try_to_lock miss, zero peers) was withheld FOREVER: the next +// walk breaks on the first marked hash and nothing ever re-pushes it. +// +// Red-able: WalkStrandedWhenMarkedBeforeSend reproduces the pre-fix marking +// order side by side with the shipped one and asserts they differ — restoring +// mark-before-send makes the shipped half fail. GateWithholdsUnbackedShare +// asserts a skip count of 1; deleting the gate flips it red. +// +// Rides the already-allowlisted dgb_share_test executable — no build.yml +// --target allowlist change, no NOT_BUILT sentinel risk. +// Consensus surface: NONE. These primitives decide only WHETHER a share is put +// on the wire; share bytes, minting and payout are untouched. +// --------------------------------------------------------------------------- + +#include + +#include +#include +#include + +#include +#include + +namespace { + +uint256 h(const char* hex) { uint256 v; v.SetHex(hex); return v; } + +// Share hashes S1..S3 and tx hashes TA..TC. +const uint256 S1 = h("11"), S2 = h("22"), S3 = h("33"); +const uint256 TA = h("aa"), TB = h("bb"), TC = h("cc"); + +// Stand-in for a share on the broadcast path: its hash plus the new-tx hashes +// it references. The gate is agnostic to everything else in a share. +struct FakeShare { + uint256 hash; + std::vector new_txs; +}; + +std::vector refs_of(FakeShare& s) { return s.new_txs; } + +// A peer that accepts exactly the hashes send_shares would have written to it. +struct FakePeer { std::set accepts; }; + +} // namespace + +// F3: a share referencing a tx whose bytes we do not hold is removed from the +// outgoing batch; the backable ones survive, in order. +TEST(DgbBroadcastGate, GateWithholdsUnbackedShare) +{ + const std::set held{TA, TB}; // TC bytes NOT held + + std::vector batch{ + {S1, {TA}}, + {S2, {TC}}, // references an unheld tx -> must be withheld + {S3, {TA, TB}}, + }; + + const std::size_t skipped = dgb::partition_backable(batch, held, refs_of); + + EXPECT_EQ(skipped, 1u); + ASSERT_EQ(batch.size(), 2u); + EXPECT_EQ(batch[0].hash, S1); + EXPECT_EQ(batch[1].hash, S3); // order preserved +} + +// F3: a share referencing no new txs is trivially backable (the common case for +// an empty-template share) — the gate must not withhold it. +TEST(DgbBroadcastGate, ShareWithNoTxRefsIsAlwaysSendable) +{ + const std::set held; // hold nothing at all + std::vector batch{{S1, {}}}; + + EXPECT_EQ(dgb::partition_backable(batch, held, refs_of), 0u); + ASSERT_EQ(batch.size(), 1u); + EXPECT_EQ(batch[0].hash, S1); +} + +// F3: when every share is unbacked the batch empties, and send_shares reports +// nothing sent (which is what keeps F2 from marking them). +TEST(DgbBroadcastGate, WholeBatchUnbackedEmptiesTheSend) +{ + const std::set held; + std::vector batch{{S1, {TA}}, {S2, {TB}}}; + + EXPECT_EQ(dgb::partition_backable(batch, held, refs_of), 2u); + EXPECT_TRUE(batch.empty()); +} + +// F2: a batch no peer accepted must leave the de-dup set untouched, so the next +// broadcast walk re-offers it. +TEST(DgbBroadcastMarking, NothingMarkedWhenNoPeerAccepted) +{ + std::set marked; + std::map peers{{1, {}}, {2, {}}}; + + const std::size_t n = dgb::broadcast_and_mark( + marked, peers, std::vector{S1, S2}, + [](FakePeer&) { return std::vector{}; }); + + EXPECT_EQ(n, 0u); + EXPECT_TRUE(marked.empty()); +} + +// F2: with zero peers connected nothing is sent, therefore nothing is marked. +TEST(DgbBroadcastMarking, NothingMarkedWithZeroPeers) +{ + std::set marked; + std::map peers; + + EXPECT_EQ(dgb::broadcast_and_mark(marked, peers, std::vector{S1, S2}, + [](FakePeer&) { return std::vector{}; }), + 0u); + EXPECT_TRUE(marked.empty()); +} + +// F2: exactly the union of what the peers accepted gets marked — no more. +TEST(DgbBroadcastMarking, OnlyAcceptedHashesAreMarked) +{ + std::set marked; + std::map peers{{1, {{S1}}}, {2, {{S1, S3}}}}; + + const std::size_t n = dgb::broadcast_and_mark( + marked, peers, std::vector{S1, S2, S3}, [](FakePeer& p) { + return std::vector(p.accepts.begin(), p.accepts.end()); + }); + + EXPECT_EQ(n, 2u); + EXPECT_TRUE(marked.count(S1)); + EXPECT_TRUE(marked.count(S3)); + EXPECT_FALSE(marked.count(S2)); // withheld by the gate -> retryable +} + +// The regression itself. broadcast_share walks back from the tip and BREAKS on +// the first already-marked hash. Round 1: TB is missing so S2 is withheld and no +// peer accepts anything. Pre-fix (mark first) the walk is stranded forever; +// shipped (mark after send) it re-offers the batch in round 2, when TB arrives. +TEST(DgbBroadcastMarking, WalkStrandedWhenMarkedBeforeSend) +{ + // The tip-first walk broadcast_share performs, verbatim in its essentials. + auto walk = [](const std::vector& chain_tip_first, + const std::set& marked) { + std::vector to_send; + for (const auto& hash : chain_tip_first) { + if (marked.count(hash)) + break; + to_send.push_back(hash); + } + return to_send; + }; + + const std::vector chain{S2, S1}; // S2 is the tip + std::map one_peer{{1, {}}}; + + // ---- pre-fix ordering: mark the walk, THEN discover nothing was sent ---- + { + std::set marked; + auto to_send = walk(chain, marked); + ASSERT_EQ(to_send.size(), 2u); + for (const auto& hash : to_send) marked.insert(hash); // the bug + dgb::broadcast_and_mark(marked, one_peer, to_send, + [](FakePeer&) { return std::vector{}; }); + + // Round 2, TB has since arrived and S2 is backable — but the walk is + // dead: the tip is marked, so nothing is ever re-pushed. Silent, + // permanent share loss with no retry path. + EXPECT_TRUE(walk(chain, marked).empty()); + } + + // ---- shipped ordering: mark only what a peer accepted -------------------- + { + std::set marked; + auto to_send = walk(chain, marked); + ASSERT_EQ(to_send.size(), 2u); + dgb::broadcast_and_mark(marked, one_peer, to_send, + [](FakePeer&) { return std::vector{}; }); + EXPECT_TRUE(marked.empty()); + + // Round 2: the walk still yields the full batch, so the share is retried. + auto retry = walk(chain, marked); + ASSERT_EQ(retry.size(), 2u); + EXPECT_EQ(retry[0], S2); + + // ...and once a peer accepts them they are marked exactly once. + std::map live{{1, {{S1, S2}}}}; + dgb::broadcast_and_mark(marked, live, retry, [](FakePeer& p) { + return std::vector(p.accepts.begin(), p.accepts.end()); + }); + EXPECT_EQ(marked.size(), 2u); + EXPECT_TRUE(walk(chain, marked).empty()); // now correctly de-duped + } +} diff --git a/src/impl/dgb/test/known_txs_retention_test.cpp b/src/impl/dgb/test/known_txs_retention_test.cpp new file mode 100644 index 000000000..664404cfe --- /dev/null +++ b/src/impl/dgb/test/known_txs_retention_test.cpp @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// F3 backable-broadcast gate + F1 template-identity retention — DGB unit KAT. +// +// Covers the two pure primitives ported from the DASH variant into +// src/impl/dgb/known_txs_retention.hpp: +// +// retain_template_txs() — F1 template-identity dedup: N re-registrations of +// ONE template consume ONE slot while the rolling window still holds N +// DISTINCT templates; a tx leaves known_txs only once it is absent from +// EVERY retained set. +// +// all_txs_backable() — F3 broadcast gate: a share is backable (safe to send +// without the peer's "referenced unknown transaction" disconnect) only when +// we hold the bytes of every referenced new-tx. +// +// Non-hollow guard: DgbKnownTxsBackable.UnheldTxIsNotBackable asserts false for a share +// referencing an unheld tx; deleting the gate (all_txs_backable → always true) +// flips it red. DgbKnownTxsRetention.OneTemplateConsumesOneSlot asserts size==1 after N +// re-registrations; dropping the dedup (push every registration) flips it red. + +#include + +#include +#include +#include +#include + +#include +#include + +namespace { + +// Distinct, deterministic uint256 tx hashes A..E. +uint256 h(const char* hex) +{ + uint256 v; v.SetHex(hex); return v; +} +const uint256 A = h("aa"); +const uint256 B = h("bb"); +const uint256 C = h("cc"); +const uint256 D = h("dd"); +const uint256 E = h("ee"); + +// A tx-bytes stand-in; the retention logic is agnostic to the payload type. +using TxMap = std::map; + +} // namespace + +// F1: re-registering the SAME template (identical tx-hash set) N times consumes +// exactly ONE slot and refreshes recency — it must NOT collapse the window. +TEST(DgbKnownTxsRetention, OneTemplateConsumesOneSlot) +{ + std::deque> recent; + TxMap known; + + const std::vector hashes{A, B}; + const std::vector txs{1, 2}; + + for (int i = 0; i < 5; ++i) + dgb::retain_template_txs(recent, known, hashes, txs, /*cap=*/3); + + EXPECT_EQ(recent.size(), 1u); // dedup: 5 registrations -> 1 slot + EXPECT_EQ(known.size(), 2u); // A,B inserted once + EXPECT_TRUE(known.count(A)); + EXPECT_TRUE(known.count(B)); +} + +// F1: the window holds N DISTINCT templates, and a tx is evicted only once it +// has fallen out of EVERY retained set (B survives in T2 after T1 is evicted). +TEST(DgbKnownTxsRetention, WindowHoldsDistinctTemplatesAndEvictsCorrectly) +{ + std::deque> recent; + TxMap known; + const std::vector two{1, 2}; + + dgb::retain_template_txs(recent, known, std::vector{A, B}, two, 3); // T1 + dgb::retain_template_txs(recent, known, std::vector{B, C}, two, 3); // T2 + dgb::retain_template_txs(recent, known, std::vector{C, D}, two, 3); // T3 + EXPECT_EQ(recent.size(), 3u); + for (const auto& x : {A, B, C, D}) EXPECT_TRUE(known.count(x)) << "missing before evict"; + + // T4 pushes the window past cap=3 -> oldest (T1={A,B}) evicted. + dgb::retain_template_txs(recent, known, std::vector{D, E}, two, 3); // T4 + EXPECT_EQ(recent.size(), 3u); + EXPECT_FALSE(known.count(A)); // A only lived in T1 -> erased + EXPECT_TRUE(known.count(B)); // B still retained via T2 -> kept + EXPECT_TRUE(known.count(C)); + EXPECT_TRUE(known.count(D)); + EXPECT_TRUE(known.count(E)); +} + +// F3: a share whose referenced new-txs we all hold is backable; one unheld tx +// makes it NOT backable (must not be broadcast). +TEST(DgbKnownTxsBackable, UnheldTxIsNotBackable) +{ + const std::set held{A, B}; + + EXPECT_TRUE(dgb::all_txs_backable(std::vector{A, B}, held)); // all held -> send + EXPECT_FALSE(dgb::all_txs_backable(std::vector{A, C}, held)); // C unheld -> withhold + EXPECT_TRUE(dgb::all_txs_backable(std::vector{}, held)); // no refs -> trivially backable +} From 5530ceb53d706d73106c56f4abece466ce214612 Mon Sep 17 00:00:00 2001 From: frstrtr Date: Sun, 26 Jul 2026 13:08:45 +0400 Subject: [PATCH 2/4] dgb: rescope to the reachable readvertise path; label broadcast_share pre-wiring A caller-side reachability trace shows DGB is not symmetric with btc/bch: main_dgb.cpp binds no create/mint-share fn, so dgb::NodeImpl::broadcast_share and notify_local_share have zero callers repo-wide -- DGB never mints a local share today (#884). DGB's live path into send_shares is readvertise_best_share() (ROOT-2 re-advert on best-change plus a 10s one-shot timer), which re-pushes PEER-RECEIVED shares and deliberately bypasses the de-dup set. Verified independently: set_mint_share_fn / set_create_share_fn / broadcast_share / notify_local_share have zero occurrences in main_dgb.cpp; the only set_mint_share_fn binding in the tree is main_dash.cpp:1854 plus dgb's own work_source_test. readvertise_best_share is called from node.cpp:1767 and 1774. BTC (main_btc.cpp:1336 after lk.unlock() at :1323) and BCH (pool_entrypoint.hpp:564 after lk.unlock() at :562) are both live and unchanged. Changes: - dgb: extract the readvertise recording rule into readvertise_and_record() and wire readvertise_best_share() through it. This is DGB's live reward-critical marking discipline: m_last_broadcast_to is what a peer disconnect within 10s converts into m_rejected_share_hashes, and the readvertise walk `continue`s past rejected hashes permanently. Recording the OFFERED batch would let a share the F3 gate merely withheld be blamed for someone else's drop and excluded from every future re-advert -- a propagation loss for the whole sharechain, since DGB re-advertises peer-received shares. - dgb: rescope the KATs onto that live path. The gate cases (partition_backable) and the new DgbReadvertiseRecording cases drive code production executes; the broadcast_and_mark cases are renamed DgbBroadcastMarkingPrewired_NotReachedToday so no reader mistakes them for live protection. Removes the broadcast_share de-dup-walk KAT, which modelled an unreachable function -- the shape of the LTC #873 miss. - dgb: label NodeImpl::broadcast_share in source as pre-wiring for #884. The F2 fix stays so the seam is correct when the mint path is bound and so the three coins remain symmetric, but it protects nothing at runtime today. - btc/bch: document the ROOT-2 interaction at readvertise_best(). Marking is now a strict subset of what the pre-fix code marked, so the walk breaks later or in the same place, never earlier: the re-advert re-pushes a superset of what it used to. Monotonically better, strictly better for head shares minted while no peer was connected. It does not fully close ROOT-2 here -- shares already accepted by another peer stay marked and the walk still breaks, as before. That needs the ltc/dgb-style de-dup-bypass readvertise; pre-existing, out of scope. No behaviour change in this commit for btc/bch. Mutation-verified red on the DGB live path: recording the offered batch instead of what was written reddens WithheldShareIsNeverBlamedForAPeerDrop; dropping the empty-send skip reddens PeerThatReceivedNothingIsNotRecorded; deleting the gate reddens 2 DgbBroadcastGate cases. --- src/impl/bch/node.cpp | 14 ++ src/impl/btc/node.cpp | 14 ++ src/impl/dgb/known_txs_retention.hpp | 50 +++++ src/impl/dgb/node.cpp | 25 ++- src/impl/dgb/test/broadcast_gate_test.cpp | 238 +++++++++++++--------- 5 files changed, 234 insertions(+), 107 deletions(-) diff --git a/src/impl/bch/node.cpp b/src/impl/bch/node.cpp index c231fcb53..9863ed5cf 100644 --- a/src/impl/bch/node.cpp +++ b/src/impl/bch/node.cpp @@ -1062,6 +1062,20 @@ uint256 NodeImpl::advertised_best_share() return uint256::ZERO; } +// ROOT-2 re-advert. Unlike ltc/dgb (which have a dedicated readvertise that +// bypasses the de-dup set), this delegates to broadcast_share, so the walk still +// breaks on the first hash already in m_shared_share_hashes. +// +// Interaction with the F2 marking change: marking is now a strict SUBSET of what +// the pre-fix code marked (only hashes a peer actually accepted). The walk breaks +// later or in the same place, never earlier, so this re-advert re-pushes a +// superset of what it used to — monotonically better, no regression. It is +// strictly better in the case that motivated ROOT-2: head shares minted while no +// peer was connected are no longer marked, so a peer that handshook during the +// empty window now receives them. It does NOT fully close ROOT-2 here — head +// shares already accepted by some OTHER peer stay marked and the walk still +// breaks, exactly as before. Closing that needs the ltc/dgb-style de-dup-bypass +// readvertise; pre-existing gap, out of scope for this change. void NodeImpl::readvertise_best() { if (m_peers.empty()) diff --git a/src/impl/btc/node.cpp b/src/impl/btc/node.cpp index 30fc798b9..1d5a089eb 100644 --- a/src/impl/btc/node.cpp +++ b/src/impl/btc/node.cpp @@ -1135,6 +1135,20 @@ uint256 NodeImpl::advertised_best_share() return uint256::ZERO; } +// ROOT-2 re-advert. Unlike ltc/dgb (which have a dedicated readvertise that +// bypasses the de-dup set), this delegates to broadcast_share, so the walk still +// breaks on the first hash already in m_shared_share_hashes. +// +// Interaction with the F2 marking change: marking is now a strict SUBSET of what +// the pre-fix code marked (only hashes a peer actually accepted). The walk breaks +// later or in the same place, never earlier, so this re-advert re-pushes a +// superset of what it used to — monotonically better, no regression. It is +// strictly better in the case that motivated ROOT-2: head shares minted while no +// peer was connected are no longer marked, so a peer that handshook during the +// empty window now receives them. It does NOT fully close ROOT-2 here — head +// shares already accepted by some OTHER peer stay marked and the walk still +// breaks, exactly as before. Closing that needs the ltc/dgb-style de-dup-bypass +// readvertise; pre-existing gap, out of scope for this change. void NodeImpl::readvertise_best() { if (m_peers.empty()) diff --git a/src/impl/dgb/known_txs_retention.hpp b/src/impl/dgb/known_txs_retention.hpp index b1004ba9d..b6ce35a72 100644 --- a/src/impl/dgb/known_txs_retention.hpp +++ b/src/impl/dgb/known_txs_retention.hpp @@ -33,6 +33,22 @@ // try_to_lock missed, or there were no peers — because the de-dup set then // breaks the walk forever and nothing re-pushes it. // +// readvertise_and_record() — DGB-ONLY. The marking discipline for the +// advertise-only ROOT-2 re-push, which is DGB's LIVE path (see below). +// +// DGB REACHABILITY — differs from btc/bch; read this before trusting the file. +// main_dgb.cpp binds NO create/mint-share fn, so dgb::NodeImpl::broadcast_share +// and notify_local_share have ZERO callers repo-wide: DGB never mints a local +// share today (tracked as #884). The live DGB path into send_shares is +// readvertise_best_share() (ROOT-2 re-advert, on best-change and on a timer), +// which re-pushes PEER-RECEIVED shares and deliberately bypasses the de-dup +// set. Therefore, on DGB: +// partition_backable() — LIVE (send_shares, reached via readvertise) +// readvertise_and_record() — LIVE (readvertise_best_share) +// broadcast_and_mark() — PRE-WIRED; dead until #884 binds the mint seam. +// Anything asserting the DGB gate through a locally-minted share broadcast is +// exercising a path production never takes. +// // NOTE (de-dup): the same primitives now exist per-coin (btc/dgb/bch/dash) while // the LTC lane hoists all_txs_backable into src/core/. Once both land, these // per-coin headers should be collapsed onto that core hoist. @@ -178,4 +194,38 @@ inline std::size_t broadcast_and_mark(MarkedSet& marked, Peers& peers, return actually_sent.size(); } +// Advertise-only ROOT-2 re-push (readvertise_best_share) — DGB's LIVE marking +// path. Offers `to_send` to every peer via `send`, which reports what it +// ACTUALLY wrote, and records that report — never the offered batch — through +// `record(peer, sent)`. A peer that received nothing gets NO record at all. +// Returns the number of peers reached. +// +// Why the recording rule is reward-critical and not cosmetic: m_last_broadcast_to +// is what converts a peer disconnect within 10s into m_rejected_share_hashes +// entries, and the readvertise walk `continue`s past every rejected hash — +// permanently. Recording the OFFERED batch would let a share the F3 completeness +// gate merely withheld be blamed for someone else's disconnect, turning a +// transient, self-healing skip into permanent exclusion from every future +// re-advert. DGB re-advertises PEER-RECEIVED shares, so that is a propagation +// loss for the whole sharechain, not only for us. +template +inline std::size_t readvertise_and_record(Peers& peers, + const std::vector& to_send, + SendFn send, + RecordFn record) +{ + if (to_send.empty()) + return 0; + + std::size_t peers_reached = 0; + for (auto& entry : peers) { + const std::vector sent = send(entry.second); + if (sent.empty()) + continue; // nothing written -> nothing a later drop may blame + record(entry.second, sent); + ++peers_reached; + } + return peers_reached; +} + } // namespace dgb diff --git a/src/impl/dgb/node.cpp b/src/impl/dgb/node.cpp index e7fd55c11..316354458 100644 --- a/src/impl/dgb/node.cpp +++ b/src/impl/dgb/node.cpp @@ -841,6 +841,12 @@ std::vector NodeImpl::send_shares(peer_ptr peer, return sent; } +// PRE-WIRING — NOT REACHED ON DGB TODAY (#884). main_dgb.cpp binds no +// create/mint-share fn, so this function and notify_local_share have zero +// callers repo-wide: DGB never mints a local share. The F2 mark-after-send fix +// below is applied so the seam is correct on the day #884 binds the mint path, +// and so btc/dgb/bch stay symmetric — it protects nothing at runtime yet. DGB's +// LIVE send path is readvertise_best_share() above. void NodeImpl::broadcast_share(const uint256& share_hash) { // try_to_lock per the architectural rule (node.hpp:67) — see freeze @@ -1116,16 +1122,19 @@ void NodeImpl::readvertise_best_share() if (to_send.empty()) return; + // Advertise-only path: deliberately ignores the de-dup set, so nothing is + // marked here. But record only what was ACTUALLY written — a share the F3 + // completeness gate withheld must not be blamed for a later peer drop, which + // would mark it rejected and make the walk above `continue` past it forever. + // This is DGB's LIVE reward-critical marking path; broadcast_share below is + // dead until the mint seam is bound (#884). auto now = std::chrono::steady_clock::now(); - for (auto& [nonce, peer] : m_peers) { - // Advertise-only path: deliberately ignores the de-dup set, so nothing - // is marked here. Still record only what was ACTUALLY written — a - // gate-skipped share must not be blamed for a later peer drop (that - // would mark it rejected and block every future re-broadcast). - std::vector sent = send_shares(peer, to_send); - if (!sent.empty()) + readvertise_and_record( + m_peers, to_send, + [&](peer_ptr& peer) { return send_shares(peer, to_send); }, + [&](peer_ptr& peer, const std::vector& sent) { m_last_broadcast_to[peer->addr()] = {sent, now}; - } + }); LOG_INFO << "[readvertise] re-pushed " << to_send.size() << " head share(s) to " << m_peers.size() << " peer(s) (ROOT-2)"; } diff --git a/src/impl/dgb/test/broadcast_gate_test.cpp b/src/impl/dgb/test/broadcast_gate_test.cpp index 3cd250df7..5af1c13f7 100644 --- a/src/impl/dgb/test/broadcast_gate_test.cpp +++ b/src/impl/dgb/test/broadcast_gate_test.cpp @@ -1,27 +1,30 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // --------------------------------------------------------------------------- -// F3 tx-completeness broadcast gate + F2 mark-only-what-was-sent — DGB KATs. +// F3 tx-completeness broadcast gate + broadcast marking discipline — DGB KATs. // -// Both primitives are the ones send_shares()/broadcast_share() in -// src/impl/dgb/node.cpp actually call, so these KATs pin the shipped policy: +// READ FIRST — DGB REACHABILITY. DGB is NOT symmetric with btc/bch here, and +// these KATs are deliberately scoped to the path production actually takes: // -// partition_backable() — F3. send_shares must NOT write a share whose -// referenced new-tx BYTES we do not hold: canonical p2pool disconnects on -// "referenced unknown transaction" (p2p.py), which isolates us from the -// sharechain and orphans our shares. Pre-fix, send_shares looked the tx up -// and simply omitted it when absent (`if (it != m_known_txs.end())` with no -// else), sending the share anyway. +// main_dgb.cpp binds NO create/mint-share fn, so dgb::NodeImpl::broadcast_share +// and notify_local_share have ZERO callers repo-wide — DGB never mints a local +// share today (tracked as #884). The LIVE DGB path into send_shares is +// readvertise_best_share() (ROOT-2 re-advert, fired on best-change and on a +// timer), which re-pushes PEER-RECEIVED shares and deliberately bypasses the +// broadcast de-dup set. // -// broadcast_and_mark() — F2. broadcast_share may add a hash to -// m_shared_share_hashes only AFTER a peer accepted it. Pre-fix it marked the -// whole chain-walk up front, so a batch that send_shares then abandoned (F3 -// skip, tracker try_to_lock miss, zero peers) was withheld FOREVER: the next -// walk breaks on the first marked hash and nothing ever re-pushes it. +// LIVE on DGB: partition_backable() (send_shares, via readvertise) +// readvertise_and_record() (readvertise_best_share) +// PRE-WIRED, dead: broadcast_and_mark() (broadcast_share; #884) // -// Red-able: WalkStrandedWhenMarkedBeforeSend reproduces the pre-fix marking -// order side by side with the shipped one and asserts they differ — restoring -// mark-before-send makes the shipped half fail. GateWithholdsUnbackedShare -// asserts a skip count of 1; deleting the gate flips it red. +// So the reward-critical assertions below drive readvertise_and_record, NOT +// broadcast_and_mark. A DGB test that proved the gate by broadcasting a locally +// minted share would be exercising a path production never takes — the exact +// shape of the LTC #873 miss, where both the test AND its negative control +// passed against an unreachable function. +// +// The pre-wired broadcast_and_mark coverage is kept but named +// DgbBroadcastMarkingPrewired_NotReachedToday so a reader cannot mistake it for +// live protection; it exists so the seam is correct on the day #884 lands. // // Rides the already-allowlisted dgb_share_test executable — no build.yml // --target allowlist change, no NOT_BUILT sentinel risk. @@ -58,9 +61,27 @@ std::vector refs_of(FakeShare& s) { return s.new_txs; } // A peer that accepts exactly the hashes send_shares would have written to it. struct FakePeer { std::set accepts; }; +// The readvertise_best_share tip walk, verbatim in its essentials: it does NOT +// consult the de-dup set, but it DOES skip every peer-rejected hash — and that +// skip is permanent, which is what makes the recording rule reward-critical. +std::vector readvertise_walk(const std::vector& chain_tip_first, + const std::set& rejected) +{ + std::vector to_send; + for (const auto& hash : chain_tip_first) { + if (rejected.count(hash)) + continue; + to_send.push_back(hash); + } + return to_send; +} + } // namespace -// F3: a share referencing a tx whose bytes we do not hold is removed from the +// =========================== LIVE PATH: F3 gate ============================= +// Reached in production via readvertise_best_share -> send_shares. + +// A share referencing a tx whose bytes we do not hold is removed from the // outgoing batch; the backable ones survive, in order. TEST(DgbBroadcastGate, GateWithholdsUnbackedShare) { @@ -80,8 +101,8 @@ TEST(DgbBroadcastGate, GateWithholdsUnbackedShare) EXPECT_EQ(batch[1].hash, S3); // order preserved } -// F3: a share referencing no new txs is trivially backable (the common case for -// an empty-template share) — the gate must not withhold it. +// A share referencing no new txs is trivially backable (the common case for an +// empty-template share) — the gate must not withhold it. TEST(DgbBroadcastGate, ShareWithNoTxRefsIsAlwaysSendable) { const std::set held; // hold nothing at all @@ -92,8 +113,8 @@ TEST(DgbBroadcastGate, ShareWithNoTxRefsIsAlwaysSendable) EXPECT_EQ(batch[0].hash, S1); } -// F3: when every share is unbacked the batch empties, and send_shares reports -// nothing sent (which is what keeps F2 from marking them). +// When every share is unbacked the batch empties, so send_shares reports nothing +// written — which is what keeps the readvertise record empty below. TEST(DgbBroadcastGate, WholeBatchUnbackedEmptiesTheSend) { const std::set held; @@ -103,35 +124,114 @@ TEST(DgbBroadcastGate, WholeBatchUnbackedEmptiesTheSend) EXPECT_TRUE(batch.empty()); } -// F2: a batch no peer accepted must leave the de-dup set untouched, so the next -// broadcast walk re-offers it. -TEST(DgbBroadcastMarking, NothingMarkedWhenNoPeerAccepted) +// ================= LIVE PATH: readvertise recording discipline ============== +// Reached in production via readvertise_best_share (best-change + timer). + +// The DGB regression. readvertise offers three shares; the F3 gate withholds S2 +// because we lack its tx bytes. The per-peer record must contain ONLY what was +// written, because that record is what a peer disconnect within 10s converts +// into m_rejected_share_hashes — and the readvertise walk skips rejected hashes +// PERMANENTLY. Recording the offered batch would blame S2 for someone else's +// drop and exclude it from every future re-advert. +TEST(DgbReadvertiseRecording, WithheldShareIsNeverBlamedForAPeerDrop) { - std::set marked; + const std::vector offered{S1, S2, S3}; + // The peer receives the gated batch: S2 was withheld by partition_backable. + std::map peers{{1, {{S1, S3}}}}; + + std::vector recorded; + bool recorded_any = false; + const std::size_t reached = dgb::readvertise_and_record( + peers, offered, + [](FakePeer& p) { + return std::vector(p.accepts.begin(), p.accepts.end()); + }, + [&](FakePeer&, const std::vector& sent) { + recorded = sent; + recorded_any = true; + }); + + EXPECT_EQ(reached, 1u); + ASSERT_TRUE(recorded_any); + const std::set rec(recorded.begin(), recorded.end()); + EXPECT_TRUE(rec.count(S1)); + EXPECT_TRUE(rec.count(S3)); + EXPECT_FALSE(rec.count(S2)) << "withheld share must not be in the record"; + + // The peer now drops inside the 10s window: everything recorded for it is + // marked rejected. S2 must survive that, and stay eligible for re-advert. + const auto retry = readvertise_walk({S3, S2, S1}, rec); + ASSERT_EQ(retry.size(), 1u); + EXPECT_EQ(retry[0], S2) << "S2 must still be re-advertisable after the drop"; +} + +// A peer that received NOTHING gets no record at all, so a later drop has +// nothing to blame — the whole batch stays eligible for the next re-advert. +TEST(DgbReadvertiseRecording, PeerThatReceivedNothingIsNotRecorded) +{ + const std::vector offered{S1, S2}; std::map peers{{1, {}}, {2, {}}}; - const std::size_t n = dgb::broadcast_and_mark( - marked, peers, std::vector{S1, S2}, - [](FakePeer&) { return std::vector{}; }); + std::size_t records = 0; + const std::size_t reached = dgb::readvertise_and_record( + peers, offered, + [](FakePeer&) { return std::vector{}; }, + [&](FakePeer&, const std::vector&) { ++records; }); - EXPECT_EQ(n, 0u); - EXPECT_TRUE(marked.empty()); + EXPECT_EQ(reached, 0u); + EXPECT_EQ(records, 0u); + + // Nothing rejected -> the next walk still offers the whole batch. + EXPECT_EQ(readvertise_walk({S2, S1}, /*rejected=*/{}).size(), 2u); } -// F2: with zero peers connected nothing is sent, therefore nothing is marked. -TEST(DgbBroadcastMarking, NothingMarkedWithZeroPeers) +// With zero peers connected nothing is sent and nothing is recorded. +TEST(DgbReadvertiseRecording, ZeroPeersRecordsNothing) { - std::set marked; std::map peers; + std::size_t records = 0; + EXPECT_EQ(dgb::readvertise_and_record( + peers, std::vector{S1}, + [](FakePeer&) { return std::vector{}; }, + [&](FakePeer&, const std::vector&) { ++records; }), + 0u); + EXPECT_EQ(records, 0u); +} - EXPECT_EQ(dgb::broadcast_and_mark(marked, peers, std::vector{S1, S2}, - [](FakePeer&) { return std::vector{}; }), +// An empty offer is a no-op: readvertise_best_share returns before the peer loop. +TEST(DgbReadvertiseRecording, EmptyOfferIsANoOp) +{ + std::map peers{{1, {{S1}}}}; + std::size_t sends = 0, records = 0; + EXPECT_EQ(dgb::readvertise_and_record( + peers, std::vector{}, + [&](FakePeer&) { ++sends; return std::vector{S1}; }, + [&](FakePeer&, const std::vector&) { ++records; }), 0u); + EXPECT_EQ(sends, 0u); + EXPECT_EQ(records, 0u); +} + +// ============ PRE-WIRED (NOT REACHED ON DGB TODAY — see #884) =============== +// broadcast_and_mark backs dgb::NodeImpl::broadcast_share, which has zero +// callers because main_dgb.cpp binds no mint seam. These KATs pin the seam so it +// is correct the day #884 binds it; they assert NOTHING about DGB today. The +// live equivalents are the DgbReadvertiseRecording cases above. + +TEST(DgbBroadcastMarkingPrewired_NotReachedToday, NothingMarkedWhenNoPeerAccepted) +{ + std::set marked; + std::map peers{{1, {}}, {2, {}}}; + + const std::size_t n = dgb::broadcast_and_mark( + marked, peers, std::vector{S1, S2}, + [](FakePeer&) { return std::vector{}; }); + + EXPECT_EQ(n, 0u); EXPECT_TRUE(marked.empty()); } -// F2: exactly the union of what the peers accepted gets marked — no more. -TEST(DgbBroadcastMarking, OnlyAcceptedHashesAreMarked) +TEST(DgbBroadcastMarkingPrewired_NotReachedToday, OnlyAcceptedHashesAreMarked) { std::set marked; std::map peers{{1, {{S1}}}, {2, {{S1, S3}}}}; @@ -146,63 +246,3 @@ TEST(DgbBroadcastMarking, OnlyAcceptedHashesAreMarked) EXPECT_TRUE(marked.count(S3)); EXPECT_FALSE(marked.count(S2)); // withheld by the gate -> retryable } - -// The regression itself. broadcast_share walks back from the tip and BREAKS on -// the first already-marked hash. Round 1: TB is missing so S2 is withheld and no -// peer accepts anything. Pre-fix (mark first) the walk is stranded forever; -// shipped (mark after send) it re-offers the batch in round 2, when TB arrives. -TEST(DgbBroadcastMarking, WalkStrandedWhenMarkedBeforeSend) -{ - // The tip-first walk broadcast_share performs, verbatim in its essentials. - auto walk = [](const std::vector& chain_tip_first, - const std::set& marked) { - std::vector to_send; - for (const auto& hash : chain_tip_first) { - if (marked.count(hash)) - break; - to_send.push_back(hash); - } - return to_send; - }; - - const std::vector chain{S2, S1}; // S2 is the tip - std::map one_peer{{1, {}}}; - - // ---- pre-fix ordering: mark the walk, THEN discover nothing was sent ---- - { - std::set marked; - auto to_send = walk(chain, marked); - ASSERT_EQ(to_send.size(), 2u); - for (const auto& hash : to_send) marked.insert(hash); // the bug - dgb::broadcast_and_mark(marked, one_peer, to_send, - [](FakePeer&) { return std::vector{}; }); - - // Round 2, TB has since arrived and S2 is backable — but the walk is - // dead: the tip is marked, so nothing is ever re-pushed. Silent, - // permanent share loss with no retry path. - EXPECT_TRUE(walk(chain, marked).empty()); - } - - // ---- shipped ordering: mark only what a peer accepted -------------------- - { - std::set marked; - auto to_send = walk(chain, marked); - ASSERT_EQ(to_send.size(), 2u); - dgb::broadcast_and_mark(marked, one_peer, to_send, - [](FakePeer&) { return std::vector{}; }); - EXPECT_TRUE(marked.empty()); - - // Round 2: the walk still yields the full batch, so the share is retried. - auto retry = walk(chain, marked); - ASSERT_EQ(retry.size(), 2u); - EXPECT_EQ(retry[0], S2); - - // ...and once a peer accepts them they are marked exactly once. - std::map live{{1, {{S1, S2}}}}; - dgb::broadcast_and_mark(marked, live, retry, [](FakePeer& p) { - return std::vector(p.accepts.begin(), p.accepts.end()); - }); - EXPECT_EQ(marked.size(), 2u); - EXPECT_TRUE(walk(chain, marked).empty()); // now correctly de-duped - } -} From d3e51513b0dd605a76df7d1092a12ab2bad9bc41 Mon Sep 17 00:00:00 2001 From: frstrtr Date: Mon, 27 Jul 2026 09:28:23 +0000 Subject: [PATCH 3/4] btc/bch/dgb: route F3 send-path tx-completeness gate through the per-coin new_tx_hashes SSOT The old flat m_new_transaction_hashes probe was FALSE for every share variant (v17/v33 nest the refs in m_tx_info; v34+ carry none), so the F3 gate collected empty refs and treated every share as vacuously backable -- it never withheld an incomplete share on any version. Route each coin through its SSOT accessor from #913/#914 (btc/bch::new_tx_hashes, dgb::append_share_tx_refs) so the gate sees the real refs. Adds BtcBroadcastGate.RealShareTxInfoRefsWithheld driving a REAL v17/v33 share (not a top-level mock); mutation-verify confirms it goes red when the probe is reverted to the dead flat form. --- src/impl/bch/node.cpp | 9 +- src/impl/btc/node.cpp | 11 ++- src/impl/btc/share_tx_refs.hpp | 68 ++++++++++++++ src/impl/btc/test/broadcast_gate_test.cpp | 105 ++++++++++++++++++++++ src/impl/dgb/node.cpp | 8 +- 5 files changed, 192 insertions(+), 9 deletions(-) create mode 100644 src/impl/btc/share_tx_refs.hpp diff --git a/src/impl/bch/node.cpp b/src/impl/bch/node.cpp index 9863ed5cf..bc84717b3 100644 --- a/src/impl/bch/node.cpp +++ b/src/impl/bch/node.cpp @@ -746,9 +746,12 @@ std::vector NodeImpl::send_shares(peer_ptr peer, shares, m_known_txs, [](ShareType& share) { std::vector hashes; share.invoke([&](auto* obj) { - if constexpr (requires { obj->m_new_transaction_hashes; }) - hashes.assign(obj->m_new_transaction_hashes.begin(), - obj->m_new_transaction_hashes.end()); + // #880: route through the bch::new_tx_hashes SSOT (#913) + // instead of the dead flat probe -- v17/v33 nest the list in + // m_tx_info, v34+ carry none. The old probe was FALSE for + // every variant, so this F3 gate never withheld a share. + if (const auto* new_txs = bch::new_tx_hashes(obj)) + hashes.assign(new_txs->begin(), new_txs->end()); }); return hashes; }); diff --git a/src/impl/btc/node.cpp b/src/impl/btc/node.cpp index 1d5a089eb..ba54148fc 100644 --- a/src/impl/btc/node.cpp +++ b/src/impl/btc/node.cpp @@ -1,6 +1,7 @@ // SPDX-License-Identifier: AGPL-3.0-or-later #include "node.hpp" #include "known_txs_retention.hpp" // F3 retain_template_txs + select_backable_shares +#include "share_tx_refs.hpp" // btc::new_tx_hashes -- uniform gate probe (#880) #include "coin/template_other_txs.hpp" // deserialize_template_other_txs (GBT data[] -> MutableTransaction) #include @@ -787,9 +788,13 @@ std::vector NodeImpl::send_shares(peer_ptr peer, const std::vector hashes; share.invoke([&](auto* obj) { - if constexpr (requires { obj->m_new_transaction_hashes; }) - hashes.assign(obj->m_new_transaction_hashes.begin(), - obj->m_new_transaction_hashes.end()); + // #880: route through the btc::new_tx_hashes SSOT instead + // of the dead flat probe -- v17/v33 nest the list in + // m_tx_info, v34+ carry none. The old probe was FALSE for + // every variant, so this F3 gate saw empty refs -> every + // share vacuously backable -> the gate no-op'd all versions. + if (const auto* new_txs = btc::new_tx_hashes(obj)) + hashes.assign(new_txs->begin(), new_txs->end()); }); return hashes; }); diff --git a/src/impl/btc/share_tx_refs.hpp b/src/impl/btc/share_tx_refs.hpp new file mode 100644 index 000000000..1db302b5b --- /dev/null +++ b/src/impl/btc/share_tx_refs.hpp @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// +// Uniform access to a share's "new transaction hashes" list across the BTC +// share variants. +// +// WHY THIS EXISTS. The BTC share variants do NOT all spell this list the same +// way, and they do not all HAVE it: +// +// btc::Share (v17) -> obj->m_tx_info.m_new_transaction_hashes +// btc::NewShare (v33) -> obj->m_tx_info.m_new_transaction_hashes +// btc::SegwitMiningShare (v34) -> none (carries types::DataSegwitShare, no +// btc::PaddingBugfixShare (v35) m_tx_info member -- share_check leaves the +// btc::MergedMiningShare (v36) tx list empty for version >= 34) +// +// node.cpp's send_shares() F3 broadcast gate (partition_backable) probed +// `requires { obj->m_new_transaction_hashes; }` directly on the share object. +// That expression is FALSE for every one of the five variants -- v17/v33 nest +// the list inside m_tx_info, v34+ have no list -- so the gate collected an empty +// ref list for every share, every share was vacuously backable, and the gate +// no-op'd for ALL versions (#880): a v17/v33 share referencing a tx the peer +// lacked was broadcast anyway, tripping the canonical "referenced unknown +// transaction" disconnect. The per_share_refs relay block below the gate already +// spells the nested list correctly (#871), which is why relay worked and only +// the gate probe was dead. +// +// Probing through one accessor removes the class of bug: a new share variant +// either exposes the list in one of the two known spellings and is handled, or +// exposes none and is reported as "no list" (nothing to gate on) -- never +// silently dead because of a member-name mismatch. Mirrors bch::new_tx_hashes +// (src/impl/bch/share_tx_refs.hpp, #913) and ltc::new_tx_hashes +// (src/impl/ltc/share_tx_refs.hpp, #873); BTC's variant topology is identical. + +#pragma once + +#include + +#include + +namespace btc { + +// Pointer to the share's new-transaction-hash list, or nullptr when this share +// variant carries none. A pointer (not a copy) so the broadcast hot path does no +// per-share allocation. Lifetime is the share object's. +template +inline auto new_tx_hashes(ShareObj* obj) +{ + if constexpr (requires { obj->m_tx_info.m_new_transaction_hashes; }) + return &obj->m_tx_info.m_new_transaction_hashes; + else if constexpr (requires { obj->m_new_transaction_hashes; }) + return &obj->m_new_transaction_hashes; + else + return static_cast*>(nullptr); +} + +// Compile-time pin of the fact above: true iff the share variant exposes the +// list under the FLAT spelling that the old probe assumed. Held false by the KAT +// for every BTC variant -- if this ever becomes true, the flat branch of +// new_tx_hashes() is what handles it, not a silent skip. +template +inline constexpr bool has_flat_new_tx_hashes = + requires(ShareObj* o) { o->m_new_transaction_hashes; }; + +// True iff the share variant nests the list inside m_tx_info (v17 / v33). +template +inline constexpr bool has_nested_new_tx_hashes = + requires(ShareObj* o) { o->m_tx_info.m_new_transaction_hashes; }; + +} // namespace btc diff --git a/src/impl/btc/test/broadcast_gate_test.cpp b/src/impl/btc/test/broadcast_gate_test.cpp index 502dc7946..06be34d73 100644 --- a/src/impl/btc/test/broadcast_gate_test.cpp +++ b/src/impl/btc/test/broadcast_gate_test.cpp @@ -33,10 +33,13 @@ #include #include +#include #include #include #include +#include // real btc share variants + ShareType +#include // btc::new_tx_hashes SSOT (#880) namespace { @@ -206,3 +209,105 @@ TEST(BtcBroadcastMarking, WalkStrandedWhenMarkedBeforeSend) EXPECT_TRUE(walk(chain, marked).empty()); // now correctly de-duped } } + + +// --------------------------------------------------------------------------- +// #880 REGRESSION: the F3 gate above must be driven by the PRODUCTION probe, +// not a hand-rolled stand-in. The FakeShare KATs exercise partition_backable's +// MECHANICS with a top-level new_txs member and a bespoke refs_of -- exactly +// the shape that let the real bug hide. In production send_shares probed +// `requires { obj->m_new_transaction_hashes; }`, which is FALSE for every BTC +// share variant (the list is nested in m_tx_info), so the gate collected empty +// refs, every share was vacuously backable, and the gate silently no-op'd for +// ALL versions -- a share referencing a tx the peer lacked was broadcast anyway +// and tripped the canonical "referenced unknown transaction" disconnect. +// +// These KATs build REAL btc share types and drive them through the SAME +// btc::new_tx_hashes SSOT that node.cpp's partition_backable refs_of now uses. +// FAILS-BEFORE: revert btc::new_tx_hashes (or the gate) to the flat probe and +// the v17/v33 expectations collapse -- the unbacked share is no longer withheld. +// --------------------------------------------------------------------------- +namespace { + +// The exact refs_of node.cpp installs on partition_backable: route the variant +// through the btc::new_tx_hashes SSOT. +std::vector production_refs_of(btc::ShareType& share) +{ + std::vector hashes; + share.invoke([&](auto* obj) { + if (const auto* new_txs = btc::new_tx_hashes(obj)) + hashes.assign(new_txs->begin(), new_txs->end()); + }); + return hashes; +} + +} // namespace + +// Accessor SSOT: a REAL v17 share nests its new-tx hashes in m_tx_info; the +// production probe must surface them (the dead flat probe returned nullptr). +TEST(BtcBroadcastGate, RealV17ShareRefsResolvedViaSsot) +{ + btc::Share s; + s.m_tx_info.m_new_transaction_hashes = {TA, TC}; + + const auto* refs = btc::new_tx_hashes(&s); + ASSERT_NE(refs, nullptr); + ASSERT_EQ(refs->size(), 2u); // dead flat probe -> nullptr + EXPECT_EQ((*refs)[0], TA); + EXPECT_EQ((*refs)[1], TC); +} + +// Accessor SSOT: v33 uses the same nested carrier. +TEST(BtcBroadcastGate, RealV33ShareRefsResolvedViaSsot) +{ + btc::NewShare s; + s.m_tx_info.m_new_transaction_hashes = {TC}; + + const auto* refs = btc::new_tx_hashes(&s); + ASSERT_NE(refs, nullptr); + ASSERT_EQ(refs->size(), 1u); + EXPECT_EQ((*refs)[0], TC); +} + +// Accessor SSOT: v34/v35/v36 carry no m_tx_info -> nullptr, compiled out. +TEST(BtcBroadcastGate, SegwitAndMergedVariantsHaveNoRefs) +{ + btc::SegwitMiningShare v34; + btc::PaddingBugfixShare v35; + btc::MergedMiningShare v36; + EXPECT_EQ(btc::new_tx_hashes(&v34), nullptr); + EXPECT_EQ(btc::new_tx_hashes(&v35), nullptr); + EXPECT_EQ(btc::new_tx_hashes(&v36), nullptr); +} + +// The gate end-to-end: REAL share variants + the production refs_of through +// btc::partition_backable. A v17 share referencing a tx whose bytes we do NOT +// hold must be withheld; the backable ones survive, in order. Pre-fix the dead +// probe made every share vacuously backable -> skipped==0, nothing withheld. +TEST(BtcBroadcastGate, RealShareTxInfoRefsWithheld) +{ + std::vector raws; // own the heap shares for cleanup + auto v17 = [&](std::vector refs) { + auto* raw = new btc::Share(); + raw->m_tx_info.m_new_transaction_hashes = std::move(refs); + raws.push_back(raw); + btc::ShareType sv; sv = raw; return sv; + }; + + const std::set held{TA, TB}; // TC bytes NOT held + + std::vector batch; + batch.push_back(v17({TA})); // backable + batch.push_back(v17({TC})); // references unheld TC -> withheld + batch.push_back(v17({TA, TB})); // backable + + const std::size_t skipped = + btc::partition_backable(batch, held, production_refs_of); + + EXPECT_EQ(skipped, 1u); // dead-probe regression -> 0 + ASSERT_EQ(batch.size(), 2u); + EXPECT_EQ(production_refs_of(batch[0]).front(), TA); // order preserved + EXPECT_EQ(production_refs_of(batch[1]).front(), TA); + + for (auto* raw : raws) delete raw; +} diff --git a/src/impl/dgb/node.cpp b/src/impl/dgb/node.cpp index 316354458..c939209ad 100644 --- a/src/impl/dgb/node.cpp +++ b/src/impl/dgb/node.cpp @@ -722,9 +722,11 @@ std::vector NodeImpl::send_shares(peer_ptr peer, shares, m_known_txs, [](ShareType& share) { std::vector hashes; share.invoke([&](auto* obj) { - if constexpr (requires { obj->m_new_transaction_hashes; }) - hashes.assign(obj->m_new_transaction_hashes.begin(), - obj->m_new_transaction_hashes.end()); + // #880: route through the dgb::append_share_tx_refs SSOT + // (#914) instead of the dead flat probe -- the refs live in + // m_tx_info on v17/v33, absent v34+. The old probe was FALSE + // for every version, so this F3 gate never withheld a share. + dgb::append_share_tx_refs(obj, hashes); }); return hashes; }); From 8a39213c7224dbe9e8eff99022f5303c628fd11e Mon Sep 17 00:00:00 2001 From: frstrtr Date: Mon, 27 Jul 2026 20:16:57 +0000 Subject: [PATCH 4/4] btc: mirror ltc broadcast lock-discipline trace; pin send-path reachability Instrument btc::NodeImpl::broadcast_share with the deferred/acquired/ reached-send counters and IsNull/contains guards that ltc already carries, and add broadcast_lock_discipline_test.cpp driving a REAL btc::NodeImpl against a populated sharechain. Proves the F3 send path (tx-completeness gate + send_shares + mark-after-send) is reachable, not dead code behind an always-held lock: reached_send is 0 when the caller holds the tracker mutex EXCLUSIVELY on the calling thread and 1 when it does not -- the shape main_btc create_share_fn guarantees by dropping the exclusive lock (lk.unlock) before calling broadcast_share. Rides the allowlisted btc_share_test target (no new add_executable, #769). --- src/impl/btc/node.cpp | 35 ++++ src/impl/btc/node.hpp | 16 ++ src/impl/btc/test/CMakeLists.txt | 2 +- .../test/broadcast_lock_discipline_test.cpp | 184 ++++++++++++++++++ 4 files changed, 236 insertions(+), 1 deletion(-) create mode 100644 src/impl/btc/test/broadcast_lock_discipline_test.cpp diff --git a/src/impl/btc/node.cpp b/src/impl/btc/node.cpp index ba54148fc..09165cdb3 100644 --- a/src/impl/btc/node.cpp +++ b/src/impl/btc/node.cpp @@ -948,6 +948,9 @@ std::vector NodeImpl::send_shares(peer_ptr peer, const std::vector lock(m_tracker_mutex, std::try_to_lock); if (!lock.owns_lock()) { + const uint64_t deferred = + m_broadcast_deferred.fetch_add(1, std::memory_order_relaxed) + 1; static int defer_log = 0; if (defer_log++ % 50 == 0) LOG_INFO << "[broadcast_share] tracker busy — deferring broadcast of " << share_hash.GetHex().substr(0, 16) << " (next cycle will pick it up)"; + // A long run of deferrals with ZERO successes is not contention — it + // is a caller holding m_tracker_mutex EXCLUSIVELY across this call on + // this thread, which can never grant a shared lock, so the node + // silently broadcasts nothing at all. Say so loudly and once. + if (deferred >= 20 && m_broadcast_acquired.load(std::memory_order_relaxed) == 0) { + static bool warned = false; + if (!warned) { + warned = true; + LOG_WARNING << "[broadcast_share] " << deferred << " consecutive" + << " deferrals with ZERO successful acquisitions — a" + " caller is almost certainly holding the tracker" + " mutex EXCLUSIVELY across this call, which can" + " never grant a shared lock. NO SHARE IS BEING" + " BROADCAST. Drop that lock before calling" + " broadcast_share (main_btc create_share_fn does)."; + } + } return; } + m_broadcast_acquired.fetch_add(1, std::memory_order_relaxed); + + // The share may have been pruned between mint and here; get_height / + // get_chain on an absent hash is not a defined query, so bail rather + // than walk garbage (mirrors the ltc guard). + if (!m_chain || !m_chain->contains(share_hash)) + return; // Walk the chain back from share_hash, collecting un-broadcast shares. // @@ -990,6 +1019,12 @@ void NodeImpl::broadcast_share(const uint256& share_hash) if (to_send.empty()) return; + // The walk produced a non-empty batch and we hold the shared lock: the + // per-peer send loop (F3 gate + send_shares + F2 mark) is now entered. + // This is the count that stays at ZERO if a caller holds the exclusive + // lock across the call. + m_broadcast_reached_send.fetch_add(1, std::memory_order_relaxed); + auto now = std::chrono::steady_clock::now(); broadcast_and_mark(m_shared_share_hashes, m_peers, to_send, [&](peer_ptr& peer) { std::vector sent = send_shares(peer, to_send); diff --git a/src/impl/btc/node.hpp b/src/impl/btc/node.hpp index 8981e771a..37df4294d 100644 --- a/src/impl/btc/node.hpp +++ b/src/impl/btc/node.hpp @@ -398,6 +398,19 @@ class NodeImpl : public pool::SharechainNode()> m_local_miner_scripts_fn; std::string m_node_payout_script_hex; std::set m_shared_share_hashes; // de-dup set for broadcast_share + std::atomic m_broadcast_deferred{0}; + std::atomic m_broadcast_acquired{0}; + std::atomic m_broadcast_reached_send{0}; std::set m_rejected_share_hashes; // shares rejected by peers — never re-broadcast std::set m_downloading_shares; // hashes currently being fetched diff --git a/src/impl/btc/test/CMakeLists.txt b/src/impl/btc/test/CMakeLists.txt index b70819940..0ab6572a6 100644 --- a/src/impl/btc/test/CMakeLists.txt +++ b/src/impl/btc/test/CMakeLists.txt @@ -1,7 +1,7 @@ if (BUILD_TESTING AND GTest_FOUND) # btc twin of ltc share_test — uniquely named to avoid the CMP0002 target # collision with src/impl/ltc/test (both subdirs build in the same tree). - add_executable(btc_share_test share_test.cpp f11_donation_invariance_test.cpp f10b_version_punish_removal_test.cpp g01_share_format_parity_test.cpp auto_ratchet_sim_test.cpp block_broadcast_guard_test.cpp block_broadcast_connect_test.cpp regtest_sharechain_isolation_test.cpp stratum_merkle_branch_test.cpp witness_commitment_merkle_test.cpp finder_fee_integer_exact_test.cpp rpc_conf_test.cpp genesis_check_test.cpp won_share_dualpath_test.cpp gentx_unpack_test.cpp block_assembly_test.cpp gentx_coinbase_test.cpp template_capture_test.cpp template_other_txs_test.cpp reconstruct_test.cpp known_txs_retention_test.cpp won_block_mints_share_test.cpp broadcast_gate_test.cpp) + add_executable(btc_share_test share_test.cpp f11_donation_invariance_test.cpp f10b_version_punish_removal_test.cpp g01_share_format_parity_test.cpp auto_ratchet_sim_test.cpp block_broadcast_guard_test.cpp block_broadcast_connect_test.cpp regtest_sharechain_isolation_test.cpp stratum_merkle_branch_test.cpp witness_commitment_merkle_test.cpp finder_fee_integer_exact_test.cpp rpc_conf_test.cpp genesis_check_test.cpp won_share_dualpath_test.cpp gentx_unpack_test.cpp block_assembly_test.cpp gentx_coinbase_test.cpp template_capture_test.cpp template_other_txs_test.cpp reconstruct_test.cpp known_txs_retention_test.cpp won_block_mints_share_test.cpp broadcast_gate_test.cpp broadcast_lock_discipline_test.cpp) target_link_libraries(btc_share_test PRIVATE GTest::gtest_main GTest::gtest core btc diff --git a/src/impl/btc/test/broadcast_lock_discipline_test.cpp b/src/impl/btc/test/broadcast_lock_discipline_test.cpp new file mode 100644 index 000000000..6a910ec33 --- /dev/null +++ b/src/impl/btc/test/broadcast_lock_discipline_test.cpp @@ -0,0 +1,184 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// +// Wiring regression for btc::NodeImpl::broadcast_share, driven against a REAL +// btc::NodeImpl and a REAL populated sharechain. +// +// The bug this pins is not a decision-function bug and no KAT over the pure +// helpers (partition_backable / broadcast_and_mark, see broadcast_gate_test) +// can see it. broadcast_share opens with +// +// std::shared_lock lock(m_tracker_mutex, std::try_to_lock); +// +// and a std::shared_mutex REFUSES a shared lock to a thread that already holds +// it EXCLUSIVELY. The stratum mining-submit path in main_btc.cpp creates the +// local share while holding exactly that mutex under a unique_lock. If it were +// to call broadcast_share inside that scope, every broadcast of every locally +// minted share would take the "tracker busy — deferring" early return and this +// node would put none of its own shares on the wire — and everything downstream +// of that return (the F3 tx-completeness gate this PR #880 routes through the +// send path, the per-peer send, the mark-only-what-was-sent bookkeeping) would +// be unreachable code. A gate on dead code passes every test while gating +// nothing; this is the failure mode this trace exists to rule out. +// +// The production caller does the right thing: main_btc create_share_fn drops +// the exclusive lock (lk.unlock()) BEFORE calling broadcast_share, so the send +// loop is reachable. These cases hold that down from both sides: called under +// an exclusive lock on the calling thread nothing downstream runs; called +// without one the walk runs and the per-peer send loop is entered. A future +// refactor that moves broadcast_share back inside the lock scope reddens this. +// +// Folded into the EXISTING allowlisted btc_share_test target (a new +// add_executable would be absent from build.yml -- reported "Not Run" by CTest, +// the #769 trap). + +#include + +#include +#include +#include + +#include +#include +#include + +namespace { + +uint256 H(uint64_t n) { return uint256(n); } + +// Minimal concrete NodeImpl: the default btc::NodeImpl ctor takes no io_context, +// opens no LevelDB and starts no timers, so a unit test can own one. The +// io_context ctor is what normally points m_chain at the tracker chain; we do +// that by hand here. +struct TestNode : public btc::NodeImpl +{ + TestNode() : btc::NodeImpl() { m_chain = &m_tracker.chain; } + + // Satisfy core::ICommunicator: this unit test drives no sockets. + void handle(std::unique_ptr, const NetService&) override {} + + using btc::NodeImpl::broadcast_share; + using btc::NodeImpl::m_shared_share_hashes; + using btc::NodeImpl::m_tracker; + + // Append a v36 share (the version this lane mints — carries no new-tx list, + // so the F3 gate is a no-op for it) on top of `prev`. + void add_share(const uint256& hash, const uint256& prev) + { + auto* s = new btc::MergedMiningShare(hash, prev); + m_tracker.chain.add(s); + } +}; + +// A two-share chain: genesis (null parent) then a tip on top of it, so the +// 5-deep walk in broadcast_share has something to collect. +std::unique_ptr make_node_with_chain(uint256& tip_out) +{ + auto node = std::make_unique(); + node->add_share(H(1), uint256::ZERO); + node->add_share(H(2), H(1)); + tip_out = H(2); + return node; +} + +} // namespace + +TEST(BtcBroadcastLockDiscipline, ExclusiveLockOnCallingThreadBlocksTheEntireBroadcast) +{ + // THE defect this trace exists to rule out, reproduced exactly: the caller + // holds the tracker mutex exclusively and calls broadcast_share on the same + // thread. + uint256 tip; + auto node = make_node_with_chain(tip); + + { + std::unique_lock exclusive(node->tracker_mutex()); + ASSERT_TRUE(exclusive.owns_lock()); + + node->broadcast_share(tip); + + EXPECT_EQ(node->broadcast_deferred_count(), 1u); + EXPECT_EQ(node->broadcast_acquired_count(), 0u) + << "a shared_mutex cannot grant a shared lock to a thread already " + "holding it exclusively"; + EXPECT_EQ(node->broadcast_reached_send_count(), 0u) + << "nothing downstream of the try-lock runs — the F3 gate, the " + "per-peer send and the mark bookkeeping are all unreachable"; + } + + EXPECT_TRUE(node->m_shared_share_hashes.empty()); +} + +TEST(BtcBroadcastLockDiscipline, WithoutTheCallersLockTheBroadcastReachesTheSendLoop) +{ + // The main_btc create_share_fn shape: the submit path drops its exclusive + // lock (lk.unlock()) before calling broadcast_share, so by the time we get + // here no exclusive lock is held on this thread. + uint256 tip; + auto node = make_node_with_chain(tip); + + node->broadcast_share(tip); + + EXPECT_EQ(node->broadcast_deferred_count(), 0u); + EXPECT_EQ(node->broadcast_acquired_count(), 1u); + EXPECT_EQ(node->broadcast_reached_send_count(), 1u) + << "the walk produced a batch and the per-peer send loop was entered — " + "this is what stays at zero if the caller holds the exclusive lock"; +} + +TEST(BtcBroadcastLockDiscipline, DeferredThenRetriedSucceeds) +{ + // A deferral must be recoverable: the share is still in our chain and stays + // un-marked, so the next cycle picks it up. This is the property that makes + // the try-lock safe — and that an always-locked inline call site could + // never reach, because its lock would be held on every attempt. + uint256 tip; + auto node = make_node_with_chain(tip); + + { + std::unique_lock exclusive(node->tracker_mutex()); + node->broadcast_share(tip); + } + ASSERT_EQ(node->broadcast_reached_send_count(), 0u); + + node->broadcast_share(tip); + EXPECT_EQ(node->broadcast_deferred_count(), 1u); + EXPECT_EQ(node->broadcast_reached_send_count(), 1u); +} + +TEST(BtcBroadcastLockDiscipline, ZeroPeersMarksNothingOnTheRealNode) +{ + // F2 on the real node: the walk ran and the send loop was entered, but with + // no peers connected nothing reached a socket, so nothing may be marked + // broadcast. Marking at walk time would retire both shares here — the next + // walk breaks on a marked hash, with no retry path. + uint256 tip; + auto node = make_node_with_chain(tip); + + node->broadcast_share(tip); + + ASSERT_EQ(node->broadcast_reached_send_count(), 1u); + EXPECT_TRUE(node->m_shared_share_hashes.empty()) + << "no peer, no byte on the wire, therefore no mark"; + + // ...and because nothing was marked, a later attempt still has work to do. + node->broadcast_share(tip); + EXPECT_EQ(node->broadcast_reached_send_count(), 2u) + << "an unmarked share is retried, not silently retired"; +} + +TEST(BtcBroadcastLockDiscipline, NullAndUnknownShareHashesAreRejectedNotWalked) +{ + // Both guards must short-circuit before the chain walk: a null hash and a + // hash that was never added to our chain must not reach the send loop. + uint256 tip; + auto node = make_node_with_chain(tip); + + node->broadcast_share(uint256::ZERO); + EXPECT_EQ(node->broadcast_reached_send_count(), 0u); + + node->broadcast_share(H(9999)); // never added to the chain + EXPECT_EQ(node->broadcast_reached_send_count(), 0u); + + node->broadcast_share(tip); // still works for a live hash + EXPECT_EQ(node->broadcast_reached_send_count(), 1u); +}