Skip to content

Commit c60fcaf

Browse files
frstrtrfrstrtr
committed
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.
1 parent e56fe92 commit c60fcaf

17 files changed

Lines changed: 1469 additions & 45 deletions

‎src/impl/bch/known_txs_retention.hpp‎

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
// SPDX-License-Identifier: AGPL-3.0-or-later
2+
//
3+
// Pure, unit-testable bookkeeping for the known-transaction pool that backs
4+
// share broadcast (remember_tx forwarding). Ported to the BCH variant from
5+
// src/impl/dash/known_txs_retention.hpp so the retention/eviction and
6+
// broadcast-gate semantics can be exercised without a live node.
7+
//
8+
// Two concerns:
9+
// retain_template_txs() — the rolling-window retention with template-identity
10+
// dedup (code review F1): register_template_txs is invoked per producer-job
11+
// cache MISS, i.e. per (prev_share_hash, payout_script), NOT per template
12+
// rotation. A ~50-payout-script cluster therefore re-registers the SAME
13+
// template ~50 times on one tip; pushing each unconditionally collapses the
14+
// N-slot window to a single template within seconds. Dedup by the template's
15+
// tx-hash SET (its identity): a set already retained refreshes recency and
16+
// consumes NO new slot, so N re-registrations of one template use ONE slot
17+
// and the window holds N DISTINCT templates as intended.
18+
//
19+
// all_txs_backable() — the canonical broadcast gate (F3): a share may be sent
20+
// only when we HOLD the bytes of every referenced new-tx (so remember_tx can
21+
// always forward them). This is what makes the "referenced unknown
22+
// transaction" disconnect unreachable by construction, and — paired with the
23+
// mint-time decline of shares whose txs are not retained — makes "every share
24+
// we mint/broadcast is backable" a by-construction invariant.
25+
//
26+
// partition_backable() — the batch form of the gate, applied by send_shares()
27+
// to the shares it is about to write to one peer.
28+
//
29+
// broadcast_and_mark() — the F2 marking policy: broadcast_share() may mark a
30+
// share "already shared" only AFTER a peer actually accepted it. Marking the
31+
// whole chain-walk up front (the pre-fix pattern) loses every share in a
32+
// batch that send_shares then abandons — the gate skipped it, the tracker
33+
// try_to_lock missed, or there were no peers — because the de-dup set then
34+
// breaks the walk forever and nothing re-pushes it.
35+
//
36+
// NOTE (de-dup): the same primitives now exist per-coin (btc/dgb/bch/dash) while
37+
// the LTC lane hoists all_txs_backable into src/core/. Once both land, these
38+
// per-coin headers should be collapsed onto that core hoist.
39+
40+
#pragma once
41+
42+
#include <cstddef>
43+
#include <deque>
44+
#include <set>
45+
#include <vector>
46+
47+
#include <core/uint256.hpp>
48+
49+
namespace bch {
50+
51+
// Retain the tx set of a newly-registered template in a rolling window of the
52+
// last `cap` DISTINCT templates, inserting its tx bytes into `known_txs`.
53+
//
54+
// recent_sets : rolling history of distinct template tx-hash sets
55+
// (front = oldest, back = newest).
56+
// known_txs : uint256 -> tx map; needs insert_or_assign(k,v) and erase(k).
57+
// hashes/txs : parallel arrays of the template's tx hashes and bytes.
58+
// cap : number of DISTINCT templates to retain.
59+
//
60+
// A tx is erased from `known_txs` only once it has fallen out of EVERY retained
61+
// set. F1 dedup: if this template's tx set is already retained, its recency is
62+
// refreshed (moved to back) and no new slot is consumed.
63+
template <typename TxMap, typename Tx>
64+
inline void retain_template_txs(std::deque<std::set<uint256>>& recent_sets,
65+
TxMap& known_txs,
66+
const std::vector<uint256>& hashes,
67+
const std::vector<Tx>& txs,
68+
std::size_t cap)
69+
{
70+
if (cap == 0)
71+
return;
72+
73+
std::set<uint256> new_set(hashes.begin(), hashes.end());
74+
75+
// F1 dedup by template identity (the tx-hash set). If this template is
76+
// already retained, refresh its recency (move to back) — no new slot, no
77+
// re-eviction. This is what stops ~50 payout-script re-registrations of one
78+
// template from evicting the rest of the window.
79+
for (auto it = recent_sets.begin(); it != recent_sets.end(); ++it) {
80+
if (*it == new_set) {
81+
if (std::next(it) != recent_sets.end()) {
82+
std::set<uint256> refreshed = std::move(*it);
83+
recent_sets.erase(it);
84+
recent_sets.push_back(std::move(refreshed));
85+
}
86+
return;
87+
}
88+
}
89+
90+
// Distinct template: insert its tx bytes and consume a slot.
91+
const std::size_t n = std::min(hashes.size(), txs.size());
92+
for (std::size_t i = 0; i < n; ++i)
93+
known_txs.insert_or_assign(hashes[i], txs[i]);
94+
recent_sets.push_back(std::move(new_set));
95+
96+
// Evict the oldest slots beyond `cap`; a tx is removed from `known_txs` only
97+
// once it is absent from EVERY remaining retained set.
98+
while (recent_sets.size() > cap) {
99+
const std::set<uint256> evicted = std::move(recent_sets.front());
100+
recent_sets.pop_front();
101+
for (const auto& h : evicted) {
102+
bool still_retained = false;
103+
for (const auto& s : recent_sets) {
104+
if (s.count(h)) { still_retained = true; break; }
105+
}
106+
if (!still_retained)
107+
known_txs.erase(h);
108+
}
109+
}
110+
}
111+
112+
// True iff every hash in `tx_hashes` is present in `held` (the bytes we hold,
113+
// e.g. m_known_txs). The canonical broadcast gate: a share is backable — safe to
114+
// send without risking the peer's "referenced unknown transaction" disconnect —
115+
// only when we can forward every referenced tx.
116+
template <typename Held>
117+
inline bool all_txs_backable(const std::vector<uint256>& tx_hashes,
118+
const Held& held)
119+
{
120+
for (const auto& h : tx_hashes)
121+
if (held.find(h) == held.end())
122+
return false;
123+
return true;
124+
}
125+
126+
// Batch form of the F3 gate. Keeps in `shares` only the entries every one of
127+
// whose referenced new-tx hashes we hold, and returns how many were dropped.
128+
// `hashes_of(share)` yields that share's referenced new-tx hashes.
129+
//
130+
// Skipping costs no propagation: a share we downloaded is by definition already
131+
// on the network we fetched it from, and it will be re-offered on a later
132+
// broadcast once its tx bytes arrive (F2 leaves it un-marked).
133+
template <typename Share, typename Held, typename HashesOf>
134+
inline std::size_t partition_backable(std::vector<Share>& shares,
135+
const Held& held,
136+
HashesOf hashes_of)
137+
{
138+
std::vector<Share> sendable;
139+
sendable.reserve(shares.size());
140+
std::size_t skipped = 0;
141+
for (auto& share : shares) {
142+
if (all_txs_backable(hashes_of(share), held))
143+
sendable.push_back(std::move(share));
144+
else
145+
++skipped;
146+
}
147+
shares.swap(sendable);
148+
return skipped;
149+
}
150+
151+
// F2 marking policy. Offers the batch to every peer via `send(peer)` — which
152+
// returns the hashes it ACTUALLY wrote to that peer — and marks in `marked`
153+
// only the hashes that reached at least one peer. Returns that count.
154+
//
155+
// The pre-fix code marked the whole chain-walk before calling send_shares, so a
156+
// batch abandoned by every peer (gate skip, try_to_lock miss, empty peer set)
157+
// stayed marked forever: the next broadcast_share walk breaks on the first
158+
// marked hash and the share is never re-pushed. That is silent, permanent share
159+
// loss with no retry path — a PPLNS-credit loss for whoever mined it.
160+
template <typename MarkedSet, typename Peers, typename SendFn>
161+
inline std::size_t broadcast_and_mark(MarkedSet& marked, Peers& peers,
162+
const std::vector<uint256>& to_send,
163+
SendFn send)
164+
{
165+
if (to_send.empty())
166+
return 0;
167+
168+
// NOTE: `to_send` is deliberately NOT what gets marked. Marking the offered
169+
// batch here is precisely the pre-fix bug this signature exists to make
170+
// un-writable by accident — only `send`'s report may be marked.
171+
std::set<uint256> actually_sent;
172+
for (auto& entry : peers) {
173+
const std::vector<uint256> sent = send(entry.second);
174+
actually_sent.insert(sent.begin(), sent.end());
175+
}
176+
for (const auto& h : actually_sent)
177+
marked.insert(h);
178+
return actually_sent.size();
179+
}
180+
181+
} // namespace bch

‎src/impl/bch/node.cpp‎

Lines changed: 88 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// SPDX-License-Identifier: AGPL-3.0-or-later
22
#include "node.hpp"
3+
#include "known_txs_retention.hpp" // bch::all_txs_backable / partition_backable / broadcast_and_mark (F2/F3)
34

45
#include <core/common.hpp>
56
#include <core/hash.hpp>
@@ -682,8 +683,13 @@ std::vector<bch::ShareType> NodeImpl::handle_get_share(std::vector<uint256> hash
682683
return shares;
683684
}
684685

685-
void NodeImpl::send_shares(peer_ptr peer, const std::vector<uint256>& share_hashes)
686+
std::vector<uint256> NodeImpl::send_shares(peer_ptr peer,
687+
const std::vector<uint256>& share_hashes)
686688
{
689+
// Returns the hashes of the shares actually WRITTEN to this peer (F2). The
690+
// caller marks only those as broadcast, so a share skipped here — by the
691+
// try_to_lock miss below or by the F3 tx-completeness gate — is re-offered
692+
// on the next broadcast instead of being silently dropped forever.
687693
// try_to_lock per the architectural rule (node.hpp:67) — see freeze
688694
// analysis in handle_get_share above. If we can't acquire NOW, skip
689695
// this batch. The shares are still in our chain; the next broadcast
@@ -695,7 +701,7 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector<uint256>& share_hash
695701
if (defer_log++ % 50 == 0)
696702
LOG_INFO << "[send_shares] tracker busy — skipping send to "
697703
<< peer->addr().to_string() << " (will retry next cycle)";
698-
return;
704+
return {};
699705
}
700706

701707
// Collect shares that exist in our chain (skip rejected)
@@ -715,7 +721,48 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector<uint256>& share_hash
715721
}
716722

717723
if (shares.empty())
718-
return;
724+
return {};
725+
726+
// --- F3 tx-completeness broadcast gate --------------------------------
727+
// A share whose referenced new-tx BYTES we do not hold cannot be backed by
728+
// the remember_tx forward below, and canonical p2pool drops the connection
729+
// on "referenced unknown transaction" (p2p.py) — which isolates us from the
730+
// sharechain and orphans our shares. Shares pulled via sharereply arrive
731+
// WITHOUT their tx bytes, so this is reachable in normal operation.
732+
//
733+
// Gate on the BYTES in m_known_txs, not on the peer's have_tx advert: that
734+
// advert can be stale (the peer may have dropped the tx) and sending
735+
// hash-only for a tx the peer no longer holds is exactly the disconnect.
736+
// m_remote_txs is used ONLY below to choose the hash-vs-bytes encoding.
737+
//
738+
// CONSENSUS-NEUTRAL: this decides only WHETHER we broadcast. Share bytes,
739+
// minting and payout are untouched; a skipped share stays in our chain and
740+
// is re-offered once its txs arrive (send_shares reports it as unsent, so
741+
// broadcast_share leaves it un-marked).
742+
{
743+
const size_t skipped_incomplete = partition_backable(
744+
shares, m_known_txs, [](ShareType& share) {
745+
std::vector<uint256> hashes;
746+
share.invoke([&](auto* obj) {
747+
if constexpr (requires { obj->m_new_transaction_hashes; })
748+
hashes.assign(obj->m_new_transaction_hashes.begin(),
749+
obj->m_new_transaction_hashes.end());
750+
});
751+
return hashes;
752+
});
753+
if (skipped_incomplete > 0)
754+
{
755+
static int skip_log = 0;
756+
if (skip_log++ % 20 == 0)
757+
LOG_INFO << "[send_shares] skipped " << skipped_incomplete
758+
<< " share(s) whose new-tx bytes we do not hold "
759+
"(downloaded via sharereply / evicted) to "
760+
<< peer->addr().to_string()
761+
<< " — avoids canonical 'unknown transaction' disconnect";
762+
}
763+
if (shares.empty())
764+
return {};
765+
}
719766

720767
// Collect transactions that the peer doesn't know about
721768
std::set<uint256> needed_txs;
@@ -740,6 +787,8 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector<uint256>& share_hash
740787
std::vector<uint256> known_hashes; // hashes in peer's remote set
741788
std::vector<coin::MutableTransaction> full_txs; // full txs otherwise
742789

790+
size_t missing = 0;
791+
743792
for (const auto& th : needed_txs)
744793
{
745794
if (peer->m_remote_txs.count(th))
@@ -751,9 +800,20 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector<uint256>& share_hash
751800
auto it = m_known_txs.find(th);
752801
if (it != m_known_txs.end())
753802
full_txs.emplace_back(it->second);
803+
else
804+
++missing; // unreachable after the gate above — see below
754805
}
755806
}
756807

808+
// Defence in depth: the gate should already have removed every share
809+
// with an unheld tx, so this must stay at zero. If it ever fires, the
810+
// gate's view of the referenced hashes disagrees with needed_txs and
811+
// the peer may drop these shares — make that loud rather than silent.
812+
if (missing > 0)
813+
LOG_WARNING << "[send_shares] " << missing << " referenced tx(s) not in "
814+
"m_known_txs after the completeness gate — peer may drop "
815+
"these shares (share/tx hash accounting mismatch?)";
816+
757817
if (!known_hashes.empty() || !full_txs.empty())
758818
{
759819
auto rtx_msg = message_remember_tx::make_raw(known_hashes, full_txs);
@@ -790,6 +850,13 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector<uint256>& share_hash
790850

791851
LOG_INFO << "[Pool] Sent " << shares.size() << " shares (+" << needed_txs.size()
792852
<< " txs) to " << peer->addr().to_string();
853+
854+
// F2: report exactly which shares reached this peer.
855+
std::vector<uint256> sent;
856+
sent.reserve(shares.size());
857+
for (auto& share : shares)
858+
sent.push_back(share.hash());
859+
return sent;
793860
}
794861

795862
void NodeImpl::broadcast_share(const uint256& share_hash)
@@ -812,7 +879,14 @@ void NodeImpl::broadcast_share(const uint256& share_hash)
812879
return;
813880
}
814881

815-
// Walk the chain back from share_hash, collecting un-broadcast shares
882+
// Walk the chain back from share_hash, collecting un-broadcast shares.
883+
//
884+
// F2: do NOT mark them shared here. send_shares can abandon the whole batch
885+
// — the tx-completeness gate skipped it, its tracker try_to_lock missed, or
886+
// there are zero peers — and a share marked before that never gets another
887+
// chance: the next walk breaks on the first marked hash, so the tip is
888+
// permanently withheld from the sharechain and its PPLNS credit is lost.
889+
// We mark only what actually reached >= 1 peer, below.
816890
std::vector<uint256> to_send;
817891
int32_t height = m_chain->get_height(share_hash);
818892
int32_t walk = std::min(height, 5);
@@ -823,18 +897,23 @@ void NodeImpl::broadcast_share(const uint256& share_hash)
823897
break;
824898
if (m_rejected_share_hashes.count(hash))
825899
continue; // skip shares previously rejected by peers
826-
m_shared_share_hashes.insert(hash);
827900
to_send.push_back(hash);
828901
}
829902

830903
if (to_send.empty())
831904
return;
832905

833906
auto now = std::chrono::steady_clock::now();
834-
for (auto& [nonce, peer] : m_peers) {
835-
send_shares(peer, to_send);
836-
m_last_broadcast_to[peer->addr()] = {to_send, now};
837-
}
907+
broadcast_and_mark(m_shared_share_hashes, m_peers, to_send, [&](peer_ptr& peer) {
908+
std::vector<uint256> sent = send_shares(peer, to_send);
909+
// Record what we ACTUALLY wrote, not what we offered: a peer that drops
910+
// within 10s marks this record's hashes rejected, and a rejected hash is
911+
// never re-broadcast. Blaming a share we withheld would turn a transient
912+
// gate skip into permanent loss.
913+
if (!sent.empty())
914+
m_last_broadcast_to[peer->addr()] = {sent, now};
915+
return sent;
916+
});
838917
}
839918

840919
void NodeImpl::notify_local_share(const uint256& share_hash)

‎src/impl/bch/node.hpp‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -605,9 +605,13 @@ class NodeImpl : public pool::SharechainNode<bch::Config, bch::ShareChain, bch::
605605
/// Set the software version string announced to peers in P2P version messages.
606606
void set_software_version(std::string ver) { m_software_version = std::move(ver); }
607607

608-
/// Send a set of shares (with any needed txs) to a single peer.
609-
/// Skips shares that originated from that peer.
610-
void send_shares(peer_ptr peer, const std::vector<uint256>& share_hashes);
608+
/// Send a set of shares (with any needed txs) to a single peer, and return
609+
/// the subset actually written (F2). Shares dropped by the tx-completeness
610+
/// gate, or the whole batch when the tracker lock is unavailable, are
611+
/// reported as NOT sent, so the caller leaves them un-marked and retries
612+
/// them on the next broadcast.
613+
std::vector<uint256> send_shares(peer_ptr peer,
614+
const std::vector<uint256>& share_hashes);
611615

612616
/// Broadcast a locally-generated (or newly-received) share to all peers.
613617
void broadcast_share(const uint256& share_hash);

‎src/impl/bch/test/CMakeLists.txt‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,12 @@ if(BUILD_TESTING)
213213
# Needs coin/transaction.cpp for the MutableTransaction ctors, same as
214214
# block_connector_test; link set and header-only-over-coin/*.hpp posture are
215215
# otherwise identical, so per-coin isolation stays clean.
216+
# F2/F3 share-broadcast gate KATs ride bch_embedded_block_broadcast_test (an
217+
# already-allowlisted --target). The bch test tree has no GTest harness and a
218+
# standalone target would need a build.yml entry this repo's CI token cannot
219+
# add, which CTest would then report as a silently-passing NOT_BUILT sentinel.
220+
target_sources(bch_embedded_block_broadcast_test PRIVATE broadcast_gate_test.cpp)
221+
216222
add_executable(bch_g2_block_assembly_roundtrip_test
217223
g2_block_assembly_roundtrip_test.cpp
218224
${CMAKE_SOURCE_DIR}/src/impl/bch/coin/transaction.cpp)

0 commit comments

Comments
 (0)