Store dust indexes in CTxMemPoolEntry, use for ephemeral dust

instagibbs · Oct 21, 2024 · 6007146 · 6007146
1 parent 7defd31
commit 6007146
Show file tree

Hide file tree

Showing 12 changed files with 52 additions and 146 deletions.
diff --git a/src/bench/mempool_eviction.cpp b/src/bench/mempool_eviction.cpp
@@ -28,7 +28,7 @@ static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& po
     LockPoints lp;
     pool.addUnchecked(CTxMemPoolEntry(
         tx, nFee, nTime, nHeight, sequence,
-        spendsCoinbase, sigOpCost, lp));
+        spendsCoinbase, /*dust_indexes=*/{}, sigOpCost, lp));
 }
 
 // Right now this is only testing eviction performance in an extremely small

diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp
@@ -28,7 +28,7 @@ static void AddTx(const CTransactionRef& tx, CTxMemPool& pool) EXCLUSIVE_LOCKS_R
     bool spendsCoinbase = false;
     unsigned int sigOpCost = 4;
     LockPoints lp;
-    pool.addUnchecked(CTxMemPoolEntry(tx, 1000, nTime, nHeight, sequence, spendsCoinbase, sigOpCost, lp));
+    pool.addUnchecked(CTxMemPoolEntry(tx, 1000, nTime, nHeight, sequence, spendsCoinbase, /*dust_indexes=*/{}, sigOpCost, lp));
 }
 
 struct Available {

diff --git a/src/bench/rpc_mempool.cpp b/src/bench/rpc_mempool.cpp
@@ -21,7 +21,7 @@
 static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
 {
     LockPoints lp;
-    pool.addUnchecked(CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
+    pool.addUnchecked(CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*dust_indexes=*/{}, /*sigops_cost=*/4, lp));
 }
 
 static void RpcMempool(benchmark::Bench& bench)

diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
@@ -88,6 +88,7 @@ class CTxMemPoolEntry
     const bool spendsCoinbase;      //!< keep track of transactions that spend a coinbase
     const int64_t sigOpCost;        //!< Total sigop cost
     CAmount m_modified_fee;         //!< Used for determining the priority of the transaction for mining in a block
+    const std::vector<uint32_t> m_dust_indexes; //!< Used to track ephemeral dust this transaction may create
     mutable LockPoints lockPoints;  //!< Track the height and time at which tx was final
 
     // Information about descendants of this transaction that are in the
@@ -109,6 +110,7 @@ class CTxMemPoolEntry
     CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee,
                     int64_t time, unsigned int entry_height, uint64_t entry_sequence,
                     bool spends_coinbase,
+                    std::vector<uint32_t> dust_indexes,
                     int64_t sigops_cost, LockPoints lp)
         : tx{tx},
           nFee{fee},
@@ -120,6 +122,7 @@ class CTxMemPoolEntry
           spendsCoinbase{spends_coinbase},
           sigOpCost{sigops_cost},
           m_modified_fee{nFee},
+          m_dust_indexes{dust_indexes},
           lockPoints{lp},
           nSizeWithDescendants{GetTxSize()},
           nModFeesWithDescendants{nFee},
@@ -145,6 +148,7 @@ class CTxMemPoolEntry
     std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; }
     unsigned int GetHeight() const { return entryHeight; }
     uint64_t GetSequence() const { return entry_sequence; }
+    std::vector<uint32_t> GetDustIndexes() const { return m_dust_indexes; }
     int64_t GetSigOpCost() const { return sigOpCost; }
     CAmount GetModifiedFee() const { return m_modified_fee; }
     size_t DynamicMemoryUsage() const { return nUsageSize; }

diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
@@ -726,7 +726,7 @@ class ChainImpl : public Chain
     {
         if (!m_node.mempool) return {};
         LockPoints lp;
-        CTxMemPoolEntry entry(tx, 0, 0, 0, 0, false, 0, lp);
+        CTxMemPoolEntry entry(tx, 0, 0, 0, 0, false, {}, 0, lp);
         LOCK(m_node.mempool->cs);
         return m_node.mempool->CheckPackageLimits({tx}, entry.GetTxSize());
     }

diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
@@ -5,30 +5,34 @@
 #include<policy/ephemeral_policy.h>
 #include<policy/policy.h>
 
-bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_fee, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
+bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_fee, CAmount base_fee, CAmount mod_fee, std::vector<uint32_t>& dust_indexes, TxValidationState& state)
 {
+    dust_indexes.clear();
+    for (size_t i = 0; i < tx->vout.size(); ++i) {
+        const auto& output = tx->vout[i];
+        if (IsDust(output, dust_relay_fee)) dust_indexes.push_back(i);
+    }
+
     // We never want to give incentives to mine this transaction alone
     if ((base_fee != 0 || mod_fee != 0) &&
-        std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_fee); })) {
+        !dust_indexes.empty()) {
         return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0-fee");
     }
 
     return true;
 }
 
-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
+std::optional<Txid> CheckEphemeralSpends(const std::vector<CTxMemPoolEntry*>& package_entries, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
 {
-    if (!Assume(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}))) {
-        // Bail out of spend checks if caller gave us an invalid package
-        return std::nullopt;
+    std::map<Txid, CTxMemPoolEntry*> map_txid_ref;
+    for (const auto& entry : package_entries) {
+        const auto& tx = entry->GetSharedTx();
+        map_txid_ref[tx->GetHash()] = entry;
     }
 
-    std::map<Txid, CTransactionRef> map_txid_ref;
-    for (const auto& tx : package) {
-        map_txid_ref[tx->GetHash()] = tx;
-    }
+    for (const auto& entry : package_entries) {
+        const auto& tx = entry->GetSharedTx();
 
-    for (const auto& tx : package) {
         Txid txid = tx->GetHash();
         std::unordered_set<Txid, SaltedTxidHasher> processed_parent_set;
         std::unordered_set<COutPoint, SaltedOutpointHasher> unspent_parent_dust;
@@ -39,21 +43,13 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
             if (processed_parent_set.contains(parent_txid)) continue;
 
             // We look for an in-package or in-mempool dependency
-            CTransactionRef parent_ref = nullptr;
-            if (map_txid_ref.contains(parent_txid)) {
-                parent_ref = map_txid_ref[parent_txid];
-            } else {
-                parent_ref = tx_pool.get(parent_txid);
-            }
+            const CTxMemPoolEntry* parent_entry = map_txid_ref.contains(parent_txid) ? map_txid_ref[parent_txid] : tx_pool.GetEntry(parent_txid);
 
-            // Check for dust on parents
-            if (parent_ref) {
-                for (uint32_t out_index = 0; out_index < parent_ref->vout.size(); out_index++) {
-                    const auto& tx_output = parent_ref->vout[out_index];
-                    if (IsDust(tx_output, dust_relay_rate)) {
-                        unspent_parent_dust.insert(COutPoint(parent_txid, out_index));
-                    }
-                }
+            // Accumulate dust from parents
+            if (parent_entry) {
+                for (const auto& dust_index : parent_entry->GetDustIndexes()) {
+                    unspent_parent_dust.insert(COutPoint(parent_txid, dust_index));
+                }                
             }
 
             processed_parent_set.insert(parent_txid);

diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
@@ -38,15 +38,16 @@
 
 /** Must be called for each transaction once transaction fees are known.
  * Does context-less checks about a single transaction.
+ * dust_indexes is cleared and populated with dust output indexes, if any.
  * Returns false if the fee is non-zero and dust exists, populating state. True otherwise.
  */
-bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_fee, CAmount base_fee, CAmount mod_fee, TxValidationState& state);
+bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_fee, CAmount base_fee, CAmount mod_fee, std::vector<uint32_t>& dust_indexes, TxValidationState& state);
 
 /** Must be called for each transaction(package) if any dust is in the package.
  *  Checks that each transaction's parents have their dust spent by the child,
  *  where parents are either in the mempool or in the package itself.
  *  The function returns std::nullopt if all dust is properly spent, or the txid of the violating child spend.
  */
-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
+std::optional<Txid> CheckEphemeralSpends(const std::vector<CTxMemPoolEntry*>& package_entries, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
 
 #endif // BITCOIN_POLICY_EPHEMERAL_POLICY_H
diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp
@@ -107,108 +107,6 @@ static inline CTransactionRef make_ephemeral_tx(const std::vector<COutPoint>& in
     return MakeTransactionRef(mtx);
 }
 
-BOOST_FIXTURE_TEST_CASE(ephemeral_tests, RegTestingSetup)
-{
-    CTxMemPool& pool = *Assert(m_node.mempool);
-    LOCK2(cs_main, pool.cs);
-    TestMemPoolEntryHelper entry;
-    CTxMemPool::setEntries empty_ancestors;
-
-    CFeeRate minrelay(1000);
-
-    // Basic transaction with dust
-    auto grandparent_tx_1 = make_ephemeral_tx(random_outpoints(1), /*version=*/2);
-    const auto dust_txid = grandparent_tx_1->GetHash();
-
-    uint32_t dust_index = 2;
-
-    // Child transaction spending dust
-    auto dust_spend = make_tx({COutPoint{dust_txid, dust_index}}, /*version=*/2);
-
-    // We first start with nothing "in the mempool", using package checks
-
-    // Trivial single transaction with no dust
-    BOOST_CHECK(!CheckEphemeralSpends({dust_spend}, minrelay, pool).has_value());
-
-    // Now with dust, ok because the tx has no dusty parents
-    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1}, minrelay, pool).has_value());
-
-    // Dust checks pass
-    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, dust_spend}, CFeeRate(0), pool).has_value());
-    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, dust_spend}, minrelay, pool).has_value());
-
-    auto dust_non_spend = make_tx({COutPoint{dust_txid, dust_index - 1}}, /*version=*/2);
-
-    // Child spending non-dust only from parent should be disallowed even if dust otherwise spent
-    BOOST_CHECK(CheckEphemeralSpends({grandparent_tx_1, dust_non_spend, dust_spend}, minrelay, pool).has_value());
-    BOOST_CHECK(CheckEphemeralSpends({grandparent_tx_1, dust_spend, dust_non_spend}, minrelay, pool).has_value());
-    BOOST_CHECK(CheckEphemeralSpends({grandparent_tx_1, dust_non_spend}, minrelay, pool).has_value());
-
-    auto grandparent_tx_2 = make_ephemeral_tx(random_outpoints(1), /*version=*/2);
-    const auto dust_txid_2 = grandparent_tx_2->GetHash();
-
-    // Spend dust from one but not another is ok, as long as second grandparent has no child
-    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, dust_spend}, minrelay, pool).has_value());
-
-    auto dust_non_spend_both_parents = make_tx({COutPoint{dust_txid, dust_index}, COutPoint{dust_txid_2, dust_index - 1}}, /*version=*/2);
-    // But if we spend from the parent, it must spend dust
-    BOOST_CHECK(CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, dust_non_spend_both_parents}, minrelay, pool).has_value());
-
-    auto dust_spend_both_parents = make_tx({COutPoint{dust_txid, dust_index}, COutPoint{dust_txid_2, dust_index}}, /*version=*/2);
-    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, dust_spend_both_parents}, minrelay, pool).has_value());
-
-    // Spending other outputs is also correct, as long as the dusty one is spent
-    const std::vector<COutPoint> all_outpoints{COutPoint(dust_txid, 0), COutPoint(dust_txid, 1), COutPoint(dust_txid, 2),
-        COutPoint(dust_txid_2, 0), COutPoint(dust_txid_2, 1), COutPoint(dust_txid_2, 2)};
-    auto dust_spend_all_outpoints = make_tx(all_outpoints, /*version=*/2);
-    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, dust_spend_all_outpoints}, minrelay, pool).has_value());
-
-    // 2 grandparents with dust <- 1 dust-spending parent with dust <- child with no dust
-    auto parent_with_dust = make_ephemeral_tx({COutPoint{dust_txid, dust_index}, COutPoint{dust_txid_2, dust_index}}, /*version=*/2);
-    // Ok for parent to have dust
-    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, parent_with_dust}, minrelay, pool).has_value());
-    auto child_no_dust = make_tx({COutPoint{parent_with_dust->GetHash(), dust_index}}, /*version=*/2);
-    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, parent_with_dust, child_no_dust}, minrelay, pool).has_value());
-
-    // 2 grandparents with dust <- 1 dust-spending parent with dust <- child with dust
-    auto child_with_dust = make_ephemeral_tx({COutPoint{parent_with_dust->GetHash(), dust_index}}, /*version=*/2);
-    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1, grandparent_tx_2, parent_with_dust, child_with_dust}, minrelay, pool).has_value());
-
-    // Tests with parents in mempool
-
-    // Empty ancestors means this should never fail since it only checks parents' dust
-    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_1}, minrelay, pool).has_value());
-
-    // Add first grandparent to mempool and fetch entry
-    pool.addUnchecked(entry.FromTx(grandparent_tx_1));
-
-    // Ignores ancestors that aren't direct parents
-    BOOST_CHECK(!CheckEphemeralSpends({child_no_dust}, minrelay, pool).has_value());
-
-    // Valid spend of dust with grandparent in mempool
-    BOOST_CHECK(!CheckEphemeralSpends({parent_with_dust}, minrelay, pool).has_value());
-
-    // Second grandparent in same package
-    BOOST_CHECK(!CheckEphemeralSpends({parent_with_dust, grandparent_tx_2}, minrelay, pool).has_value());
-    // Order in package doesn't matter
-    BOOST_CHECK(!CheckEphemeralSpends({grandparent_tx_2, parent_with_dust}, minrelay, pool).has_value());
-
-    // Add second grandparent to mempool
-    pool.addUnchecked(entry.FromTx(grandparent_tx_2));
-
-    // Only spends single dust out of two direct parents
-    BOOST_CHECK(CheckEphemeralSpends({dust_non_spend_both_parents}, minrelay, pool).has_value());
-
-    // Spends both parents' dust
-    BOOST_CHECK(!CheckEphemeralSpends({parent_with_dust}, minrelay, pool).has_value());
-
-    // Now add dusty parent to mempool
-    pool.addUnchecked(entry.FromTx(parent_with_dust));
-
-    // Passes dust checks even with non-parent ancestors
-    BOOST_CHECK(!CheckEphemeralSpends({child_no_dust}, minrelay, pool).has_value());
-}
-
 BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup)
 {
     // Test TRUC policy helper functions

diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
@@ -560,7 +560,7 @@ std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContex
             LockPoints lp;
             m_node.mempool->addUnchecked(CTxMemPoolEntry(ptx, /*fee=*/(total_in - num_outputs * amount_per_output),
                                                          /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0,
-                                                         /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
+                                                         /*spends_coinbase=*/false, /*dust_indexes=*/{}, /*sigops_cost=*/4, lp));
         }
         --num_transactions;
     }
@@ -590,7 +590,7 @@ void TestChain100Setup::MockMempoolMinFee(const CFeeRate& target_feerate)
         m_node.mempool->m_opts.incremental_relay_feerate.GetFee(GetVirtualTransactionSize(*tx));
     m_node.mempool->addUnchecked(CTxMemPoolEntry(tx, /*fee=*/tx_fee,
                                                  /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0,
-                                                 /*spends_coinbase=*/true, /*sigops_cost=*/1, lp));
+                                                 /*spends_coinbase=*/true, /*dust_indexes=*/{}, /*sigops_cost=*/1, lp));
     m_node.mempool->TrimToSize(0);
     assert(m_node.mempool->GetMinFee() == target_feerate);
 }

diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp
@@ -37,7 +37,7 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction& tx) co
 
 CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CTransactionRef& tx) const
 {
-    return CTxMemPoolEntry{tx, nFee, TicksSinceEpoch<std::chrono::seconds>(time), nHeight, m_sequence, spendsCoinbase, sigOpCost, lp};
+    return CTxMemPoolEntry{tx, nFee, TicksSinceEpoch<std::chrono::seconds>(time), nHeight, m_sequence, spendsCoinbase, m_dust_indexes, sigOpCost, lp};
 }
 
 std::optional<std::string> CheckPackageMempoolAcceptResult(const Package& txns,

diff --git a/src/test/util/txmempool.h b/src/test/util/txmempool.h
@@ -24,6 +24,7 @@ struct TestMemPoolEntryHelper {
     uint64_t m_sequence{0};
     bool spendsCoinbase{false};
     unsigned int sigOpCost{4};
+    std::vector<uint32_t> m_dust_indexes;
     LockPoints lp;
 
     CTxMemPoolEntry FromTx(const CMutableTransaction& tx) const;

diff --git a/src/validation.cpp b/src/validation.cpp
@@ -928,20 +928,21 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
         }
     }
 
-    // Set entry_sequence to 0 when bypass_limits is used; this allows txs from a block
-    // reorg to be marked earlier than any child txs that were already in the mempool.
-    const uint64_t entry_sequence = bypass_limits ? 0 : m_pool.GetSequence();
-    entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence,
-                                    fSpendsCoinbase, nSigOpsCost, lock_points.value()));
-    ws.m_vsize = entry->GetTxSize();
-
     // Enforces 0-fee for dust transactions, no incentive to be mined alone
+    std::vector<uint32_t> dust_indexes;
     if (m_pool.m_opts.require_standard) {
-        if (!CheckValidEphemeralTx(ptx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, ws.m_modified_fees, state)) {
+        if (!CheckValidEphemeralTx(ptx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, ws.m_modified_fees, dust_indexes, state)) {
             return false; // state filled in by CheckValidEphemeralTx
         }
     }
 
+    // Set entry_sequence to 0 when bypass_limits is used; this allows txs from a block
+    // reorg to be marked earlier than any child txs that were already in the mempool.
+    const uint64_t entry_sequence = bypass_limits ? 0 : m_pool.GetSequence();
+    entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, m_active_chainstate.m_chain.Height(), entry_sequence,
+                                    fSpendsCoinbase, dust_indexes, nSigOpsCost, lock_points.value()));
+    ws.m_vsize = entry->GetTxSize();
+
     if (nSigOpsCost > MAX_STANDARD_TX_SIGOPS_COST)
         return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "bad-txns-too-many-sigops",
                 strprintf("%d", nSigOpsCost));
@@ -1463,7 +1464,9 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
     }
 
     if (m_pool.m_opts.require_standard) {
-        if (const auto ephemeral_violation{CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
+        std::vector<CTxMemPoolEntry*> package_entries;
+        package_entries.push_back(ws.m_entry.get());
+        if (const auto ephemeral_violation{CheckEphemeralSpends(package_entries, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
             const Txid& txid = ephemeral_violation.value();
             Assume(txid == ptx->GetHash());
             ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
@@ -1612,7 +1615,10 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
 
     // Now that we've bounded the resulting possible ancestry count, check parents for dust spends
     if (m_pool.m_opts.require_standard) {
-        if (const auto ephemeral_violation{CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
+        std::vector<CTxMemPoolEntry*> package_entries;
+        for (const auto& ws : workspaces) package_entries.push_back(ws.m_entry.get());
+
+        if (const auto ephemeral_violation{CheckEphemeralSpends(package_entries, m_pool.m_opts.dust_relay_feerate, m_pool)}) {
             const Txid& parent_txid = ephemeral_violation.value();
             TxValidationState child_state;
             child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",