Merge bitcoin#31279: policy: ephemeral dust followups

466e4df assert_mempool_contents: assert not duplicates expected (Greg Sanders) ea5db2f functional: only generate required blocks for test (Greg Sanders) d033acb fuzz: package_eval: let fuzzer run out input in main tx creation loop (Greg Sanders) ba35a57 CheckEphemeralSpends: return boolean, and set child state and txid outparams (Greg Sanders) cf0cee1 func: add note about lack of 1P1C propagation in tree submitpackage (Greg Sanders) 8424290 unit test: ephemeral_tests is using a dust relay rate, not minrelay (Greg Sanders) d9cfa5f CheckEphemeralSpends: no need to iterate inputs if no parent dust (Greg Sanders) 87b26e3 func: rename test_free_relay to test_no_minrelay_fee (Greg Sanders) e5709a4 func: slight elaboration on submitpackage restriction (Greg Sanders) 08e969b RPC: only enforce dust rules on priority when standardness active (Greg Sanders) ca050d1 unit test: adapt to changing MAX_DUST_OUTPUTS_PER_TX (Greg Sanders) 7c34901 fuzz: package_eval: move last_tx inside txn ctor (Greg Sanders) 445eaed fuzz: use optional status instead of should_rbf_eph_spend (Greg Sanders) 4dfdf61 fuzz: remove unused TransactionsDelta validation interface (Greg Sanders) 09ce926 func: cleanup reorg test comment (Greg Sanders) 768a0c1 func: cleanup test_dustrelay comments (Greg Sanders) bedca1c fuzz: Directly place transactions in vector (Greg Sanders) c041ad6 fuzz: explain package eval coin tracking better (Greg Sanders) bc0d98e fuzz: remove dangling reference to GetEntry (Greg Sanders) 15b6cbf unit test: make dust index less magical (Greg Sanders) 5fbcfd1 unit test: assert txid returned on CheckEphemeralSpends failures (Greg Sanders) ef94d84 bench: remove unnecessary CMTxn constructors (Greg Sanders) c5c10fd ephemeral policy doxygen cleanup (Greg Sanders) dd9044b ephemeral policy: IWYU (Greg Sanders) c6859ce Move+rename GetDustIndexes -> GetDust (Greg Sanders) 62016b3 Use std::ranges for ephemeral policy checks (Greg Sanders) 3ed930a Have HasDust and PreCheckValidEphemeralTx take CTransaction (Greg Sanders) 04a614b Rename CheckValidEphemeralTx to PreCheckEphemeralTx (Greg Sanders) cbf1a47 CheckEphemeralSpends: only compute txid of tx when needed (Greg Sanders) Pull request description: Follow-up to bitcoin#30239 Here are the parent PR's comments that should be addressed by this PR: https://github.com/bitcoin/bitcoin/pull/30239/files#r1834529646 https://github.com/bitcoin/bitcoin/pull/30239/files#r1831247308 https://github.com/bitcoin/bitcoin/pull/30239/files#r1832622481 https://github.com/bitcoin/bitcoin/pull/30239/files#r1831195216 bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) bitcoin#30239 (comment) ACKs for top commit: naumenkogs: ACK 466e4df hodlinator: ACK 466e4df theStack: lgtm ACK 466e4df glozow: utACK 466e4df Tree-SHA512: 89106f695755c238b84e0996b89446c0733e10a94c867f656d516d26697d2efe38dfc332188b8589a0a26a3d2bd2c88c6ab70c108e187ce5bfcb91bbf3fb0391
AreaLayer · Nov 25, 2024 · f7144b2 · f7144b2
2 parents 2638fdb + 466e4df
commit f7144b2
Show file tree

Hide file tree

Showing 15 changed files with 210 additions and 146 deletions.
diff --git a/src/bench/mempool_ephemeral_spends.cpp b/src/bench/mempool_ephemeral_spends.cpp
@@ -44,7 +44,7 @@ static void MempoolCheckEphemeralSpends(benchmark::Bench& bench)
     }
 
     // Tx with many outputs
-    CMutableTransaction tx1 = CMutableTransaction();
+    CMutableTransaction tx1;
     tx1.vin.resize(1);
     tx1.vout.resize(number_outputs);
     for (size_t i = 0; i < tx1.vout.size(); i++) {
@@ -56,7 +56,7 @@ static void MempoolCheckEphemeralSpends(benchmark::Bench& bench)
     const auto& parent_txid = tx1.GetHash();
 
     // Spends all outputs of tx1, other details don't matter
-    CMutableTransaction tx2 = CMutableTransaction();
+    CMutableTransaction tx2;
     tx2.vin.resize(tx1.vout.size());
     for (size_t i = 0; i < tx2.vin.size(); i++) {
         tx2.vin[0].prevout.hash = parent_txid;
@@ -74,9 +74,12 @@ static void MempoolCheckEphemeralSpends(benchmark::Bench& bench)
 
     uint32_t iteration{0};
 
+    TxValidationState dummy_state;
+    Txid dummy_txid;
+
     bench.run([&]() NO_THREAD_SAFETY_ANALYSIS {
 
-        CheckEphemeralSpends({tx2_r}, /*dust_relay_rate=*/CFeeRate(iteration * COIN / 10), pool);
+        CheckEphemeralSpends({tx2_r}, /*dust_relay_rate=*/CFeeRate(iteration * COIN / 10), pool, dummy_state, dummy_txid);
         iteration++;
     });
 }

diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
@@ -2,29 +2,39 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <consensus/validation.h>
 #include <policy/ephemeral_policy.h>
+#include <policy/feerate.h>
+#include <policy/packages.h>
 #include <policy/policy.h>
+#include <primitives/transaction.h>
+#include <txmempool.h>
+#include <util/check.h>
+#include <util/hasher.h>
 
-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
-{
-    return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
-}
+#include <algorithm>
+#include <cstdint>
+#include <map>
+#include <memory>
+#include <unordered_set>
+#include <utility>
+#include <vector>
 
-bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
+bool PreCheckEphemeralTx(const CTransaction& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
 {
     // We never want to give incentives to mine this transaction alone
-    if ((base_fee != 0 || mod_fee != 0) && HasDust(tx, dust_relay_rate)) {
+    if ((base_fee != 0 || mod_fee != 0) && !GetDust(tx, dust_relay_rate).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)
+bool CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool, TxValidationState& out_child_state, Txid& out_child_txid)
 {
-    if (!Assume(std::all_of(package.cbegin(), package.cend(), [](const auto& tx){return tx != nullptr;}))) {
+    if (!Assume(std::ranges::all_of(package, [](const auto& tx){return tx != nullptr;}))) {
         // Bail out of spend checks if caller gave us an invalid package
-        return std::nullopt;
+        return true;
     }
 
     std::map<Txid, CTransactionRef> map_txid_ref;
@@ -33,7 +43,6 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
     }
 
     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;
 
@@ -63,16 +72,23 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
             processed_parent_set.insert(parent_txid);
         }
 
+        if (unspent_parent_dust.empty()) {
+            continue;
+        }
+
         // Now that we have gathered parents' dust, make sure it's spent
         // by the child
         for (const auto& tx_input : tx->vin) {
             unspent_parent_dust.erase(tx_input.prevout);
         }
 
         if (!unspent_parent_dust.empty()) {
-            return txid;
+            out_child_txid = tx->GetHash();
+            out_child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
+                                strprintf("tx %s did not spend parent's ephemeral dust", out_child_txid.ToString()));
+            return false;
         }
     }
 
-    return std::nullopt;
+    return true;
 }
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
@@ -5,17 +5,22 @@
 #ifndef BITCOIN_POLICY_EPHEMERAL_POLICY_H
 #define BITCOIN_POLICY_EPHEMERAL_POLICY_H
 
+#include <consensus/amount.h>
 #include <policy/packages.h>
-#include <policy/policy.h>
 #include <primitives/transaction.h>
-#include <txmempool.h>
+
+#include <optional>
+
+class CFeeRate;
+class CTxMemPool;
+class TxValidationState;
 
 /** These utility functions ensure that ephemeral dust is safely
  * created and spent without unduly risking them entering the utxo
  * set.
 
  * This is ensured by requiring:
- * - CheckValidEphemeralTx checks are respected
+ * - PreCheckEphemeralTx checks are respected
  * - The parent has no child (and 0-fee as implied above to disincentivize mining)
  * - OR the parent transaction has exactly one child, and the dust is spent by that child
  *
@@ -34,22 +39,20 @@
  * are the only way to bring fees.
  */
 
-/** Returns true if transaction contains dust */
-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate);
-
 /* All the following checks are only called if standardness rules are being applied. */
 
 /** Must be called for each transaction once transaction fees are known.
  * Does context-less checks about a single transaction.
- * Returns false if the fee is non-zero and dust exists, populating state. True otherwise.
+ * @returns false if the fee is non-zero and dust exists, populating state. True otherwise.
  */
-bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state);
+bool PreCheckEphemeralTx(const CTransaction& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, 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.
+ *  Sets out_child_state and out_child_txid on failure.
+ *  @returns true if all dust is properly spent.
  */
-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
+bool CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool, TxValidationState& out_child_state, Txid& out_child_txid);
 
 #endif // BITCOIN_POLICY_EPHEMERAL_POLICY_H
diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
@@ -67,6 +67,15 @@ bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFeeIn)
     return (txout.nValue < GetDustThreshold(txout, dustRelayFeeIn));
 }
 
+std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate)
+{
+    std::vector<uint32_t> dust_outputs;
+    for (uint32_t i{0}; i < tx.vout.size(); ++i) {
+        if (IsDust(tx.vout[i], dust_relay_rate)) dust_outputs.push_back(i);
+    }
+    return dust_outputs;
+}
+
 bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_datacarrier_bytes, TxoutType& whichType)
 {
     std::vector<std::vector<unsigned char> > vSolutions;
@@ -129,7 +138,6 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
     }
 
     unsigned int nDataOut = 0;
-    unsigned int num_dust_outputs{0};
     TxoutType whichType;
     for (const CTxOut& txout : tx.vout) {
         if (!::IsStandard(txout.scriptPubKey, max_datacarrier_bytes, whichType)) {
@@ -142,13 +150,11 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
         else if ((whichType == TxoutType::MULTISIG) && (!permit_bare_multisig)) {
             reason = "bare-multisig";
             return false;
-        } else if (IsDust(txout, dust_relay_fee)) {
-            num_dust_outputs++;
         }
     }
 
     // Only MAX_DUST_OUTPUTS_PER_TX dust is permitted(on otherwise valid ephemeral dust)
-    if (num_dust_outputs > MAX_DUST_OUTPUTS_PER_TX) {
+    if (GetDust(tx, dust_relay_fee).size() > MAX_DUST_OUTPUTS_PER_TX) {
         reason = "dust";
         return false;
     }

diff --git a/src/policy/policy.h b/src/policy/policy.h
@@ -131,6 +131,8 @@ bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFee);
 
 bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_datacarrier_bytes, TxoutType& whichType);
 
+/** Get the vout index numbers of all dust outputs */
+std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate);
 
 // Changing the default transaction version requires a two step process: first
 // adapting relay policy by bumping TX_MAX_STANDARD_VERSION, and then later

diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
@@ -496,7 +496,7 @@ static RPCHelpMan prioritisetransaction()
 
     // Non-0 fee dust transactions are not allowed for entry, and modification not allowed afterwards
     const auto& tx = mempool.get(hash);
-    if (tx && HasDust(tx, mempool.m_opts.dust_relay_feerate)) {
+    if (mempool.m_opts.require_standard && tx && !GetDust(*tx, mempool.m_opts.dust_relay_feerate).empty()) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is not supported for transactions with dust outputs.");
     }