btc/dgb/bch: gate share broadcast on tx completeness + mark only what was sent

[frstrtr]

What

Wires the F3 tx-completeness broadcast gate into the actual send path and fixes the F2 mark-before-send share loss, for BTC, DGB and BCH. Both fixes are ports of the already-correct DASH implementation (src/impl/dash/node.cpp). Builds on #868 (c77dc03), which landed the BTC primitives header.

Reachability, up front. BTC and BCH are protected at runtime by this PR. DGB is only partly protected — its broadcast_share is dead code today (#884). See the Reachability section before reading the rest.

FIX A — F3 tx-completeness broadcast gate

send_shares() built its remember_tx forward with

auto it = m_known_txs.find(th);
if (it != m_known_txs.end())
    full_txs.emplace_back(it->second);

with no else branch: a referenced tx whose bytes we do not hold was silently omitted and the share was written to the peer regardless. Canonical p2pool then drops the connection on referenced unknown transaction (p2p.py), which isolates us from the sharechain and orphans our shares.

This is reachable in normal operation: shares pulled via sharereply arrive without their tx bytes.

send_shares() now filters its outgoing batch through partition_backable() before building needed_txs. The gate keys 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 still used only to choose the hash-vs-bytes encoding. The now-unreachable omission branch increments a missing counter and logs a warning (defence in depth, mirroring DASH).

Live on all three coins.

FIX B — mark / record only what was actually sent

send_shares() returned void, so callers could not tell what it wrote. It now returns the hashes actually written. Two callers consume that:

broadcast_share() (BTC, BCH live; DGB pre-wiring) inserted each walked hash into m_shared_share_hashes inside the chain-walk loop, before send_shares ran. send_shares has abandon paths — tracker try_to_lock miss, empty batch, and now the F3 gate — and a zero-peer m_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. It now marks only accepted hashes, after the peer loop, via broadcast_and_mark().

readvertise_best_share() (DGB only — DGB's live path) recorded the offered batch into m_last_broadcast_to. That record is what a peer disconnect within 10 s converts into m_rejected_share_hashes, and the readvertise walk continues past rejected hashes permanently. Recording the offered batch lets a share the F3 gate merely withheld be blamed for someone else's disconnect, turning a transient, self-healing skip into permanent exclusion from every future re-advert. Since DGB re-advertises peer-received shares, that is a propagation loss for the whole sharechain, not just for us. Now extracted into readvertise_and_record(): records what was written, and records nothing at all for a peer that received nothing. BTC/BCH m_last_broadcast_to gets the same treatment inside broadcast_and_mark.

Reachability (caller-side trace — verified independently)

coin	mint path bound?	broadcast_share reached?	send_shares reached via	status
BTC	yes — main_btc.cpp:1336, after lk.unlock() at :1323	yes	broadcast_share, readvertise_best	fully live
BCH	yes — pool_entrypoint.hpp:564, after lk.unlock() at :562 (not in main_bch.cpp)	yes	broadcast_share, readvertise_best	fully live
DGB	no — main_dgb.cpp binds no create/mint-share fn	no — zero callers repo-wide	readvertise_best_share only	gate live; broadcast_share half is pre-wiring

DGB detail, all re-verified against this branch:

• set_mint_share_fn, set_create_share_fn, broadcast_share, notify_local_share — 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.cpp.
• dgb::NodeImpl::broadcast_share and notify_local_share have zero callers repo-wide (definitions and comments only). try_mint_share always takes the null-fn branch at dgb/stratum/work_source.cpp:810-814 and logs set_mint_share_fn unbound. Tracked as dgb: set_mint_share_fn is never bound in main_dgb.cpp — DGB cannot mint local shares #884.
• readvertise_best_share() is live: called at dgb/node.cpp:1767 on best-change and armed on a 10 s one-shot timer at :1774, both from the think IO-phase.

What that means for DGB in this PR

Chosen: keep the DGB broadcast_share half, labelled as pre-wiring. Rationale: dropping it would leave a known mark-before-send bug in a function that #884 is going to bind, so whoever wires the mint seam would ship the loss; and the repo's per-coin symmetry posture means the three node.cpp bodies should stay identical. It is zero-risk because it is dead. It is labelled in three places so nobody mistakes it for protection:

• a PRE-WIRING — NOT REACHED ON DGB TODAY (#884) block above dgb::NodeImpl::broadcast_share;
• a DGB REACHABILITY block in src/impl/dgb/known_txs_retention.hpp marking each primitive LIVE vs PRE-WIRED;
• the DGB test suite name DgbBroadcastMarkingPrewired_NotReachedToday.

DGB tests were rescoped onto the reachable path. The previous revision's DGB KAT modelled the broadcast_share de-dup walk — i.e. an unreachable function. That is the shape of the #873 LTC miss, where the test and its negative control both passed against code production never runs. It is replaced by DgbReadvertiseRecording, which drives readvertise_and_record — the primitive readvertise_best_share actually calls. No DGB case constructs or broadcasts a locally minted share.

BTC/BCH ROOT-2 re-advert under the new marking rule

Asked for explicitly, and checked. btc/node.cpp:1053-1061 and bch/node.cpp:1056-1064 implement readvertise_best() as a plain call into broadcast_share, so the ROOT-2 walk still breaks on the first hash in m_shared_share_hashes — unlike LTC/DGB, which have a dedicated de-dup-bypassing readvertise_best_share().

The change is monotonically safe. Post-fix marking is a strict subset of pre-fix marking (only hashes a peer actually accepted). The walk therefore breaks later or in the same place, never earlier, so ROOT-2 re-pushes a superset of what it used to. Specifically:

• Head shares broadcast while no peer was connected, or whose batch every peer abandoned: previously marked and permanently unreachable to the re-advert; now unmarked, so a peer that handshook during the empty window does receive them. Strictly better — this is the exact scenario ROOT-2 exists for.
• Head shares already accepted by some other peer: still marked, walk still breaks. Unchanged from before.

So BTC/BCH ROOT-2 behaves as intended and is improved, but this PR does not fully close it — that still needs the LTC/DGB-style de-dup-bypass readvertise. Pre-existing gap, out of scope, documented in a comment at both call sites. No behavioural change was made to readvertise_best() itself.

Consensus / reward safety

No consensus or minting semantics change. Every change decides only whether we put a share on the wire. Share bytes, minting, PPLNS weighting and payout are untouched. A withheld share stays in our chain and is re-offered on the next broadcast/re-advert once its tx bytes arrive.

Skipping costs no propagation: a share we downloaded is by definition already on the network we fetched it from.

Verified file:line claims (corrections to the issue text)

Claim	Actual at c77dc03
btc missing-tx omission 746-750	748-750 (the else { opens at 746)
dgb missing-tx omission 726-730	728-730
bch missing-tx omission 749-753	751-753
btc broadcast_share @792, mark @823	correct
dgb broadcast_share @772, mark @803	correct
bch broadcast_share @795, mark @826	correct
DASH gate 1287-1309, decline-mint 1400-1410, missing-tx 1338-1343	all correct

Additional findings not in the issue:

• The three send_shares / broadcast_share bodies were byte-identical across btc/dgb/bch, so one patch applied cleanly to all three.
• Unlike DASH, BTC/DGB/BCH send_shares takes the tracker shared_lock itself (DASH's caller holds it). Left as-is.
• DGB has a fourth send_shares call site — readvertise_best_share() — that BTC/BCH lack. It turns out to be DGB's only live one.
• BTC/DGB/BCH have no register_template_txs / rolling template window and no add_local_share; they use core::evict_known_txs_to_cap + m_known_txs_order. So retain_template_txs ships (for btc: F3 known-txs retention/backable primitives + unit KAT #868 symmetry, with KAT coverage) but is not wired, and there is no decline-mint call site to port — the DASH add_local_share decline has no analogue here.

Tests

Folded into existing allowlisted CI targets — no new add_executable, no .github/workflows change (check_test_target_allowlist.py passes):

coin	host target	files
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 is plain int main()/assert with no GTest, so a second source cannot carry its own main(). The BCH TU instead exposes run_share_broadcast_gate_checks() and the host main() calls it — same "ride an allowlisted executable" pattern, no NOT_BUILT sentinel.

Mutation evidence (tests are load-bearing)

Gate deleted (if (all_txs_backable(...)) -> if (true)):

[  FAILED  ] BtcBroadcastGate.GateWithholdsUnbackedShare
[  FAILED  ] BtcBroadcastGate.WholeBatchUnbackedEmptiesTheSend
[  FAILED  ] DgbBroadcastGate.GateWithholdsUnbackedShare
[  FAILED  ] DgbBroadcastGate.WholeBatchUnbackedEmptiesTheSend
  FAIL: F3 gate skips exactly the unbacked share            (bch)
  FAIL: F3 gate keeps the two backable shares               (bch)
  FAIL: F3 gate skips a wholly-unbacked batch               (bch)
  FAIL: F3 gate empties a wholly-unbacked batch             (bch)

Pre-fix F2 restored — mark the offered batch instead of what was sent (btc/bch, the live broadcast_share path):

[  FAILED  ] BtcBroadcastMarking.NothingMarkedWhenNoPeerAccepted
[  FAILED  ] BtcBroadcastMarking.NothingMarkedWithZeroPeers
[  FAILED  ] BtcBroadcastMarking.OnlyAcceptedHashesAreMarked
[  FAILED  ] BtcBroadcastMarking.WalkStrandedWhenMarkedBeforeSend
  FAIL: F2 marks nothing when no peer accepted              (bch)
  FAIL: F2 marks nothing with zero peers                    (bch)
  FAIL: F2 leaves a withheld share un-marked                (bch)
  FAIL: shipped ordering marks nothing on an abandoned send (bch)
  FAIL: shipped ordering re-offers the batch                (bch)

DGB live path (readvertise_and_record) — record the offered batch instead of what was written:

[  FAILED  ] DgbReadvertiseRecording.WithheldShareIsNeverBlamedForAPeerDrop

DGB live path — drop the "peer that received nothing gets no record" skip:

[  FAILED  ] DgbReadvertiseRecording.PeerThatReceivedNothingIsNotRecorded

broadcast_and_mark() and readvertise_and_record() both deliberately take the offered batch as a parameter, so "record/mark the offered batch" is expressible at the primitive level and therefore detectable by the KAT.

Green runs (local, gcc-13 Release, same conan profile as CI)

btc_share_test                                        119 tests / 119 PASSED
dgb_share_test                                         46 tests /  46 PASSED
ctest -R '^(Btc|Dgb)(Broadcast|KnownTxs|Readvertise)'  22/22 passed
ctest -R '^bch_' (build_bch)                           31/31 passed
tools/ci/check_test_target_allowlist.py                OK, 119 targets across 5 lanes

Binaries build clean: c2pool (-DCOIN_DGB=ON) and c2pool-bch (-DCOIN_BCH=ON), so all three node.cpp TUs compile.

Not verified

• No integration test of the live call sites; the KATs pin the primitives those functions call. A reversion at a call site (re-inlining the old ordering) would not redden them. Mitigated by making the wrong policy expressible-and-detectable at the primitive, and by broadcast_share / readvertise_best_share having no other write into m_shared_share_hashes / m_last_broadcast_to.
• No live-network or soak validation of the gate under real sharereply-pulled shares.

For the integrator — follow-up de-duplication

A sibling lane is hoisting all_txs_backable into src/core/ for LTC. This PR deliberately does not create a competing src/core/ copy; it follows #868's per-coin header pattern. Once both land there will be four per-coin copies (btc, dgb, bch, dash) plus the core hoist — these should be collapsed onto the src/core/ version in a follow-up. A note to that effect is in each header. readvertise_and_record() is intentionally DGB-only, since DGB is the only coin with a de-dup-bypassing re-advert.

Ownership fences respected

Nothing touched in src/core/socket.cpp, src/core/web_server.cpp, src/impl/ltc/, src/impl/dash/, or any protocol_legacy.cpp.

[frstrtr]

🛑 BLOCKING — do not merge. This PR wires its core feature into code that can never execute.

I was about to merge this as a green non-draft. It is not safe.

The defect

#880 adds three new instances of the #905 dead probe — one per lane, inside the new partition_backable helper that implements the F3 tx-completeness gate:

if constexpr (requires { obj->m_new_transaction_hashes; })

Added in: src/impl/bch/node.cpp, src/impl/btc/node.cpp, src/impl/dgb/node.cpp.

That expression is ALWAYS FALSE in all three lanes. Verified on master:

lane	member declared	ShareTxInfo m_tx_info in share.hpp
btc	share_types.hpp:107 (inside ShareTxInfo)	2 decls
bch	share_types.hpp:116 (inside ShareTxInfo)	2 decls
dgb	share_types.hpp:110 (inside ShareTxInfo)	2 decls

The member is nested at m_tx_info.m_new_transaction_hashes. A top-level probe for obj->m_new_transaction_hashes cannot bind, if constexpr silently discards the branch, and partition_backable sees an empty new-tx list for every share.

Consequence: skipped_incomplete is always 0, the gate never withholds anything, and F3 — the entire point of this PR — is a no-op on all three lanes.

It is also a REGRESSION on btc

src/impl/btc/node.cpp on master is already fixed:

:767   // m_new_transaction_hashes — the old `requires { obj->m_new_transaction_hashes }`
:778   if constexpr (requires { obj->m_tx_info; })

There is even a comment at :767 explaining why the old form was wrong. This PR reintroduces the old form in a new call site in that same file. Whoever wrote :778 already learned this lesson; please read that comment.

Why CI cannot catch this

This is the #873 trap verbatim, and it is the reason I am reading diffs rather than trusting green. In #873's first cut: core_test 99/99, share_test 55/55, and reverting the fix reddened 4 of 9 cases — every one of those signals true, and every one meaningless, because the function never ran in production. if constexpr on a false requires compiles clean: no warning, no error, no counter.

A passing test suite is not evidence here. Please state explicitly whether broadcast_gate_test.cpp exercises a share type where the probe actually binds — if it uses a mock with a top-level m_new_transaction_hashes, the tests pass while production does nothing.

Required before this can merge

1. Use requires { obj->m_tx_info; } and read through m_tx_info.m_new_transaction_hashes, matching btc/node.cpp:778. This is a port, not a design.
2. Add a test that fails if the probe goes false — e.g. assert skipped_incomplete > 0 for a share with known-missing tx bytes. A test that passes on dead code is worthless.
3. Confirm the tests drive the real share types for each lane, not a mock that happens to expose the member at top level.
4. Coordinate with bch,dgb: tx-forwarding send path is dead code — same requires-probe defect as #875 (ltc) #905, which tracks the same defect at bch/node.cpp:725 and dgb/node.cpp:702 and is assigned to those stewards. Fixing the pre-existing sites and adding three new broken ones in parallel would be the worst outcome.

What is good here

The gate semantics are right, and the header comments are unusually clear — particularly "gate on the BYTES in m_known_txs, not on the peer's have_tx advert: that advert can be stale", and the re-offer-not-drop invariant ("a share skipped here … is re-offered on the next broadcast instead of being silently dropped forever"). That reasoning is exactly right for a reward-adjacent path and I want it kept. Only the member access is wrong.

Re-request review once the probe is fixed. This is otherwise close.

[frstrtr]

Still blocked — partially fixed, and the partial state is worse than either extreme.

Re-measured on the current head:

old broken form added:  3   ← requires { obj->m_new_transaction_hashes; }
correct form added:     1   ← requires { obj->m_tx_info; }

One of the four sites was corrected; three still carry the always-false probe. That is a worse outcome than the original, because the PR now looks fixed on a spot-check while two thirds of the F3 gate remains inert.

Both reference patterns are on master — copy one

• dgb: fix silently-dead tx-relay probe in send_shares (#905) #914 (dgb, 37515dc7) — requires { obj->m_tx_info; }, matching BTC exactly.
• bch: forward txs on the send path via bch::new_tx_hashes accessor (#905) #913 (bch, d83f5393) — a two-tier probe: m_tx_info.m_new_transaction_hashes first, falling back to the top-level name. Slightly more robust; handles either layout.

btc/node.cpp:778 is the original reference and carries an explanatory comment at :767 describing exactly why the old form is wrong. Nothing needs designing.

Why this cannot merge green

if constexpr on a false requires compiles clean — no warning, no error, no counter. The branch is discarded and skipped_incomplete stays 0 forever, so the tx-completeness gate never withholds anything. Green CI is not evidence here; it is the specific failure mode.

Please also state whether broadcast_gate_test.cpp drives the real per-lane share types or a mock that happens to expose the member at top level. A mock would make a dead-code fix look green — that is how #873 passed 99/99 with a passing negative control while wired into unreachable code.

Required to unblock

1. All four sites on the correct form.
2. A test that fails if the probe goes false — e.g. assert skipped_incomplete > 0 for a share with known-missing tx bytes.
3. Confirmation the tests use real share types per lane.

The gate semantics and the header comments in this PR are good — particularly "gate on the BYTES in m_known_txs, not on the peer's have_tx advert" and the re-offer-not-drop invariant. Only the member access is wrong, and it is now wrong in three of four places.

[frstrtr]

Converted to DRAFT to prevent accidental merge. Not a judgement on the work — a guard.

I cannot use --request-changes (the review token owns this PR), and this is now 40/41 green, which makes it look ready. It is not: it still ships a no-op.

old broken form added:  3   ← requires { obj->m_new_transaction_hashes; }  ALWAYS FALSE
correct form added:     1

Unchanged across ~20 hours and several pushes; the latest commit improves lock-discipline tracing while the core defect stands. One of four sites is correct — a partial fix is worse than none, because it survives a spot-check while two thirds of the F3 gate stays inert.

41 passing checks confirm the code compiles, not that it runs. if constexpr on a false requires is discarded silently: no warning, no error, no counter, skipped_incomplete pinned at 0 forever.

Mark it ready again the moment all four sites use the correct form — both patterns are already on master (#914 37515dc7 dgb, #913 d83f5393 bch), and btc/node.cpp:778 carries a comment at :767 explaining why the old form is wrong. Nothing needs designing.

Also still needed: a test that fails if the probe goes false, and confirmation that broadcast_gate_test.cpp drives the real per-lane share types rather than a mock exposing the member at top level.

The gate semantics and header comments in this PR are good and I want it to land. Only the member access is wrong.

[frstrtr]

Reviewed. My framing of this PR was wrong, and the "3 of 4 probes still dead" measurement was a grep artifact. Correcting the record.

All four probe sites already carry the correct nested-first form as of d3e51513. Reproducing the earlier measurement against merge base 37515dc7, the three apparent "broken form" hits are:

hit	what it actually is
btc/share_tx_refs.hpp:16	a comment explaining why the flat probe was wrong
btc/share_tx_refs.hpp:49	the deliberate second tier of the two-tier #913 pattern
btc/test/broadcast_gate_test.cpp:219	a comment in the regression test

The grep was counting prose and the intended fallback branch as defects.

Live probe sites verified reachable, with caller-side lock traces on all three lanes — no exclusive lock is held at any call site (btc node.cpp:796, bch node.cpp:753, dgb node.cpp:729). Two line refs in the PR body have drifted: main_btc.cpp:1336 is actually :1342, and dgb/node.cpp:1767/:1774 are actually :1787/:1793. Substance unaffected.

What WAS genuinely missing

Items 2 and 3 of the unblock list were done for BTC only. BCH and DGB gate tests drove FakeShare — a mock declaring the ref list at top level, precisely the shape that makes a dead-code fix look green.

This was proven by mutation, not asserted. Reverting all three accessors to the flat probe:

FAILED  BtcBroadcastGate.RealV17ShareRefsResolvedViaSsot / RealV33... / RealShareTxInfoRefsWithheld
FAILED  DgbBroadcastGate.RealV17... / RealV33... / RealShareTxInfoRefsWithheld / WhollyUnbackedRealBatchEmpties
FAIL:   v17/v33 refs resolve through the production SSOT              (bch)
FAIL:   F3 gate withholds a REAL share whose tx bytes we lack         (bch)

Every pre-existing FakeShare case still passed under that mutation, on all three lanes.

Branch: coins/f3-gate-real-share-type-guards (5586c649, on top of 8a39213c)

Test-only — 3 files, +323 lines, no production file touched. I verified this myself rather than taking it on report. Adds static_assert topology pins (the existing has_flat_new_tx_hashes/has_nested_new_tx_hashes traits were declared but nothing asserted on them, so they were inert) plus real-share-variant cases driving btc::ShareType/bch::ShareType/dgb::ShareType through the exact refs_of lambda node.cpp installs.

Leaving #880 draft. Fold the branch in when convenient.

Separate finding, filed as #941

While tracing, a genuinely dead callee turned up on LTC: notify_local_share()'s attempt_verify is unreachable on the mint path — main_ltc.cpp:4853 holds m_tracker_mutex exclusively (violating the shared_lock contract documented at node.hpp:568) with no unlock before the call at :5014. I verified this independently. The dead body is the kr1z1s SIGSEGV fix.

Latent hazard — NOT introduced here, NOT fixed here

On all three lanes broadcast_share/readvertise_best_share holds a shared_lock on m_tracker_mutex and calls send_shares, which re-acquires shared_lock(..., try_to_lock) on the same thread. Recursive shared acquisition of std::shared_mutex is UB per [thread.sharedmutex.requirements]; glibc's reader-preferring rwlock grants it in practice, which is why it has never surfaced. If it ever failed, send_shares would take its early return and the whole F3 gate would go silently dead. Present on origin/master (btc/node.cpp:736/:888/:919). Reward-path lock surgery — needs its own PR and a soak.