Skip to content

Commit d3e5151

Browse files
committed
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.
1 parent 5530ceb commit d3e5151

5 files changed

Lines changed: 192 additions & 9 deletions

File tree

src/impl/bch/node.cpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -746,9 +746,12 @@ std::vector<uint256> NodeImpl::send_shares(peer_ptr peer,
746746
shares, m_known_txs, [](ShareType& share) {
747747
std::vector<uint256> hashes;
748748
share.invoke([&](auto* obj) {
749-
if constexpr (requires { obj->m_new_transaction_hashes; })
750-
hashes.assign(obj->m_new_transaction_hashes.begin(),
751-
obj->m_new_transaction_hashes.end());
749+
// #880: route through the bch::new_tx_hashes SSOT (#913)
750+
// instead of the dead flat probe -- v17/v33 nest the list in
751+
// m_tx_info, v34+ carry none. The old probe was FALSE for
752+
// every variant, so this F3 gate never withheld a share.
753+
if (const auto* new_txs = bch::new_tx_hashes(obj))
754+
hashes.assign(new_txs->begin(), new_txs->end());
752755
});
753756
return hashes;
754757
});

src/impl/btc/node.cpp

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// SPDX-License-Identifier: AGPL-3.0-or-later
22
#include "node.hpp"
33
#include "known_txs_retention.hpp" // F3 retain_template_txs + select_backable_shares
4+
#include "share_tx_refs.hpp" // btc::new_tx_hashes -- uniform gate probe (#880)
45
#include "coin/template_other_txs.hpp" // deserialize_template_other_txs (GBT data[] -> MutableTransaction)
56

67
#include <core/common.hpp>
@@ -787,9 +788,13 @@ std::vector<uint256> NodeImpl::send_shares(peer_ptr peer, const std::vector<uint
787788
shares, m_known_txs, [](ShareType& share) {
788789
std::vector<uint256> hashes;
789790
share.invoke([&](auto* obj) {
790-
if constexpr (requires { obj->m_new_transaction_hashes; })
791-
hashes.assign(obj->m_new_transaction_hashes.begin(),
792-
obj->m_new_transaction_hashes.end());
791+
// #880: route through the btc::new_tx_hashes SSOT instead
792+
// of the dead flat probe -- v17/v33 nest the list in
793+
// m_tx_info, v34+ carry none. The old probe was FALSE for
794+
// every variant, so this F3 gate saw empty refs -> every
795+
// share vacuously backable -> the gate no-op'd all versions.
796+
if (const auto* new_txs = btc::new_tx_hashes(obj))
797+
hashes.assign(new_txs->begin(), new_txs->end());
793798
});
794799
return hashes;
795800
});

src/impl/btc/share_tx_refs.hpp

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
// SPDX-License-Identifier: AGPL-3.0-or-later
2+
//
3+
// Uniform access to a share's "new transaction hashes" list across the BTC
4+
// share variants.
5+
//
6+
// WHY THIS EXISTS. The BTC share variants do NOT all spell this list the same
7+
// way, and they do not all HAVE it:
8+
//
9+
// btc::Share (v17) -> obj->m_tx_info.m_new_transaction_hashes
10+
// btc::NewShare (v33) -> obj->m_tx_info.m_new_transaction_hashes
11+
// btc::SegwitMiningShare (v34) -> none (carries types::DataSegwitShare, no
12+
// btc::PaddingBugfixShare (v35) m_tx_info member -- share_check leaves the
13+
// btc::MergedMiningShare (v36) tx list empty for version >= 34)
14+
//
15+
// node.cpp's send_shares() F3 broadcast gate (partition_backable) probed
16+
// `requires { obj->m_new_transaction_hashes; }` directly on the share object.
17+
// That expression is FALSE for every one of the five variants -- v17/v33 nest
18+
// the list inside m_tx_info, v34+ have no list -- so the gate collected an empty
19+
// ref list for every share, every share was vacuously backable, and the gate
20+
// no-op'd for ALL versions (#880): a v17/v33 share referencing a tx the peer
21+
// lacked was broadcast anyway, tripping the canonical "referenced unknown
22+
// transaction" disconnect. The per_share_refs relay block below the gate already
23+
// spells the nested list correctly (#871), which is why relay worked and only
24+
// the gate probe was dead.
25+
//
26+
// Probing through one accessor removes the class of bug: a new share variant
27+
// either exposes the list in one of the two known spellings and is handled, or
28+
// exposes none and is reported as "no list" (nothing to gate on) -- never
29+
// silently dead because of a member-name mismatch. Mirrors bch::new_tx_hashes
30+
// (src/impl/bch/share_tx_refs.hpp, #913) and ltc::new_tx_hashes
31+
// (src/impl/ltc/share_tx_refs.hpp, #873); BTC's variant topology is identical.
32+
33+
#pragma once
34+
35+
#include <vector>
36+
37+
#include <core/uint256.hpp>
38+
39+
namespace btc {
40+
41+
// Pointer to the share's new-transaction-hash list, or nullptr when this share
42+
// variant carries none. A pointer (not a copy) so the broadcast hot path does no
43+
// per-share allocation. Lifetime is the share object's.
44+
template <typename ShareObj>
45+
inline auto new_tx_hashes(ShareObj* obj)
46+
{
47+
if constexpr (requires { obj->m_tx_info.m_new_transaction_hashes; })
48+
return &obj->m_tx_info.m_new_transaction_hashes;
49+
else if constexpr (requires { obj->m_new_transaction_hashes; })
50+
return &obj->m_new_transaction_hashes;
51+
else
52+
return static_cast<const std::vector<uint256>*>(nullptr);
53+
}
54+
55+
// Compile-time pin of the fact above: true iff the share variant exposes the
56+
// list under the FLAT spelling that the old probe assumed. Held false by the KAT
57+
// for every BTC variant -- if this ever becomes true, the flat branch of
58+
// new_tx_hashes() is what handles it, not a silent skip.
59+
template <typename ShareObj>
60+
inline constexpr bool has_flat_new_tx_hashes =
61+
requires(ShareObj* o) { o->m_new_transaction_hashes; };
62+
63+
// True iff the share variant nests the list inside m_tx_info (v17 / v33).
64+
template <typename ShareObj>
65+
inline constexpr bool has_nested_new_tx_hashes =
66+
requires(ShareObj* o) { o->m_tx_info.m_new_transaction_hashes; };
67+
68+
} // namespace btc

src/impl/btc/test/broadcast_gate_test.cpp

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,13 @@
3333

3434
#include <map>
3535
#include <set>
36+
#include <utility>
3637
#include <vector>
3738

3839
#include <core/uint256.hpp>
3940
#include <impl/btc/known_txs_retention.hpp>
41+
#include <impl/btc/share.hpp> // real btc share variants + ShareType
42+
#include <impl/btc/share_tx_refs.hpp> // btc::new_tx_hashes SSOT (#880)
4043

4144
namespace {
4245

@@ -206,3 +209,105 @@ TEST(BtcBroadcastMarking, WalkStrandedWhenMarkedBeforeSend)
206209
EXPECT_TRUE(walk(chain, marked).empty()); // now correctly de-duped
207210
}
208211
}
212+
213+
214+
// ---------------------------------------------------------------------------
215+
// #880 REGRESSION: the F3 gate above must be driven by the PRODUCTION probe,
216+
// not a hand-rolled stand-in. The FakeShare KATs exercise partition_backable's
217+
// MECHANICS with a top-level new_txs member and a bespoke refs_of -- exactly
218+
// the shape that let the real bug hide. In production send_shares probed
219+
// `requires { obj->m_new_transaction_hashes; }`, which is FALSE for every BTC
220+
// share variant (the list is nested in m_tx_info), so the gate collected empty
221+
// refs, every share was vacuously backable, and the gate silently no-op'd for
222+
// ALL versions -- a share referencing a tx the peer lacked was broadcast anyway
223+
// and tripped the canonical "referenced unknown transaction" disconnect.
224+
//
225+
// These KATs build REAL btc share types and drive them through the SAME
226+
// btc::new_tx_hashes SSOT that node.cpp's partition_backable refs_of now uses.
227+
// FAILS-BEFORE: revert btc::new_tx_hashes (or the gate) to the flat probe and
228+
// the v17/v33 expectations collapse -- the unbacked share is no longer withheld.
229+
// ---------------------------------------------------------------------------
230+
namespace {
231+
232+
// The exact refs_of node.cpp installs on partition_backable: route the variant
233+
// through the btc::new_tx_hashes SSOT.
234+
std::vector<uint256> production_refs_of(btc::ShareType& share)
235+
{
236+
std::vector<uint256> hashes;
237+
share.invoke([&](auto* obj) {
238+
if (const auto* new_txs = btc::new_tx_hashes(obj))
239+
hashes.assign(new_txs->begin(), new_txs->end());
240+
});
241+
return hashes;
242+
}
243+
244+
} // namespace
245+
246+
// Accessor SSOT: a REAL v17 share nests its new-tx hashes in m_tx_info; the
247+
// production probe must surface them (the dead flat probe returned nullptr).
248+
TEST(BtcBroadcastGate, RealV17ShareRefsResolvedViaSsot)
249+
{
250+
btc::Share s;
251+
s.m_tx_info.m_new_transaction_hashes = {TA, TC};
252+
253+
const auto* refs = btc::new_tx_hashes(&s);
254+
ASSERT_NE(refs, nullptr);
255+
ASSERT_EQ(refs->size(), 2u); // dead flat probe -> nullptr
256+
EXPECT_EQ((*refs)[0], TA);
257+
EXPECT_EQ((*refs)[1], TC);
258+
}
259+
260+
// Accessor SSOT: v33 uses the same nested carrier.
261+
TEST(BtcBroadcastGate, RealV33ShareRefsResolvedViaSsot)
262+
{
263+
btc::NewShare s;
264+
s.m_tx_info.m_new_transaction_hashes = {TC};
265+
266+
const auto* refs = btc::new_tx_hashes(&s);
267+
ASSERT_NE(refs, nullptr);
268+
ASSERT_EQ(refs->size(), 1u);
269+
EXPECT_EQ((*refs)[0], TC);
270+
}
271+
272+
// Accessor SSOT: v34/v35/v36 carry no m_tx_info -> nullptr, compiled out.
273+
TEST(BtcBroadcastGate, SegwitAndMergedVariantsHaveNoRefs)
274+
{
275+
btc::SegwitMiningShare v34;
276+
btc::PaddingBugfixShare v35;
277+
btc::MergedMiningShare v36;
278+
EXPECT_EQ(btc::new_tx_hashes(&v34), nullptr);
279+
EXPECT_EQ(btc::new_tx_hashes(&v35), nullptr);
280+
EXPECT_EQ(btc::new_tx_hashes(&v36), nullptr);
281+
}
282+
283+
// The gate end-to-end: REAL share variants + the production refs_of through
284+
// btc::partition_backable. A v17 share referencing a tx whose bytes we do NOT
285+
// hold must be withheld; the backable ones survive, in order. Pre-fix the dead
286+
// probe made every share vacuously backable -> skipped==0, nothing withheld.
287+
TEST(BtcBroadcastGate, RealShareTxInfoRefsWithheld)
288+
{
289+
std::vector<btc::Share*> raws; // own the heap shares for cleanup
290+
auto v17 = [&](std::vector<uint256> refs) {
291+
auto* raw = new btc::Share();
292+
raw->m_tx_info.m_new_transaction_hashes = std::move(refs);
293+
raws.push_back(raw);
294+
btc::ShareType sv; sv = raw; return sv;
295+
};
296+
297+
const std::set<uint256> held{TA, TB}; // TC bytes NOT held
298+
299+
std::vector<btc::ShareType> batch;
300+
batch.push_back(v17({TA})); // backable
301+
batch.push_back(v17({TC})); // references unheld TC -> withheld
302+
batch.push_back(v17({TA, TB})); // backable
303+
304+
const std::size_t skipped =
305+
btc::partition_backable(batch, held, production_refs_of);
306+
307+
EXPECT_EQ(skipped, 1u); // dead-probe regression -> 0
308+
ASSERT_EQ(batch.size(), 2u);
309+
EXPECT_EQ(production_refs_of(batch[0]).front(), TA); // order preserved
310+
EXPECT_EQ(production_refs_of(batch[1]).front(), TA);
311+
312+
for (auto* raw : raws) delete raw;
313+
}

src/impl/dgb/node.cpp

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -722,9 +722,11 @@ std::vector<uint256> NodeImpl::send_shares(peer_ptr peer,
722722
shares, m_known_txs, [](ShareType& share) {
723723
std::vector<uint256> hashes;
724724
share.invoke([&](auto* obj) {
725-
if constexpr (requires { obj->m_new_transaction_hashes; })
726-
hashes.assign(obj->m_new_transaction_hashes.begin(),
727-
obj->m_new_transaction_hashes.end());
725+
// #880: route through the dgb::append_share_tx_refs SSOT
726+
// (#914) instead of the dead flat probe -- the refs live in
727+
// m_tx_info on v17/v33, absent v34+. The old probe was FALSE
728+
// for every version, so this F3 gate never withheld a share.
729+
dgb::append_share_tx_refs(obj, hashes);
728730
});
729731
return hashes;
730732
});

0 commit comments

Comments
 (0)