Merge pull request #913 from frstrtr/bch/tx-forward-probe-fix

bch: forward txs on the send path via bch::new_tx_hashes accessor (#905)

Author: frstrtr

.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 \

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 <core/common.hpp>
 #include <core/hash.hpp>
 #include <core/random.hpp>
@@ -722,14 +724,21 @@ void NodeImpl::send_shares(peer_ptr peer, const std::vector<uint256>& 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);
             }
         });
     }

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 <vector>
+
+#include <core/uint256.hpp>
+
+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 <typename ShareObj>
+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<const std::vector<uint256>*>(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 <typename ShareObj>
+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 <typename ShareObj>
+inline constexpr bool has_nested_new_tx_hashes =
+    requires(ShareObj* o) { o->m_tx_info.m_new_transaction_hashes; };
+
+} // namespace bch

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)

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 <iostream>
+#include <type_traits>
+#include <vector>
+
+#include <core/uint256.hpp>
+
+#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<bch::Share>,              "v17 must NOT expose the flat list");
+static_assert(!bch::has_flat_new_tx_hashes<bch::NewShare>,          "v33 must NOT expose the flat list");
+static_assert(!bch::has_flat_new_tx_hashes<bch::SegwitMiningShare>,  "v34 must NOT expose the flat list");
+static_assert(!bch::has_flat_new_tx_hashes<bch::PaddingBugfixShare>, "v35 must NOT expose the flat list");
+static_assert(!bch::has_flat_new_tx_hashes<bch::MergedMiningShare>,  "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<bch::Share>,              "v17 must nest the list in m_tx_info");
+static_assert(bch::has_nested_new_tx_hashes<bch::NewShare>,          "v33 must nest the list in m_tx_info");
+static_assert(!bch::has_nested_new_tx_hashes<bch::SegwitMiningShare>,  "v34 carries no new-tx list");
+static_assert(!bch::has_nested_new_tx_hashes<bch::PaddingBugfixShare>, "v35 carries no new-tx list");
+static_assert(!bch::has_nested_new_tx_hashes<bch::MergedMiningShare>,  "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 <typename S>
+constexpr bool returns_null_branch =
+    std::is_same_v<decltype(bch::new_tx_hashes(std::declval<S*>())),
+                   const std::vector<uint256>*>;
+static_assert(returns_null_branch<bch::SegwitMiningShare>,  "v34 accessor must be the null branch");
+static_assert(returns_null_branch<bch::PaddingBugfixShare>, "v35 accessor must be the null branch");
+static_assert(returns_null_branch<bch::MergedMiningShare>,  "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 <typename ShareVariant>
+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;
+}