Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions doc/release-notes-30239.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
P2P and network changes
-----------------------

Ephemeral dust is a new concept that allows a single
dust output in a transaction, provided the transaction
is zero fee. In order to spend any unconfirmed outputs
from this transaction, the spender must also spend
this dust in addition to any other outputs.

In other words, this type of transaction
should be created in a transaction package where
the dust is both created and spent simultaneously.
1 change: 1 addition & 0 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ add_library(bitcoin_node STATIC EXCLUDE_FROM_ALL
node/utxo_snapshot.cpp
node/warnings.cpp
noui.cpp
policy/ephemeral_policy.cpp
policy/fees.cpp
policy/fees_args.cpp
policy/packages.cpp
Expand Down
1 change: 1 addition & 0 deletions src/bench/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ add_executable(bench_bitcoin
load_external.cpp
lockedpool.cpp
logging.cpp
mempool_ephemeral_spends.cpp
mempool_eviction.cpp
mempool_stress.cpp
merkle_root.cpp
Expand Down
83 changes: 83 additions & 0 deletions src/bench/mempool_ephemeral_spends.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
// Copyright (c) 2011-2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <bench/bench.h>
#include <consensus/amount.h>
#include <kernel/cs_main.h>
#include <policy/ephemeral_policy.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
#include <script/script.h>
#include <sync.h>
#include <test/util/setup_common.h>
#include <txmempool.h>
#include <util/check.h>

#include <cstdint>
#include <memory>
#include <vector>


static void AddTx(const CTransactionRef& tx, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
{
int64_t nTime{0};
unsigned int nHeight{1};
uint64_t sequence{0};
bool spendsCoinbase{false};
unsigned int sigOpCost{4};
uint64_t fee{0};
LockPoints lp;
pool.addUnchecked(CTxMemPoolEntry(
tx, fee, nTime, nHeight, sequence,
spendsCoinbase, sigOpCost, lp));
}

static void MempoolCheckEphemeralSpends(benchmark::Bench& bench)
{
const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();

int number_outputs{1000};
if (bench.complexityN() > 1) {
number_outputs = static_cast<int>(bench.complexityN());
}

// Tx with many outputs
CMutableTransaction tx1 = CMutableTransaction();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and for tx2:

Suggested change
CMutableTransaction tx1 = CMutableTransaction();
CMutableTransaction tx1;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe touch in follow-up

tx1.vin.resize(1);
tx1.vout.resize(number_outputs);
for (size_t i = 0; i < tx1.vout.size(); i++) {
tx1.vout[i].scriptPubKey = CScript();
// Each output progressively larger
tx1.vout[i].nValue = i * CENT;
}

const auto& parent_txid = tx1.GetHash();

// Spends all outputs of tx1, other details don't matter
CMutableTransaction tx2 = CMutableTransaction();
tx2.vin.resize(tx1.vout.size());
for (size_t i = 0; i < tx2.vin.size(); i++) {
tx2.vin[0].prevout.hash = parent_txid;
tx2.vin[0].prevout.n = i;
}
tx2.vout.resize(1);

CTxMemPool& pool = *Assert(testing_setup->m_node.mempool);
LOCK2(cs_main, pool.cs);
// Create transaction references outside the "hot loop"
const CTransactionRef tx1_r{MakeTransactionRef(tx1)};
const CTransactionRef tx2_r{MakeTransactionRef(tx2)};

AddTx(tx1_r, pool);

uint32_t iteration{0};

bench.run([&]() NO_THREAD_SAFETY_ANALYSIS {

CheckEphemeralSpends({tx2_r}, /*dust_relay_rate=*/CFeeRate(iteration * COIN / 10), pool);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the diff:

diff --git a/src/bench/mempool_ephemeral_spends.cpp b/src/bench/mempool_ephemeral_spends.cpp
index e867c61752..dc5e577caf 100644
--- a/src/bench/mempool_ephemeral_spends.cpp
+++ b/src/bench/mempool_ephemeral_spends.cpp
@@ -72,12 +72,16 @@ static void MempoolCheckEphemeralSpends(benchmark::Bench& bench)
     AddTx(tx1_r, pool);
 
     uint32_t iteration{0};
+    uint32_t failure{0};
 
     bench.run([&]() NO_THREAD_SAFETY_ANALYSIS {
 
-        CheckEphemeralSpends({tx2_r}, /*dust_relay_rate=*/CFeeRate(iteration * COIN / 10), pool);
+        if (CheckEphemeralSpends({tx2_r}, /*dust_relay_rate=*/CFeeRate(iteration * COIN / 10), pool))
+            ++failure;
         iteration++;
     });
+
+    printf("success: %d, failure: %d\n", iteration - failure, failure);
 }
 
 BENCHMARK(MempoolCheckEphemeralSpends, benchmark::PriorityLevel::HIGH);

The result was:

₿ build/src/bench/bench_bitcoin -filter=MempoolCheckEphemeralSpends -min-time=30000
|               ns/op |                op/s |    err% |          ins/op |          cyc/op |    IPC |         bra/op |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|          234,976.30 |            4,255.75 |    0.4% |    2,384,033.36 |      859,700.17 |  2.773 |     178,732.26 |    1.3% |     32.29 | `MempoolCheckEphemeralSpends`
success: 1, failure: 138329

Is the intention that we only succeed on iteration 0? Could maybe initialize the iteration to 1 and change the feerate expression, making sure it still succeeds at least once?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't effect the complexity much, as the cost is scanning all the parents' outputs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider benching with the parent in package? I suppose it's about the same.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be the same, or even better since it's not being fetched from the mempool?

iteration++;
});
}

BENCHMARK(MempoolCheckEphemeralSpends, benchmark::PriorityLevel::HIGH);
1 change: 1 addition & 0 deletions src/kernel/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ add_library(bitcoinkernel
../node/blockstorage.cpp
../node/chainstate.cpp
../node/utxo_snapshot.cpp
../policy/ephemeral_policy.cpp
../policy/feerate.cpp
../policy/packages.cpp
../policy/policy.cpp
Expand Down
78 changes: 78 additions & 0 deletions src/policy/ephemeral_policy.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// Copyright (c) 2024-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <policy/ephemeral_policy.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: includes are not quite up to date (in both files)

git diff on d5c564a24f to make iwyu happy
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..3a9a3fa530 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,6 +5,22 @@
 #include <policy/ephemeral_policy.h>
 #include <policy/policy.h>
 
+#include <consensus/validation.h>
+#include <policy/feerate.h>
+#include <policy/packages.h>
+#include <primitives/transaction.h>
+#include <txmempool.h>
+#include <util/check.h>
+#include <util/hasher.h>
+
+#include <algorithm>
+#include <cstdint>
+#include <map>
+#include <memory>
+#include <unordered_set>
+#include <utility>
+#include <vector>
+
 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); });
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
index 26140f9a02..742cb30280 100644
--- a/src/policy/ephemeral_policy.h
+++ b/src/policy/ephemeral_policy.h
@@ -5,10 +5,15 @@
 #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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will touch in follow-up

#include <policy/policy.h>

bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This function is independent of the ephemeral nature and is generic enough to lie outside this file, near where IsDust() is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leaving as-is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In d5c564a24f96ec384b2de68bf87e6eb3ad9239d2

related: the code could be simplified a bit (imo) by changing HasDust to GetDust, which would require to move it to policy.h

git diff on d5c564a24f
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..6066d9b3ac 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,15 +5,10 @@
 #include <policy/ephemeral_policy.h>
 #include <policy/policy.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); });
-}
-
 bool CheckValidEphemeralTx(const CTransactionRef& 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");
     }
 
@@ -52,11 +47,8 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
 
             // 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));
-                    }
+                for (const auto& out_index : GetDust(*parent_ref, dust_relay_rate)) {
+                    unspent_parent_dust.insert(COutPoint{parent_txid, out_index});
                 }
             }
 
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
index 26140f9a02..98f40d38ff 100644
--- a/src/policy/ephemeral_policy.h
+++ b/src/policy/ephemeral_policy.h
@@ -34,9 +34,6 @@
  * 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.
diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
index 21c35af5cc..ed33692823 100644
--- 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
index 0488f8dbee..9e36d3f610 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -129,6 +129,9 @@ CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFee);
 
 bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFee);
 
+/** Get the vout index numbers of all dust outputs */
+std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate);
+
 bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_datacarrier_bytes, TxoutType& whichType);
 
 

That said, the current code is fine too, and I suspect you'll find this too big of a change for a PR of this size that otherwise seems close to merge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will handle on follow-up

{
return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In d5c564a24f96ec384b2de68bf87e6eb3ad9239d2:

nit: could make this a bit nicer with std::ranges:

git diff on d5c564a24f
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..569f9a95b5 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -7,7 +7,7 @@
 
 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); });
+    return std::ranges::any_of(tx->vout, [&](const auto& output) { return IsDust(output, dust_relay_rate); });
 }
 
 bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
@@ -22,7 +22,7 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
 
 std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
 {
-    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;
     }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will touch on followup

}

bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for these to be const CTransactionRef& instead of const CTransaction&? The latter would avoid nullptr issues as well as improve reusability.

git diff on d5c564a24f
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..c9f1ba076d 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,12 +5,12 @@
 #include <policy/ephemeral_policy.h>
 #include <policy/policy.h>
 
-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate)
+bool HasDust(const CTransaction& tx, CFeeRate dust_relay_rate)
 {
-    return std::any_of(tx->vout.cbegin(), tx->vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
+    return std::any_of(tx.vout.cbegin(), tx.vout.cend(), [&](const auto& output) { return IsDust(output, dust_relay_rate); });
 }
 
-bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate, CAmount base_fee, CAmount mod_fee, TxValidationState& state)
+bool CheckValidEphemeralTx(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)) {
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
index 26140f9a02..10a5edf337 100644
--- a/src/policy/ephemeral_policy.h
+++ b/src/policy/ephemeral_policy.h
@@ -35,7 +35,7 @@
  */
 
 /** Returns true if transaction contains dust */
-bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate);
+bool HasDust(const CTransaction& tx, CFeeRate dust_relay_rate);
 
 /* All the following checks are only called if standardness rules are being applied. */
 
@@ -43,7 +43,7 @@ bool HasDust(const CTransactionRef& tx, CFeeRate dust_relay_rate);
  * Does context-less checks about a single transaction.
  * 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 CheckValidEphemeralTx(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,
diff --git a/src/validation.cpp b/src/validation.cpp
index 2f3e7d61a8..8387a7f971 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -930,7 +930,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 
     // Enforces 0-fee for dust transactions, no incentive to be mined alone
     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(tx, m_pool.m_opts.dust_relay_feerate, ws.m_base_fees, ws.m_modified_fees, state)) {
             return false; // state filled in by CheckValidEphemeralTx
         }
     }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will touch on follow-up

{
// We never want to give incentives to mine this transaction alone
if ((base_fee != 0 || mod_fee != 0) && HasDust(tx, dust_relay_rate)) {
return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0-fee");
}

return true;
}
Comment on lines +13 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ephemeral dust is a new concept that allows a single
dust output in a transaction, provided the transaction
is zero fee.

As per this definition, shouldn't this function also check that there is only one dust output in the transaction? I can see it's checked in the IsStandardTx() but IMO this function that should check the complete validity of the ephemeral transaction should encapsulate this single dust output check as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an implementation perspective it was cleaner to enforce it in IsStandardTx, and I'm not sure I see the value in doubling up enforcement of it vs a single location.

If we already had fee information by the time we would call IsStandardTx it might be even cleaner, but I expect it would be a very difficult to evaluate diff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could rename it PreCheckValidEphemeralTx to signify that it doesn't perform full validation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, will rename in follow-up


std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is a pretty neat function that I enjoyed going through!

{
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, CTransactionRef> map_txid_ref;
for (const auto& tx : package) {
map_txid_ref[tx->GetHash()] = tx;
}

for (const auto& tx : package) {
Txid txid = tx->GetHash();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const reference would be better

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: txid is only used once in the return statement at the bottom of the loop, so could skip creating this variable...

It seems to be introduced in response to #30239 (comment) - arguably we should probably tolerate GetHash() until the day it is renamed/aliased to GetTxid(). Lifetime is unnecessarily long as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably used it more than once before, will remove in follow-up

std::unordered_set<Txid, SaltedTxidHasher> processed_parent_set;
std::unordered_set<COutPoint, SaltedOutpointHasher> unspent_parent_dust;

for (const auto& tx_input : tx->vin) {
const Txid& parent_txid{tx_input.prevout.hash};
// Skip parents we've already checked dust for
if (processed_parent_set.contains(parent_txid)) continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Since we unconditionally insert at the end of the loop, we might as well insert up here instead and check if it already existed in the set.

Suggested change
if (processed_parent_set.contains(parent_txid)) continue;
if (!processed_parent_set.insert(parent_txid).second) continue;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be heretical but I find as-is easier to read? Can add to follow-up if demand is there.


// We look for an in-package or in-mempool dependency
CTransactionRef parent_ref = nullptr;
if (auto it = map_txid_ref.find(parent_txid); it != map_txid_ref.end()) {
parent_ref = it->second;
} else {
parent_ref = tx_pool.get(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));
}
Comment on lines +58 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it seems correct to run the loop for the outputs of the parent transaction but as per the definition of Ephemeral Dust, we can break (and end) the loop here after the dust is found because only one is allowed per parent transaction?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it would make the logic simpler or significantly more performant, and would rather the IsStandardTx check not be so tightly bound.

}
}

processed_parent_set.insert(parent_txid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something simple, but could this line insert the parent txid into processed_parent_set even when the parent couldn't be checked for dust (parent_ref is nullptr)? If so, would we want to handle this as an error condition?

Maybe it's not an issue currently (e.g. reliance on 1P1C), but could be defensive to check to protect in the event of future change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not in mempool or the package, then it can't have unconfirmed dust. Is there a scenario you're concerned about?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just focused on handling edge cases (and preventing harder to notice issues later). Not sure if there's a scenario involving orphaning that might play out (haven't thought through all scenarios).

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this is much easier to follow with the codepaths merged, thanks. However on further review, I am concerned about how much work we might do in this function.

As written, if a transaction had 1000 inputs, which all spend from a single parent with 1000 outputs, then we'd do 1M loop iterations, because we're not deduplicating parents for each input of a transaction (so for each input of the child, we'd be looking at all 1000 outputs of the parent). If we deduplicate the parents first, then the number of parent outputs we might possibly have to look at is bounded by the ancestor size limit (or the cluster size limit, in the future), to something on the order of a few thousand outputs.

If we are relying on the ancestor size limits for a work bound, then we should make sure that all those limits are checked prior to this function being invoked. I think in the package acceptance case, that would involve moving this check to come after the CheckPackageLimits() check that happens in AcceptMultipleTransactions.

I think this might be enough, but we could consider going further -- what if we were to cache how many dust outputs a transaction has in the CTxMemPoolEntry itself? Then we would be reduced to just looping over the inputs of the incoming transactions and counting, for each distinct parent, how many of the dust outputs are spent. This is probably not simple to do at the moment (since in the package case, we don't have CTxMemPoolEntry objects to work on yet), but I have some refactors in mind for mempool acceptance which would make this easier to consider in the future.

Anyway, it might be nice to add a micro-benchmark for transaction validation of a single transaction with 1000 inputs, which all come from a single parent with 1000 outputs, just to make sure we're not slowing that case down inordinately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't have CTxMemPoolEntry objects to work on yet

I don't think that's true? PreChecks has built a mempool entry for each package tx. We could track the exact outpoint that is dust in the mempool entry. I'll give that a shot.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdaftuar this should do it if you think something like this is reasonable: instagibbs@6007146

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I I think that approach uses an extra 24 bytes per mempool entry, which strikes me as a bit heavy for what we need. Let's see if this is fast enough as-is and if so, call it a day.

Copy link
Member Author

@instagibbs instagibbs Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative is to store a bool, and use that to filter 99.9% of transactions that won't have dust, and then eat the O(num_outputs) cost to find the dust when at spending time.

With 1000 inputs/outputs and a small number of dust outputs, I get:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           60,600.53 |           16,501.51 |    0.7% |      0.01 | `MempoolCheckEphemeralSpends`

When I make all the outputs dust(which can't happen currently due to IsStandard):

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           75,160.69 |           13,304.83 |    0.6% |      1.07 | `MempoolCheckEphemeralSpends`

number of outputs/inputs can be varied in the benchmark via -asymptote=X

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be fine for performance. Large transactions take orders of magnitude more time to validate than what is being added here (and for small transactions this is extremely fast).


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could speed up the common case a bit by checking unspent_parent_dust.empty() here, but perhaps not significantly

// 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;
}
}

return std::nullopt;
}
55 changes: 55 additions & 0 deletions src/policy/ephemeral_policy.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright (c) 2024-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_POLICY_EPHEMERAL_POLICY_H
#define BITCOIN_POLICY_EPHEMERAL_POLICY_H

#include <policy/packages.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
#include <txmempool.h>

/** 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
* - 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
*
* Imagine three transactions:
* TxA, 0-fee with two outputs, one non-dust, one dust
* TxB, spends TxA's non-dust
* TxC, spends TxA's dust
*
* All the dust is spent if TxA+TxB+TxC is accepted, but the mining template may just pick
* up TxA+TxB rather than the three "legal configurations:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing closing double quote

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fixup if I end up touching things

* 1) None
* 2) TxA+TxB+TxC
* 3) TxA+TxC
* By requiring the child transaction to sweep any dust from the parent txn, we ensure that
* there is a single child only, and this child, or the child's descendants,
* 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.
*/
bool CheckValidEphemeralTx(const CTransactionRef& 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.
*/
std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning a falsy value for a successful check is an antipattern. This would be nicely addressed by #25665, but until that's merged I think I'd prefer returning a std::pair<bool, std::optional<Txid>>, even if it's a bit more verbose:

git diff on d5c564a24f
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..10a8ab1938 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -5,6 +5,8 @@
 #include <policy/ephemeral_policy.h>
 #include <policy/policy.h>
 
+#include <utility>
+
 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); });
@@ -20,11 +22,11 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
     return true;
 }
 
-std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool)
+std::pair<bool, std::optional<Txid>> CheckEphemeralSpends(const Package& package, 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;
+        return {true, std::nullopt};
     }
 
     std::map<Txid, CTransactionRef> map_txid_ref;
@@ -70,9 +72,9 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
         }
 
         if (!unspent_parent_dust.empty()) {
-            return txid;
+            return {false, txid};
         }
     }
 
-    return std::nullopt;
+    return {true, std::nullopt};
 }
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
index 26140f9a02..7235dd7500 100644
--- a/src/policy/ephemeral_policy.h
+++ b/src/policy/ephemeral_policy.h
@@ -10,6 +10,8 @@
 #include <primitives/transaction.h>
 #include <txmempool.h>
 
+#include <utility>
+
 /** These utility functions ensure that ephemeral dust is safely
  * created and spent without unduly risking them entering the utxo
  * set.
@@ -50,6 +52,6 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
  *  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::pair<bool, std::optional<Txid>> CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, const CTxMemPool& tx_pool);
 
 #endif // BITCOIN_POLICY_EPHEMERAL_POLICY_H
diff --git a/src/validation.cpp b/src/validation.cpp
index 2f3e7d61a8..94cbdf2766 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1456,11 +1456,11 @@ 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)}) {
-            const Txid& txid = ephemeral_violation.value();
-            Assume(txid == ptx->GetHash());
+        const auto& [is_ephemeral, offending_txid] = CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool);
+        if (!is_ephemeral) {
+            Assume(offending_txid.value() == ptx->GetHash());
             ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
-                          strprintf("tx %s did not spend parent's ephemeral dust", txid.ToString()));
+                          strprintf("tx %s did not spend parent's ephemeral dust", offending_txid.value().ToString()));
             return MempoolAcceptResult::Failure(ws.m_state);
         }
     }
@@ -1605,13 +1605,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
 
     // Now that we've bounded the resulting possible ancestry count, check package 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)}) {
-            const Txid& child_txid = ephemeral_violation.value();
+        const auto& [is_ephemeral, offending_txid] = CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool);
+        if (!is_ephemeral) {
             TxValidationState child_state;
             child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
-                          strprintf("tx %s did not spend parent's ephemeral dust", child_txid.ToString()));
+                          strprintf("tx %s did not spend parent's ephemeral dust", offending_txid.value().ToString()));
             package_state.Invalid(PackageValidationResult::PCKG_TX, "unspent-dust");
-            results.emplace(child_txid, MempoolAcceptResult::Failure(child_state));
+            results.emplace(offending_txid.value(), MempoolAcceptResult::Failure(child_state));
             return PackageMempoolAcceptResult(package_state, std::move(results));
         }
     }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, returning a bool and adding a TxValidationState& out-parameter would simultaneously avoid the anti-pattern and reduce code duplication:

git diff on d5c564a24f
diff --git a/src/policy/ephemeral_policy.cpp b/src/policy/ephemeral_policy.cpp
index 6854822e35..94868f8fcb 100644
--- a/src/policy/ephemeral_policy.cpp
+++ b/src/policy/ephemeral_policy.cpp
@@ -20,11 +20,11 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
     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& child_state)
 {
     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;
+        return true;
     }
 
     std::map<Txid, CTransactionRef> map_txid_ref;
@@ -70,9 +70,11 @@ std::optional<Txid> CheckEphemeralSpends(const Package& package, CFeeRate dust_r
         }
 
         if (!unspent_parent_dust.empty()) {
-            return txid;
+            child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
+                                strprintf("tx %s did not spend parent's ephemeral dust", txid.ToString()));
+            return false;
         }
     }
 
-    return std::nullopt;
+    return true;
 }
diff --git a/src/policy/ephemeral_policy.h b/src/policy/ephemeral_policy.h
index 26140f9a02..bf2c9691a7 100644
--- a/src/policy/ephemeral_policy.h
+++ b/src/policy/ephemeral_policy.h
@@ -48,8 +48,8 @@ bool CheckValidEphemeralTx(const CTransactionRef& tx, CFeeRate dust_relay_rate,
 /** 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.
+ *  The function 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& child_state);
 
 #endif // BITCOIN_POLICY_EPHEMERAL_POLICY_H
diff --git a/src/validation.cpp b/src/validation.cpp
index 2f3e7d61a8..f6d05956ee 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1456,11 +1456,7 @@ 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)}) {
-            const Txid& txid = ephemeral_violation.value();
-            Assume(txid == ptx->GetHash());
-            ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
-                          strprintf("tx %s did not spend parent's ephemeral dust", txid.ToString()));
+        if (!CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool, ws.m_state)) {
             return MempoolAcceptResult::Failure(ws.m_state);
         }
     }
@@ -1605,13 +1601,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
 
     // Now that we've bounded the resulting possible ancestry count, check package 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)}) {
-            const Txid& child_txid = ephemeral_violation.value();
-            TxValidationState child_state;
-            child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
-                          strprintf("tx %s did not spend parent's ephemeral dust", child_txid.ToString()));
+        if (TxValidationState child_state; !CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool, child_state)) {
             package_state.Invalid(PackageValidationResult::PCKG_TX, "unspent-dust");
-            results.emplace(child_txid, MempoolAcceptResult::Failure(child_state));
+            results.emplace(txns.back()->GetHash(), MempoolAcceptResult::Failure(child_state));
             return PackageMempoolAcceptResult(package_state, std::move(results));
         }
     }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will take a crack at this in follow-up

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed a lightly modified version that also reports the child txid on failure, rather than guessing it's the ultimate tx in the package. thanks! #31279

Comment on lines +37 to +53
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The comment blocks before each function could be updated to use doxygen commands. Could be left for follow-up.

Example:

 /** 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);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will touch in follow-up


#endif // BITCOIN_POLICY_EPHEMERAL_POLICY_H
10 changes: 8 additions & 2 deletions src/policy/policy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ 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)) {
Expand All @@ -142,11 +143,16 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
reason = "bare-multisig";
return false;
} else if (IsDust(txout, dust_relay_fee)) {
reason = "dust";
return false;
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) {
reason = "dust";
return false;
}
Comment on lines +150 to +154
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, IsStandardTx() allows having only 1 dust output per tx and makes such txs technically standard but without any constraint of the 0-fee part that comes later down in the PreChecks(). Although there are no other usages of the IsStandardTx() in the source code (besides the CPP tests) but if later usages do arise, they would miss out on the 0-fee check.
TL;DR: The 0-fee check and the 1-dust output check that are tied by the Ephemeral Dust concept don't exist together within one function in code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, IsStandardTx check is very early in PreChecks, not requiring access to any utxo information. including fees.


// only one OP_RETURN txout is permitted
if (nDataOut > 1) {
reason = "multi-op-return";
Expand Down
4 changes: 4 additions & 0 deletions src/policy/policy.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ static const unsigned int MAX_OP_RETURN_RELAY = 83;
*/
static constexpr unsigned int EXTRA_DESCENDANT_TX_SIZE_LIMIT{10000};

/**
* Maximum number of ephemeral dust outputs allowed.
*/
static constexpr unsigned int MAX_DUST_OUTPUTS_PER_TX{1};
Comment on lines +80 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a underlying assumption here that this constant is used only for the ephemeral dust use case but it is not evident in the naming of this constant. Related to the previous comment on IsStandardTx().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can change to MAX_EPHEMERAL_DUST_OUTPUTS_PER_TX if I touch things


/**
* Mandatory script verification flags that all new transactions must comply with for
Expand Down
11 changes: 10 additions & 1 deletion src/rpc/mining.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <node/context.h>
#include <node/miner.h>
#include <node/warnings.h>
#include <policy/ephemeral_policy.h>
#include <pow.h>
#include <rpc/blockchain.h>
#include <rpc/mining.h>
Expand Down Expand Up @@ -491,7 +492,15 @@ static RPCHelpMan prioritisetransaction()
throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is no longer supported, dummy argument to prioritisetransaction must be 0.");
}

EnsureAnyMemPool(request.context).PrioritiseTransaction(hash, nAmount);
CTxMemPool& mempool = EnsureAnyMemPool(request.context);

// 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)) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is not supported for transactions with dust outputs.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Priority is not supported for transactions with dust outputs.

Nit: IIUC, there should not be more than 1 dust output per transaction but this reads otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is as intended. We don't allow priority on anything with dust, no matter how many non-zero dust outputs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be added to release notes for RPC? I think the only way it'd be relevant to existing usage is in a acceptnonstdtxn=1 and non-0 dustrelayfee situation. But worth noting imo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively could just check mempool.m_opts.require_standard as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, adding mempool.m_opts.require_standard to the if condition would make sense - acceptnonstdtxn should turn off all dust checks including this one imo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will add in follow-up

}
Comment on lines +499 to +501
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is good to limit dust in the UTXO-set, I'm not sure adding this limitation is wise.

If we want mining pools to use as un-patched versions of Bitcoin Core as possible, we probably shouldn't try to limit what they can do with this RPC.

IMO this commit d147d929f5093f71c39cb7efbb0dd3888f6c8114 should be split out to its own PR.

(My argument is weakened by the fact that this function already requires the transaction to have made it into the mempool in the first place).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I independently came to the same conclusion, moving my review comment here:

I feel like d147d929f5093f71c39cb7efbb0dd3888f6c8114 could just be dropped entirely? If a miner has a reason to prioritize a transaction, I don't see why they'd be okay with that not being possible just because it has a dust output? So since this is a mining RPC, I feel like practically speaking they'd just patch it out, and there'd be no real use case left for this exception, so let's just save everyone involved the hassle?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same argument should be made for not allowing modified fees too right? There's an argument to be made, but I think it's more expansive than just the RPC.

Copy link
Contributor

@hodlinator hodlinator Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean, seems you are arguing in the reverse direction or talking past?

I'm thinking the main use-case is for a miner or pool to increase priority of a transaction due to them getting paid out-of-band. While that is bad from a centralization aspect, having such a facility in unpatched versions of Bitcoin Core means miners/pools have an easier time keeping their nodes up-to-date (less moving parts), meaning they are more responsive to our releases.

Edit: (Any added hoops increase likelihood of them having to patch).

Copy link
Member Author

@instagibbs instagibbs Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean, seems you are arguing in the reverse direction or talking past?

There are two places where we disallow modified fees to take effect:

  1. prioritisetransaction, when the transaction is already in the mempool
  2. on entry to mempool, in CheckValidEphemeralTx

This ensures the invariant that unspent dust doesn't enter the utxo set ever (modulo reducing minrelay and block mining fee to 0).

We're assuming node operators/miners care about dust. If they do not, they can already set their dust relay rate to 0 and avoid all of these mechanisms entirely, ephemeral dust becomes a no-op. (e.g., MARA reportedly doesn't enforce dust limits and runs an accelerator already)

If we end up being wrong and miners are ok putting one dust output in the utxo set but not multiple or whatever, we can revisit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying when modified fees are ignored.

We're assuming node operators/miners care about dust. If they do not, they can already set their dust relay rate to 0 and avoid all of these mechanisms entirely, ephemeral dust becomes a no-op. (e.g., MARA reportedly doesn't enforce dust limits and runs an accelerator already)

If we end up being wrong and miners are ok putting one dust output in the utxo set but not multiple or whatever, we can revisit?

My assumption would be that pools are fine with helping limit dust by default, but if they get paid through their accelerator service for a specific tx, any concern over dust goes out the window. Fighting an uncooperative Bitcoin Core after they already got paid out-of-band would annoy them.

Maybe it results in them changing their configs to no longer limit dust for any transactions when they otherwise would have.


mempool.PrioritiseTransaction(hash, nAmount);
return true;
},
};
Expand Down
Loading