dash(node): tx-forwarding known-tx retention — fix p2pool-dash join convergence (#834)

Fix the DASH sharechain join-convergence bug: c2pool was disconnected by canonical p2pool-dash for 'referenced unknown transaction' — register_template_txs evicted the previous template's txs wholesale every rotation, so a stale-job re-announced share could no longer forward its referenced txs. Now retains a rolling window of distinct template tx-sets (template-identity dedup so ~50 payout scripts consume one slot), declines a mint whose template txs left the window (by-construction: every minted share is fully forwardable; a won BLOCK is never affected — mutually-exclusive branch), sends remember_tx bytes before shares, marks shares broadcast only after reaching a peer. DASH-only, consensus/reward-neutral. Fable-approved (both blockers refuted); 6/6 retention KATs.

Author: frstrtr

src/impl/dash/known_txs_retention.hpp

@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+//
+// Pure, unit-testable bookkeeping for the known-transaction pool that backs
+// share broadcast (remember_tx forwarding). Factored out of NodeImpl so the
+// retention/eviction 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.
+
+#pragma once
+
+#include <cstddef>
+#include <deque>
+#include <set>
+#include <vector>
+
+#include <core/uint256.hpp>
+
+namespace dash {
+
+// 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 <typename TxMap, typename Tx>
+inline void retain_template_txs(std::deque<std::set<uint256>>& recent_sets,
+                                TxMap& known_txs,
+                                const std::vector<uint256>& hashes,
+                                const std::vector<Tx>& txs,
+                                std::size_t cap)
+{
+    if (cap == 0)
+        return;
+
+    std::set<uint256> 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<uint256> 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<uint256> 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 <typename Held>
+inline bool all_txs_backable(const std::vector<uint256>& tx_hashes,
+                             const Held& held)
+{
+    for (const auto& h : tx_hashes)
+        if (held.find(h) == held.end())
+            return false;
+    return true;
+}
+
+} // namespace dash

src/impl/dash/node.cpp

@@ -2,6 +2,7 @@
 #include "node.hpp"
 #include "share.hpp"
 #include "mint_runloop.hpp"   // dash::mint::elect_best_share (election policy SSOT)
+#include "known_txs_retention.hpp"  // dash::retain_template_txs / all_txs_backable (F1/F3)
 
 #include <core/uint256.hpp>
 #include <core/common.hpp>
@@ -1140,27 +1141,43 @@ void NodeImpl::broadcast_share(const uint256& share_hash)
     if (!m_chain->contains(share_hash))
         return;
 
-    // Walk back up to 5 shares, collecting the not-yet-broadcast ones.
+    // Walk back up to 5 shares, collecting the not-yet-broadcast ones. code
+    // review F2: do NOT mark them shared here — the tx-completeness gate in
+    // send_shares can skip a share for ALL peers (unbacked now, or zero peers),
+    // and marking before the send would leave a skipped chain TIP permanently
+    // "already shared" and never re-pushed → silent PPLNS-credit loss. We mark
+    // only what actually reached >= 1 peer, below.
     std::vector<uint256> to_send;
     int32_t height = m_chain->get_height(share_hash);
     int32_t walk = std::min(height, 5);
     for (auto [hash, data] : m_chain->get_chain(share_hash, walk)) {
         if (m_shared_share_hashes.count(hash))
             break;
-        m_shared_share_hashes.insert(hash);
         to_send.push_back(hash);
     }
     if (to_send.empty())
         return;
 
-    for (auto& [nonce, peer] : m_peers)
-        send_shares(peer, to_send);
+    std::set<uint256> actually_sent;
+    for (auto& [nonce, peer] : m_peers) {
+        auto sent = send_shares(peer, to_send);
+        actually_sent.insert(sent.begin(), sent.end());
+    }
+    // Mark ONLY shares that reached a peer; anything skipped for every peer (or
+    // with no peers connected) stays un-marked and is retried on the next
+    // broadcast — once its txs are retained or a peer appears.
+    for (const auto& h : actually_sent)
+        m_shared_share_hashes.insert(h);
 }
 
-void NodeImpl::send_shares(peer_ptr peer, const std::vector<uint256>& share_hashes)
+std::vector<uint256> NodeImpl::send_shares(peer_ptr peer,
+                                           const std::vector<uint256>& share_hashes)
 {
     // Caller (broadcast_share) already holds a shared tracker lock; this
     // method only READS chain + m_known_txs and writes to the peer socket.
+    // Returns the hashes of the shares actually SENT to this peer (F2): the
+    // caller marks only those as broadcast, so a share skipped for all peers is
+    // retried rather than silently withheld.
     std::vector<ShareType> shares;
     for (const auto& hash : share_hashes) {
         if (!m_chain->contains(hash))
@@ -1171,7 +1188,58 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector<uint256>& share_hash
         }
     }
     if (shares.empty())
-        return;
+        return {};
+
+    // ── Tx-completeness gate (canonical p2pool p2p.py:404 parity) ───────────
+    // A v13+ share references its new transactions by hash
+    // (new_transaction_hashes); a canonical peer that cannot resolve those txs
+    // DISCONNECTS us with "referenced unknown transaction" (p2p.py:403-404,
+    // handle_shares / handle_remember_tx). We re-announce the best chain's head
+    // and up to 5 ancestors (ROOT-2 re-announce, broadcast_share), and shares
+    // pulled via sharereply arrive WITHOUT their txs (the reply carries no tx
+    // bytes), so their tx bytes are absent from m_known_txs. Sending such a
+    // share unbacked is exactly what makes the public canonical seeds drop the
+    // connection ~300ms after handshake — the isolation root cause.
+    //
+    // So only broadcast a share when we HOLD the bytes of EVERY referenced new
+    // tx (m_known_txs), so remember_tx below can always forward them. code
+    // review F3: gating on m_remote_txs (the peer's have_tx advert) alone is
+    // weaker than canonical — 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. Requiring the bytes is the canonical invariant
+    // (sendShares broadcasts only txs in known_txs); m_remote_txs is used ONLY
+    // below to choose hash-vs-bytes encoding. Skipping the rest costs no
+    // propagation — a downloaded share is by definition already on the network we
+    // fetched it from, and our own minted shares are backable by construction
+    // (add_local_share declines a solve whose txs are not retained).
+    {
+        std::vector<ShareType> sendable;
+        sendable.reserve(shares.size());
+        size_t skipped_incomplete = 0;
+        for (auto& share : shares) {
+            bool backable = true;
+            share.invoke([&](auto* obj) {
+                backable = dash::all_txs_backable(obj->m_new_transaction_hashes,
+                                                  m_known_txs);
+            });
+            if (backable)
+                sendable.push_back(std::move(share));
+            else
+                ++skipped_incomplete;
+        }
+        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 / template-evicted) to "
+                         << peer->addr().to_string()
+                         << " — avoids canonical 'unknown transaction' disconnect";
+        }
+        if (sendable.empty())
+            return {};
+        shares.swap(sendable);
+    }
 
     // Forward the txs the peer needs BEFORE the shares (p2pool remember_tx
     // protocol) — a share referencing unknown txs makes the receiving oracle
@@ -1225,6 +1293,14 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector<uint256>& share_hash
 
     LOG_INFO << "[Pool] Sent " << shares.size() << " share(s) (+"
              << needed_txs.size() << " tx refs) to " << peer->addr().to_string();
+
+    // F2: report exactly which shares reached this peer so the caller marks only
+    // those broadcast (a share skipped for all peers stays un-marked → retried).
+    std::vector<uint256> sent;
+    sent.reserve(shares.size());
+    for (auto& share : shares)
+        sent.push_back(share.hash());
+    return sent;
 }
 
 uint256 NodeImpl::add_local_share(ShareType share)
@@ -1248,6 +1324,31 @@ uint256 NodeImpl::add_local_share(ShareType share)
             return uint256::ZERO;
         }
 
+        // F1-sub (by-construction backability): DECLINE a solve whose template
+        // txs are no longer retained in m_known_txs, rather than mint a share
+        // send_shares would then have to withhold. FrozenJobRegistry
+        // (mint_runloop.hpp) is a FIFO cap-512 with NO TTL and re-put refreshes,
+        // so a solve can arrive from an arbitrarily-old job — no finite retention
+        // window is a provable bound, so the only by-construction guarantee is to
+        // fail closed here: every share we add is fully forwardable. The miner
+        // keeps the pseudoshare credit; the next solve on a fresh job mints. The
+        // deduped rolling template window (register_template_txs) keeps declines
+        // rare — this fires only for genuinely stale solves past the window.
+        {
+            bool backable = true;
+            share.invoke([&](auto* obj) {
+                backable = dash::all_txs_backable(obj->m_new_transaction_hashes,
+                                                  m_known_txs);
+            });
+            if (!backable) {
+                LOG_WARNING << "[MINT] local share " << hash.GetHex().substr(0, 16)
+                            << " references template tx(s) no longer retained — "
+                               "declined (stale job past retention window; retry "
+                               "on next solve)";
+                return uint256::ZERO;
+            }
+        }
+
         m_tracker.add(share);
         // Local share: verify inline (p2pool: set_best_share → think verifies;
         // the inline attempt keeps the just-mint tip usable immediately).
@@ -1274,17 +1375,22 @@ void NodeImpl::register_template_txs(const std::vector<coin::Transaction>& txs,
                     << " hashes=" << hashes.size() << " — skipped";
         return;
     }
-    // Drop the PREVIOUS template's leftovers that the new template no longer
-    // carries, then insert the new set — m_known_txs stays bounded by one
-    // template (plus reception-protocol remembered txs, which are peer-scoped).
-    std::set<uint256> new_set(hashes.begin(), hashes.end());
-    for (const auto& old_hash : m_template_tx_hashes) {
-        if (!new_set.count(old_hash))
-            m_known_txs.erase(old_hash);
-    }
-    for (size_t i = 0; i < txs.size(); ++i)
-        m_known_txs.insert_or_assign(hashes[i], txs[i]);
-    m_template_tx_hashes = std::move(new_set);
+    // Retain a rolling window of the last RETAINED_TEMPLATE_TX_SETS DISTINCT
+    // templates; a tx leaves m_known_txs only once it has fallen out of EVERY
+    // retained set — so a share minted on any recent job (the miner may solve a
+    // job whose template has since rotated) and a ROOT-2 re-announce a few
+    // rotations later can still forward the referenced tx bytes via remember_tx.
+    //
+    // code review F1: register_template_txs is called per producer-job cache
+    // MISS = per (prev_share_hash, payout_script) (main_dash.cpp:1539, key
+    // :1450), NOT per template rotation. A ~50-payout-script cluster re-registers
+    // the SAME template ~50 times on one tip, so pushing each unconditionally
+    // would collapse the window to a single template. retain_template_txs()
+    // dedups by TEMPLATE IDENTITY (the tx-hash set): a set already retained just
+    // refreshes recency and consumes no slot, so the window holds N DISTINCT
+    // templates (≈ minutes of stale-solve coverage), not N re-registrations.
+    dash::retain_template_txs(m_recent_template_tx_sets, m_known_txs,
+                              hashes, txs, RETAINED_TEMPLATE_TX_SETS);
 }
 
 } // namespace dash

src/impl/dash/node.hpp

@@ -42,6 +42,7 @@
 #include <atomic>
 #include <chrono>
 #include <functional>
+#include <deque>
 #include <map>
 #include <memory>
 #include <mutex>
@@ -242,10 +243,18 @@ class NodeImpl : public pool::BaseNode<dash::Config, dash::ShareChain, dash::Pee
 
     // De-dup set for broadcast_share (hashes already relayed to peers).
     std::set<uint256> m_shared_share_hashes;
-    // Hashes of the CURRENT template's txs registered in m_known_txs by
-    // register_template_txs — replaced wholesale each registration so the
-    // known-tx pool stays bounded by one template.
-    std::set<uint256> m_template_tx_hashes;
+    // Rolling history of recent templates' tx-hash sets (register_template_txs).
+    // A locally minted share references its job's template txs; the miner may
+    // solve a job whose template has since rotated (especially at high hashrate),
+    // and ROOT-2 re-announce re-broadcasts a share after several rotations. If we
+    // evict a tx the moment its template rotates (the old single-template bound),
+    // send_shares can no longer forward its bytes via remember_tx, and the
+    // canonical peer drops us with "referenced unknown transaction" (p2p.py:404).
+    // Keep the last RETAINED_TEMPLATE_TX_SETS template tx-sets so a share minted
+    // on any recent job stays fully backable; a tx leaves m_known_txs only once it
+    // has fallen out of EVERY retained set (peer-remembered txs are separate).
+    static constexpr size_t RETAINED_TEMPLATE_TX_SETS = 8;
+    std::deque<std::set<uint256>> m_recent_template_tx_sets;
     // Fired on the IO thread when run_think() elects a new best share
     // (p2pool: new_work_event) — main_dash binds the stratum work refresh.
     std::function<void()> m_on_best_share_changed;
@@ -760,7 +769,10 @@ class NodeImpl : public pool::BaseNode<dash::Config, dash::ShareChain, dash::Pee
     /// the message_shares wire codec (+ remember_tx/forget_tx for txs the
     /// peer lacks). try_to_lock; deferred if the compute thread is busy.
     void broadcast_share(const uint256& share_hash);
-    void send_shares(peer_ptr peer, const std::vector<uint256>& share_hashes);
+    /// Returns the hashes actually SENT to `peer` (those passing the tx-
+    /// completeness gate); broadcast_share marks only these as shared (F2).
+    std::vector<uint256> send_shares(peer_ptr peer,
+                                     const std::vector<uint256>& share_hashes);
 
     /// Insert a locally-minted, ALREADY self-verified share into the tracker
     /// (+ LevelDB persist + attempt_verify), then broadcast + run_think.

test/CMakeLists.txt

@@ -402,7 +402,8 @@ if (BUILD_TESTING AND GTest_FOUND)
         test_dash_share_producer_bind.cpp
         test_dash_mint_runloop.cpp
         test_dash_share_download.cpp
-        test_dash_v36_share.cpp)
+        test_dash_v36_share.cpp
+        test_dash_known_txs_retention.cpp)
     target_link_libraries(test_dash_share_hash_link PRIVATE
         GTest::gtest_main GTest::gtest
         dash_x11 core
@@ -412,6 +413,17 @@ if (BUILD_TESTING AND GTest_FOUND)
     target_link_libraries(test_dash_share_hash_link PRIVATE c2pool_payout c2pool_hashrate c2pool_merged_mining)  # OBJECT-lib SCC direct-naming (#22/#39): core stratum/web_server TUs
     gtest_add_tests(test_dash_share_hash_link "" AUTO)
 
+    # test_dash_known_txs_retention.cpp: pure retention/eviction semantics of the
+    # known-tx pool that backs share broadcast (register_template_txs) — rolling
+    # window, evict-only-after-a-tx-falls-out-of-ALL-retained-sets, the F1
+    # template-identity dedup (N payout scripts on one template => ONE slot, not
+    # N), recency refresh, and the F3/F2 broadcast gate. Header-only over
+    # impl/dash/known_txs_retention.hpp + core (uint256). FOLDED into the
+    # EXISTING allowlisted target test_dash_share_hash_link above (added to its
+    # sources) — avoids the #769 new-target/allowlist trap (a standalone target
+    # is not in build.yml's --target list, so CI never builds it and CTest
+    # reports the tests "Not Run"). Its gtest cases run under that target.
+
     # DASH block-subsidy + UTXO adapter (S7 foundation slice): GetBlockSubsidy /
     # masternode + platform reward split (subsidy.hpp) consumed through the
     # header-only utxo_adapter view shim. Both are header-only over