diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1707ba177..0f807e0dc 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -478,6 +478,7 @@ jobs: bch_donation_flip_kat_test \ bch_cashtokens_transparency_test \ bch_cashaddr_kat_test \ + bch_share_tx_refs_kat_test \ bch_block_connector_test bch_block_ordering_test \ bch_utxo_connect_test bch_reorg_connect_test bch_reorg_rerequest_test \ bch_compact_block_connector_test \ @@ -613,6 +614,7 @@ jobs: bch_donation_flip_kat_test \ bch_cashtokens_transparency_test \ bch_cashaddr_kat_test \ + bch_share_tx_refs_kat_test \ bch_block_connector_test bch_block_ordering_test \ bch_utxo_connect_test bch_reorg_connect_test bch_reorg_rerequest_test \ bch_compact_block_connector_test \ diff --git a/src/impl/bch/node.cpp b/src/impl/bch/node.cpp index 692c136b5..9e0e29cbd 100644 --- a/src/impl/bch/node.cpp +++ b/src/impl/bch/node.cpp @@ -1,6 +1,8 @@ // SPDX-License-Identifier: AGPL-3.0-or-later #include "node.hpp" +#include "share_tx_refs.hpp" // bch::new_tx_hashes -- uniform send-side probe (#905) + #include #include #include @@ -722,14 +724,21 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector& share_hash for (auto& share : shares) { share.invoke([&](auto* obj) { - if constexpr (requires { obj->m_new_transaction_hashes; }) + // Route through the accessor instead of probing + // obj->m_new_transaction_hashes directly (as this used to). That flat + // probe is FALSE for every BCH variant -- v17/v33 nest the list inside + // m_tx_info, v34+ carry no list -- so this whole forwarding block was + // unreachable and BCH forwarded no tx bytes (#905). new_tx_hashes() + // returns the nested list for v17/v33 and nullptr for v34+ (nothing to + // forward), never a silent skip. + const auto* new_txs = bch::new_tx_hashes(obj); + if (!new_txs) + return; + for (const auto& th : *new_txs) { - for (const auto& th : obj->m_new_transaction_hashes) - { - if (!peer->m_remote_txs.count(th) && - !peer->m_remembered_txs.count(th)) - needed_txs.insert(th); - } + if (!peer->m_remote_txs.count(th) && + !peer->m_remembered_txs.count(th)) + needed_txs.insert(th); } }); } diff --git a/src/impl/bch/share_tx_refs.hpp b/src/impl/bch/share_tx_refs.hpp new file mode 100644 index 000000000..f9d74e9ba --- /dev/null +++ b/src/impl/bch/share_tx_refs.hpp @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// +// Uniform access to a share's "new transaction hashes" list across the BCH +// share variants. +// +// WHY THIS EXISTS. The BCH share variants do NOT all spell this list the same +// way, and they do not all HAVE it: +// +// bch::Share (v17) -> obj->m_tx_info.m_new_transaction_hashes +// bch::NewShare (v33) -> obj->m_tx_info.m_new_transaction_hashes +// bch::SegwitMiningShare (v34) -> none (carries types::DataSegwitShare, no +// bch::PaddingBugfixShare (v35) m_tx_info member -- BCH has no SegWit so +// bch::MergedMiningShare (v36) v34+ never populate a new-tx list at all) +// +// node.cpp's tx-forwarding SEND path (send_shares) 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 whole needed_txs / +// remember_tx forwarding block was unreachable: BCH never forwarded a single tx +// byte, and a relayed v17/v33 share referencing a tx the peer lacked was exactly +// the canonical "referenced unknown transaction" disconnect (#905). The RECEIVE +// side (protocol_actual.cpp:167 / protocol_legacy.cpp:173 / node.hpp:291) spells +// the nested list correctly, which is why receive worked and only send 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 forward) -- never +// silently skipped because of a member-name mismatch. Mirrors ltc::new_tx_hashes +// (src/impl/ltc/share_tx_refs.hpp, #873); BCH's variant topology is identical. + +#pragma once + +#include + +#include + +namespace bch { + +// 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 BCH 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 bch diff --git a/src/impl/bch/test/CMakeLists.txt b/src/impl/bch/test/CMakeLists.txt index 40d418b0a..9a343c10b 100644 --- a/src/impl/bch/test/CMakeLists.txt +++ b/src/impl/bch/test/CMakeLists.txt @@ -28,6 +28,7 @@ if(BUILD_TESTING) coinbase_author_kat_test # KAT G2: v36 coinbase output-assembly byte-parity vs p2pool-merged-v36 oracle (pure) donation_flip_kat_test # KAT G2-C3: donation P2PK<->P2SH byte-correctness across the v35<->v36 flip (pure) cashaddr_kat_test # M4: CashAddr codec KAT -- encode/decode roundtrip + PolyMod/version-byte parity vs BCHN vectors, prefix/checksum rejection (pure) + share_tx_refs_kat_test # #905: bch::new_tx_hashes accessor -- send-side flat-probe regression pin (v17/v33 nested, v34+ none) (pure) ) foreach(t IN LISTS BCH_ABLA_TESTS) add_executable(bch_${t} ${t}.cpp) diff --git a/src/impl/bch/test/share_tx_refs_kat_test.cpp b/src/impl/bch/test/share_tx_refs_kat_test.cpp new file mode 100644 index 000000000..bece2fa2e --- /dev/null +++ b/src/impl/bch/test/share_tx_refs_kat_test.cpp @@ -0,0 +1,137 @@ +// --------------------------------------------------------------------------- +// bch::new_tx_hashes accessor KAT -- the guard for the #905 send-side fix. +// +// #905: BCH's tx-forwarding SEND path (node.cpp send_shares) probed +// `requires { obj->m_new_transaction_hashes; }` directly on the share object. +// That FLAT probe is false for EVERY BCH share variant -- v17/v33 nest the list +// inside m_tx_info, v34/v35/v36 carry no list at all -- so the whole needed_txs +// forwarding block was dead code and BCH forwarded no tx bytes. share_tx_refs.hpp +// replaces the flat probe with bch::new_tx_hashes(), which returns the nested +// list for v17/v33 and nullptr for v34+. +// +// This test pins two things so the regression cannot silently return: +// A. Compile-time topology (static_assert): the FLAT spelling is absent on +// every variant (== the old probe was dead), the NESTED spelling is present +// exactly on v17/v33, and new_tx_hashes() selects the right branch. +// B. Runtime behaviour: the accessor delivers the v17/v33 list by reference +// (send loop sees every referenced hash) and returns nullptr for v34+. +// +// Build-INERT wrt consensus surface: pure header-only over share.hpp + +// share_tx_refs.hpp, no node/RPC. p2pool-merged-v36 surface: NONE (this is a +// LOCAL relay-plumbing accessor; the share wire-format is unchanged). +// --------------------------------------------------------------------------- + +#include +#include +#include + +#include + +#include "../share.hpp" +#include "../share_tx_refs.hpp" + +namespace { + +int failures = 0; +#define CHECK(cond) do { if (!(cond)) { \ + std::cerr << "FAIL: " #cond " @ line " << __LINE__ << "\n"; ++failures; } } while (0) + +// ---- A. Compile-time topology -------------------------------------------- +// The FLAT spelling the old probe assumed is absent on EVERY variant: that is +// precisely why the old send path was dead. Held here so it stays absent. +static_assert(!bch::has_flat_new_tx_hashes, "v17 must NOT expose the flat list"); +static_assert(!bch::has_flat_new_tx_hashes, "v33 must NOT expose the flat list"); +static_assert(!bch::has_flat_new_tx_hashes, "v34 must NOT expose the flat list"); +static_assert(!bch::has_flat_new_tx_hashes, "v35 must NOT expose the flat list"); +static_assert(!bch::has_flat_new_tx_hashes, "v36 must NOT expose the flat list"); + +// The NESTED spelling is present exactly on v17/v33 (which carry m_tx_info) and +// absent on v34+ (SegWit-less variants carry no new-tx list). +static_assert(bch::has_nested_new_tx_hashes, "v17 must nest the list in m_tx_info"); +static_assert(bch::has_nested_new_tx_hashes, "v33 must nest the list in m_tx_info"); +static_assert(!bch::has_nested_new_tx_hashes, "v34 carries no new-tx list"); +static_assert(!bch::has_nested_new_tx_hashes, "v35 carries no new-tx list"); +static_assert(!bch::has_nested_new_tx_hashes, "v36 carries no new-tx list"); + +// new_tx_hashes() selects a non-null pointer branch for v17/v33 and the null +// pointer branch for v34+ -- checked at compile time on the return type / at run +// time on the value below. +template +constexpr bool returns_null_branch = + std::is_same_v())), + const std::vector*>; +static_assert(returns_null_branch, "v34 accessor must be the null branch"); +static_assert(returns_null_branch, "v35 accessor must be the null branch"); +static_assert(returns_null_branch, "v36 accessor must be the null branch"); + +// ---- B. Runtime behaviour ------------------------------------------------- + +// Reproduce the send-side loop over the accessor result and count the hashes it +// would forward -- this is exactly what node.cpp send_shares now does. +template +std::size_t forwardable_count(ShareVariant& s) +{ + std::size_t n = 0; + const auto* new_txs = bch::new_tx_hashes(&s); + if (!new_txs) + return 0; + for (const auto& th : *new_txs) + { + (void)th; + ++n; + } + return n; +} + +void test_v17_v33_deliver_nested_list() +{ + bch::Share s17; + s17.m_tx_info.m_new_transaction_hashes = {uint256(1), uint256(2), uint256(3)}; + + const auto* p = bch::new_tx_hashes(&s17); + CHECK(p != nullptr); + // Same object, by reference -- no copy: the send path reads the live list. + CHECK(p == &s17.m_tx_info.m_new_transaction_hashes); + CHECK(p->size() == 3); + CHECK(forwardable_count(s17) == 3); + + bch::NewShare s33; + s33.m_tx_info.m_new_transaction_hashes = {uint256(7)}; + CHECK(bch::new_tx_hashes(&s33) != nullptr); + CHECK(forwardable_count(s33) == 1); + + // Empty nested list: accessor non-null, nothing to forward (not the same as + // "no list" -- a v17 share with no new txs still probes cleanly). + bch::Share s17_empty; + CHECK(bch::new_tx_hashes(&s17_empty) != nullptr); + CHECK(forwardable_count(s17_empty) == 0); +} + +void test_v34plus_have_no_list() +{ + bch::SegwitMiningShare s34; + bch::PaddingBugfixShare s35; + bch::MergedMiningShare s36; + CHECK(bch::new_tx_hashes(&s34) == nullptr); + CHECK(bch::new_tx_hashes(&s35) == nullptr); + CHECK(bch::new_tx_hashes(&s36) == nullptr); + CHECK(forwardable_count(s34) == 0); + CHECK(forwardable_count(s35) == 0); + CHECK(forwardable_count(s36) == 0); +} + +} // namespace + +int main() +{ + test_v17_v33_deliver_nested_list(); + test_v34plus_have_no_list(); + + if (failures) + { + std::cerr << failures << " CHECK(s) failed\n"; + return 1; + } + std::cout << "bch_share_tx_refs_kat_test: OK\n"; + return 0; +}