Ephemeral Dust #30239

In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"

nit: Use contains, here and elsewhere.

                Assume(!map_tx_dust.contains(tx->GetHash()));

done

In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"

Could you document what the return value is?

documented

In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"

I'm a little confused about this return value. The usage of this function suggests that the return value is the wtxid of a parent transaction that has unspent dust. However, this appears to be returning the child txid.

switched to txid since that's how it's being tracked

In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"

CheckEphemeralSpends appears to only return txids, not wtxid, suggest renaming this variable:

            const auto parent_txid = ephemeral_violation.value();

done

In 872c06dacde6c45c2add69c8c15c94571b08119d "policy: Allow dust in transactions, spent in-mempool"

It's not clear to me why there is a rule that a descendant must also spend the dust if they spent any other output from an ancestor. This rule does not seem to be mentioned anywhere in the PR description.

ISTM it would be easier and just as effective to check that all dust outputs end up being spent rather than this slightly convoluted check.

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: 0) None

1. TxA+TxB+TxC
2. 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 is the only transaction possible for bringing fees, or itself being spent by another child, and so on.

I can update the documentation reflecting this if you find the explanation compelling.

Ah, I see. Any child must spend the dust output, and I see that this is being enforced in the other variant too.

Additional documentation would be appreciated.

added some documentation

                return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx has more than 1 dust output");

never actually reachable as this is already caught in IsStandardTx, removed this error condition

        return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0 fee");

done

could go in test_framework/mempool_util.py?

done

        self.log.info("Test that ephemeral dust is allowed for non-0 dust values")

done

acceptnonstdtxn

done

I think all of these assert_mempool_contents can also have a self.sync_mempools() to check that the ephemeral dust packages propagate.

added an optional sync argument, only unused during reorg test

    // Now with dust, ok because the tx has no dusty parents

done

            const Txid& parent_txid = ephemeral_violation.value();

done

if you make 1 a constexpr MAX_DUST_OUTPUTS_PER_TX, you can static_assert that it == 1 as an assumption in CheckEphemeralSpends

great idea, added

    // Spending other outputs is also correct, as long as the dusty one is spent

done

Could comment that tx_input.prevout.n doesn't need to equal map_tx_dust.at(tx_input.prevout.hash); a child can spend multiple outputs of a parent.

elaborated a bit, let me know if it helps

Any reason to use a CTransaction& instead of a CTransactionRef?

nope, changed

I think this language is a little bit too strong. It's not risk-free, I think we're just checking that there is no visible incentive for the dust to enter the utxo set.

added a hedge word

 * Returns false if the fee is non-zero and dust exists, populating state. True otherwise.

taken

I don't really understand the "or itself being spent by another child, and so on" part

clarified comment, let me know if that's more clear

?

    if (txfee != 0 && std::any_of(tx.vout.cbegin(), tx.vout.cbegin(), [&](const auto& output) { return IsDust(output, dust_relay_fee); })) {
        return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "dust", "tx with dust output must be 0-fee");
    }

taken

Can you add documentation for when each function must be called, like we did for TRUC?

added a sentence to each saying when they should be called

No need to restart node1?

would cause the nodes to fail to sync their mempools, no? I find it easiest when the restarts are done in sync anyways.

This isn't really the desirable behavior, perhaps worth commenting. If we fix this through something like #27476, this should be replaced with a test for the opposite case.

Yes, if it's fixed this will fail, letting the author know to write a new case. Added a note that this isn't desired, just descriptive,

nit: sentence structure kind of weird. the transaction ignores modified? I know what you mean, but maybe confusing wording

dropped the reference to modified as it probably isn't clarifying

A little bit brittle, as it assumes the utxo selected is from a coinbase.

        assert_equal(int(COIN * dusty_tx["fee"]), sats_fee) # main output is not dust

added suggestion and a direct assertion that the main output amount is >330 the taproot dust amount

I can't remember if we discussed this in the past... we definitely don't want people to be able to bypass the rule using prioritisetransaction, but have you considered adding a CheckValidEphemeralTx(tx, m_pool.m_opts.dust_relay_feerate, ws.m_modified_fees, state) in addition to this check? Basically equivalent to "you can't prioritisetransaction at all," I think.

yes, this was my open question #30239 (comment)

Even if we do this change it can still be prioritized for mining after entering the mempool; we would need heavier machinery to stop ever treating it as prioritised. This would mean someone would have to create a CPFP transaction then cycle it away, while also using prioritisetransaction to get it mined without the dust spent? @sdaftuar you have any thoughts?

I don't have a strong view, but I'm trying to think about what a mining pool might want the behavior to be. One guess is that some miners may want to have a way to prioritise normal-looking transactions but not inadvertently prioritise something that would create a dust output and spam the UTXO set with garbage that will never be spent.

If that is the case, then we could try to build the heavier machinery you're talking about: perhaps change the behavior of prioritisetransaction to produce an error by default when invoked on an in-mempool transaction that has a dust output, coupled with what I think you guys are talking about here of rejecting transactions that are prioritised already on entry. But probably someone should talk to mining pools that offer transaction acceleration to see what they would intend and if what I just described is worth implementing; if they don't care about UTXO bloat then it's not hard to bypass these rules regardless of what we do here.

@sdaftuar

Re: dust UTXO creation, MARA has told me directly that they do not enforce the dust limit at all, and IIUC F2Pool does not as well. I haven't however personally tested this.

@petertodd I know for their slipstream accelerator at a minimum they do not, and perhaps they updated all their nodes to not enforce, I'm not sure. @sdaftuar Ok that's not much machinery, and seems to make sense combined with base and modified fee both being checked.

I think any node runner who wants to ignore dust rules they'll simply set -dustrelayfee=0, which works out of the box as expected in this PR, and even if we're more controlling on when prioritisetransaction can be called.

I pushed a commit which makes the changes and can rework history if people think the changes are agreeable. Miners can evade this by simply turning dust levels to 0, like they could before.

@instagibbs What MARA told me was that their nodes do not enforce the dust limit. Notably, that means if Libre Relay or some other implementation removed the dust limit, transactions violating it would get to MARA and be mined.

Could add a note that this is basically checking that v2 is subject to min relay feerate requirements, and you can flip this test if that changes

dropped a note

Thoughts on adding this test before the changes in commit 1?

good idea, moved this subtest to mempool_dust.py as its own commit, switched it to testing dustrelay arg instead of standardness as I think that's more directly testing what we want.

spend spend

fixed

Do we need this assert()? Can we just do anAssume() instead?

My general philosophy is that we should avoid calling assert() in codepaths that run on data received from the network, unless it really would be better for (potentially) the ENTIRE network to go down rather than just continue, in the event that the assert() can be remotely triggered.

I see that we might crash further down if we somehow there is a nullptr here, so my suggestion for error handling would be to Assume() everything is non-null, and return early if anything is nullptr.

changed assert to Assume as requested, let it stumble along

I would love to merge the codepaths for the package and non-package case, so that we have no concerns over whether all the right checks have been run. What if we did something like this:

• Add all passed-in transactions to a map: Txid -> CTransactionRef. This will allow us to look up transactions either locally (from a package) or in the mempool.

• For every transaction passed in:

  • For every input in the transaction:
    • Look up the input in the local map. If not found, look up in the mempool.
    • If a parent transaction was found in either the local map or mempool, check to see if it creates a dust output. If it does, add the dust output to an unspent_dust unordered map, like in the single-transaction case below.
  • For every input in the transaction, erase from the unspent_dust unordered_map (like in the single-transaction case below).
  • If any entries remain in unspent_dust, return an error.

done, put the new version in both AcceptMultipleTransactions and AcceptSingleTransaciton

I don't think it makes sense to gate this on !bypass_limits -- doing so only has an effect for transactions that spend a non-dust output that are coming from the same block as a transaction that creates a dust output, which seems like both (a) a very small effect that (b) we don't obviously want to do...

Specifically, it seems strange that we would only accept a transaction that was reorged out of a block that has a dust output if it has exactly 1 such dust output and exactly 0 fee, yet we would take a spend of such a transaction regardless of the ephemeral dust rules. Presumably if we don't care about enforcing the ephemeral dust rules during a reorg, then we should not enforce them on the parent transaction either?

Given that we're going to make a choice, I think it makes more sense to enforce those rules on both transactions -- the use case where this matters is for a miner who has not chosen to set the -dustrelayfee to 0. For such a user, I think we might as well continue to enforce the rules on both transactions, so that we help such a miner avoid inadvertently creating dust in the UTXO set.

The question remains as to what to do about transactions that might already be in the mempool that spend a non-dust output of a transaction that creates ephemeral dust. While I think it'd be somewhat better to evict such transactions, I don't think it would be worth doing if it came at a significant CPU cost -- so I wouldn't want us to add a slow operation to the removeForReorg() code that gets invoked for every mempool transaction. However, if we did something smart by tracking transactions from a block that create dust outputs and use mapNextTx to look up spends to evict, that would probably work and be pretty fast.

Removed the bypass_limits along with unification of CheckEphemeralSpends.

However, if we did something smart by tracking transactions from a block that create dust outputs and use mapNextTx to look up spends to evict, that would probably work and be pretty fast.

I'll take a look at the latter idea

nit: s/spend/spent/ ?

fixed

Just want to make sure I understand this comment, does "any size" refer to the value in the output?

yes, clarified

Can we assert that this transaction is different from the one already in the mempool?

done

Might be nice to assert that this transaction has a higher feerate than the one in the mempool, just to be sure that this would be a successful RBF if not for the ephemeral dust issue.

done

Did you mean const CTransactionRef& tx_ref here?

sure, changed

I think this should be const CTransactionRef& tx (and in the header file as well)

done

Perhaps we could let bypass_limits be filled in by the fuzzer, rather than hardcoded to false?

done, previously this would have allowed non-dust-spending children and invalidate the invariant checks, now it shouldn't matter

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.

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.

@sdaftuar this should do it if you think something like this is reasonable: https://github.com/instagibbs/bitcoin/commit/6007146613fe663f0af30c24c47a19792f8e691d

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.

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

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

It seems to me that this would apply to any transaction with a dust output rather than just one that fulfills the criteria of "ephemeral dust".

        throw JSONRPCError(RPC_INVALID_PARAMETER, "Priority is not supported for transactions with dust outputs.");

done

dust output in a transaction, provided the transaction

done

Is this logging correct? The parent_txid here is really the txid of the child that didn't spend all of some parent's dust. I don't think we know which actual parent of that transaction had the unspent dust, unless we add more information to the return value of CheckEphemeralSpends.

out of date logging, updated

Is it also worth testing that this fails if submitted via sendrawtransaction as well, so we're not just testing the AcceptPackage() code path?

done

I think all the parents in-mempool at this point?

correct, updated comment

Just want to make sure I understand what you mean by "sibling" here. Is the idea that if we have a topology like this:

flowchart TD
  TxA --> TxC
  TxB --> TxC

Where Tx A has ephemeral dust outputs and is in the mempool, and Tx C spends them and is in the mempool, than you want to return an outpoint of Tx B, whether or not Tx B itself is in the mempool, and whether or not Tx B itself has ephemeral dust outputs?

yeah that's confusing, reworded

If it's not too hard, I was thinking it would be nice to have this fuzz test cover the package RBF case as well.

I haven't looked lately but these harnesses should trigger package RBF behavior. I added the package mempool results checking to check the related invariants.

edit: To be clear, I validated that package rbfs were happening prior, it's just not constructed "intentionally" by the harness.

Comment needs to be updated I think?

updated

could avoid the duplicate map lookup:

            if (auto it = map_txid_ref.find(parent_txid); it != map_txid_ref.end()) {
                parent_ref = it->second;

(though it probably doesn't really matter that much as map_txid_ref won't have more than two entries currently; also in light of #30239 (review))

taken

the dust is both created and spent simultaneously.

done

nit: calling the RPC via the .rpc member shouldn't be needed? (here and below)

        dust_txid = self.nodes[0].sendrawtransaction(hexstring=dusty_tx["hex"], maxfeerate=0)

fixed

whitespace nit

#include <policy/ephemeral_policy.h>
#include <policy/policy.h>

fixed

potential follow-up nit: could create a helper like HasDustOutputs (or IsDusty) and use that here and in the prioritisetransaction RPC

Added HasDust to make it more obvious where these checks are being done

    // Non-0 fee dust transactions are not allowed for entry, and modification not allowed afterwards

done

follow-up idea: could take use of that helper in mempool_package_rbf.py (might be a nice "good first issue")

Deferring to Future Work

nit: My naive LSP jumped to the PackageRBFTest-version when going to the definition of assert_mempool_contents from EphemeralDustTest. Would prefer if the commit introducing this one was also removed the identically named function from PackageRBFTest.

follow-up idea: could put this in a helper for creating an ephemeral package (dusty_tx + sweep_tx), as that's a repeated pattern in many sub-tests; might make sense to parametrize with tx version, number of dust outputs and optional additional sponsors

deferring to Future Work

nit: here and in other subtests, could verify that the modified fees are indeed zero again with an getprioritisedtransactions call after

solid idea, done

is this change needed? seems to also pass with the dust value

My guess is that previous implementation had a different validation ordering and was causing the case to fail in an unexpected way. Removed.

191ca0506ca69caa4d24c1f38b6f68742abb84dc nit: maybe test_dustrelayfee_zero or something

in commit 6fa10b4537b03b1746fd899de20b6f10dd6e15f0: seems like the ephemeral invariants check could return too early, potentially skipping lots of mempool txs?

        if (dust_indexes.empty()) continue;

(same for the no-children condition below)

Oof, this was making it a lot harder to hit the various cases! Fixed!

nit: missing closing double quote

will fixup if I end up touching things

nit: should ideally be already part of the commit that introduces this function for minimal diff (here and in header)

that's a mistake, I'll fix if I touch the PR

nit: Missing word

        # Dust is 1 satoshi since create_self_transfer_multi disallows 0

In 28733511de9073fd923d83bc43fca1353e9041aa:

nit: I would either remove these docstrings, or fix up the grammar, I spent way more time trying to parse these (and failed) than just reading the code

        # Create two dust outputs. Both are unspent, have fees, and would have failed individual checks.
        # The amount is 1 satoshi because create_self_transfer_multi disallows 0.

will fixup in follwup

nit: I like the transaction being called "dusty" as it creates dust but is not itself dust. Dust refers only to outputs, not to transactions. Would prefer dusty_txid.

nit: const reference would be better

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 (review) - arguably we should probably tolerate GetHash() until the day it is renamed/aliased to GetTxid(). Lifetime is unnecessarily long as well.

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

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

Question, why minrelay instead of DUST_RELAY_TX_FEE?

was this supposed to spend the non-dust from dust_txid 1?

I think it's correct, spending dust from first dusty tx but not second

It also doesn't know about ancestors, given that the parent isn't given, right?

maybe not the most important check, just makes sure that being an ancestor in mempool alone somehow isn't enough to get checked for dust

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

leaving as-is

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

<details> <summary>git diff on d5c564a24f</summary>

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

</details>

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.

will handle on follow-up

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.

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.

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

good idea, will rename in follow-up

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?

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.

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

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.

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

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().

Can change to MAX_EPHEMERAL_DUST_OUTPUTS_PER_TX if I touch things

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.

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.

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

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

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

will add in follow-up

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,

Should this loop run only for dust inputs by adding a IsDust() right at the beginning of this loop? I'm assuming this function is not responsible for the standard transaction validity checks as per the comment in the header file.

Aah nvm, IsDust works on TxOuts.

Noting multiple ephemeral txns spent by 1 child is allowed in this logic, but not won't propagate via opportunistic package relay. This would still be the case if we relax the child-with-unconfirmed-parents requirement because none of the parents could be submitted alone.

could be worth a comment

I think "other topology" is a little under-specified here, could be helpful to name what tests you think could be added

nit: test_no_minrelay_fee could be better

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.

            if (!processed_parent_set.insert(parent_txid).second) continue;

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

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

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?

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.

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