Merge pull request #623 from frstrtr/dash/g1-txselect-determinism

dash(G1): deterministic equal-feerate tx selection (byte-parity seam)

Author: frstrtr

src/impl/dash/coin/mempool.hpp

@@ -49,6 +49,7 @@
 #include <cstdint>
 #include <ctime>
 #include <map>
+#include <set>
 #include <mutex>
 #include <optional>
 #include <utility>
@@ -72,6 +73,32 @@ struct MempoolEntry {
     }
 };
 
+/// Deterministic block-template selection key. Sorts highest-feerate
+/// first; ties broken by txid ascending. Byte-reproducible across
+/// nodes/runs AND bit-for-bit conformant with dashcore BlockAssembler
+/// ordering. dashcore CompareTxMemPoolEntryByAncestorFee compares two
+/// entries by cross-multiplication of (fee, size) as doubles --
+/// f1 = a.fee * b.size vs f2 = b.fee * a.size -- explicitly "avoid
+/// division by rewriting (a/b > c/d) as (a*d > c*b)". A pre-divided
+/// fee/base_size double rounds where the cross-multiply does not, so the
+/// two representations can order a tie-pair differently. We therefore
+/// carry (fee, base_size) and reproduce the exact double cross-multiply;
+/// equal feerate is broken by GetHash()/txid ascending, as the oracle
+/// does. (No ancestor packages in this simplified mempool, so
+/// GetModFeeAndSize reduces to the entry's own (fee, base_size).)
+struct FeeKey {
+    uint64_t fee;        // satoshi
+    uint32_t base_size;  // serialized bytes (>0 for every indexed entry)
+    uint256  txid;
+    bool operator<(const FeeKey& o) const {
+        // dashcore CompareTxMemPoolEntryByAncestorFee, division-free form.
+        const double f1 = static_cast<double>(fee)   * o.base_size;
+        const double f2 = static_cast<double>(o.fee) * base_size;
+        if (f1 != f2) return f1 > f2;   // higher feerate first
+        return txid < o.txid;           // txid asc tiebreak (oracle-conformant)
+    }
+};
+
 class Mempool {
 public:
     static constexpr size_t DEFAULT_MAX_BYTES   = 300ULL * 1024 * 1024;
@@ -139,7 +166,7 @@ class Mempool {
         // recompute_unknown_fees() resolves them after a UTXO update.
         // Uses negative feerate as the multimap key so begin() = best.
         if (stored.fee_known) {
-            m_feerate_index.emplace(stored.feerate_satvb(), txid);
+            m_feerate_index.insert(FeeKey{stored.fee, stored.base_size, txid});
         }
 
         // Periodic stats — every 100 entries + first 5 + every eviction.
@@ -243,7 +270,7 @@ class Mempool {
         for (auto& [txid, entry] : m_pool) {
             if (entry.fee_known) continue;
             if (compute_fee_locked(entry, utxo)) {
-                m_feerate_index.emplace(entry.feerate_satvb(), txid);
+                m_feerate_index.insert(FeeKey{entry.fee, entry.base_size, txid});
                 resolved_fees += entry.fee;
                 ++resolved;
             } else {
@@ -346,8 +373,8 @@ class Mempool {
         uint32_t total_bytes = 0;
         auto* utxo = m_utxo.load();
 
-        for (const auto& [feerate, txid] : m_feerate_index) {
-            (void)feerate;
+        for (const auto& fk : m_feerate_index) {
+            const uint256& txid = fk.txid;
             auto pit = m_pool.find(txid);
             if (pit == m_pool.end()) continue;
             const auto& entry = pit->second;
@@ -388,7 +415,7 @@ class Mempool {
     std::map<std::pair<uint256, uint32_t>, uint256>    m_spent_outputs;
     // Step 2: feerate-sorted index. greater<double> means begin() =
     // highest feerate, so iteration is best-first.
-    std::multimap<double, uint256, std::greater<double>> m_feerate_index;
+    std::set<FeeKey>                                   m_feerate_index;
     size_t                                             m_total_bytes{0};
     size_t                                             m_max_bytes;
     time_t                                             m_expiry_sec;
@@ -415,13 +442,7 @@ class Mempool {
 
         // Drop from feerate index (only present if fee was known).
         if (it->second.fee_known) {
-            auto frng = m_feerate_index.equal_range(it->second.feerate_satvb());
-            for (auto rit = frng.first; rit != frng.second; ++rit) {
-                if (rit->second == txid) {
-                    m_feerate_index.erase(rit);
-                    break;
-                }
-            }
+            m_feerate_index.erase(FeeKey{it->second.fee, it->second.base_size, txid});
         }
 
         // Drop from spent-outputs index.

test/test_dash_mempool.cpp

@@ -25,12 +25,16 @@
 #include <core/uint256.hpp>
 #include <core/coin/utxo_view_cache.hpp>
 
+#include <algorithm>
 #include <cstdint>
+#include <set>
+#include <vector>
 
 using dash::coin::Mempool;
 using dash::coin::MutableTransaction;
 using dash::coin::dash_txid;
 using dash::coin::BlockType;
+using dash::coin::FeeKey;
 using ::core::coin::UTXOViewCache;
 using ::core::coin::Outpoint;
 using ::core::coin::Coin;
@@ -254,3 +258,146 @@ TEST(DashMempool, SortedSelectionHighestFeerateFirst)
     EXPECT_EQ(dash_txid(selected[1].tx), dash_txid(tx_lo));
     EXPECT_EQ(total_fees, 11'000u);
 }
+
+// ─── G1 byte-parity: equal-feerate selection is deterministic ────────────────
+//
+// The embedded GBT template builder serializes txs in the order
+// get_sorted_txs_with_fees() returns them. For txs sharing the SAME
+// feerate the old std::multimap<double,uint256> kept them in mempool
+// INSERTION order, so two nodes with the same mempool contents but a
+// different arrival order produced different template bytes — a
+// non-deterministic seam that breaks G1 byte-parity against the
+// p2pool-dash / dashcore oracle. FeeKey now breaks feerate ties by txid
+// ascending (matches dashcore CompareTxMemPoolEntryByAncestorFee's
+// GetHash() tiebreak), so the projection is byte-reproducible.
+//
+// This KAT pins that: identical equal-feerate sets added in OPPOSITE
+// orders must yield the SAME selection, ordered by txid ascending.
+static std::vector<uint256> equal_feerate_selection(bool reverse_insertion)
+{
+    UTXOViewCache utxo(nullptr);
+    // 5 distinct prevouts, identical value ⇒ identical fee & base_size
+    // ⇒ identical feerate for every spend below.
+    constexpr int N = 5;
+    std::vector<MutableTransaction> txs;
+    for (int i = 0; i < N; ++i) {
+        uint256 prev = mint_hash(200 + i);
+        utxo.add_coin(Outpoint(prev, 0), Coin(100'000, {}, 1, false));
+        txs.push_back(make_spend(prev, 0, /*out=*/95'000, /*salt=*/300 + i)); // fee 5000
+    }
+
+    Mempool mp;
+    mp.set_utxo(&utxo);
+    if (reverse_insertion)
+        for (auto it = txs.rbegin(); it != txs.rend(); ++it) EXPECT_TRUE(mp.add_tx(*it));
+    else
+        for (auto& t : txs) EXPECT_TRUE(mp.add_tx(t));
+
+    auto [selected, total_fees] = mp.get_sorted_txs_with_fees(/*max_bytes=*/1u << 20);
+    EXPECT_EQ(total_fees, static_cast<uint64_t>(N) * 5'000u);
+    std::vector<uint256> out;
+    for (auto& s : selected) out.push_back(dash_txid(s.tx));
+    return out;
+}
+
+TEST(DashMempool, EqualFeerateSelectionIsTxidAscendingAndInsertionOrderIndependent)
+{
+    auto forward = equal_feerate_selection(/*reverse_insertion=*/false);
+    auto reverse = equal_feerate_selection(/*reverse_insertion=*/true);
+
+    ASSERT_EQ(forward.size(), 5u);
+    ASSERT_EQ(reverse.size(), 5u);
+
+    // Insertion order must not affect the projected order.
+    EXPECT_EQ(forward, reverse)
+        << "equal-feerate tx order must be independent of mempool arrival order";
+
+    // And that stable order is txid ascending (dashcore GetHash() tiebreak).
+    auto sorted = forward;
+    std::sort(sorted.begin(), sorted.end());
+    EXPECT_EQ(forward, sorted)
+        << "equal-feerate ties must resolve to txid-ascending, oracle-conformant order";
+}
+
+
+// --- G1 byte-parity: feerate compare is dashcore division-free cross-multiply
+//
+// dashcore CompareTxMemPoolEntryByAncestorFee compares two entries by
+// cross-multiplication -- f1 = a.fee * b.size vs f2 = b.fee * a.size --
+// explicitly to "avoid division by rewriting (a/b > c/d) as (a*d > c*b)".
+// c2pool previously keyed the sorted index on a PRE-DIVIDED double
+// (fee / base_size). That division rounds, so it can collapse a strict
+// dashcore order into a tie -- or split a dashcore tie into a strict
+// order -- making the two representations disagree on the selection
+// order of certain (fee, size) pairs. A different selection order is a
+// different template byte-serialization: a latent G1 byte-parity seam
+// against the p2pool-dash / dashcore oracle. FeeKey now carries
+// (fee, base_size) and reproduces the exact double cross-multiply.
+//
+// Divergence vector (found by exhaustive search). The disagreement only
+// manifests at fee magnitudes >~1e14 sat, where the fee/size division
+// loses ULPs the cross-multiply keeps -- for realistic magnitudes the
+// two representations agree, so this fix is exact-oracle-conformance
+// hardening, not a realistic-value bug:
+//   A = (fee 182912374030878, size 3535)
+//   B = (fee 4415613369921651, size 85337)
+// Pre-divided doubles: A -> 51743245836.174819946 < B -> 51743245836.174827576,
+//   i.e. the OLD code ranks B strictly ABOVE A.
+// Cross-multiply: A.fee*B.size == B.fee*A.size exactly -> a genuine
+//   feerate TIE, resolved by txid ascending (dashcore GetHash()).
+TEST(DashMempool, FeerateCompareIsDivisionFreeCrossMultiplyNotPreDividedDouble)
+{
+    // Independent dashcore-style reference (division-free cross-multiply).
+    auto oracle_less = [](uint64_t fa, uint32_t sa, const uint256& ta,
+                          uint64_t fb, uint32_t sb, const uint256& tb) {
+        const double f1 = static_cast<double>(fa) * sb;
+        const double f2 = static_cast<double>(fb) * sa;
+        if (f1 != f2) return f1 > f2;   // higher feerate first
+        return ta < tb;                 // txid ascending
+    };
+
+    // Two distinct txids; assign the SMALLER to A so a correct
+    // (cross-multiply => tie => txid-asc) key orders A before B, whereas
+    // the old pre-divide code would have put B first ("B higher feerate").
+    const uint256 t0 = mint_hash(9001);
+    const uint256 t1 = mint_hash(9002);
+    const uint256 ta = std::min(t0, t1);
+    const uint256 tb = std::max(t0, t1);
+    ASSERT_TRUE(ta < tb);
+
+    const uint64_t fa = 182912374030878ULL;
+    const uint32_t sa = 3535u;
+    const uint64_t fb = 4415613369921651ULL;
+    const uint32_t sb = 85337u;
+
+    // Precondition on the vector: a genuine cross-multiply tie that the
+    // buggy pre-divide would (wrongly) have seen as a strict order.
+    EXPECT_EQ(static_cast<double>(fa) * sb, static_cast<double>(fb) * sa)
+        << "vector must be a genuine cross-multiply feerate tie";
+    EXPECT_LT(static_cast<double>(fa) / sa, static_cast<double>(fb) / sb)
+        << "vector must be a strict (wrong) order under the old pre-divide";
+
+    const FeeKey A{fa, sa, ta};
+    const FeeKey B{fb, sb, tb};
+
+    // FeeKey resolves the cross-multiply tie by txid ascending => A first.
+    EXPECT_TRUE(A < B) << "cross-multiply tie must fall to txid-ascending (A<B)";
+    EXPECT_FALSE(B < A);
+
+    // FeeKey ordering must agree with the independent oracle on this pair.
+    EXPECT_EQ(A < B, oracle_less(fa, sa, ta, fb, sb, tb));
+    EXPECT_EQ(B < A, oracle_less(fb, sb, tb, fa, sa, ta));
+
+    // The real index type (std::set<FeeKey>) must iterate A best-first.
+    std::set<FeeKey> idx{B, A};
+    ASSERT_EQ(idx.size(), 2u);
+    EXPECT_EQ(idx.begin()->txid, ta)
+        << "best-first iteration must yield A (smaller txid) on the tie";
+
+    // Sanity: the feerate arm still dominates. Strictly higher feerate
+    // must precede regardless of a smaller-txid competitor.
+    const FeeKey hi{20000u, 250u, tb};   // 80 sat/byte
+    const FeeKey lo{10000u, 250u, ta};   // 40 sat/byte, but smaller txid
+    EXPECT_TRUE(hi < lo) << "higher feerate must precede regardless of txid";
+    EXPECT_FALSE(lo < hi);
+}